Re: m_move_pkthdr leaves m_nextpkt 'dangling'
On 16 October 2017 at 10:57, Gleb Smirnoff 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 ___ 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"
Re: m_move_pkthdr leaves m_nextpkt 'dangling'
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; -- Gleb Smirnoff ___ 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"
Re: m_move_pkthdr leaves m_nextpkt 'dangling'
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.
___
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"
Re: m_move_pkthdr leaves m_nextpkt 'dangling'
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
___
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"
Re: m_move_pkthdr leaves m_nextpkt 'dangling'
Gleb, what do you think?
-a
On 12 October 2017 at 13:42, Karim Fodil-Lemelin
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.
>
>
___
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"
Re: m_move_pkthdr leaves m_nextpkt 'dangling'
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"
Re: m_move_pkthdr leaves m_nextpkt 'dangling'
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.
--
WBR, Andrey V. Elsukov
signature.asc
Description: OpenPGP digital signature
Re: m_move_pkthdr leaves m_nextpkt 'dangling'
On 30 June 2017 at 08:42, Karim Fodil-Lemelin
wrote:
> Hi,
>
> 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?
-adrian
___
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"
