Date: Sat, 21 Mar 2015 10:14:29 +0100 From: Hans Petter Selasky <hps@selasky.org> To: Karim Fodil-Lemelin <fodillemlinkarim@gmail.com>, freebsd-net@freebsd.org Subject: Re: Another fragment question / patch Message-ID: <550D3675.5030900@selasky.org> In-Reply-To: <550C7D0C.3090603@gmail.com> References: <550C3A62.3080403@gmail.com> <550C5F6C.3080302@selasky.org> <550C7D0C.3090603@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------030400090601090601060304
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
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
--------------030400090601090601060304
Content-Type: text/x-diff;
name="ip_output.c.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="ip_output.c.diff"
Index: ip_output.c
===================================================================
--- ip_output.c (revision 279890)
+++ ip_output.c (working copy)
@@ -787,11 +787,13 @@
IPSTAT_INC(ips_odropped);
goto done;
}
- /* make sure the flowid is the same for the fragmented mbufs */
- M_HASHTYPE_SET(m, M_HASHTYPE_GET(m0));
- m->m_pkthdr.flowid = m0->m_pkthdr.flowid;
- /* copy multicast flag, if any */
- m->m_flags |= (m0->m_flags & M_MCAST);
+ /* make sure the packet header gets copied */
+ if (m_dup_pkthdr(m, m0, M_NOWAIT) == 0) {
+ m_free(m);
+ error = ENOBUFS;
+ IPSTAT_INC(ips_odropped);
+ goto done;
+ }
/*
* In the first mbuf, leave room for the link header, then
* copy the original IP header including options. The payload
@@ -821,11 +823,9 @@
goto done;
}
m->m_pkthdr.len = mhlen + len;
- m->m_pkthdr.rcvif = NULL;
#ifdef MAC
mac_netinet_fragment(m0, m);
#endif
- m->m_pkthdr.csum_flags = m0->m_pkthdr.csum_flags;
mhip->ip_off = htons(mhip->ip_off);
mhip->ip_sum = 0;
if (m->m_pkthdr.csum_flags & CSUM_IP & ~if_hwassist_flags) {
--------------030400090601090601060304--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?550D3675.5030900>