kern/176144: Bug in m_split() when splitting M_EXT mbufs

[Jacques Fourie]

>Number:         176144
>Category:       kern
>Synopsis:       Bug in m_split() when splitting M_EXT mbufs
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Feb 14 13:00:00 UTC 2013
>Closed-Date:
>Last-Modified:
>Originator:     Jacques Fourie
>Release:        10-current
>Organization:
Netronome Systems
>Environment:
FreeBSD fbsd10vm 10.0-CURRENT FreeBSD 10.0-CURRENT #7 r+45b4c99-dirty: Mon Jan  7 09:40:08 SAST 2013     root at fbsd10vm:/usr/obj/usr/home/jfourie/freebsd.git/sys/FBSD10VM  amd64
>Description:
The following text is copied from my original mail to -hackers:

Could someone please verify if m_split as in svn rev 245286 is doing the
right thing in the scenario where a mbuf chain is split with len0 falling
on a mbuf boundary and the mbuf in question being a M_EXT mbuf? Consider
the following example where m0 is a mbuf chain consisting of 2 M_EXT mbufs,
both 1448 bytes in length. Let len0 be 1448. The 'len0 > m->m_len' check
will be false so the for loop will not be entered in this case. We now have
len = 1448 and remain = 0 and m still points to the first mbuf in the
chain. Also assume that m0 is a pkthdr mbuf. A new pkthdr mbuf n will be
allocated and initialized before the following piece of code is executed :

extpacket:
         if (m->m_flags & M_EXT) {
                n->m_data = m->m_data + len;
                mb_dupcl(n, m);
         } else {
                bcopy(mtod(m, caddr_t) + len, mtod(n, caddr_t), remain);
         } 
         n->m_len = remain;
         m->m_len = len;
         n->m_next = m->m_next;
         m->m_next = NULL;

As m is a M_EXT mbuf the code in the if() clause will be executed. The
problem is that m still points to the first mbuf so effectively the data
pointer for n is assigned to the end of m's data pointer. It should
actually point to the start of the data pointer of the next mbuf in the
original m0 chain, right?
>How-To-Repeat:

>Fix:
Attached patch fixes the issue for me

Patch attached with submission follows:

diff --git a/sys/kern/uipc_mbuf.c b/sys/kern/uipc_mbuf.c
index ab6163d..5c397fa 100644
--- a/sys/kern/uipc_mbuf.c
+++ b/sys/kern/uipc_mbuf.c
@@ -1132,6 +1132,23 @@ m_split(struct mbuf *m0, int len0, int wait)
 		return (NULL);
 	remain = m->m_len - len;
 	if (m0->m_flags & M_PKTHDR) {
+		if (remain == 0) {
+			if (m->m_next == NULL)
+				return (NULL);
+			if (!(m->m_next->m_flags & M_PKTHDR)) {
+				MGETHDR(n, wait, m0->m_type);
+				if (n == NULL)
+					return (NULL);
+				MH_ALIGN(n, 0);
+				n->m_next = m->m_next;
+			} else 
+				n = m->m_next;
+			m->m_next = NULL;
+			n->m_pkthdr.rcvif = m0->m_pkthdr.rcvif;
+			n->m_pkthdr.len = m0->m_pkthdr.len - len0;
+			m0->m_pkthdr.len = len0;
+			return (n);
+		}
 		MGETHDR(n, wait, m0->m_type);
 		if (n == NULL)
 			return (NULL);


>Release-Note:
>Audit-Trail:
>Unformatted: