Date: Mon, 27 Jan 2014 19:37:43 +0100 From: Roman Divacky <rdivacky@freebsd.org> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: Dimitry Andric <dim@FreeBSD.org>, fs@FreeBSD.org Subject: Re: BUG: possible NULL pointer dereference in nfs server Message-ID: <20140127183743.GA74668@freebsd.org> In-Reply-To: <1647701889.16319715.1390701954380.JavaMail.root@uoguelph.ca> References: <20140126051258.M1750@besplex.bde.org> <1647701889.16319715.1390701954380.JavaMail.root@uoguelph.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Jan 25, 2014 at 09:05:54PM -0500, Rick Macklem wrote:
> Bruce Evans wrote:
> > On Sat, 25 Jan 2014, Dimitry Andric wrote:
> >
> > > On 25 Jan 2014, at 01:38, Rick Macklem <rmacklem@uoguelph.ca>
> > > wrote:
> > > ...
> > > If it inlines this, the result looks approximately like:
> > >
> > > 1 {
> > > 2 fhandle_t *fhp = NULL;
> > > 3 struct nfslockfile *new_lfp;
> > > 4 int error;
> > > 5
> > > 6 if (new_stp->ls_flags & NFSLCK_OPEN) {
> > > 7 new_lfp = *NULL;
> > > 8 fhp = &new_lfp->lf_fh;
> > > 9 } else if (&nfh) {
> > > 10 fhp = &nfh;
> > > 11 } else {
> > > 12 panic("nfsrv_getlockfh");
> > > 13 }
> > > 14 error = nfsvno_getfh(vp, fhp, p);
> > > 15 NFSEXITCODE(error);
> > > 16 getlckret = error;
> > > 17 }
> > >
> > > The code in line 7 is the problematic part. Since this is
> > > undefined,
> > > the compiler inserts a trap instruction here. I think the problem
> > > Roman
> > > encountered is that on sparc64, there is no equivalent to x86's ud2
> > > instruction, so it inserts a call to abort() instead, and that
> > > function
> > > is not available in kernel-land.
> >
> > Compiler bug. abort() is not available in freestanding
> > implementations.
> > The behaviour is only undefined if the null pointer is dereferenced
> > at
> > runtime, so it doesn't include failing to link to abort() at compile
> > time.
> >
> > > ...
> > >> Sorry, I'm not a compiler guy, so I don't know why a compiler
> > >> would
> > >> generate a trap instruction, but since new_lfpp is never NULL when
> > >> this is executed, I don't see a problem.
> > >>
> > >> If others feel that this needs to be re-coded, please let me know
> > >> what
> > >> you think the code should look like? (A test for non-NULL with a
> > >> panic()
> > >> before it is used?)
> > >>
> > >> Is a trap instruction that never gets executed a problem?
> > >
> > > It's better to avoid undefined behavior in any case. Just add a
> > > NULL
> > > check, that should be sufficient.
> >
> > That might only add bloat and unimprove debugging. Since the null
> > pointer
> > case cannot happen, it cannot be handed properly. It can be
> > mishandled in
> > the following ways:
> > - return an error, so that all callers have to handle the null
> > pointer case
> > that can't happen. If the compiler is too smart, it will notice
> > more
> > undefined behaviour (that can't happen) in callers and "force" you
> > to
> > handle it there too
> > - KASSERT() that the pointer cannot be null. Then:
> > - on production systems where KASSERT() is null, this won't work
> > around
> > the compiler bug. Use a panic() instead. To maximize source
> > code
> > bloat, ifdef all of this.
> > - when KASSERT() is not null, it will work around the compiler
> > bug.
> > If the case that can't happen actually happens, then this
> > unimproves
> > the debugging by messing up stack traces and turning restartable
> > null pointer or SIGILL traps to non-restartable panics.
> > Optimizations that replace a large block of code ending with a
> > null pointer trap by a single unimplemented instruction would
> > probably break restarting anyway.
> >
> > Bruce
> >
> So Roman, all I can suggest is to try adding something like:
> if (new_lfpp == NULL)
> panic("new_lfpp NULL");
> after line#6. If that makes the compiler happy, I can commit it in
> April. (Can't do commits before then.)
The compiler already inserts "trap" instruction when such a condition
happens so this seem superfluous.
> I agree with Bruce, but the check might be a good idea, in case a
> future code change introduces a bug where the function is called with
> new_lfpp NULL and NFSLCK_OPEN set.
>
> If this doesn't make the compiler happy, all I can suggest is to
> play around until you come up with something that works.
KASSERT() doesnt communicate that it's an assert, because it can
just log into console a carry on. Would you be ok with this patch?
Index: fs/nfsserver/nfs_nfsdstate.c
===================================================================
--- fs/nfsserver/nfs_nfsdstate.c (revision 261037)
+++ fs/nfsserver/nfs_nfsdstate.c (working copy)
@@ -1384,7 +1384,8 @@
* If we are doing Lock/LockU and local locking is enabled, sleep
* lock the nfslockfile structure.
*/
- getlckret = nfsrv_getlockfh(vp, new_stp->ls_flags, NULL, &nfh, p);
+ KASSERT((new_stp->ls_flags & NFSLCK_OPEN) == 0, ("nfsrv_lockctrl: calling nfsrv_getlockfh with NFSLCK_OPEN"));
+ getlckret = nfsrv_getlockfh(vp, new_stp->ls_flags & ~NFSLCK_OPEN, NULL, &nfh, p);
NFSLOCKSTATE();
if (getlckret == 0) {
if ((new_stp->ls_flags & (NFSLCK_LOCK | NFSLCK_UNLOCK)) != 0 &&
> Have fun with it, rick
> ps: I haven't seen this reported by tinderbox. Is the problem
> specific to your setup?
It is present even in your setup :) Just "objdump -d kernel | grep ud2" on kernel compiled
by clang.
Roman
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140127183743.GA74668>