Page MenuHomeFreeBSD

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

Authored by mjg on Feb 6 2020, 12:16 AM.
Tags
None
Referenced Files
F84669183: D23528.id68058.diff
Sun, May 26, 9:17 PM
F84641780: D23528.diff
Sun, May 26, 5:27 PM
Unknown Object (File)
Apr 19 2024, 9:59 PM
Unknown Object (File)
Feb 11 2024, 7:55 AM
Unknown Object (File)
Feb 11 2024, 6:24 AM
Unknown Object (File)
Feb 10 2024, 4:39 AM
Unknown Object (File)
Jan 16 2024, 3:25 AM
Unknown Object (File)
Dec 23 2023, 12:59 AM
Subscribers

Details

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

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.

This revision is now accepted and ready to land.Feb 9 2020, 8:26 PM
This revision was automatically updated to reflect the committed changes.