m_move_pkthdr leaves m_nextpkt 'dangling'

[Gleb Smirnoff]

On Fri, Oct 13, 2017 at 12:59:47AM -0700, Adrian Chadd wrote:
A> >>>> When doing so m_move_pkthdr is called to copy the current PKTHDR fields
A> >>>> (tags and flags) to the mbuf that was prepended. The function also does:
A> >>>>
A> >>>> to->m_pkthdr = from->m_pkthdr;
A> >>>>
A> >>>> This, for the case I am interested in, essentially leaves the 'from'
A> >>>> mbuf
A> >>>> with a dangling pointer m_nextpkt pointing to the next fragment. While
A> >>>> this
A> >>>> is mostly harmless because only mbufs of pkthdr types are supposed to
A> >>>> have
A> >>>> m_nextpkt it triggers some panics when running with INVARIANTS in
A> >>>> NetGraph
A> >>>> (see ng_base.c :: CHECK_DATA_MBUF(m)):
A> >>>>
A> >>>> ...
A> >>>>                          if (n->m_nextpkt != NULL)
A> >>>> \
A> >>>>                                  panic("%s: m_nextpkt", __func__);
A> >>>> \
A> >>>>                  }
A> >>>> ...
A> >>>>
A> >>>> So I would like to propose the following patch:
A> >>>>
A> >>>> @@ -442,10 +442,11 @@ m_move_pkthdr(struct mbuf *to, struct mbuf *from)
A> >>>>          if ((to->m_flags & M_EXT) == 0)
A> >>>>                  to->m_data = to->m_pktdat;
A> >>>>          to->m_pkthdr = from->m_pkthdr;          /* especially tags */
A> >>>>          SLIST_INIT(&from->m_pkthdr.tags);       /* purge tags from src
A> >>>> */
A> >>>>          from->m_flags &= ~M_PKTHDR;
A> >>>> +       from->m_nextpkt = NULL;
A> >>>>   }

Not only mbufs of M_PKTHDR may have m_nextpkt set. However, I tend to agree
with the patch. But shouldn't we first copy the m_nextpkt to the new mbuf:

+	to->m_nextpkt = from->m_nextpkt;
+	from->m_nextpkt = NULL;

Same way as we deal with tags.


-- 
Gleb Smirnoff