unionfs: various locking fixes
ClosedPublic
Actions

Authored by jah on Oct 24 2021, 9:59 PM.

Details

Reviewers

kib
markj

Commits

rG5f73b3338ee1: unionfs: Improve vnode validation
rGfb273fe70f8b: unionfs: replace zero-length read check with KASSERT
rG66191a76ace5: unionfs: Improve locking assertions
rG3ecefc4a61cc: unionfs: assorted style fixes
rG866dd6335a6c: unionfs: various locking fixes

Summary

--Clearing cached subdirectories in unionfs_noderem() should be done

under the vnode interlock

--When preparing to switch the vnode lock in both unionfs_node_update()

and unionfs_noderem(), the incoming lock should be acquired before
updating the v_vnlock field to point to it.  Otherwise we effectively
break the locking contract for a brief window.

unionfs: assorted style fixes

No functional change intended, beyond slightly different panic strings

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 42451
Build 39339: arc lint + arc unit

Event Timeline

jah created this revision.Oct 24 2021, 9:59 PM

Herald added a subscriber: imp. · View Herald TranscriptOct 24 2021, 9:59 PM

jah requested review of this revision.Oct 24 2021, 9:59 PM

Harbormaster completed remote builds in B42333: Diff 97375.Oct 24 2021, 9:59 PM

phabricator doesn't make it very clear, but the locking and style changes are 2 different commits.

kib added inline comments.Oct 24 2021, 11:16 PM

sys/fs/unionfs/union_subr.c
212–215	There should be spaces around binary ops, like `uppervp == NULLVP`
442	I do not think that () are needed there. I am not sure what to do about possible lock recursion there. It would be enough to assert that either of lower/upper vnodes are not recursively locked, if exist. unionfs_node_update() does handle the recursion. You might also add the LK_NOWAIT flag, since the lock must not be owned by anybody.
1256–1257	This is a strange block. Either we cannot handle such situation (empty read) at all, or we ignore it, under DIAGNOSTIC or not. Most likely, this check should be moved somewhere into vop_readdir_post under INVARIANTS.
1315	Again this is strange, panic will re-enter the debugger. Perhaps it should be KASSERT, and then remove notyet?

jah added inline comments.Oct 26 2021, 3:40 PM

sys/fs/unionfs/union_subr.c
1256–1257	Agreed. For now I'll make this a KASSERT in the same place as the current check.
1315	I'm not sure why the KDB part was written like this; I suspect the vnops check under notyet was pasted from the equivalent nullfs code, But in any case, I'd rather have a KASSERT here to catch some strange corner case where we're not using a unionfs node, before trying to access fields of that node.

Further style fixes, improve locking assertions and vnode checking

Harbormaster completed remote builds in B42451: Diff 97729.Oct 30 2021, 5:07 AM

kib accepted this revision.Oct 31 2021, 1:46 AM

kib added inline comments.

sys/fs/unionfs/union.h
119
121
122	Generally, VOP_LOCK() implementations _must_ be able to handle reclaimed vnodes. Because we might wait for the lock while VOP_RECLAIM() is running. I.e. I think the case is there to stay.
sys/fs/unionfs/union_subr.c
186	Are uvp/lvp supposed to be locked there (I assume yes)?
445

This revision is now accepted and ready to land.Oct 31 2021, 1:46 AM

jah added inline comments.Oct 31 2021, 1:47 PM

sys/fs/unionfs/union_subr.c
186	Good point, actually no they won't be locked for unionfs_get_cached_vnode(). unionfs_nodeget() first tries to lookup any matching vnode from the cache and returns if it finds one. Only after that does it lock lvp/uvp, at which point lvp/uvp are also checked for VI_DOOMED. lvp/uvp will therefore be locked at the time unionfs_ins_cached_vnode() is called, and they will not be passed to unionfs_ins_cached_vnode() if they are doomed, so the VDIR assert there is valid. Here they will be referenced but not locked, so v_type may still transition to VBAD.