Possible bug in m_split() when splitting M_EXT mbufs

[Jacques Fourie]

On Fri, Jan 11, 2013 at 10:12 AM, Gleb Smirnoff <glebius at freebsd.org> 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.
>