1/3 vfs: prevent recursion on the vnode lock in vrele
ClosedPublic
Actions

Authored by mjg on Feb 6 2020, 12:16 AM.

Details

Reviewers

kib
jeff

Commits

rS357729: vfs: fix lock recursion in vrele

Summary

vrele is supposed to be called with an unlocked vnode, but this was never asserted for if v_usecount was > 0. For such counts the lock is never touched by the routine. As a result the kernel has several consumers which expect vunref semantics and get away with calling vrele since they happen to never do it when this is the last reference (and for some of them this may happen to be a guarantee). I tried patching the consumers , but after 4 patches and getting a bootable kernel stress2 instantly ran into something. I don't think fixing them is feasible.

Thus instead I work around the problem by changing vrele semantics to tolerate being called with a lock. This eliminates a possible bug where the lock is already held and vputx takes it anyway.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 29197

Event Timeline

mjg created this revision.Feb 6 2020, 12:16 AM

Herald added a subscriber: imp. · View Herald TranscriptFeb 6 2020, 12:16 AM

Harbormaster completed remote builds in B29197: Diff 67844.Feb 6 2020, 12:16 AM

So why not fold vunref() semantic into vrele() and remove vunref() ?

vunref provides an assertable state. imo vrele in the current form needs to get eliminated in the long run, but it's not worth doing it right now

also note vunref can try to upgrade the lock since it has to be taken. with vrele we have no idea whether it is and we have to defer inactive if it is shared-locked

In D23528#516799, @mjg wrote:

vunref provides an assertable state. imo vrele in the current form needs to get eliminated in the long run, but it's not worth doing it right now

also note vunref can try to upgrade the lock since it has to be taken. with vrele we have no idea whether it is and we have to defer inactive if it is shared-locked

You can tryupgrade anyway, or reverse, vunref also postpones inactivation if upgrade failed.

I can't tryupgrade because if there is exactly one sharer and it's not curthread, this will incorrectly convert the lock to LK_EXCLUSIVE held by curthread. Also note checking td_lk_slocks is of little use here. If the count is not zero and the lock is shared-locked, it still may be that we share lock a *different* vnode.

tested by pho

kib accepted this revision.Feb 9 2020, 8:26 PM

This revision is now accepted and ready to land.Feb 9 2020, 8:26 PM

Closed by commit rS357729: vfs: fix lock recursion in vrele (authored by mjg). · Explain WhyFeb 10 2020, 1:55 PM

This revision was automatically updated to reflect the committed changes.

mjg added a commit: rS357729: vfs: fix lock recursion in vrele.