Date: Fri, 25 Jan 2013 10:24:51 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-fs@freebsd.org, Christian Gusenbauer <c47g@gmx.at>, yongari@freebsd.org Subject: Re: 9.1-stable crashes while copying data from a NFS mounted directory Message-ID: <582361609.2352469.1359127491504.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <1436756651.2351478.1359126414802.JavaMail.root@erie.cs.uoguelph.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
I wrote:
> Konstantin Belousov wrote:
> > On Thu, Jan 24, 2013 at 09:39:38PM -0500, Rick Macklem wrote:
> > > John Baldwin wrote:
> > > > On Thursday, January 24, 2013 4:22:12 pm Konstantin Belousov
> > > > wrote:
> > > > > On Thu, Jan 24, 2013 at 09:50:52PM +0100, Christian Gusenbauer
> > > > > wrote:
> > > > > > On Thursday 24 January 2013 20:37:09 Konstantin Belousov
> > > > > > wrote:
> > > > > > > On Thu, Jan 24, 2013 at 07:50:49PM +0100, Christian
> > > > > > > Gusenbauer
> > > > > > > wrote:
> > > > > > > > On Thursday 24 January 2013 19:07:23 Konstantin Belousov
> > > > > > > > wrote:
> > > > > > > > > On Thu, Jan 24, 2013 at 08:03:59PM +0200, Konstantin
> > > > > > > > > Belousov wrote:
> > > > > > > > > > On Thu, Jan 24, 2013 at 06:05:57PM +0100, Christian
> > > > > > > > > > Gusenbauer wrote:
> > > > > > > > > > > Hi!
> > > > > > > > > > >
> > > > > > > > > > > I'm using 9.1 stable svn revision 245605 and I get
> > > > > > > > > > > the
> > > > > > > > > > > panic below
> > > > > > > > > > > if I execute the following commands (as single
> > > > > > > > > > > user):
> > > > > > > > > > >
> > > > > > > > > > > # swapon -a
> > > > > > > > > > > # dumpon /dev/ada0s3b
> > > > > > > > > > > # mount -u /
> > > > > > > > > > > # ifconfig age0 inet 192.168.2.2 mtu 6144 up
> > > > > > > > > > > # mount -t nfs -o rsize=32768 data:/multimedia
> > > > > > > > > > > /mnt
> > > > > > > > > > > # cp /mnt/Movies/test/a.m2ts /tmp
> > > > > > > > > > >
> > > > > > > > > > > then the system panics almost immediately. I'll
> > > > > > > > > > > attach
> > > > > > > > > > > the stack
> > > > > > > > > > > trace.
> > > > > > > > > > >
> > > > > > > > > > > Note, that I'm using jumbo frames (6144 byte) on a
> > > > > > > > > > > 1Gbit
> > > > > > > > > > > network,
> > > > > > > > > > > maybe that's the cause for the panic, because the
> > > > > > > > > > > bcopy
> > > > > > > > > > > (see stack
> > > > > > > > > > > frame #15) fails.
> > > > > > > > > > >
> > > > > > > > > > > Any clues?
> > > > > > > > > >
> > > > > > > > > > I tried a similar operation with the nfs mount of
> > > > > > > > > > rsize=32768 and mtu
> > > > > > > > > > 6144, but the machine runs HEAD and em instead of
> > > > > > > > > > age.
> > > > > > > > > > I
> > > > > > > > > > was unable
> > > > > > > > > > to reproduce the panic on the copy of the 5GB file
> > > > > > > > > > from
> > > > > > > > > > nfs mount.
> > > > > > > >
> > > > > > > > Hmmm, I did a quick test. If I do not change the MTU, so
> > > > > > > > just
> > > > > > > > configuring
> > > > > > > > age0 with
> > > > > > > >
> > > > > > > > # ifconfig age0 inet 192.168.2.2 up
> > > > > > > >
> > > > > > > > then I can copy all files from the mounted directory
> > > > > > > > without
> > > > > > > > any
> > > > > > > > problems, too. So it's probably age0 related?
> > > > > > >
> > > > > > > From your backtrace and the buffer printout, I see
> > > > > > > somewhat
> > > > > > > strange thing.
> > > > > > > The buffer data address is 0xffffff8171418000, while
> > > > > > > kernel
> > > > > > > faulted
> > > > > > > at the attempt to write at 0xffffff8171413000, which is is
> > > > > > > lower
> > > > > > > then
> > > > > > > the buffer data pointer, at the attempt to bcopy to the
> > > > > > > buffer.
> > > > > > >
> > > > > > > The other data suggests that there were no overflow of the
> > > > > > > data
> > > > > > > from the
> > > > > > > server response. So it might be that mbuf_len(mp) returned
> > > > > > > negative number
> > > > > > > ? I am not sure is it possible at all.
> > > > > > >
> > > > > > > Try this debugging patch, please. You need to add
> > > > > > > INVARIANTS
> > > > > > > etc
> > > > > > > to the
> > > > > > > kernel config.
> > > > > > >
> > > > > > > diff --git a/sys/fs/nfs/nfs_commonsubs.c
> > > > > > > b/sys/fs/nfs/nfs_commonsubs.c
> > > > > > > index efc0786..9a6bda5 100644
> > > > > > > --- a/sys/fs/nfs/nfs_commonsubs.c
> > > > > > > +++ b/sys/fs/nfs/nfs_commonsubs.c
> > > > > > > @@ -218,6 +218,7 @@ nfsm_mbufuio(struct nfsrv_descript
> > > > > > > *nd,
> > > > > > > struct uio
> > > > > > > *uiop, int siz) }
> > > > > > > mbufcp = NFSMTOD(mp, caddr_t);
> > > > > > > len = mbuf_len(mp);
> > > > > > > + KASSERT(len > 0, ("len %d", len));
> > > > > > > }
> > > > > > > xfer = (left > len) ? len : left;
> > > > > > > #ifdef notdef
> > > > > > > @@ -239,6 +240,8 @@ nfsm_mbufuio(struct nfsrv_descript
> > > > > > > *nd,
> > > > > > > struct uio
> > > > > > > *uiop, int siz) uiop->uio_resid -= xfer;
> > > > > > > }
> > > > > > > if (uiop->uio_iov->iov_len <= siz) {
> > > > > > > + KASSERT(uiop->uio_iovcnt > 1, ("uio_iovcnt %d",
> > > > > > > + uiop->uio_iovcnt));
> > > > > > > uiop->uio_iovcnt--;
> > > > > > > uiop->uio_iov++;
> > > > > > > } else {
> > > > > > >
> > > > > > > I thought that server have returned too long response, but
> > > > > > > it
> > > > > > > seems to
> > > > > > > be not the case from your data. Still, I think the patch
> > > > > > > below
> > > > > > > might be
> > > > > > > due.
> > > > > > >
> > > > > > > diff --git a/sys/fs/nfsclient/nfs_clrpcops.c
> > > > > > > b/sys/fs/nfsclient/nfs_clrpcops.c index be0476a..a89b907
> > > > > > > 100644
> > > > > > > --- a/sys/fs/nfsclient/nfs_clrpcops.c
> > > > > > > +++ b/sys/fs/nfsclient/nfs_clrpcops.c
> > > > > > > @@ -1444,7 +1444,7 @@ nfsrpc_readrpc(vnode_t vp, struct
> > > > > > > uio
> > > > > > > *uiop, struct
> > > > > > > ucred *cred, NFSM_DISSECT(tl, u_int32_t *, NFSX_UNSIGNED);
> > > > > > > eof = fxdr_unsigned(int, *tl);
> > > > > > > }
> > > > > > > - NFSM_STRSIZ(retlen, rsize);
> > > > > > > + NFSM_STRSIZ(retlen, len);
> > > > > > > error = nfsm_mbufuio(nd, uiop, retlen);
> > > > > > > if (error)
> > > > > > > goto nfsmout;
> > > > > >
> > > I think this patch is appropriate, although I don't see it as too
> > > critical. It just tightens the "sanity check" on the read reply
> > > length (which should never exceed what the client requested).
> > I agree, but client cannot control the server response.
> Righto. However, the server can put the correct "size" at the
> beginning
> of the reply and then follow it with the wrong amount of data. Those
> are
> the cases that the EBADRPC reply from nfsm_mbufuio() hopefully
> catches.
>
> But, I agree that changing "rsize" to "len" is correct in this case
> and
> would catch the case where the server replies with too large a "size".
>
> > Anyway, I
> > think
> > there is too much things that could go wrong if the server actively
> > exploit the client code.
> >
> > >
> > > nfsm_mbufuio() shouldn't transfer more than the uio structure can
> > > handle, even if the replied read size is larger than requested.
> > Yes, this what happen, I suppose, due to the decrement of the
> > uio_iovcnt
> > and the EBADRPC error return at the beginning of loop. But IMO the
> > situation should be catched and asserted instead. This is why I
> > added
> > KASSERT(uio_iovcnt > 1) before the decrement.
> >
> > I do not think that we should both add my KASSERT for iovcnt and
> > leave
> > the EBADRPC return. What is your preference there ?
> >
> Well, if a server sends a reply with "size == 16384", but then follows
> it with 32768bytes of data, I don't think that should panic the
> client,
> since it isn't a client bug. I think this is where the EBADRPC reply
> for
> iovcnt will happen.
Oops, I realize this wouldn't do the iovcnt EBADRPC reply. I would just
use the bytes after 16384 as the next field of the reply. I think this
one can be a KASSERT(), assuming the patch that changes "rsize" to "len"
for NFSM_STRSIZ() is applied.
I'll take another look at the code and email again, if I still haven't
gotten this right;-)
rick
> The reverse: A reply with "size == 32768", but followed by 16384bytes
> of
> data, would hit the end of the mbuf list. I don't think that should be
> a KASSERT() either since, again, it is a server bug and not a client
> one.
>
> Now, the case where the mbuf list is bogus (bad m_len or ???) I see
> the argument for KASSERT()s, since it is a bug somewhere in the client
> machine and that bug may soon (or have already) corrupted other data
> structures, even if there is no damage done for this case once caught.
> I'm fine with KASSERT()s for these. (A KASSERT() panic with a message
> is probably easier to debug than a bcopy() crash, although you did the
> latter amazingly well;-)
>
> > >
> > > It does seem that nfsm_mbufuio() should apply a sanity check on
> > > m_len. I think m_len == 0 is ok, but negative or very large should
> > > be checked for. Maybe just return EBADRPC after a printf() instead
> > > of a KASSERT(), as a safety belt against a trashed m_len from a
> > > driver
> > > or ???
> > The same as with the overflowed size, it would only hide another bug
> > in
> > the kernel. Probably, the assert m_len >= 0 and other useful
> > assertions
> > should be performed in the central place in the network stack, to
> > catch
> > an error earlier and for all consumers.
> >
> Yes. That sounds like a good idea to me. To be honest, there are a lot
> of places where a bogus mbuf list can trash the NFS code, so catching
> it in a central place would be good, I think.
>
> Thanks for looking into this, rick
>
> > >
> > > rick
> > >
> > > > > > I applied your patches and now I get a
> > > > > >
> > > > > > panic: len -4
> > > > > > cpuid = 1
> > > > > > KDB: enter: panic
> > > > > > Dumping 377 out of 6116
> > > > > > MB:..5%..13%..22%..34%..43%..51%..64%..73%..81%..94%
> > > > > >
> > > > > This means that the age driver either produced corrupted mbuf
> > > > > chain,
> > > > > or filled wrong negative value into the mbuf len field. I am
> > > > > quite
> > > > > certain that the issue is in the driver.
> > > > >
> > > > > I added the net@ to Cc:, hopefully you could get help there.
> > > >
> > > > And I've cc'd Pyun who has written most of this driver and is
> > > > likely
> > > > the one
> > > > most familiar with its handling of jumbo frames.
> > > >
> > > > --
> > > > 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"
> _______________________________________________
> 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?582361609.2352469.1359127491504.JavaMail.root>