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.
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
