Possible bug in m_split() when splitting M_EXT mbufs

[Jacques Fourie]

On Fri, Jan 11, 2013 at 10:53 AM, Jacques Fourie
<jacques.fourie at gmail.com>wrote:

>
>
>
> 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.
>>
>
> 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);
+               }