m_move_pkthdr leaves m_nextpkt 'dangling'

[Adrian Chadd]

On 16 October 2017 at 10:57, Gleb Smirnoff <glebius at freebsd.org> wrote:
>   Karim,
>
> On Mon, Oct 16, 2017 at 10:37:02AM -0400, Karim Fodil-Lemelin wrote:
> K> > Not only mbufs of M_PKTHDR may have m_nextpkt set. However, I tend to agree
> K> > with the patch. But shouldn't we first copy the m_nextpkt to the new mbuf:
> K> >
> K> > +  to->m_nextpkt = from->m_nextpkt;
> K> > +  from->m_nextpkt = NULL;
> K> >
> K> > Same way as we deal with tags.
> K> >
> K> >
> K>
> K> I think you are correct. If we look at the 'spirit' of m_move_pkthdr();
> K> In my mind, it is to deep copy all fields related to a packet header and
> K> since m_nextpkt should only be carried by packet headers, it makes sense
> K> to copy it within m_move_pkthdr().
> K>
> K> This also raises the question (my apologies in advance from bringing
> K> this up...) of weather or not m_nextpkt belongs in struct m_hdr and not
> K> in struct pkthdr.
> K>
> K> In our case we are copying it explicitly outside the function as most of
> K> users of m_move_pkthdr() do.
>
> Moving m_nextpkt from m_hdr to m_pkthdr would be much more intrusive
> change, we can't handle that.
>
> I think an mbuf with m_nextpkt and no M_PKTRHDR is a valid one. In
> a datagram socket buffer that could hold a record. (didn't check that,
> just guessing).
>
> So, any objections on commiting this addition to m_move_pkthdr?
>
> +  to->m_nextpkt = from->m_nextpkt;
> +  from->m_nextpkt = NULL;

None from me. (I haven't checked to see if you've done it yet or not.)



-adrian

> --
> Gleb Smirnoff