Date: Mon, 23 Mar 2015 09:40:33 -0400 From: Karim Fodil-Lemelin <fodillemlinkarim@gmail.com> To: freebsd-net@freebsd.org Subject: Re: Another fragment question / patch Message-ID: <551017D1.60008@gmail.com> In-Reply-To: <550D3675.5030900@selasky.org> References: <550C3A62.3080403@gmail.com> <550C5F6C.3080302@selasky.org> <550C7D0C.3090603@gmail.com> <550D3675.5030900@selasky.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?551017D1.60008>