Page MenuHomeFreeBSD

unionfs: fix potential deadlock in VOP_RMDIR
ClosedPublic

Authored by jah on Nov 14 2021, 7:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Sep 20, 1:22 PM
Unknown Object (File)
Thu, Sep 19, 3:31 PM
Unknown Object (File)
Thu, Sep 19, 10:25 AM
Unknown Object (File)
Wed, Sep 18, 6:43 PM
Unknown Object (File)
Wed, Sep 18, 2:01 AM
Unknown Object (File)
Aug 3 2024, 11:50 PM
Unknown Object (File)
Aug 3 2024, 11:50 PM
Unknown Object (File)
Jul 29 2024, 7:28 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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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
1453

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.