Another fragment question / patch

[Karim Fodil-Lemelin]

On 2015-03-21 5:14 AM, Hans Petter Selasky wrote:
> 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
>
Hi,

Your patch seems to work as well as mine, albeit completely swinging the 
other way now since your copying a lot more from the packet header with 
the call to m_dup_pkthdr() (including the M_COPYFLAGS, vlan tags, etc..).

I think this is fine, one would expect IP fragments to be somewhat very 
close to the original packet.

Thanks,

Karim.