Date: Mon, 16 Oct 2017 10:37:02 -0400 From: Karim Fodil-Lemelin <kfodil-lemelin@xiplink.com> To: Gleb Smirnoff <glebius@FreeBSD.org> Cc: Adrian Chadd <adrian.chadd@gmail.com>, FreeBSD Net <freebsd-net@freebsd.org>, "Andrey V. Elsukov" <bu7cher@yandex.ru> Subject: Re: m_move_pkthdr leaves m_nextpkt 'dangling' Message-ID: <59E4C40E.9060103@xiplink.com> In-Reply-To: <20171013211026.GB1055@FreeBSD.org> References: <59567148.1020902@xiplink.com> <CAJ-VmomhJVbZO-G1Ki2sg5Wxrn6xL-zYU1ggoEKS-qPGuocG2g@mail.gmail.com> <31535133-f95a-5db6-a04c-acc0175fa287@yandex.ru> <59DFD3CC.2000401@xiplink.com> <CAJ-Vmo=JhFwo%2B7FgsZUgQMwOSimcoS8zHL%2BAJFONKS-%2Btv7Eww@mail.gmail.com> <20171013211026.GB1055@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2017-10-13 5:10 PM, Gleb Smirnoff wrote:
> 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.
>
>
Hi Gleb,
I think you are correct. If we look at the 'spirit' of m_move_pkthdr();
In my mind, it is to deep copy all fields related to a packet header and
since m_nextpkt should only be carried by packet headers, it makes sense
to copy it within m_move_pkthdr().
This also raises the question (my apologies in advance from bringing
this up...) of weather or not m_nextpkt belongs in struct m_hdr and not
in struct pkthdr.
In our case we are copying it explicitly outside the function as most of
users of m_move_pkthdr() do.
Thanks for looking in to this.
Karim.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?59E4C40E.9060103>