Page MenuHomeFreeBSD

unionfs: fix potential deadlock in VOP_RMDIR
ClosedPublic

Authored by jah on Nov 14 2021, 7:33 AM.
Tags
None
Referenced Files
F105778018: D32986.id98501.diff
Fri, Dec 20, 1:40 PM
F105777369: D32986.id98502.diff
Fri, Dec 20, 1:25 PM
F105776645: D32986.id98486.diff
Fri, Dec 20, 1:08 PM
F105774999: D32986.id98495.diff
Fri, Dec 20, 12:49 PM
Unknown Object (File)
Tue, Dec 10, 7:20 PM
Unknown Object (File)
Tue, Dec 3, 1:39 PM
Unknown Object (File)
Thu, Nov 21, 2:26 PM
Unknown Object (File)
Thu, Nov 21, 2:26 PM
Subscribers

Details

Summary

VOP_RMDIR() is called with both parent and child directory vnodes
locked. The relookup operation performed by the unionfs implementation
may relock both vnodes. Accordingly, unionfs_relookup() drops the
parent vnode lock, but the child vnode lock is never dropped.
Although relookup() will very likely try to relock the child vnode
which is already locked, in most cases this doesn't produce a deadlock
because unionfs_lock() forces LK_CANRECURSE (!). However, relocking
of the parent vnode while the child vnode remains locked effectively
reverses the expected parent->child lock order, which can produce
a deadlock e.g. in the presence of a concurrent unionfs_lookup()
operation. Address the issue by dropping the child lock around
the unionfs_relookup() call in unionfs_rmdir().

Reported by: pho

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 42780
Build 39668: arc lint + arc unit

Event Timeline

jah requested review of this revision.Nov 14 2021, 7:33 AM

A general note: lookup() cannot be called if you have any vnode locked, even if it is allowed to lock it by LK_RECURSE or somehow you have a guarantee that this vnode would not be used in lookup(). The reason is that lookup might busy mp, and order is upper mp vnode lock->busy mounted mp->mounted mp vnode lock, see the long comment before vfs_busy() in vfs_subr.c

sys/fs/unionfs/union_vnops.c
1448

From what I understand in unionfs_lock, uvp might be relocked and correspondingly reclaimed during this vn_lock() call, am I right? If yes, it is not save to call VOP_RMDIR() below.

sys/fs/unionfs/union_vnops.c
1448

Yes, that's right. The unionfs vnode (a_vp) holds a reference to uvp, but uvp may still be doomed by forcible unmount. unionfs_relookup() should ultimately fail if a_dvp or udvp is doomed, but it probably wouldn't be a good idea to call VOP_RMDIR against a valid udvp with a doomed uvp.

Check for doomed uvp on re-lock

kib added inline comments.
sys/fs/unionfs/union_vnops.c
1452

Perhaps fix this style in nearby commit?

This revision is now accepted and ready to land.Nov 14 2021, 7:56 PM
markj added inline comments.
sys/fs/unionfs/union_vnops.c
1448

unionfs_relookup() should ultimately fail if a_dvp or udvp is doomed

For a_dvp I guess this should happen since unionfs_lookup() returns an error when dvp->v_type != VDIR, right? Or is there some other mechanism that handles this?

sys/fs/unionfs/union_vnops.c
1448

For udvp it does not matter much because VOP is dispatched on the first argument. If udvp is reclaimed and vop vector is reassigned to deadfs, we end up in dead_rename().

On the other hand, typical real fs' VOP_RENAME() is not prepared to handle doomed vnodes.

sys/fs/unionfs/union_vnops.c
1448

Right, ok, I forgot about the vop vector reassignment.

This revision now requires review to proceed.Nov 14 2021, 10:37 PM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 15 2021, 4:01 AM
This revision was automatically updated to reflect the committed changes.