[RFC PATCH 1/3] Avoid making inappropriate requests of NETIF_F_V[46]_CSUM devices

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3328 bytes --]

Devices with the NETIF_F_V[46]_CSUM feature(s) are *only* required to
handle checksumming of UDP and TCP.

In netif_skb_features() we attempt to filter out the capabilities which
are inappropriate for the device that the skb will actually be sent
from... but there we assume that NETIF_F_V4_CSUM devices can handle
*all* Legacy IP, and that NETIF_F_V6_CSUM devices can handle *all* IPv6.

This may have been OK in the days when CHECKSUM_PARTIAL packets would
*only* be produced by the local stack, and we knew the local stack
didn't generate them for anything but UDP and TCP. But these days that's
not true. When a tun device receives a packet from userspace with
VIRTIO_NET_HDR_F_NEEDS_CSUM, that translates fairly directly into
setting CHECKSUM_PARTIAL on the resulting skb. Since virtio_net
advertises NETIF_F_HW_CSUM to its guests, we should expect to be asked
to checksum *anything*.

This patch attempts to cope with that by checking skb->csum_offset for
such devices. If that doesn't match the offset for UDP or TCP, then we
don't use hardware checksum. It won't catch 100% of cases, but a full
check of the actual skb contents in the fast path isn't a good idea.
It'll probably do well enough for now.

This expands the check in can_checksum_protocol() to make it more
readable, but doing so shouldn't make the resulting code any *bigger*,
except obviously for the additional checks.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

diff --git a/net/core/dev.c b/net/core/dev.c
index 515473e..f1048b6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2229,22 +2229,39 @@ static int dev_gso_segment(struct sk_buff *skb, netdev_features_t features)
 	return 0;
 }
 
-static bool can_checksum_protocol(netdev_features_t features, __be16 protocol)
+static bool can_checksum_protocol(netdev_features_t features, __be16 protocol,
+				  __u16 csum_offset)
 {
-	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)));
+	if (features & NETIF_F_GEN_CSUM)
+		return 1;
+
+	if ((features & NETIF_F_FCOE_CRC) && protocol == htons(ETH_P_FCOE))
+		return 1;
+
+	/*
+	 * Only allow NETIF_F_V[46]_CSUM for UDP/TCP packets. This is an
+	 * overly permissive check, but it's very unlikely to have false
+	 * positives in practice, and actually looking in the packet for
+	 * a proper confirmation would be very slow.
+	 */
+	if (csum_offset != offsetof(struct udphdr, check) &&
+	    csum_offset != offsetof(struct tcphdr, check))
+		return 0;
+
+	if ((features & NETIF_F_V4_CSUM) && protocol == htons(ETH_P_IP))
+		return 1;
+
+	if ((features & NETIF_F_V6_CSUM) && protocol == htons(ETH_P_IPV6))
+		return 1;
+
+	return 0;
 }
 
 static netdev_features_t harmonize_features(struct sk_buff *skb,
 	__be16 protocol, netdev_features_t features)
 {
 	if (skb->ip_summed != CHECKSUM_NONE &&
-	    !can_checksum_protocol(features, protocol)) {
+	    !can_checksum_protocol(features, protocol, skb->csum_offset)) {
 		features &= ~NETIF_F_ALL_CSUM;
 		features &= ~NETIF_F_SG;
 	} else if (illegal_highdma(skb->dev, skb)) {


-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]