From owner-freebsd-hackers@FreeBSD.ORG Mon Jan 14 08:09:08 2013 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 4D9118EB; Mon, 14 Jan 2013 08:09:08 +0000 (UTC) (envelope-from jacques.fourie@gmail.com) Received: from mail-lb0-f173.google.com (mail-lb0-f173.google.com [209.85.217.173]) by mx1.freebsd.org (Postfix) with ESMTP id A559FAEF; Mon, 14 Jan 2013 08:09:07 +0000 (UTC) Received: by mail-lb0-f173.google.com with SMTP id c1so2744927lbg.32 for ; Mon, 14 Jan 2013 00:09:06 -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=N98fiZL8T2dU3S9sHGC2zPwv69F30MvheBzxKFks+7I=; b=f53blvDS9OVhjKh48S3/FWpqEhoK+3wvZ2YJ4DbiM37w7uDTbsOjQDdK1I3X+KKAQJ 14VbJWz2DcDyD+89+qF7/DdNmxdg4GIif6J0IR0vzy8fYHz/vE76cFaQo3U4dnl1U1Au LLqmNc3lLjRWKARgYTmVjkgmb1as8sLsLgj+OroxOZdb4hPFiIUTw9UBy9fxOzMTvfbU QIhsn8vrkzWWq55cUkWB/t9D27mTJM4jm8hM6gYl7dpLo1dfVTRgRU0nduNzg5zlCrhA 039Xnpt+wQZaZ7P3da43+ynjmfAZrASwSvDI3eekhf13+OMVCFZw2mAxuLsycuUvOjjf JERQ== MIME-Version: 1.0 Received: by 10.152.114.42 with SMTP id jd10mr19146917lab.31.1358150946331; Mon, 14 Jan 2013 00:09:06 -0800 (PST) Received: by 10.152.13.36 with HTTP; Mon, 14 Jan 2013 00:09:06 -0800 (PST) In-Reply-To: References: <20130111081254.GG82815@FreeBSD.org> Date: Mon, 14 Jan 2013 10:09:06 +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: Mon, 14 Jan 2013 08:09:08 -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 tested your proposed patch and can confirm that it doesn't work. The issue is probably what I mentioned above - the wrong mbuf is now treated as the last part of the head portion of the original mbuf. Regards, Jacques