Date: Fri, 31 Jan 2014 18:52:04 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-fs@freebsd.org, Roman Divacky <rdivacky@freebsd.org>, Dimitry Andric <dim@freebsd.org>, fs@freebsd.org Subject: Re: BUG: possible NULL pointer dereference in nfs server Message-ID: <346483760.1090608.1391212324309.JavaMail.root@uoguelph.ca> In-Reply-To: <201401311642.28821.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
John Baldwin wrote: > On Tuesday, January 28, 2014 4:40:45 am Roman Divacky wrote: > > > > Yea, so long as it includes a comment that states this is done > > > > to > > > > work around a stupid compiler bug. > > > > > > Ugh. > > > > > > The above has the following bugs: > > > - gross style bugs (lines longer than 80 characters) > > > - large code to do nothing > > > - would be even larger with a comment > > > - cannot actually work around any compiler bug involving abort(), > > > since it > > > has no effect on production kernels where KASSERT() is null. > > > > > > >> It is present even in your setup :) Just "objdump -d kernel | > > > >> grep > > > >> ud2" on kernel compiled > > > >> by clang. > > > >> > > > > I actually use gcc, but I believe you. I'll admit I still don't > > > > understand > > > > why having a trap instruction that never gets executed is a bad > > > > thing? > > > > > > It isn't but trying to link to the noexistent function abort() on > > > arches that don't have any trap instruction is a bad thing. > > > According > > > the above, this occurs on sparc64. It must be a gcc bug, since > > > sparc64 > > > doesn't have clang. However, I couldn't get gcc on sparc64 to > > > generate > > > an abort() for *NULL. Similarly on x86 (gcc is hard to test on > > > x86, > > > since it is broken (not installed) on FreeBSD cluster machines > > > for > > > > I was testing clang compiled kernel on sparc64. > > > > The problem is that there's nothing making sure that the NULL > > pointer > > dereference doesnt happen. So if someone happens to call the > > function with > > wrong flags it will crash. > > > > Thats why I want to add the KASSERT, to catch possible future cases > > when this happens. > > > > Unfortunately our KASSERT is not an assert so to remove the actual > > abort/trap/ud2 I have to remove the flag. > > Why not make a simple abort() that calls panic()? It seems clumsy to > have to > add hacks in the source code. > > OTOH, the new_lfpp thing just seems to be obfuscation. Seems you can > remove > one layer of pointer there. It doesn't help you with the compiler > not being > able to see the invariant that prevents the problem though. > > Index: nfs_nfsdstate.c > =================================================================== > --- nfs_nfsdstate.c (revision 261291) > +++ nfs_nfsdstate.c (working copy) > @@ -79,7 +79,7 @@ static int nfsrv_getstate(struct nfsclient *clp, n > static void nfsrv_getowner(struct nfsstatehead *hp, struct nfsstate > *new_stp, > struct nfsstate **stpp); > static int nfsrv_getlockfh(vnode_t vp, u_short flags, > - struct nfslockfile **new_lfpp, fhandle_t *nfhp, NFSPROC_T *p); > + struct nfslockfile *new_lfp, fhandle_t *nfhp, NFSPROC_T *p); > static int nfsrv_getlockfile(u_short flags, struct nfslockfile > **new_lfpp, > struct nfslockfile **lfpp, fhandle_t *nfhp, int lockit); > static void nfsrv_insertlock(struct nfslock *new_lop, > @@ -1985,7 +1985,7 @@ tryagain: > MALLOC(new_lfp, struct nfslockfile *, sizeof (struct nfslockfile), > M_NFSDLOCKFILE, M_WAITOK); > if (vp) > - getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, &new_lfp, > + getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, new_lfp, > NULL, p); > NFSLOCKSTATE(); > /* > @@ -2235,7 +2235,7 @@ tryagain: > M_NFSDSTATE, M_WAITOK); > MALLOC(new_deleg, struct nfsstate *, sizeof (struct nfsstate), > M_NFSDSTATE, M_WAITOK); > - getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, &new_lfp, > + getfhret = nfsrv_getlockfh(vp, new_stp->ls_flags, new_lfp, > NULL, p); > NFSLOCKSTATE(); > /* > @@ -3143,10 +3143,9 @@ out: > */ > static int > nfsrv_getlockfh(vnode_t vp, u_short flags, > - struct nfslockfile **new_lfpp, fhandle_t *nfhp, NFSPROC_T *p) > + struct nfslockfile *new_lfp, fhandle_t *nfhp, NFSPROC_T *p) > { > fhandle_t *fhp = NULL; > - struct nfslockfile *new_lfp; > int error; > > /* > @@ -3154,7 +3153,6 @@ nfsrv_getlockfh(vnode_t vp, u_short flags, > * a fhandle_t on the stack. > */ > if (flags & NFSLCK_OPEN) { > - new_lfp = *new_lfpp; > fhp = &new_lfp->lf_fh; > } else if (nfhp) { > fhp = nfhp; > > Yep, this looks good to me, although I have no idea if it makes the compiler happy? I think the new_lfpp argument was a vestige created when nfsrv_getlockfh() became a separate function from nfsrv_getlockfile() during development. (The latter needs the "**", since it sets it to NULL to indicate the new structure is being used and shouldn't be free'd.) rick > -- > John Baldwin > _______________________________________________ > freebsd-fs@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-fs > To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org" >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?346483760.1090608.1391212324309.JavaMail.root>