Date: Fri, 13 Oct 2017 00:59:47 -0700 From: Adrian Chadd <adrian.chadd@gmail.com> To: Karim Fodil-Lemelin <kfodil-lemelin@xiplink.com>, Gleb Smirnoff <glebius@freebsd.org> Cc: "Andrey V. Elsukov" <bu7cher@yandex.ru>, FreeBSD Net <freebsd-net@freebsd.org> Subject: Re: m_move_pkthdr leaves m_nextpkt 'dangling' Message-ID: <CAJ-Vmo=JhFwo%2B7FgsZUgQMwOSimcoS8zHL%2BAJFONKS-%2Btv7Eww@mail.gmail.com> In-Reply-To: <59DFD3CC.2000401@xiplink.com> References: <59567148.1020902@xiplink.com> <CAJ-VmomhJVbZO-G1Ki2sg5Wxrn6xL-zYU1ggoEKS-qPGuocG2g@mail.gmail.com> <31535133-f95a-5db6-a04c-acc0175fa287@yandex.ru> <59DFD3CC.2000401@xiplink.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Gleb, what do you think?
-a
On 12 October 2017 at 13:42, Karim Fodil-Lemelin
<kfodil-lemelin@xiplink.com> wrote:
> On 2017-07-07 10:46 AM, Andrey V. Elsukov wrote:
>>
>> 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.
>>
> Hi,
>
> Any updates on this one?
>
> There is another interesting patch I would like to share. This is regarding
> the m_tag_free function pointer in the m_tag structure.
>
> As it turns out, we use this field (m_tag_free) to track some mbuf tag at
> work and, in order to properly do reference counting on it, we had to modify
> m_tag_copy() the following way in order to keep the m_tag_free function
> pointer to point to the same function the original tag was pointing to (the
> code is a lot easier to understand than the text ...).
>
>
> @@ -437,6 +437,7 @@ m_tag_copy(struct m_tag *t, int how)
> } else
> #endif
> bcopy(t + 1, p + 1, t->m_tag_len); /* Copy the data */
> + p->m_tag_free = t->m_tag_free; /* copy the 'free' function pointer */
> return p;
> }
>
> This is because m_tag_copy uses m_tag_alloc() that resets the m_tag_free
> pointer to m_tag_free_default. It would be nice if this could make its way
> into the mbuf tag base code.
>
> Best regards,
>
> Karim.
>
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-Vmo=JhFwo%2B7FgsZUgQMwOSimcoS8zHL%2BAJFONKS-%2Btv7Eww>