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.
_______________________________________________
freebsd-net@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
