Date: Tue, 23 Oct 2012 22:11:23 GMT From: Sebastian Kuzminsky <seb@lineratesystems.com> To: freebsd-gnats-submit@FreeBSD.org Subject: misc/173004: Incorrect IP checksums when forwarding results in fragmentation Message-ID: <201210232211.q9NMBNhM096905@red.freebsd.org> Resent-Message-ID: <201210232220.q9NMK0IS033458@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 173004
>Category: misc
>Synopsis: Incorrect IP checksums when forwarding results in fragmentation
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Tue Oct 23 22:20:00 UTC 2012
>Closed-Date:
>Last-Modified:
>Originator: Sebastian Kuzminsky
>Release: svn_stable_7 + local mods
>Organization:
LineRate Systems
>Environment:
>Description:
I've run into a problem with IP header checksums when fragmenting to an e1000 (em) interface, and I've narrowed it down to a very simple test. The test setup is like this:
[computer A]---(network 1)---[computer B]---(network 2)---[computer C]
That gorgeous drawing shows computer A connected to computer B via network 1, and computer B connected to computer C via network 2. Computer B is set up to forward packets between networks 1 and 2. A can see B but not C. C can see B but not A. B forwards between A and C. Pretty simple.
One of B's NICs is a Broadcom, handled by the bce driver; this one works fine in all my testing.
B's other NIC is an Intel PRO/1000 handled by the em driver. This is the one giving me trouble.
The test disables PMTUD on all three hosts. It then sets the MTU of the bce and em interfaces to the unrealistically low value of 72 bytes, and tries to pass TCP packets back and forth using nc on computers A and C (with computer B acting as a gateway). This is to force the B gateway to fragment the TCP frames it forwards.
Receiving on the em and sending on the bce works just fine (as noted above). Small TCP frames that fit in the MTU, big TCP frames that get fragmented, no problems.
Receiving on the bce and sending on the em interface works fine for small TCP frames that don't need fragmentation, but when B has to fragment the IP packets before sending them out the em, the IP header checksums in the IP packets that appear on the em's wires are wrong. I came to this conclusion by packet capture and by watching the 'bad header checksums' counter of 'netstat -s -p ip', both running on the computer receiving the fragments.
Ok, those are all my observations, next comes thoughts about the cause & a proposed fix.
The root of the problem is two-fold:
1. ip_output.c:ip_fragment() does not clear the CSUM_IP flag in the mbuf when it does software IP checksum computation, so the mbuf still looks like it needs IP checksumming.
2. The em driver does not advertise IP checksum offloading, but still checks the CSUM_IP flag in the mbuf and modifies the packet when that flag is set (this is in em_transmit_checksum_setup(), called by em_xmit()). Unfortunately the em driver gets the checksum wrong in this case, i guess that's why it doesn't advertise this capability in its if_hwassist!
So the fragments that ip_fastfwd.c:ip_fastforward() gets from ip_output.c:ip_fragment() have ip->ip_sum set correctly, but the mbuf->m_pkthdr.csum_flags incorrectly has CSUM_IP still set, and this causes the em driver to emit incorrect packets.
There are some other callers of ip_fragment(), notably ip_output(). ip_output() clears CSUM_IP in the mbuf csum_flags itself if it's not in if_hwassist, so avoids this problem.
So, the fix is simple: clear the mbuf's CSUM_IP when computing ip->ip_sum in ip_fragment(). The first attached patch (against gitorious/svn_stable_7) does this.
In looking at this issue, I noticed that ip_output()'s use of sw_csum is inconsistent. ip_output() splits the mbuf's csum_flags into two parts: the stuff that hardware will assist with (these flags get left in the mbuf) and the stuff that software needs to do (these get moved to sw_csum). But later ip_output() calls functions that don't get sw_csum, or that don't know to look in it and look in the mbuf instead. My second patch fixes these kinds of issues and (IMO) simplifies the code by leaving all the packet's checksumming needs in the mbuf, getting rid of sw_csum entirely.
>How-To-Repeat:
>Fix:
I've attached a shar file containing two patches, as described in the Full Description above. There's also a git branch on gitorious that contains these patches:
https://gitorious.org/freebsd/sebkuzminskys-freebsd/graph/frag-fix-7
Patch attached with submission follows:
# This is a shell archive. Save it in a file, remove anything before
# this line, and then unpack it by entering "sh file". Note, it may
# create directories; files and directories will be owned by you and
# have default permissions.
#
# This archive contains:
#
# 0001-Update-the-mbuf-csum_flags-of-IP-fragments-when-comp.patch.txt
# 0002-Simplify-the-tracking-of-mbuf-checksumming-needs.patch.txt
#
echo x - 0001-Update-the-mbuf-csum_flags-of-IP-fragments-when-comp.patch.txt
sed 's/^X//' >0001-Update-the-mbuf-csum_flags-of-IP-fragments-when-comp.patch.txt << 'END-of-0001-Update-the-mbuf-csum_flags-of-IP-fragments-when-comp.patch.txt'
XFrom c04a7a95890ef5d032e6998675496bb438c3a14b Mon Sep 17 00:00:00 2001
XFrom: Sebastian Kuzminsky <seb@lineratesystems.com>
XDate: Mon, 22 Oct 2012 21:08:40 -0600
XSubject: [PATCH 1/2] Update the mbuf csum_flags of IP fragments when
X computing their IP checksum
X
XBefore this commit, the ip_fragment() function does not clear the
Xmbuf CSUM_IP flag ("this mbuf needs an IP header checksum"), even
Xwhen it computes the IP header checksum itself. This behavior is
Xacceptable when ip_fragment() is called from ip_output(), because
Xip_output() clears the mbuf's flag. But it is not acceptable when
Xip_fragment() is called from ip_fastforward(), because ip_fastforward()
Xdoes not clear the mbuf's flag.
X
XThe result is that, when forwarding a packet that needs fragmentation,
Xand the fragments are sent by a NIC that does not advertise hardware
XIP checksum offloading, and that NIC *does* check for for the CSUM_IP
Xflag anyway and then gets the IP checksum wrong, *then* the fragments
Xgoing out on the wire would have the wrong checksum.
X
XThe em driver does not advertise IP header checksum offloading, but
Xdoes try to set up IP header checksum offloading anyway when the
Xmbuf is marked CSUM_IP, and it gets the IP checksum wrong.
X
XThe fix is to clear the CSUM_IP flag in the mbuf in ip_fragment()
Xwhen the IP checksum is computed, to let the lower layers know that
Xthey don't need to do it.
X---
X sys/netinet/ip_fastfwd.c | 9 ++++++++-
X sys/netinet/ip_output.c | 30 ++++++++++++++++++++++++++++--
X 2 files changed, 36 insertions(+), 3 deletions(-)
X
Xdiff --git a/sys/netinet/ip_fastfwd.c b/sys/netinet/ip_fastfwd.c
Xindex e84695e..ae65bfe 100644
X--- a/sys/netinet/ip_fastfwd.c
X+++ b/sys/netinet/ip_fastfwd.c
X@@ -558,12 +558,19 @@ passout:
X goto consumed;
X } else {
X /*
X- * We have to fragment the packet
X+ * We have to fragment this packet, and the fragments
X+ * will need all-new IP checksums. (The payload
X+ * checksums, if any, don't need to be modified
X+ * because the payload will be reassembled before
X+ * delivery.)
X */
X m->m_pkthdr.csum_flags |= CSUM_IP;
X /*
X * ip_fragment expects ip_len and ip_off in host byte
X * order but returns all packets in network byte order
X+ * If if_hwassist doesn't advertise IP checksum
X+ * offloading, ask ip_fragment to do it for us in
X+ * software now.
X */
X if (ip_fragment(ip, &m, mtu, ifp->if_hwassist,
X (~ifp->if_hwassist & CSUM_DELAY_IP))) {
Xdiff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
Xindex adbb074..08c4185 100644
X--- a/sys/netinet/ip_output.c
X+++ b/sys/netinet/ip_output.c
X@@ -506,12 +506,30 @@ passout:
X }
X }
X
X+ /* Annotate the outgoing packet: it needs its IP header checksummed. */
X m->m_pkthdr.csum_flags |= CSUM_IP;
X+
X+ /* sw_csum is everything the packet needs that *won't* be done in
X+ * hardware.
X+ */
X sw_csum = m->m_pkthdr.csum_flags & ~ifp->if_hwassist;
X+
X+ /* Do payload checksumming in software, now, if needed & wanted. */
X if (sw_csum & CSUM_DELAY_DATA) {
X in_delayed_cksum(m);
X sw_csum &= ~CSUM_DELAY_DATA;
X }
X+
X+ /* Clear all the packet's needs that'll be done by software.
X+ * At this point the packet's needs are (m_pkthdr.csum_flags | sw_csum),
X+ * and software should do the stuff in sw_csum.
X+ *
X+ * FIXME: This is a bug, stuff in the code paths after this
X+ * (for example ip_fragment) expect m_pkthdr->csum_flags to be the
X+ * list of stuff the packet needs. in_delayed_cksum() above also
X+ * has this expectation, which is why this code is convoluted to
X+ * call it before clearing m's csum_flags.
X+ */
X m->m_pkthdr.csum_flags &= ifp->if_hwassist;
X
X /*
X@@ -526,6 +544,10 @@ passout:
X ip->ip_sum = 0;
X if (sw_csum & CSUM_DELAY_IP)
X ip->ip_sum = in_cksum(m, hlen);
X+ /* Normally we'd clear CSUM_DELAY_IP out of sw_csum
X+ * here, but that variable is not used again before
X+ * it passes out of scope.
X+ */
X
X /*
X * Record statistics for this interface address.
X@@ -743,8 +765,10 @@ smart_frag_failure:
X m->m_pkthdr.csum_flags = m0->m_pkthdr.csum_flags;
X mhip->ip_off = htons(mhip->ip_off);
X mhip->ip_sum = 0;
X- if (sw_csum & CSUM_DELAY_IP)
X+ if (sw_csum & CSUM_DELAY_IP) {
X mhip->ip_sum = in_cksum(m, mhlen);
X+ m->m_pkthdr.csum_flags &= ~CSUM_DELAY_IP;
X+ }
X *mnext = m;
X mnext = &m->m_nextpkt;
X }
X@@ -764,8 +788,10 @@ smart_frag_failure:
X ip->ip_off |= IP_MF;
X ip->ip_off = htons(ip->ip_off);
X ip->ip_sum = 0;
X- if (sw_csum & CSUM_DELAY_IP)
X+ if (sw_csum & CSUM_DELAY_IP) {
X ip->ip_sum = in_cksum(m0, hlen);
X+ m0->m_pkthdr.csum_flags &= ~CSUM_DELAY_IP;
X+ }
X
X done:
X *m_frag = m0;
X--
X1.7.8.3
X
END-of-0001-Update-the-mbuf-csum_flags-of-IP-fragments-when-comp.patch.txt
echo x - 0002-Simplify-the-tracking-of-mbuf-checksumming-needs.patch.txt
sed 's/^X//' >0002-Simplify-the-tracking-of-mbuf-checksumming-needs.patch.txt << 'END-of-0002-Simplify-the-tracking-of-mbuf-checksumming-needs.patch.txt'
XFrom ebbbd7ad64a1cafd9a4b1182ede182fbc373b529 Mon Sep 17 00:00:00 2001
XFrom: Sebastian Kuzminsky <seb@lineratesystems.com>
XDate: Tue, 23 Oct 2012 10:59:20 -0600
XSubject: [PATCH 2/2] Simplify the tracking of mbuf checksumming needs
X
XThe IP code tracks outgoing packets' checksumming needs inconsistently.
XThe sw_csum variable complicates but does not add value.
X
XThe sw_csum variable is not needed. This commit removes it. The
Xmbuf's m_pkthdr->csum_flags are now the one record of what the
Xpacket needs.
X---
X sys/contrib/pf/net/pf.c | 14 +++++---------
X sys/net/if_bridge.c | 8 ++++++--
X sys/netinet/ip_fastfwd.c | 3 +--
X sys/netinet/ip_mroute.c | 3 ++-
X sys/netinet/ip_output.c | 40 ++++++++++------------------------------
X sys/netinet/ip_var.h | 2 +-
X 6 files changed, 25 insertions(+), 45 deletions(-)
X
Xdiff --git a/sys/contrib/pf/net/pf.c b/sys/contrib/pf/net/pf.c
Xindex 20e925b..0cdd217 100644
X--- a/sys/contrib/pf/net/pf.c
X+++ b/sys/contrib/pf/net/pf.c
X@@ -6253,9 +6253,6 @@ pf_route(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
X struct pf_addr naddr;
X struct pf_src_node *sn = NULL;
X int error = 0;
X-#ifdef __FreeBSD__
X- int sw_csum;
X-#endif
X #ifdef IPSEC
X struct m_tag *mtag;
X #endif /* IPSEC */
X@@ -6361,8 +6358,7 @@ pf_route(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
X #ifdef __FreeBSD__
X /* Copied from FreeBSD 5.1-CURRENT ip_output. */
X m0->m_pkthdr.csum_flags |= CSUM_IP;
X- sw_csum = m0->m_pkthdr.csum_flags & ~ifp->if_hwassist;
X- if (sw_csum & CSUM_DELAY_DATA) {
X+ if (m0->m_pkthdr.csum_flags & CSUM_DELAY_DATA & ~ifp->if_hwassist) {
X /*
X * XXX: in_delayed_cksum assumes HBO for ip->ip_len (at least)
X */
X@@ -6371,9 +6367,8 @@ pf_route(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
X in_delayed_cksum(m0);
X HTONS(ip->ip_len);
X HTONS(ip->ip_off);
X- sw_csum &= ~CSUM_DELAY_DATA;
X+ m0->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA;
X }
X- m0->m_pkthdr.csum_flags &= ifp->if_hwassist;
X
X if (ntohs(ip->ip_len) <= ifp->if_mtu ||
X (m0->m_pkthdr.csum_flags & ifp->if_hwassist & CSUM_TSO) != 0 ||
X@@ -6384,7 +6379,7 @@ pf_route(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
X * ip->ip_off = htons(ip->ip_off);
X */
X ip->ip_sum = 0;
X- if (sw_csum & CSUM_DELAY_IP) {
X+ if (m0->m_pkthdr.csum_flags & CSUM_DELAY_IP & ~ifp->if_hwassist) {
X /* From KAME */
X if (ip->ip_v == IPVERSION &&
X (ip->ip_hl << 2) == sizeof(*ip)) {
X@@ -6392,6 +6387,7 @@ pf_route(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
X } else {
X ip->ip_sum = in_cksum(m0, ip->ip_hl << 2);
X }
X+ m0->m_pkthdr.csum_flags &= ~CSUM_DELAY_IP;
X }
X PF_UNLOCK();
X error = (*ifp->if_output)(ifp, m0, sintosa(dst), ro->ro_rt);
X@@ -6478,7 +6474,7 @@ pf_route(struct mbuf **m, struct pf_rule *r, int dir, struct ifnet *oifp,
X */
X NTOHS(ip->ip_len);
X NTOHS(ip->ip_off);
X- error = ip_fragment(ip, &m0, ifp->if_mtu, ifp->if_hwassist, sw_csum);
X+ error = ip_fragment(ip, &m0, ifp->if_mtu, ifp->if_hwassist);
X #else
X error = ip_fragment(m0, ifp, ifp->if_mtu);
X #endif
Xdiff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c
Xindex 4d1b9da..a2a2b0d 100644
X--- a/sys/net/if_bridge.c
X+++ b/sys/net/if_bridge.c
X@@ -3347,8 +3347,12 @@ bridge_fragment(struct ifnet *ifp, struct mbuf *m, struct ether_header *eh,
X goto out;
X ip = mtod(m, struct ip *);
X
X- error = ip_fragment(ip, &m, ifp->if_mtu, ifp->if_hwassist,
X- CSUM_DELAY_IP);
X+ /* We're going to fragment the IP packet, the fragments will need
X+ * new IP checksums.
X+ */
X+ m->m_pkthdr.csum_flags |= CSUM_DELAY_IP;
X+
X+ error = ip_fragment(ip, &m, ifp->if_mtu, ifp->if_hwassist);
X if (error)
X goto out;
X
Xdiff --git a/sys/netinet/ip_fastfwd.c b/sys/netinet/ip_fastfwd.c
Xindex ae65bfe..852ea8e 100644
X--- a/sys/netinet/ip_fastfwd.c
X+++ b/sys/netinet/ip_fastfwd.c
X@@ -572,8 +572,7 @@ passout:
X * offloading, ask ip_fragment to do it for us in
X * software now.
X */
X- if (ip_fragment(ip, &m, mtu, ifp->if_hwassist,
X- (~ifp->if_hwassist & CSUM_DELAY_IP))) {
X+ if (ip_fragment(ip, &m, mtu, ifp->if_hwassist)) {
X goto drop;
X }
X KASSERT(m != NULL, ("null mbuf and no error"));
Xdiff --git a/sys/netinet/ip_mroute.c b/sys/netinet/ip_mroute.c
Xindex d60e8bd..dc5b0af 100644
X--- a/sys/netinet/ip_mroute.c
X+++ b/sys/netinet/ip_mroute.c
X@@ -2630,7 +2630,8 @@ pim_register_prepare(struct ip *ip, struct mbuf *m)
X ip->ip_sum = in_cksum(mb_copy, ip->ip_hl << 2);
X } else {
X /* Fragment the packet */
X- if (ip_fragment(ip, &mb_copy, mtu, 0, CSUM_DELAY_IP) != 0) {
X+ mb_copy->m_pkthdr.csum_flags |= CSUM_DELAY_IP;
X+ if (ip_fragment(ip, &mb_copy, mtu, 0) != 0) {
X m_freem(mb_copy);
X return NULL;
X }
Xdiff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
Xindex 08c4185..c6f2d2d 100644
X--- a/sys/netinet/ip_output.c
X+++ b/sys/netinet/ip_output.c
X@@ -112,7 +112,7 @@ ip_output(struct mbuf *m, struct mbuf *opt, struct route *ro, int flags,
X int len, error = 0;
X struct sockaddr_in *dst = NULL; /* keep compiler happy */
X struct in_ifaddr *ia = NULL;
X- int isbroadcast, sw_csum;
X+ int isbroadcast;
X struct route iproute;
X struct in_addr odst;
X #ifdef IPFIREWALL_FORWARD
X@@ -509,29 +509,12 @@ passout:
X /* Annotate the outgoing packet: it needs its IP header checksummed. */
X m->m_pkthdr.csum_flags |= CSUM_IP;
X
X- /* sw_csum is everything the packet needs that *won't* be done in
X- * hardware.
X- */
X- sw_csum = m->m_pkthdr.csum_flags & ~ifp->if_hwassist;
X-
X /* Do payload checksumming in software, now, if needed & wanted. */
X- if (sw_csum & CSUM_DELAY_DATA) {
X+ if (m->m_pkthdr.csum_flags & CSUM_DELAY_DATA & ~ifp->if_hwassist) {
X in_delayed_cksum(m);
X- sw_csum &= ~CSUM_DELAY_DATA;
X+ m->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA;
X }
X
X- /* Clear all the packet's needs that'll be done by software.
X- * At this point the packet's needs are (m_pkthdr.csum_flags | sw_csum),
X- * and software should do the stuff in sw_csum.
X- *
X- * FIXME: This is a bug, stuff in the code paths after this
X- * (for example ip_fragment) expect m_pkthdr->csum_flags to be the
X- * list of stuff the packet needs. in_delayed_cksum() above also
X- * has this expectation, which is why this code is convoluted to
X- * call it before clearing m's csum_flags.
X- */
X- m->m_pkthdr.csum_flags &= ifp->if_hwassist;
X-
X /*
X * If small enough for interface, or the interface will take
X * care of the fragmentation for us, we can just send directly.
X@@ -542,12 +525,10 @@ passout:
X ip->ip_len = htons(ip->ip_len);
X ip->ip_off = htons(ip->ip_off);
X ip->ip_sum = 0;
X- if (sw_csum & CSUM_DELAY_IP)
X+ if ((m->m_pkthdr.csum_flags & CSUM_DELAY_IP) & ~ifp->if_hwassist) {
X ip->ip_sum = in_cksum(m, hlen);
X- /* Normally we'd clear CSUM_DELAY_IP out of sw_csum
X- * here, but that variable is not used again before
X- * it passes out of scope.
X- */
X+ m->m_pkthdr.csum_flags &= ~CSUM_DELAY_IP;
X+ }
X
X /*
X * Record statistics for this interface address.
X@@ -589,7 +570,7 @@ passout:
X * Too large for interface; fragment if possible. If successful,
X * on return, m will point to a list of packets to be sent.
X */
X- error = ip_fragment(ip, &m, mtu, ifp->if_hwassist, sw_csum);
X+ error = ip_fragment(ip, &m, mtu, ifp->if_hwassist);
X if (error)
X goto bad;
X for (; m; m = m0) {
X@@ -633,11 +614,10 @@ bad:
X * chain of fragments that should be freed by the caller.
X *
X * if_hwassist_flags is the hw offload capabilities (see if_data.ifi_hwassist)
X- * sw_csum contains the delayed checksums flags (e.g., CSUM_DELAY_IP).
X */
X int
X ip_fragment(struct ip *ip, struct mbuf **m_frag, int mtu,
X- u_long if_hwassist_flags, int sw_csum)
X+ u_long if_hwassist_flags)
X {
X int error = 0;
X int hlen = ip->ip_hl << 2;
X@@ -765,7 +745,7 @@ smart_frag_failure:
X m->m_pkthdr.csum_flags = m0->m_pkthdr.csum_flags;
X mhip->ip_off = htons(mhip->ip_off);
X mhip->ip_sum = 0;
X- if (sw_csum & CSUM_DELAY_IP) {
X+ if (m->m_pkthdr.csum_flags & CSUM_DELAY_IP & ~if_hwassist_flags) {
X mhip->ip_sum = in_cksum(m, mhlen);
X m->m_pkthdr.csum_flags &= ~CSUM_DELAY_IP;
X }
X@@ -788,7 +768,7 @@ smart_frag_failure:
X ip->ip_off |= IP_MF;
X ip->ip_off = htons(ip->ip_off);
X ip->ip_sum = 0;
X- if (sw_csum & CSUM_DELAY_IP) {
X+ if (m0->m_pkthdr.csum_flags & CSUM_DELAY_IP & ~if_hwassist_flags) {
X ip->ip_sum = in_cksum(m0, hlen);
X m0->m_pkthdr.csum_flags &= ~CSUM_DELAY_IP;
X }
Xdiff --git a/sys/netinet/ip_var.h b/sys/netinet/ip_var.h
Xindex 19e9b7e..8cbc74d 100644
X--- a/sys/netinet/ip_var.h
X+++ b/sys/netinet/ip_var.h
X@@ -195,7 +195,7 @@ int ip_ctloutput(struct socket *, struct sockopt *sopt);
X void ip_drain(void);
X void ip_fini(void *xtp);
X int ip_fragment(struct ip *ip, struct mbuf **m_frag, int mtu,
X- u_long if_hwassist_flags, int sw_csum);
X+ u_long if_hwassist_flags);
X void ip_forward(struct mbuf *m, int srcrt);
X void ip_init(void);
X extern int
X--
X1.7.8.3
X
END-of-0002-Simplify-the-tracking-of-mbuf-checksumming-needs.patch.txt
exit
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201210232211.q9NMBNhM096905>