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

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

[David Woodhouse]

[-- 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 --]

[RFC PATCH 2/3] Prepare to allow for hardware checksum of ICMPv6

[David Woodhouse]

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

If netif_skb_features() has to filter out non-TCP and non-UDP frames
anyway for devices with limited checksum support, there's no reason we
shouldn't generate them in our *own* stack. This allows ICMPv6 to do
so...

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

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index b4a9fd5..e474df9 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -235,12 +235,19 @@ static int icmpv6_push_pending_frames(struct sock *sk, struct flowi6 *fl6, struc
 	icmp6h->icmp6_cksum = 0;
 
 	if (skb_queue_len(&sk->sk_write_queue) == 1) {
-		skb->csum = csum_partial(icmp6h,
-					sizeof(struct icmp6hdr), skb->csum);
-		icmp6h->icmp6_cksum = csum_ipv6_magic(&fl6->saddr,
-						      &fl6->daddr,
-						      len, fl6->flowi6_proto,
-						      skb->csum);
+		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			skb->csum_start = skb_transport_header(skb) - skb->head;
+			skb->csum_offset = offsetof(struct icmp6hdr, icmp6_cksum);
+			icmp6h->icmp6_cksum = ~csum_ipv6_magic(&fl6->saddr, &fl6->daddr,
+							       len, fl6->flowi6_proto, 0);
+		} else {
+			skb->csum = csum_partial(icmp6h,
+						 sizeof(struct icmp6hdr), skb->csum);
+			icmp6h->icmp6_cksum = csum_ipv6_magic(&fl6->saddr,
+							      &fl6->daddr,
+							      len, fl6->flowi6_proto,
+							      skb->csum);
+		}
 	} else {
 		__wsum tmp_csum = 0;
 


-- 
dwmw2


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

[RFC PATCH 3/3] Use hardware checksum for UDPv6 and ICMPv6

[David Woodhouse]

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

This actually enables the use of CHECKSUM_PARTIAL for outbound ICMPv6
frames. My check in the driver for non-hw-csum frames was *also*
triggering when I was running 'nc -u' over IPv6, and this appears to fix
that too. Is there a reason it wasn't happening already?

I only see the driver check trigger for ndisc and igmp frames now, and I
don't think we care very much about those?

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

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 5552d13..8a27090 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1370,7 +1370,12 @@ alloc_new_skb:
 			/*
 			 *	Fill in the control structures
 			 */
-			skb->ip_summed = CHECKSUM_NONE;
+			if ((sk->sk_protocol == IPPROTO_ICMPV6 ||
+			     sk->sk_protocol == IPPROTO_UDP) &&
+			    rt->dst.dev->features & NETIF_F_HW_CSUM)
+				skb->ip_summed = CHECKSUM_PARTIAL;
+			else
+				skb->ip_summed = CHECKSUM_NONE;
 			skb->csum = 0;
 			/* reserve for fragmentation and ipsec header */
 			skb_reserve(skb, hh_len + sizeof(struct frag_hdr) +


-- 
dwmw2


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

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

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Mon, 14 Jan 2013 12:10:31 +0000

> 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*.

My opinion on this is that the injectors of packets are responsible
for ensuring checksum types are set on SKBs in an appropriate way.

So we ensure this in the local protocol stacks that generate packets,
and if foreign alien entities can inject SKBs with these checksum
settings (like the tun device can) the burdon of verification falls
upon whatever layer allows that to happen.

So really, the fix is in the tun device and the virtio layer.

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

[David Woodhouse]

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

On Wed, 2013-01-16 at 15:54 -0500, David Miller wrote:
> 
> My opinion on this is that the injectors of packets are responsible
> for ensuring checksum types are set on SKBs in an appropriate way.
> 
> So we ensure this in the local protocol stacks that generate packets,
> and if foreign alien entities can inject SKBs with these checksum
> settings (like the tun device can) the burdon of verification falls
> upon whatever layer allows that to happen.
> 
> So really, the fix is in the tun device and the virtio layer.

The virtio layer (and the tun device) expose the equivalent of the
NETIF_F_HW_CSUM capability to the guest. In the case where we have a
real device on the host which *also* has NETIF_F_HW_CSUM capability, are
you saying that the tun driver should do the checksum for non-UDP/TCP
packets in software *anyway*, just because the packet might end up going
out a device *without* that capability, and the check in
harmonize_features() isn't sophisticated enough to cope properly?

-- 
dwmw2


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

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

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 16 Jan 2013 22:34:18 +0000

> On Wed, 2013-01-16 at 15:54 -0500, David Miller wrote:
>> 
>> My opinion on this is that the injectors of packets are responsible
>> for ensuring checksum types are set on SKBs in an appropriate way.
>> 
>> So we ensure this in the local protocol stacks that generate packets,
>> and if foreign alien entities can inject SKBs with these checksum
>> settings (like the tun device can) the burdon of verification falls
>> upon whatever layer allows that to happen.
>> 
>> So really, the fix is in the tun device and the virtio layer.
> 
> The virtio layer (and the tun device) expose the equivalent of the
> NETIF_F_HW_CSUM capability to the guest. In the case where we have a
> real device on the host which *also* has NETIF_F_HW_CSUM capability, are
> you saying that the tun driver should do the checksum for non-UDP/TCP
> packets in software *anyway*, just because the packet might end up going
> out a device *without* that capability, and the check in
> harmonize_features() isn't sophisticated enough to cope properly?

I'm saying that tun can't inject unchecked crap into our stack.

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

[David Woodhouse]

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

On Wed, 2013-01-16 at 18:00 -0500, David Miller wrote:
> I'm saying that tun can't inject unchecked crap into our stack.

That's a very strange way of putting it.

Our stack has explicit support for sane hardware devices with
NETIF_F_HW_CSUM capability that can checksum anything.

And it has checks (harmonize_features) on output, so that if the device
on which a packet is being emitted *doesn't* have appropriate hardware
checksum capability, we'll do the checksum in software.

Except that the check in harmonize_features() doesn't do that check
*properly*. It only catches *some* of the packets that the device can't
handle, and lets others through.

So we basically can't use NETIF_F_HW_CSUM in the general case, for
anything like SCTP or any other protocol, because harmonize_features()
is buggy and will let it go out a device that can't handle it.

Um, SCTP *does* use CHECKSUM_PARTIAL. Am I missing something or does
that suffer the same problem?

-- 
dwmw2


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

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

[David Woodhouse]

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

On Thu, 2013-01-17 at 00:03 +0000, David Woodhouse wrote:
> Um, SCTP *does* use CHECKSUM_PARTIAL. Am I missing something or does
> that suffer the same problem?

Am I mistaken, or does SCTP end up sending un-checksummed packets
because of the same 'bug' in harmonize_features()?

-- 
dwmw2


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

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

[David Woodhouse]

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

On Wed, 2013-01-16 at 18:00 -0500, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Wed, 16 Jan 2013 22:34:18 +0000
> 
> > On Wed, 2013-01-16 at 15:54 -0500, David Miller wrote:
> >> 
> >> My opinion on this is that the injectors of packets are responsible
> >> for ensuring checksum types are set on SKBs in an appropriate way.
> >> 
> >> So we ensure this in the local protocol stacks that generate packets,
> >> and if foreign alien entities can inject SKBs with these checksum
> >> settings (like the tun device can) the burdon of verification falls
> >> upon whatever layer allows that to happen.
> >> 
> >> So really, the fix is in the tun device and the virtio layer.
> > 
> > The virtio layer (and the tun device) expose the equivalent of the
> > NETIF_F_HW_CSUM capability to the guest. In the case where we have a
> > real device on the host which *also* has NETIF_F_HW_CSUM capability, are
> > you saying that the tun driver should do the checksum for non-UDP/TCP
> > packets in software *anyway*, just because the packet might end up going
> > out a device *without* that capability, and the check in
> > harmonize_features() isn't sophisticated enough to cope properly?
> 
> I'm saying that tun can't inject unchecked crap into our stack.

Did we ever resolve this? AFAICT from inspecting the code the
virtio_net device still advertises hardware csum capabilities to the
guest. And accepts packets which need checksumming, calling
skb_partial_csum_set() as appropriate. Likewise tun, xen, macvtap and
af_packet.

And that works fine — it's a nice performance win because it means that
VM guests (and other clients) can make full use of the HW csum
capabilities of the real network hardware. And when the outbound
netdevice *doesn't* have HW csum support, we generally do the right
thing and complete the csum in software in the host kernel before
transmitting it.

Perhaps I'm missing something, but I'm not sure why you refer to that
as 'injecting unchecked crap'. Surely it's using CHECKSUM_PARTIAL
precisely as it was designed, and allowing the checksum to be completed
either by hardware or software as appropriate?

The *only* problem is the false positive in harmonize_features(), which
was addressed by my patch which started this thread (in 2013). The
problem is that an IP packet that *isn't* TCP or UDP, being sent out a
device that has only NETIF_F_IP_CSUM capability, ends up being handed
to the device unchecksummed because harmonize_features() fails to clear
the HW csum flag as it (arguably) should.

Original thread at
http://comments.gmane.org/gmane.linux.network/254981

I'm only looking at it again because I'm pondering enabling HW csum in
8139cp (now that I've fixed TSO), and it reminded me of this...


-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


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

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

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1454 bytes --]

On Mon, 2015-09-21 at 17:29 +0100, David Woodhouse wrote:
> 
> Did we ever resolve this? AFAICT from inspecting the code the
> virtio_net device still advertises hardware csum capabilities to the
> guest. And accepts packets which need checksumming, calling
> skb_partial_csum_set() as appropriate. Likewise tun, xen, macvtap and
> af_packet.

Here's a test case which provokes the network stack into handing a
CHECKSUM_PARTIAL skb to a device which it knows can't handle it. (It
obviously needs the AF_PACKET endianness ABI fix I sent earlier.)

You might well be right to refer to this as 'injecting unchecked crap',
but we are *gaining* injection points with the ability to do this, and
for not entirely insane reasons — people want to be able to make full
use of hardware offload capabilities.

And we *have* a safety check, to avoid handing CHECKSUM_PARTIAL buffers
to devices which can't handle them. We already do check the
capabilities of the device we end up routing it to, and complete the
checksum in software if the device can't cope.

All we're talking about here is a corner case when that existing check
doesn't actually give the right results, because it assumes a device
with NETIF_F_IP_CSUM can checksum *all* Legacy IP packets, not just TCP
and UDP.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #1.2: raw.c --]
[-- Type: text/x-csrc, Size: 3051 bytes --]



#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <sys/socket.h>
#include <netpacket/packet.h>
#include <net/ethernet.h>
#include <net/if.h>
#include <sys/ioctl.h>
#include <netinet/in.h>
#include <unistd.h>
//#include <linux/if_packet.h>
#define PACKET_VNET_HDR 15

struct virtio_net_hdr
{
    uint8_t flags;
    uint8_t gso_type;
    uint16_t hdr_len;
    uint16_t gso_size;
    uint16_t csum_start;
    uint16_t csum_offset;
};
#define VIRTIO_NET_HDR_F_NEEDS_CSUM     1

unsigned char eth_hdr[] = {
	0x52, 0x54, 0x00, 0x3a, 0xbe, 0x28, 0x52, 0x54, 0x00, 0x74, 0x2f, 0xfd, 0x08, 0x00
};
unsigned char icmp_pkt[] = {
        0x45, 0x00, 0x00, 0x54, 0x11, 0xec, 0x40, 0x00, 0x40, 0x01, 0xb3, 0x58, 0xc0, 0xa8, 0x7a, 0x12,
        0xc0, 0xa8, 0x7a, 0x01, 0x08, 0x00, 0x00, 0x00, 0x07, 0xd2, 0x00, 0x01, 0x63, 0x7f, 0x02, 0x56,
        0x00, 0x00, 0x00, 0x00, 0x7c, 0x1b, 0x0b, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x11, 0x12, 0x13,
        0x14, 0x15, 0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23,
        0x24, 0x25, 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x30, 0x31, 0x32, 0x33,
        0x34, 0x35, 0x36, 0x37
};

unsigned char udp_pkt[] = {
	0x45, 0x00, 0x00, 0x22, 0xc4, 0x25, 0x40, 0x00, 0x40, 0x11, 0x01, 0x41, 0xc0, 0xa8, 0x7a, 0x12,
        0xc0, 0xa8, 0x7a, 0x01, 0xae, 0xc7, 0x1f, 0x90, 0x00, 0x0e, 0x75, 0x84, 0x68, 0x65, 0x6c, 0x6c,
        0x6f, 0x0a
};

int main(int argc, char **argv)
{
	struct ifreq req;
	struct sockaddr_ll lladdr;
	int fd, ret, val;
	struct {
		struct virtio_net_hdr vnet;
		unsigned char eth[14];
		unsigned char pkt[2048];
	} buf;

	if (argc != 2) {
		fprintf(stderr, "Usage: %s <ifname>\n", argv[0]);
		exit(1);
	}
	fd = socket(PF_PACKET, SOCK_RAW, htons(ETH_P_IP));
	if (fd < 0) {
		perror("socket");
		exit(1);
	}

	memset(&req, 0, sizeof(req));
	strncpy(req.ifr_name, argv[1], IFNAMSIZ-1);
	ret = ioctl(fd, SIOCGIFINDEX, &req);
	if (ret < 0) {
		perror("SIOGIFINDEX");
		exit(1);
	}

	memset(&lladdr, 0, sizeof(lladdr));
	lladdr.sll_family   = AF_PACKET;
	lladdr.sll_protocol = htons(ETH_P_IP);
	lladdr.sll_ifindex  = req.ifr_ifindex;
	ret = bind(fd, (const struct sockaddr *)&lladdr, sizeof(lladdr));
	if (ret < 0) {
		perror("bind");
		exit(1);
	}

        val = 1;
        ret = setsockopt(fd, SOL_PACKET, PACKET_VNET_HDR, (const char *)&val,
			 sizeof(val));
        if (ret < 0) {
		perror("setsockopt(SOL_PACKET, PACKET_VNET_HDR)");
		exit(1);
	}

	memset(&buf.vnet, 0, sizeof(buf.vnet));
	memcpy(&buf.eth, eth_hdr, sizeof(eth_hdr));

	memcpy(&buf.pkt, udp_pkt, sizeof(udp_pkt));
	buf.vnet.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
	buf.vnet.csum_start = 0x22;
	buf.vnet.csum_offset = 0x6;
	if (write(fd, (void *)&buf, sizeof(buf.vnet) + 14 + sizeof(udp_pkt)) < 0) {
	        perror("Write UDP packet");
		exit(1);
	}

	memcpy(&buf.pkt, icmp_pkt, sizeof(icmp_pkt));
	buf.vnet.csum_offset = 0x2;
	if (write(fd, (void *)&buf, sizeof(buf.vnet) + 14 + sizeof(icmp_pkt)) < 0) {
		perror("Write ICMP packet");
		exit(1);
	}
	return 0;
}

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