m_move_pkthdr leaves m_nextpkt 'dangling'

[Andrey V. Elsukov]

On 05.07.2017 19:23, Adrian Chadd wrote:
>> As many of you know, when dealing with IP fragments the kernel will build a
>> list of packets (fragments) chained together through the m_nextpkt pointer.
>> This is all good until someone tries to do a M_PREPEND on one of the packet
>> in the chain and the M_PREPEND has to create an extra mbuf to prepend at the
>> beginning of the chain.
>>
>> When doing so m_move_pkthdr is called to copy the current PKTHDR fields
>> (tags and flags) to the mbuf that was prepended. The function also does:
>>
>> to->m_pkthdr = from->m_pkthdr;
>>
>> This, for the case I am interested in, essentially leaves the 'from' mbuf
>> with a dangling pointer m_nextpkt pointing to the next fragment. While this
>> is mostly harmless because only mbufs of pkthdr types are supposed to have
>> m_nextpkt it triggers some panics when running with INVARIANTS in NetGraph
>> (see ng_base.c :: CHECK_DATA_MBUF(m)):
>>
>> ...
>>                         if (n->m_nextpkt != NULL)                       \
>>                                 panic("%s: m_nextpkt", __func__);       \
>>                 }
>> ...
>>
>> So I would like to propose the following patch:
>>
>> @@ -442,10 +442,11 @@ m_move_pkthdr(struct mbuf *to, struct mbuf *from)
>>         if ((to->m_flags & M_EXT) == 0)
>>                 to->m_data = to->m_pktdat;
>>         to->m_pkthdr = from->m_pkthdr;          /* especially tags */
>>         SLIST_INIT(&from->m_pkthdr.tags);       /* purge tags from src */
>>         from->m_flags &= ~M_PKTHDR;
>> +       from->m_nextpkt = NULL;
>>  }
>>
>> It will reset the m_nextpkt so we don't have two mbufs pointing to the same
>> next packet. This is fairly harmless and solves a problem for us here at
>> XipLink.
> 
> This seems like a no-brainer. :-) Any objections?

I think the change is reasonable.
But from other side m_demote_pkthdr() may also need this change.
Maybe we can wait when Gleb will be back and review this? Also he is the
author of the mentioned assertion in netgraph code.

-- 
WBR, Andrey V. Elsukov

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 553 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/freebsd-net/attachments/20170707/07eba5f5/attachment.sig>