Bug 232673
| Summary: | nfs client panic: nfs_advlock traps on doomed vnode via NFS_ISV4 | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Ryan Libby <rlibby> | ||||
| Component: | kern | Assignee: | Rick Macklem <rmacklem> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | CC: | cem, rmacklem, vangyzen | ||||
| Priority: | --- | Keywords: | crash | ||||
| Version: | CURRENT | Flags: | rmacklem:
mfc-stable12+
rmacklem: mfc-stable11+ rmacklem: mfc-stable10+ |
||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
|
Description
Ryan Libby
2018-10-25 06:28:39 UTC
I'll take a look and see if there is a simple patch for this particular crash. However I do know that, in general, that the NLM is not safe for forced dismounts and I could never figure out a way to make it safe. (Essentially, whenever the vnode is unlocked, a forced dismount can cause a crash. However, there are areas in the code where the vnode must be unlocked. I could reduce the likelyhood of a crash doing something similar to what you suggest, where the code sets local variables before unlocking the vnode and using those instead of the vnode, but I couldn't do all cases.) At the very least, it should check for a doomed vnode and return EBADF when it does lock the vnode. I've said this many times on the mailing lists, but I'll mention it here too. The NLM is a fundamentally flawed protocol that was never published as an RFC. I think avoiding use of it is the best way to go. If locks for a file don't need to be visible to other clients, the "nolockd" mount option should work. For the cases (not very common) where the locks do need to be visible to other client(s), using NFSv4 is a better bet. (And NFSv4.1 fixed a lot of things, so I suggest NFSv4.1 over NFSv4.0 if possible.) Created attachment 198644 [details]
Fix crash in nfs_advlock() during forced dismount
Duh, oops...
I didn't look at the crash before making the last comment. This case
appears to happen right in nfs_advlock() before any call to the NLM.
I think this patch should fix this crash and make it work for the non-NLM
cases. ("nolockd" option or NFSv4)
Maybe the reported can test this patch and report back?
Thanks for reporting this, rick
(In reply to Rick Macklem from comment #2) Thanks. I think we should be able to reproduce it, I'll get back to you with test results. I was able to reproduce the panic as follows. The dtrace script inserts
a pause when lockf tries to drop the lock, which opens up the race with
unmount.
umount -f $mp
mount $mp
daemon dtrace -wn 'fbt::nfs_advlock:entry /execname == "lockf" && args[0]->a_op == 2/ {chill(500000000);}'
lockf $mp/foo true & umount -f $mp; wait; mount $mp
[...try last line again if it didn't trigger...]
The panic reproduces before, but not after, the patch in comment 2.
I did not do any kind of thorough functionality testing. I used NFSv3.
Thanks!
Good work w.r.t. the testing. Thanks. I'll be committing the patch to head tomorrow. A commit references this bug: Author: rmacklem Date: Thu Nov 1 15:27:22 UTC 2018 New revision: 339999 URL: https://svnweb.freebsd.org/changeset/base/339999 Log: Fix NFS client vnode locking to avoid a crash during forced dismount. A crash was reported where the crash occurred in nfs_advlock() when the NFS_ISV4(vp) macro was being executed. This was caused by the vnode being VI_DOOMED due to a forced dismount in progress. This patch fixes the problem by locking the vnode before executing the NFS_ISV4() macro. Tested by: rlibby PR: 232673 Reviewed by: kib MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D17757 Changes: head/sys/fs/nfsclient/nfs_clvnops.c A commit references this bug: Author: rmacklem Date: Sun Nov 18 22:59:55 UTC 2018 New revision: 340588 URL: https://svnweb.freebsd.org/changeset/base/340588 Log: MFC: r339999 Fix NFS client vnode locking to avoid a crash during forced dismount. A crash was reported where the crash occurred in nfs_advlock() when the NFS_ISV4(vp) macro was being executed. This was caused by the vnode being VI_DOOMED due to a forced dismount in progress. This patch fixes the problem by locking the vnode before executing the NFS_ISV4() macro. PR: 232673 Changes: _U stable/11/ stable/11/sys/fs/nfsclient/nfs_clvnops.c A commit references this bug: Author: rmacklem Date: Sun Nov 18 23:48:16 UTC 2018 New revision: 340589 URL: https://svnweb.freebsd.org/changeset/base/340589 Log: MFC: r339999 Fix NFS client vnode locking to avoid a crash during forced dismount. A crash was reported where the crash occurred in nfs_advlock() when the NFS_ISV4(vp) macro was being executed. This was caused by the vnode being VI_DOOMED due to a forced dismount in progress. This patch fixes the problem by locking the vnode before executing the NFS_ISV4() macro. PR: 232673 Changes: _U stable/10/ stable/10/sys/fs/nfsclient/nfs_clvnops.c A commit references this bug: Author: rmacklem Date: Mon Nov 19 00:49:08 UTC 2018 New revision: 340590 URL: https://svnweb.freebsd.org/changeset/base/340590 Log: MFC: r339999 Fix NFS client vnode locking to avoid a crash during forced dismount. A crash was reported where the crash occurred in nfs_advlock() when the NFS_ISV4(vp) macro was being executed. This was caused by the vnode being VI_DOOMED due to a forced dismount in progress. This patch fixes the problem by locking the vnode before executing the NFS_ISV4() macro. PR: 232673 Changes: _U stable/12/ stable/12/sys/fs/nfsclient/nfs_clvnops.c Patch has been MFC'd. |