Page MenuHomeFreeBSD

fix NFS client advisory locking when a forced dismount is performed
ClosedPublic

Authored by rmacklem on Oct 30 2018, 12:19 AM.
Tags
None
Referenced Files
F105995947: D17757.diff
Mon, Dec 23, 3:50 PM
F105924666: D17757.diff
Sun, Dec 22, 5:56 PM
Unknown Object (File)
Oct 9 2024, 2:24 AM
Unknown Object (File)
Oct 5 2024, 10:39 PM
Unknown Object (File)
Oct 5 2024, 10:39 PM
Unknown Object (File)
Oct 5 2024, 12:01 PM
Unknown Object (File)
Oct 4 2024, 9:43 AM
Unknown Object (File)
Oct 3 2024, 6:16 PM
Subscribers

Details

Summary

A crash has been reported via PR#232673 where the NFS client VOP_ADVLOCK()/nfs_advlock() is called
on a VI_DOOMED vnode because a forced dismount it being done on the mount.
The crash appears to be caused by the NFS_ISV4(vp) macro being performed on the doomed vnode.
This patch moves locking the vnode to above where the NFS_ISV4(vp) macro is being executed,
with a failure of the vn_lock() call { behind the NFSVOPLOCK() macro } being returned as a EBADF error.
The patch replaced LK_EXCLUSIVE with LK_UPGRADE for the NFSv4 case, since the vnode lock is already LK_SHARED
locked and unlocks the vnode for all three "if/else" cases.

Test Plan

I have tested it by doing forced dismounts while NFS locking is being done on the mount, using
the connectathon lock tests and have not been able to reproduce a crash.
The reporter of the PR has also been asked to try and test this patch and I am waiting to here how that goes.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This looks fine, even with my dislike of the upgrade.

BTW, since you are locking the vnode before the selection of the code paths, would it make sense to move unlock right before the last return statement and use it for all return paths ? In other words, replacing repeating 'NFSVOPUNLOCK();; return();' fragments with 'goto out' ?

Hmm, not sure. If you look at the code near the end of nfs_advlock() { around line#3100-3120 in head's sys/fs/nfsclient/nfs_clvnops.c } that isn't really
shown in the patch, then where the vnode gets VOP_UNLOCK()'d gets more convoluted. Such as before the lf_advlock() call.
I could do an errout: case so that many of the VOP_UNLOCK(); return(X); cases could become error = X; goto errout; but I'm not sure that
makes the code more readable.
If you think it does make it more readable, let me know and I'll change it.
{ Some like single return points and others don't like goto's. I have no strong opinion w.r.t. this. }

I was not about single return, but more about simplification of the flow control for the calls to unlock.

This revision is now accepted and ready to land.Oct 31 2018, 3:59 AM
This revision was automatically updated to reflect the committed changes.