Summary: | panic: main-n259921-17f2b12a3877 panicked (Fatal trap 12: page fault while in kernel mode) | ||
---|---|---|---|
Product: | Base System | Reporter: | Masachika ISHIZUKA <ish> |
Component: | kern | Assignee: | Rick Macklem <rmacklem> |
Status: | Closed FIXED | ||
Severity: | Affects Some People | CC: | grahamperrin, ish, kib, mjg, rmacklem |
Priority: | --- | Keywords: | crash |
Version: | CURRENT | ||
Hardware: | Any | ||
OS: | Any | ||
URL: | https://cgit.freebsd.org/src/commit/?id=65127e982b94caeada8864465ad09756a7e80125 | ||
See Also: |
https://reviews.freebsd.org/D34470 https://reviews.freebsd.org/D38032 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=268971 |
||
Attachments: |
Description
Masachika ISHIZUKA
2023-01-08 14:27:48 UTC
Created attachment 239354 [details]
revert removal of a check for ni_startdir being non-NULL
This patch reverts a small part of 65127e982b94,
which I think caused this crash.
I believe this crash was caused by 65127e982b94 which removed a check for ni_startdir being non-NULL before doing vrele()s for the create case with nd_repstat set non-zero. The patch I attached reverts this change and, I think, will fix the crash. 651273982b94 is dated Nov. 10. I have no idea if your kernel version has this patch, but I suspect it does. textdump.tar.1.xz at comment #0 comprises five files: % ls -hln total 1 -rw------- 1 1002 0 4.3K 8 Jan 14:13 config.txt -rw------- 1 1002 0 48K 8 Jan 14:13 ddb.txt -rw------- 1 1002 0 20K 8 Jan 14:13 msgbuf.txt -rw------- 1 1002 0 10B 8 Jan 14:13 panic.txt -rw------- 1 1002 0 154B 8 Jan 14:13 version.txt % (In reply to Rick Macklem from comment #2) Thank you, > … 651273982b94 is dated Nov. 10. … version.txt (above) gives us: FreeBSD 14.0-CURRENT #16 main-n259921-17f2b12a3877-dirty: Tue Jan 3 18:13:32 JST 2023 ishizuka@okra.ish.org:/usr/obj/usr/src/amd64.amd64/sys/SG_UDF2 (In reply to Rick Macklem from comment #2) Thank you to give me a patch. After applying this patch, it works fine. Part of the point of the aformentioned change was to stop exporting ni_startdir. I probably should have added some poisononing of that value so that this would have been found sooner. Anyhow, for vrele to be called in this case both dvp and vp have to be null, which means the lookup failed to find anything? Should this code even be reachable in such a case? As a quick hack the following will probably do it: diff --git a/sys/fs/nfsserver/nfs_nfsdport.c b/sys/fs/nfsserver/nfs_nfsdport.c index 665e2c00ce08..012f84f8befa 100644 --- a/sys/fs/nfsserver/nfs_nfsdport.c +++ b/sys/fs/nfsserver/nfs_nfsdport.c @@ -1918,7 +1918,7 @@ nfsvno_open(struct nfsrv_descript *nd, struct nameidata *ndp, } } else { nfsvno_relpathbuf(ndp); - if (create == NFSV4OPEN_CREATE) { + if (create == NFSV4OPEN_CREATE && ndp->ni_dvp != NULL) { if (ndp->ni_dvp == ndp->ni_vp) vrele(ndp->ni_dvp); else I'm not sure if your patch will work, since it depends on ni_dvp being set to NULL long before any call to nfsvno_namei()/vfs_lookup(). That code could be added, but I will need to look to see it gets set non-NULL in the correct place. What about ni_rootdir. Can I use that one? Basically, it is a test "has nfsvno_namei() been called", done by setting the field NULL at the beginning of the function and then setting in non-NULL in nfsvno_namei() at the correct place (ni_rootdir gets set in the same place as ni_startdir). Created attachment 239370 [details]
Check ni_dvp is not NULL before attempting vrele()
This version of the patch should fix the problem
as well. It does what mjg@ suggested plus initializes
ni_dvp NULL. This is necessary, since nfsrv_parsename()
can set nd_repstat so that nfsvno_namei() does not get
called (it calls vfs_lookup(), which would set ni_dvp).
This avoids use of ni_startdir as requested by mjg@.
If anyone can test this patch variant, that would be
appreciated.
as there is only one consumer of nfsrvd_open, perhaps you can denote that namei was called in the struct you pass around? admittedly the ni_dvp thing is also a giant hack, the content of nameidata should be considered indeterminate if namei was not called Created attachment 239398 [details]
Add an argument to nfsvno_open() to indicate if nfsvno_namei() was called
This version of the patch uses a new argument to
nfsvno_open() to indicate if nfsvno_namei() has
been called before nfsvno_open(), then only do
the vrele() if it has been called (which indicates
that ni_dvp will be properly set).
Maybe this version will make mjg@ happy?
as long as it does not depend on internal namei state (which ni_stardir now is) it is not my business :) I'll see about hiding ni_stardir behind a setter and otherwise making it not available. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=dcfa3ee44da2b139f51a8aedb0f55735c6dfe3f3 commit dcfa3ee44da2b139f51a8aedb0f55735c6dfe3f3 Author: Rick Macklem <rmacklem@FreeBSD.org> AuthorDate: 2023-01-13 00:45:26 +0000 Commit: Rick Macklem <rmacklem@FreeBSD.org> CommitDate: 2023-01-13 00:48:53 +0000 nfsserver: Fix vrele() panic in nfsvno_open() Commit 65127e982b94 removed a check for ni_startdir != NULL. This allowed the vrele(ndp->ni_dvp) to be called with a NULL argument. This patch adds a new boolean argument to nfsvno_open() that can be checked instead of ni_startdir, since mjg@ requested that ni_startdir not be used. (Discussed in PR#268828.) PR: 268828 Reviewed by: mjg Differential Revision: https://reviews.freebsd.org/D38032 sys/fs/nfs/nfs_var.h | 2 +- sys/fs/nfsserver/nfs_nfsdport.c | 4 ++-- sys/fs/nfsserver/nfs_nfsdserv.c | 13 ++++++++----- 3 files changed, 11 insertions(+), 8 deletions(-) Patch has been committed. |