From owner-freebsd-hackers@FreeBSD.ORG Fri Jan 11 09:09:44 2013 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 322D29C9; Fri, 11 Jan 2013 09:09:44 +0000 (UTC) (envelope-from jacques.fourie@gmail.com) Received: from mail-la0-f49.google.com (mail-la0-f49.google.com [209.85.215.49]) by mx1.freebsd.org (Postfix) with ESMTP id 858A0AE9; Fri, 11 Jan 2013 09:09:43 +0000 (UTC) Received: by mail-la0-f49.google.com with SMTP id fk20so1505494lab.8 for ; Fri, 11 Jan 2013 01:09:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=ioos5t51dGxkZhVTwJh9K3m2ahD43YsIr8F4Zwa9ITE=; b=Tb5pYmulNnXy0JANWfnzdAKWim7u2VJyqR/tLJSNiHmSRTL0Y4IQZ/+lPH4ADA+B+N Q3SMrs+47a6wft7DP8+OIbgGFQxPijBFl/P2e5+Y9bXqT66FuTCo+ndxIlcWuHroZxT9 +varw8Fy9wcSczFII+nrNZycXbQXwL1fIzNRiJvHRxrgmu3iLchyH+TkipCz/7NIMMLT YaufWJju0mFBpRFN1v2jXv0YrJrrr888VgLvnyvPdf3nTRCKgcZbmQprgM4fXLsK4mhY kDYboIHBkc0wvPkYEekjEsxHNux/7UrY444cInJOt0roVNRaGxA+KD9B+iAl9FaQU0S9 NvUQ== MIME-Version: 1.0 Received: by 10.152.114.42 with SMTP id jd10mr9995020lab.31.1357894972373; Fri, 11 Jan 2013 01:02:52 -0800 (PST) Received: by 10.152.13.36 with HTTP; Fri, 11 Jan 2013 01:02:52 -0800 (PST) In-Reply-To: References: <20130111081254.GG82815@FreeBSD.org> Date: Fri, 11 Jan 2013 11:02:52 +0200 Message-ID: Subject: Re: Possible bug in m_split() when splitting M_EXT mbufs From: Jacques Fourie To: Gleb Smirnoff Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.14 Cc: Hackers freeBSD X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Jan 2013 09:09:44 -0000 On Fri, Jan 11, 2013 at 10:53 AM, Jacques Fourie wrote: > > > > On Fri, Jan 11, 2013 at 10:12 AM, Gleb Smirnoff wrote: > >> Jacques, >> >> On Fri, Jan 11, 2013 at 09:34:32AM +0200, Jacques Fourie wrote: >> J> Could someone please verify if m_split as in svn rev 245286 is doing >> the >> J> right thing in the scenario where a mbuf chain is split with len0 >> falling >> J> on a mbuf boundary and the mbuf in question being a M_EXT mbuf? >> Consider >> J> the following example where m0 is a mbuf chain consisting of 2 M_EXT >> mbufs, >> J> both 1448 bytes in length. Let len0 be 1448. The 'len0 > m->m_len' >> check >> J> will be false so the for loop will not be entered in this case. We now >> have >> J> len = 1448 and remain = 0 and m still points to the first mbuf in the >> J> chain. Also assume that m0 is a pkthdr mbuf. A new pkthdr mbuf n will >> be >> J> allocated and initialized before the following piece of code is >> executed : >> J> >> J> extpacket: >> J> if (m->m_flags & M_EXT) { >> J> n->m_data = m->m_data + len; >> J> mb_dupcl(n, m); >> J> } else { >> J> bcopy(mtod(m, caddr_t) + len, mtod(n, caddr_t), >> remain); >> J> } >> J> n->m_len = remain; >> J> m->m_len = len; >> J> n->m_next = m->m_next; >> J> m->m_next = NULL; >> J> return (n); >> J> >> J> As m is a M_EXT mbuf the code in the if() clause will be executed. The >> J> problem is that m still points to the first mbuf so effectively the >> data >> J> pointer for n is assigned to the end of m's data pointer. It should >> J> actually point to the start of the data pointer of the next mbuf in the >> J> original m0 chain, right? >> >> Thanks for analysis, Jacques. >> >> IMHO, the following patch should suffice and won't break anything: >> >> Index: uipc_mbuf.c >> =================================================================== >> --- uipc_mbuf.c (revision 245223) >> +++ uipc_mbuf.c (working copy) >> @@ -1126,7 +1126,7 @@ >> u_int len = len0, remain; >> >> MBUF_CHECKSLEEP(wait); >> - for (m = m0; m && len > m->m_len; m = m->m_next) >> + for (m = m0; m && len >= m->m_len; m = m->m_next) >> len -= m->m_len; >> if (m == NULL) >> return (NULL); >> >> Can you please test it? > > > I think that your patch may cause other issues - m now points to the first > mbuf in the tail portion. The final piece of code under the extpacket: > label will then set m->m_len to 0 for example. > >> > > >> -- >> Totus tuus, Glebius. >> > > I have been using the following patch that seems to fix the issue for me : 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); + }