m_move_pkthdr leaves m_nextpkt 'dangling'

[Adrian Chadd]

On 30 June 2017 at 08:42, Karim Fodil-Lemelin
<kfodil-lemelin at xiplink.com> wrote:
> Hi,
>
> 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?



-adrian