fix(inode): reference the lower path across getattr - #186
Merged
Conversation
smoothfs_getattr snapshotted si->lower_path with no lock and no reference, then handed that snapshot to vfs_getattr_nosec. A placement cutover replaces lower_path under inode_lock and then, after unlocking, dputs the old lower dentry and mntputs its vfsmount while tierd removes the source-tier copy. A stat racing a migration therefore lands one of two ways: vfs_getattr_nosec runs against a lower that has been unlinked on the source tier and returns -ESTALE for a file that plainly exists, or the cutover frees the dentry while getattr is still dereferencing it. The first is what a caller sees -- a walk over a large tree during tiering fails on arbitrary files, and a caller that treats ESTALE as absence silently loses them. The second is a use-after-free. open_lower_now already had the correct idiom two files over: take the snapshot under inode_lock_shared, path_get it, drop the lock, path_put when done. This applies the same to getattr. Every other si->lower_path reader was already protected -- the movement.c sites hold inode_lock, inode.c:383 documents its i_rwsem and cutover_srcu coverage, super.c takes a reference -- so getattr was the only unguarded one, most likely from the Phase 3 STAT p99 work that removed mirror-on-getattr. The lock is a shared rwsem acquisition on the stat path and excludes only the exclusive cutover writer. movement_getattr_cutover.sh drives eight concurrent statters across six alternating cutovers and fails on any ESTALE. The window is a few microseconds between snapshot and vfs_getattr, so the harness widens it by concurrency rather than by timing assumptions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
smoothfs_getattrsnapshotssi->lower_pathwith no lock and no reference, then hands that snapshot tovfs_getattr_nosec:A placement cutover replaces
lower_pathunderinode_lockand then, after unlocking, releases the old refs while tierd removes the source-tier copy (movement.c):So a
stat()racing a migration lands one of two ways:vfs_getattr_nosecruns against a lower that has been unlinked on the source tier and returns-ESTALEfor a file that plainly exists.How it was found
A corpus scan over ~257 git repositories on a tiered pool died with
OSError: [Errno 116] Stale file handleonlinux/include/linux/spi/tle62x0.h. The file stats and reads fine on retry — it was simply being migrated. The application-side impact is worse than a failed scan: a caller that treats ESTALE as "file is gone" silently drops files, which is exactly what our indexer did before we fixed it on that side too.The fix
open_lower_nowinlower.calready had the correct idiom — snapshot underinode_lock_shared,path_get, unlock,path_putwhen done. This applies the same to getattr.I checked every
si->lower_pathreader: themovement.csites holdinode_lock,inode.c:383documents its i_rwsem + cutover_srcu coverage,super.ctakes a reference. getattr was the only unguarded one — most likely from the Phase 3 STAT p99 work that removed mirror-on-getattr.Cost is a shared rwsem acquisition on the stat path, excluding only the exclusive cutover writer.
Verification
make verifypasses.7.0.11-smoothkernel(LD [M] smoothfs.ko), no new warnings frominode.c.movement_getattr_cutover.shadded and registered: eight concurrent statters across six alternating cutovers, failing on any ESTALE.Validation-pending, stated plainly: I have not executed the new runtime harness. It needs root, loop devices, XFS and a load of the patched module, and the only machine here with matching kernel headers is currently holding a live training corpus and checkpoint backups — loading a patched filesystem module under it mid-run was not a risk worth taking. The harness needs a run in the VM harness (
ci_run_in_vm.sh) before this is considered proven, including a run against the unpatched module to confirm it reproduces.