Another fragment question / patch

[Karim Fodil-Lemelin]

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);

Cheers!

Karim.