[RFC PATCH] Fix false positives in can_checksum_protocol()
flat view
STALE4037d
From: David Woodhouse <dwmw2@infradead.org>
Date: 2015-09-25 12:56:00
Subsystem:
networking drivers, networking [general], the rest · Maintainers:
Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
The check in harmonize_features() is supposed to match the skb against the features of the device in question, and prevent us from handing a skb to a device which can't handle it. It doesn't work correctly. A device with NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM capabilities is only required to checksum TCP or UDP, on Legacy IP and IPv6 respectively. But the existing check will allow us to pass it *any* ETH_P_IP/ETH_P_IPV6 packets for hardware checksum offload. Depending on the driver in use, this leads to a BUG, a WARNING, or just silent data corruption. This is one approach to fixing that, and my test program at http://bombadil.infradead.org/~dwmw2/raw.c can no longer trivially reproduce the problem. The test does now have false *negatives*, but those shouldn't happen for locally-generated packets; only packets injected from af_packet, tun, virtio_net and other places that allow us to inject CHECKSUM_PARTIAL packets in order to make use of hardware offload features. And false negatives aren't anywhere near as much of a problem as false positives are — we just finish the checksum in software and send the packet anyway. It would be possible to fix those false negatives, if we really wanted to — perhaps by adding an additional bit in the skbuff which indicates that it *is* a TCP or UDP packet, rather than using ->sk->sk_protocol. Then that bit could be set if appropriate in skb_partial_csum_set(), as well as the places where we locally generate such packets. And the check in can_checksum_protocol() would just check for that bit. Signed-off-by: David Woodhouse <redacted> --- Since can_checksum_protocol is inline, the compiler ought to know that it doesn't even need to dereference skb->sk in the case where the device has the NETIF_F_GEN_CSUM feature. So the additional check should not slow down the (hopefully) common case in the fast path.
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2d15e38..76c8330 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h@@ -3628,15 +3628,23 @@ struct sk_buff *skb_gso_segment(struct sk_buff *skb, netdev_features_t features) __be16 skb_network_protocol(struct sk_buff *skb, int *depth); static inline bool can_checksum_protocol(netdev_features_t features, - __be16 protocol) -{ - return ((features & NETIF_F_GEN_CSUM) || - ((features & NETIF_F_V4_CSUM) && - protocol == htons(ETH_P_IP)) || - ((features & NETIF_F_V6_CSUM) && - protocol == htons(ETH_P_IPV6)) || - ((features & NETIF_F_FCOE_CRC) && - protocol == htons(ETH_P_FCOE))); + __be16 protocol, u8 sk_protocol) +{ + if ((features & NETIF_F_GEN_CSUM) || + ((features & NETIF_F_FCOE_CRC) && protocol == htons(ETH_P_FCOE))) + return 1; + + /* NETIF_F_V[46]_CSUM are defined to work only on TCP and UDP. + * That is, when it needs to start checksumming at the transport + * header, and place the result at an offset of either 6 (for UDP) + * or 16 (for TCP). + */ + if ((((features & NETIF_F_V4_CSUM) && protocol == htons(ETH_P_IP)) || + ((features & NETIF_F_V6_CSUM) && protocol == htons(ETH_P_IPV6))) && + (sk_protocol == IPPROTO_TCP || sk_protocol == IPPROTO_UDP)) + return 1; + + return 0; } #ifdef CONFIG_BUG
diff --git a/net/core/dev.c b/net/core/dev.c
index 6bb6470..3c40957 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c@@ -2613,7 +2613,8 @@ static netdev_features_t harmonize_features(struct sk_buff *skb, features = net_mpls_features(skb, features, type); if (skb->ip_summed != CHECKSUM_NONE && - !can_checksum_protocol(features, type)) { + !can_checksum_protocol(features, type, + skb->sk ? skb->sk->sk_protocol : 0)) { features &= ~NETIF_F_ALL_CSUM; } else if (illegal_highdma(skb->dev, skb)) { features &= ~NETIF_F_SG;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index dad4dd3..9126c6f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c@@ -3004,7 +3004,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, return ERR_PTR(-EINVAL); csum = !head_skb->encap_hdr_csum && - !!can_checksum_protocol(features, proto); + !!can_checksum_protocol(features, proto, + head_skb->sk ? head_skb->sk->sk_protocol : 0); headroom = skb_headroom(head_skb); pos = skb_headlen(head_skb);
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
Attachments
- smime.p7s [application/x-pkcs7-signature] 5691 bytes