infinite recursion in m_split()

[Manuel Bouyer]

Hi,
While debugging NFS server issues I found a infinite recursion in m_split
when it's called with len0 == 0.
it's called this way from nfsrv_getstream() (and this is even documented
in the comment :):
/*
* Now get the record part.
*
* Note that slp->ns_reclen may be 0. Linux sometimes
* generates 0-length records.
*/
if (slp->ns_cc == slp->ns_reclen) {
recm = slp->ns_raw;
slp->ns_raw = slp->ns_rawend = (struct mbuf *)0;
slp->ns_cc = slp->ns_reclen = 0;
} else if (slp->ns_cc > slp->ns_reclen) {
recm = slp->ns_raw;
m = m_split(recm, slp->ns_reclen, waitflag);

Then m_split() calls m_split0, which, if (m0->m_flags & M_PKTHDR)
is true and (m->m_flags & M_EXT) is false, will call m_split() with
the same mbuf and the same m0. This is an infinite loop (well, infinite
until stack is exhausted).

Documentatio doesn't mention that calling m_split() with a 0 len0 is invalid.
My fix is to check for (len0 == 0) in m_split() and return m0 in this case.
Is it OK ?

[YAMAMOTO Takashi]

hi,

Post by Manuel Bouyer
Hi,
While debugging NFS server issues I found a infinite recursion in m_split
when it's called with len0 == 0.
it's called this way from nfsrv_getstream() (and this is even documented
/*
* Now get the record part.
*
* Note that slp->ns_reclen may be 0. Linux sometimes
* generates 0-length records.
*/
if (slp->ns_cc == slp->ns_reclen) {
recm = slp->ns_raw;
slp->ns_raw = slp->ns_rawend = (struct mbuf *)0;
slp->ns_cc = slp->ns_reclen = 0;
} else if (slp->ns_cc > slp->ns_reclen) {
recm = slp->ns_raw;
m = m_split(recm, slp->ns_reclen, waitflag);
Then m_split() calls m_split0, which, if (m0->m_flags & M_PKTHDR)
is true and (m->m_flags & M_EXT) is false, will call m_split() with
the same mbuf and the same m0. This is an infinite loop (well, infinite
until stack is exhausted).

good catch.

Post by Manuel Bouyer
Documentatio doesn't mention that calling m_split() with a 0 len0 is invalid.
My fix is to check for (len0 == 0) in m_split() and return m0 in this case.
Is it OK ?

the fix seems wrong because the caller of m_split will keep a reference to
the original m0 as well and it will be double-freed eventually.
i think it's more straightforward to fix nfsrv_getstream.

YAMAMOTO Takashi

Post by Manuel Bouyer
--
NetBSD: 26 ans d'experience feront toujours la difference
--

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Manuel Bouyer]

Post by YAMAMOTO Takashi

Post by Manuel Bouyer
Documentatio doesn't mention that calling m_split() with a 0 len0 is invalid.
My fix is to check for (len0 == 0) in m_split() and return m0 in this case.
Is it OK ?

the fix seems wrong because the caller of m_split will keep a reference to
the original m0 as well and it will be double-freed eventually.
i think it's more straightforward to fix nfsrv_getstream.

Right, it appeared to me this morning (sleep gives good things sometimes :)
The obvious place to fix it is indeed nfsrv_getstream() and add a KASSERT
in m_split() (and update the documentation).

To bring the implementation in sync with the documentation we would need to
return a copy of m0 in this case, but this may be overkill. It's better
to let the caller deal with it.

[Manuel Bouyer]

Post by Manuel Bouyer

Post by YAMAMOTO Takashi

Post by Manuel Bouyer
Documentatio doesn't mention that calling m_split() with a 0 len0 is invalid.
My fix is to check for (len0 == 0) in m_split() and return m0 in this case.
Is it OK ?

the fix seems wrong because the caller of m_split will keep a reference to
the original m0 as well and it will be double-freed eventually.
i think it's more straightforward to fix nfsrv_getstream.

Right, it appeared to me this morning (sleep gives good things sometimes :)
The obvious place to fix it is indeed nfsrv_getstream() and add a KASSERT
in m_split() (and update the documentation).
To bring the implementation in sync with the documentation we would need to
return a copy of m0 in this case, but this may be overkill. It's better
to let the caller deal with it.

Here's my proposed patch. I didn't see a panic, but I do get strange errors
with a build.sh -j16 on the linux box: build tools complaining about a
nonexistent file, when the file should have been created just before
(e.g. nbinstall complaining "can't rename nbpax.foo to nbpax: no such file or
directory"). Maybe there's something going wrong with NFS retries.
Maybe we should still copy the mbuf to be able to add it at the tail of
the slp->ns_frag chain ?

I also do get a lot of:
Apr 3 10:35:08 horn /netbsd: nfsd send error 32

I don't know if it's releated.

[Manuel Bouyer]

Post by YAMAMOTO Takashi
hi,

Post by Manuel Bouyer
Hi,
While debugging NFS server issues I found a infinite recursion in m_split
when it's called with len0 == 0.
it's called this way from nfsrv_getstream() (and this is even documented
/*
* Now get the record part.
*
* Note that slp->ns_reclen may be 0. Linux sometimes
* generates 0-length records.
*/
if (slp->ns_cc == slp->ns_reclen) {
recm = slp->ns_raw;
slp->ns_raw = slp->ns_rawend = (struct mbuf *)0;
slp->ns_cc = slp->ns_reclen = 0;
} else if (slp->ns_cc > slp->ns_reclen) {
recm = slp->ns_raw;
m = m_split(recm, slp->ns_reclen, waitflag);
Then m_split() calls m_split0, which, if (m0->m_flags & M_PKTHDR)
is true and (m->m_flags & M_EXT) is false, will call m_split() with
the same mbuf and the same m0. This is an infinite loop (well, infinite
until stack is exhausted).

good catch.

Well, not so good. The conditions which cause this recursion should not
happen: If m == m0 (i.e. the loop at the start of m_split0() dind't
progress), and (m0->m_flags & M_PKTHDR) is true, then we either
have remain (= m0->m_len in this specific case) <= MHLEN or m0 has
an external storage. So the recursion happens when something wrong
already happened. The attached patch gives more details on how we get in
this situation:
m 0xc3bae000 m0 0xc3bae000 remain 1444 len_save -4 len0 0 len 0 m_len 1444 MHLEN 200
panic: m_split0

This mbuf obviously has a cluster attached (gdb on the core dump confirmed),
yet it doesn't have M_EXT set (the (m->m_flags & M_EXT) goto didn't
catch it, and gdb on the core dump also confirms). So the mbuf got partially
updated before m_split() I also guess m0->m_pkthdr.len is not supposed
to be negative, yet we have len_save as -4 (I've also seen -252 or -260).
So something is putting wrong values in the mbuf before calling m_split()
on it, but I don't know where it happens.
The printf() I added to check for len0 == 0 at the top of m_split()
fires only once, so I guess when slp->ns_reclen == 0 damage has already
been done to the associated mbuf; it could be related to this
specific condition.

Any idea very welcome.

[Manuel Bouyer]

Post by Manuel Bouyer
[...]
already happened. The attached patch gives more details on how we get in
m 0xc3bae000 m0 0xc3bae000 remain 1444 len_save -4 len0 0 len 0 m_len 1444 MHLEN 200
panic: m_split0

Sorry forgot the patch, here it is

Post by Manuel Bouyer
This mbuf obviously has a cluster attached (gdb on the core dump confirmed),
yet it doesn't have M_EXT set (the (m->m_flags & M_EXT) goto didn't
catch it, and gdb on the core dump also confirms). So the mbuf got partially
updated before m_split() I also guess m0->m_pkthdr.len is not supposed
to be negative, yet we have len_save as -4 (I've also seen -252 or -260).
So something is putting wrong values in the mbuf before calling m_split()
on it, but I don't know where it happens.
The printf() I added to check for len0 == 0 at the top of m_split()
fires only once, so I guess when slp->ns_reclen == 0 damage has already
been done to the associated mbuf; it could be related to this
specific condition.
Any idea very welcome.
--
NetBSD: 26 ans d'experience feront toujours la difference
--

[Manuel Bouyer]

Post by Manuel Bouyer
Well, not so good. The conditions which cause this recursion should not
happen: If m == m0 (i.e. the loop at the start of m_split0() dind't
progress), and (m0->m_flags & M_PKTHDR) is true, then we either
have remain (= m0->m_len in this specific case) <= MHLEN or m0 has
an external storage. So the recursion happens when something wrong
already happened. The attached patch gives more details on how we get in
m 0xc3bae000 m0 0xc3bae000 remain 1444 len_save -4 len0 0 len 0 m_len 1444 MHLEN 200
panic: m_split0
This mbuf obviously has a cluster attached (gdb on the core dump confirmed),
yet it doesn't have M_EXT set (the (m->m_flags & M_EXT) goto didn't
catch it, and gdb on the core dump also confirms). So the mbuf got partially
updated before m_split() I also guess m0->m_pkthdr.len is not supposed
to be negative, yet we have len_save as -4 (I've also seen -252 or -260).
So something is putting wrong values in the mbuf before calling m_split()
on it, but I don't know where it happens.
The printf() I added to check for len0 == 0 at the top of m_split()
fires only once, so I guess when slp->ns_reclen == 0 damage has already
been done to the associated mbuf; it could be related to this
specific condition.

I have to retract this; another test showed the printf firing several time
without panic. Now the m_split() with len == 0 could have been from another
place than the NFS code ...
I'll add more instrumentation inside the NFS server code tomorow.