Fix locking and a double free in fsops rename and unlink - #615
Conversation
9372f75 to
bbdf041
Compare
bbdf041 to
5ba5fc5
Compare
5ba5fc5 to
a767658
Compare
The WORM and non-empty-directory checks ran before the lock was taken but their error paths released it. Take it first; lock order is unchanged.
After ownership moves to fromdentry, out_free freed the buffers again.
51ae222 to
3800dcb
Compare
XV02
left a comment
There was a problem hiding this comment.
This is a complex change, will keep reviewing, for the time being please check this concern, if you find it wrong please discuss it on this thread :DD
| /* Lock order: parent contents_lock, parent meta_lock, then child meta_lock */ | ||
| acquirewrite_mrsw(&parent->meta_lock); | ||
|
|
There was a problem hiding this comment.
Moving acquirewrite_mrsw(&parent->meta_lock) above the WORM and non-empty-dir checks fixes the original "release of unheld lock" bug, but creates a lock-order inversion with the non-empty-dir check. Every other site in this file (and the rest of the codebase) acquires contents_lock before meta_lock on the same node, and parent locks before child locks. Here the order is reversed: parent meta_lock is held when a child's contents_lock is taken. Any concurrent thread that holds d->contents_lock (e.g. a readdir) and then tries to acquire parent->meta_lock will deadlock with this path.
There was a problem hiding this comment.
I don't think this is an inversion, but I have reordered the code so the pairing no longer occurs.
The documented order (struct dentry, ltfs.h:279) is: "contents_lock, meta_lock ... take all parent locks before any dentry locks." Parent contents -> parent meta -> child contents -> child meta is that rule, and it is the order fs_path_lookup itself uses (fs.c:627-640). A deadlock would need the inverse pair - a child contents_lock held while taking the parent's meta_lock - and no site in the tree does that; readdir takes d->contents_lock, releases it, then d->meta_lock, and never touches the parent's meta_lock.
Still, the non-empty check only needs the child's contents_lock, so it now runs before parent->meta_lock is taken (new commit). parent->meta_lock is held only for the WORM checks and the unlink, never while d->contents_lock is acquired.
The original defect: the WORM and non-empty error paths went to out -> fs_release_dentry_unlocked(parent) -> releasewrite_mrsw(&parent->meta_lock), but the lock was only acquired later (old line 485), so they released a lock never held. The WORM flags are also meta_lock-protected (ltfs.h; written under d->meta_lock in xattr.c), so reading them without it was a second defect.
…in unlink The check needs only the child's contents_lock, so parent->meta_lock is no longer held while it is taken.
Two commits:
ltfs_fsops_rename, the directory WORM checks ran beforetodir'smeta_lockwas acquired but their error path released it (whenevertodir != fromdir);ltfs_fsops_unlinkhad the same defect forparent->meta_lockon its WORM and non-empty-directory error paths. Releasing an unheld rwlock is undefined behaviour and can corrupt the lock state. The checks now run with the locks held; lock ordering is preserved.fromdentry, a later failure freed them viaout_freeand left dangling pointers that were freed again when the dentry is disposed. Ownership is now cleared from the locals when it moves.