Discussion:
m_split() bug causing NFS server issue
(too old to reply)
Manuel Bouyer
2009-04-05 16:42:00 UTC
Permalink
Hi,
I finally found the cause of the infinite recursion in m_split()
exposted by the NFS server. It's a bug in m_split() itself,
where the m_len field of a newly allocated mbuf could be left
uninitialized (see attached patch) This would cause the size of the new
chain to be computed wrongly in various place, and other issue.
If this uninitialized m_len happens to be larger than MHLEN, m_split()
will loop on it if called again on this new chain.
This is going to also cause other issues in the NFS server when trying
to get data from this chain (it'll be copied from the wrong place).

Interestingly this bug seems to have been there for a long time;
it's already there in netbsd-3. I guess this code path would not be used
a lot before NFS became MP-safe.

I commited the attached patch and will request a pullup to netbsd-5
(and probably netbsd-4 and netbsd-3 too)
--
Manuel Bouyer <***@antioche.eu.org>
NetBSD: 26 ans d'experience feront toujours la difference
--
Masao Uebayashi
2009-04-06 02:43:54 UTC
Permalink
Post by Manuel Bouyer
I finally found the cause of the infinite recursion in m_split()
exposted by the NFS server. It's a bug in m_split() itself,
where the m_len field of a newly allocated mbuf could be left
uninitialized (see attached patch) This would cause the size of the new
chain to be computed wrongly in various place, and other issue.
Good catch!
Post by Manuel Bouyer
Index: uipc_mbuf.c
===================================================================
RCS file: /cvsroot/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.131
diff -u -p -u -r1.131 uipc_mbuf.c
--- uipc_mbuf.c 15 Mar 2009 17:14:40 -0000 1.131
+++ uipc_mbuf.c 5 Apr 2009 16:28:06 -0000
@@ -1056,6 +1056,7 @@ m_split0(struct mbuf *m0, int len0, int
if (remain > MHLEN) {
/* m can't be the lead packet */
MH_ALIGN(n, 0);
+ n->m_len = 0;
n->m_next = m_split(m, len, wait);
if (n->m_next == 0) {
(void) m_free(n);
I wonder if we can detect such a mistake somehow. How about making MGETHDR
set m_len to some poisonous value?

Masao

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