From owner-freebsd-hackers@FreeBSD.ORG Fri Jan 11 08:53:10 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 81B905B6; Fri, 11 Jan 2013 08:53:10 +0000 (UTC) (envelope-from jacques.fourie@gmail.com) Received: from mail-lb0-f171.google.com (mail-lb0-f171.google.com [209.85.217.171]) by mx1.freebsd.org (Postfix) with ESMTP id B2D03A58; Fri, 11 Jan 2013 08:53:09 +0000 (UTC) Received: by mail-lb0-f171.google.com with SMTP id gf7so1142942lbb.30 for ; Fri, 11 Jan 2013 00:53:07 -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=jlvuqVfAdEPXQYYS6/Ne6NJL16eCUle0vCwZM3EGSgc=; b=j1kUuG5OuVmqexoGkzKFUxM3rc0pbnG5nUaCMpJ8pSp5QK4TcPtHQNZzUB4/RdFTPj ZCJQdmcA5tuNxyn9q09CaOIw2x0RI7+IdPZTStfD/hNhxmGRWXTdRae3e6cDtQDh6egI +PSgQ4GmbUNfrOrwd4/IyOL50/wveNAJFgSBPOB5LQfOrL//E3oOHA9UJ2kczHG1dy9u YIunK68xYnQbKm8xXSOVdm9VrhwaTGt0BbVvtqu7Jv6mSc/fYRx07ujiVQ+9EuUq3rOW BRe8/jelUrL516k+l9LxTRF5aI4PIbLT/q5j29Xm08tV2lJCKO6AM4LijfWep/7HMfsh SuwA== MIME-Version: 1.0 Received: by 10.112.41.202 with SMTP id h10mr30573058lbl.20.1357894387073; Fri, 11 Jan 2013 00:53:07 -0800 (PST) Received: by 10.152.13.36 with HTTP; Fri, 11 Jan 2013 00:53:06 -0800 (PST) In-Reply-To: <20130111081254.GG82815@FreeBSD.org> References: <20130111081254.GG82815@FreeBSD.org> Date: Fri, 11 Jan 2013 10:53: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: Fri, 11 Jan 2013 08:53:10 -0000 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. >