Another fragment question / patch

[Hans Petter Selasky]

On 03/20/15 21:03, Karim Fodil-Lemelin wrote:
> On 2015-03-20 1:57 PM, Hans Petter Selasky wrote:
>> On 03/20/15 16:18, Karim Fodil-Lemelin wrote:
>>> Hi,
>>>
>>> While reading through a previous comment on this list about fragments
>>> I've noticed that mbuf tags aren't being copied from the leading
>>> fragment (header) to the subsequent fragment packets. In other words,
>>> one would expect that all fragments of a packet are carrying the same
>>> tags that were set on the original packet. I have built a simple test
>>> were I use ipfw with ALTQ and sent large packet (bigger then MTU) off
>>> that BSD machine. I have observed that the leading fragment (m0) packet
>>> is going through the right class although the next fragments are hitting
>>> the default class for unclassified packets.
>>>
>>> Here is a patch that makes things works as expected (all fragments carry
>>> the ALTQ tag):
>>>
>>> diff --git a/freebsd/sys/netinet/ip_output.c
>>> b/freebsd/sys/netinet/ip_output.c
>>> index d650949..7d8f041 100644
>>> --- a/freebsd/sys/netinet/ip_output.c
>>> +++ b/freebsd/sys/netinet/ip_output.c
>>> @@ -1184,7 +1184,10 @@ smart_frag_failure:
>>>                          ipstat.ips_odropped++;
>>>                          goto done;
>>>                  }
>>> -               m->m_flags |= (m0->m_flags & M_MCAST) | M_FRAG;
>>> +
>>> +               m->m_flags |= (m0->m_flags & M_COPYFLAGS) | M_FRAG;
>>> +               m_tag_copy_chain(m, m0, M_NOWAIT);
>>> +
>>>                  /*
>>>                   * In the first mbuf, leave room for the link
>>> header, then
>>>                   * copy the original IP header including options. The
>>> payload
>>> diff --git a/freebsd/sys/sys/mbuf.h b/freebsd/sys/sys/mbuf.h
>>> index 2efff38..6ad8439 100644
>>> --- a/freebsd/sys/sys/mbuf.h
>>>
>>
>> Hi,
>>
>> I see your point about copying the tags. I'm not sure however that
>> M_COPYFLAGS is correct, because it also copies M_RDONLY, which is not
>> relevant for this case. Can you explain what flags need copying in
>> addition to M_MCAST ? Maybe we need to define these flags separately.
>>
>> Thank you!
>>
>> --HPS
> Hi,
>
> Arguably the M_RDONLY is added when m_copy() is called a bit later in
> that function. m_copym() does a shallow copy (through a call to
> mb_dupcl) and will set the RDONLY flag when doing so. So the fact it was
> copied over from M_COPYFLAGS shouldn't be a problem in terms of
> functionality. A similar logic applies to the M_VLANTAG since it should
> never be set in ip_output() (severe layer violation). I guess
> M_COPYFLAGS is kinda safe but not necessarily correct.
>
> In terms of appropriate behavior (whats the real purpose of
> M_COPYFLAGS?) my initial patch is debatable and to answer your question
> I would consider to copy the remaining flags:
>
> M_PKTHDR  => already in there through the m_gethdr() call
> M_BCAST => no need to copy
> M_MCAST  => already in there in ip_fragment()
> M_PROTOFLAGS
> M_SKIP_FIREWALL => for layer 2 fire-walling?
>
> So something like?
>
> -               m->m_flags |= (m0->m_flags & M_MCAST);
> +              m->m_flags |= (m0->m_flags & (M_MCAST | M_PROTOFLAGS));
> +              m_tag_copy_chain(m, m0, M_NOWAIT);
>

Hi,

Could you have a look at the attached patch, test and give some 
comments? I believe the right thing to do in this case is to use the 
copy packet header function.

--HPS

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ip_output.c.diff
Type: text/x-diff
Size: 1103 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-net/attachments/20150321/eacfed7c/attachment.diff>