Subject: Re: kern/13807
To: None <gnats-bugs@netbsd.org>
From: Michael van Elst <mlelstv@serpens.de>
List: netbsd-bugs
Date: 02/02/2003 18:56:13
I traced down the lost mbufs to the ipsec_addaux function. The
NFS code does it's own dirty stuff poking mbuf structures and
drops packet headers and the associated aux mbuf chains.

The following patch makes the nfs code aware of mbufs with
packet headers and so far I haven't seen any mbuf-leakage
on my NFS client:

Index: nfs_subs.c
===================================================================
RCS file: /cvsroot/src/sys/nfs/nfs_subs.c,v
retrieving revision 1.103
diff -u -3 -r1.103 nfs_subs.c
--- nfs_subs.c	2002/03/17 22:22:40	1.103
+++ nfs_subs.c	2003/02/02 17:24:31
@@ -1043,27 +1043,35 @@
 			m1->m_next = m2;
 		}
 		m1->m_len = 0;
-		dst = m1->m_dat;
+		if (m1->m_flags & M_PKTHDR)
+			dst = m1->m_pktdat;
+		else
+			dst = m1->m_dat;
+		m1->m_data = dst;
 	} else {
 		/*
 		 * If the first mbuf has no external data
 		 * move the data to the front of the mbuf.
 		 */
-		if ((dst = m1->m_dat) != src)
+		if (m1->m_flags & M_PKTHDR)
+			dst = m1->m_pktdat;
+		else
+			dst = m1->m_dat;
+		m1->m_data = dst;
+		if (dst != src)
 			memmove(dst, src, left);
 		dst += left; 
 		m1->m_len = left;
 		m2 = m1->m_next;
 	}
-	m1->m_flags &= ~M_PKTHDR;
-	*cp2 = m1->m_data = m1->m_dat;   /* data is at beginning of buffer */
+	*cp2 = m1->m_data;
 	*dposp = mtod(m1, caddr_t) + siz;
 	/*
 	 * Loop through mbufs pulling data up into first mbuf until
 	 * the first mbuf is full or there is no more data to
 	 * pullup.
 	 */
-	while ((len = (MLEN - m1->m_len)) != 0 && m2) {
+	while ((len = (&m1->m_dat[MLEN] - m1->m_data - m1->m_len)) != 0 && m2) {
 		if ((len = min(len, m2->m_len)) != 0)
 			memcpy(dst, m2->m_data, len);
 		m1->m_len += len;
@@ -1139,8 +1147,9 @@
 	/* Loop around adding mbufs */
 	while (siz > 0) {
 		MGET(m1, M_WAIT, MT_DATA);
-		if (siz > MLEN)
+		if (siz > MLEN) {
 			MCLGET(m1, M_WAIT);
+		}
 		m1->m_len = NFSMSIZ(m1);
 		m2->m_next = m1;
 		m2 = m1;


I believe there are more mbuf-leaks but which are not related to
the symptoms. I have added code to some error paths:

Index: nfs_vfsops.c
===================================================================
RCS file: /cvsroot/src/sys/nfs/nfs_vfsops.c,v
retrieving revision 1.112.10.2
diff -u -3 -r1.112.10.2 nfs_vfsops.c
--- nfs_vfsops.c	2002/07/29 15:00:57	1.112.10.2
+++ nfs_vfsops.c	2003/02/02 17:48:39
@@ -171,8 +171,11 @@
 	if (v3)
 		nfsm_postop_attr(vp, retattr);
 	if (error) {
-		if (mrep != NULL)
-			m_free(mrep);
+		if (mrep != NULL) {
+			if (mrep->m_next != NULL)
+				printf("nfs_vfsops:nfs_statfs would loose buffers\n");
+			m_freem(mrep); /* XXX-mlelstv not m_free() */
+		}
 		goto nfsmout;
 	}
 	nfsm_dissect(sfp, struct nfs_statfs *, NFSX_STATFS(v3));


Index: nfs_serv.c
===================================================================
RCS file: /cvsroot/src/sys/nfs/nfs_serv.c,v
retrieving revision 1.62.10.3
diff -u -3 -r1.62.10.3 nfs_serv.c
--- nfs_serv.c	2002/09/30 13:52:26	1.62.10.3
+++ nfs_serv.c	2003/02/02 17:47:28
@@ -544,8 +544,13 @@
 out:
 	getret = VOP_GETATTR(vp, &attr, cred, procp);
 	vput(vp);
-	if (error)
+	if (error) {
 		m_freem(mp3);
+		/* FIXME-mlelstv bail out after mp3 becomes invalid */
+		nfsm_reply(2 * NFSX_UNSIGNED);
+		nfsm_srvpostop_attr(1, (struct vattr *)0);
+		return (0);
+	}
 	nfsm_reply(NFSX_POSTOPATTR(v3) + NFSX_UNSIGNED);
 	if (v3) {
 		nfsm_srvpostop_attr(getret, &attr);
@@ -555,10 +560,12 @@
 	if (uiop->uio_resid > 0) {
 		len -= uiop->uio_resid;
 		tlen = nfsm_rndup(len);
+		/* FIXME-mlelstv mp3 might be invalid */
 		nfsm_adj(mp3, NFS_MAXPATHLEN-tlen, tlen-len);
 	}
 	nfsm_build(tl, u_int32_t *, NFSX_UNSIGNED);
 	*tl = txdr_unsigned(len);
+	/* FIXME-mlelstv mp3 might be invalid */
 	mb->m_next = mp3;
 	nfsm_srvdone;
 }

Someone else please have a look at the changes.

-- 
                                Michael van Elst
Internet: mlelstv@serpens.de
                                "A potential Snark may lurk in every tree."