Commit cef401de authored by Eric Dumazet's avatar Eric Dumazet Committed by David S. Miller
Browse files

net: fix possible wrong checksum generation 



Pravin Shelar mentioned that GSO could potentially generate
wrong TX checksum if skb has fragments that are overwritten
by the user between the checksum computation and transmit.

He suggested to linearize skbs but this extra copy can be
avoided for normal tcp skbs cooked by tcp_sendmsg().

This patch introduces a new SKB_GSO_SHARED_FRAG flag, set
in skb_shinfo(skb)->gso_type if at least one frag can be
modified by the user.

Typical sources of such possible overwrites are {vm}splice(),
sendfile(), and macvtap/tun/virtio_net drivers.

Tested:

$ netperf -H 7.7.8.84
MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to
7.7.8.84 () port 0 AF_INET
Recv   Send    Send
Socket Socket  Message  Elapsed
Size   Size    Size     Time     Throughput
bytes  bytes   bytes    secs.    10^6bits/sec

 87380  16384  16384    10.00    3959.52

$ netperf -H 7.7.8.84 -t TCP_SENDFILE
TCP SENDFILE TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 7.7.8.84 ()
port 0 AF_INET
Recv   Send    Send
Socket Socket  Message  Elapsed
Size   Size    Size     Time     Throughput
bytes  bytes   bytes    secs.    10^6bits/sec

 87380  16384  16384    10.00    3216.80

Performance of the SENDFILE is impacted by the extra allocation and
copy, and because we use order-0 pages, while the TCP_STREAM uses
bigger pages.

Reported-by: default avatarPravin Shelar <pshelar@nicira.com>
Signed-off-by: default avatarEric Dumazet <edumazet@google.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 61550022

drivers/net/macvtap.c

+2 −1
Original line number Diff line number Diff line
@@ -543,6 +543,7 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from, 
		skb->data_len += len;

		skb->len += len;

		skb->truesize += truesize;

		skb_shinfo(skb)->gso_type |= SKB_GSO_SHARED_FRAG;

		atomic_add(truesize, &skb->sk->sk_wmem_alloc);

		while (len) {

			int off = base & ~PAGE_MASK;
@@ -598,7 +599,7 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb, 


	if (vnet_hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {

		skb_shinfo(skb)->gso_size = vnet_hdr->gso_size;

		skb_shinfo(skb)->gso_type = gso_type;

		skb_shinfo(skb)->gso_type |= gso_type;



		/* Header must be checked, and gso_segs computed. */

		skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;

drivers/net/tun.c

+8 −4
Original line number Diff line number Diff line
@@ -1005,6 +1005,7 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from, 
		skb->data_len += len;

		skb->len += len;

		skb->truesize += truesize;

		skb_shinfo(skb)->gso_type |= SKB_GSO_SHARED_FRAG;

		atomic_add(truesize, &skb->sk->sk_wmem_alloc);

		while (len) {

			int off = base & ~PAGE_MASK;
@@ -1150,16 +1151,18 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, 
	}



	if (gso.gso_type != VIRTIO_NET_HDR_GSO_NONE) {

		unsigned short gso_type = 0;



		pr_debug("GSO!\n");

		switch (gso.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {

		case VIRTIO_NET_HDR_GSO_TCPV4:

			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;

			gso_type = SKB_GSO_TCPV4;

			break;

		case VIRTIO_NET_HDR_GSO_TCPV6:

			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;

			gso_type = SKB_GSO_TCPV6;

			break;

		case VIRTIO_NET_HDR_GSO_UDP:

			skb_shinfo(skb)->gso_type = SKB_GSO_UDP;

			gso_type = SKB_GSO_UDP;

			break;

		default:

			tun->dev->stats.rx_frame_errors++;
@@ -1168,9 +1171,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, 
		}



		if (gso.gso_type & VIRTIO_NET_HDR_GSO_ECN)

			skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;

			gso_type |= SKB_GSO_TCP_ECN;



		skb_shinfo(skb)->gso_size = gso.gso_size;

		skb_shinfo(skb)->gso_type |= gso_type;

		if (skb_shinfo(skb)->gso_size == 0) {

			tun->dev->stats.rx_frame_errors++;

			kfree_skb(skb);

drivers/net/virtio_net.c

+8 −4
Original line number Diff line number Diff line
@@ -220,6 +220,7 @@ static void set_skb_frag(struct sk_buff *skb, struct page *page, 
	skb->len += size;

	skb->truesize += PAGE_SIZE;

	skb_shinfo(skb)->nr_frags++;

	skb_shinfo(skb)->gso_type |= SKB_GSO_SHARED_FRAG;

	*len -= size;

}
@@ -379,16 +380,18 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len) 
		 ntohs(skb->protocol), skb->len, skb->pkt_type);



	if (hdr->hdr.gso_type != VIRTIO_NET_HDR_GSO_NONE) {

		unsigned short gso_type = 0;



		pr_debug("GSO!\n");

		switch (hdr->hdr.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {

		case VIRTIO_NET_HDR_GSO_TCPV4:

			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;

			gso_type = SKB_GSO_TCPV4;

			break;

		case VIRTIO_NET_HDR_GSO_UDP:

			skb_shinfo(skb)->gso_type = SKB_GSO_UDP;

			gso_type = SKB_GSO_UDP;

			break;

		case VIRTIO_NET_HDR_GSO_TCPV6:

			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;

			gso_type = SKB_GSO_TCPV6;

			break;

		default:

			net_warn_ratelimited("%s: bad gso type %u.\n",
@@ -397,7 +400,7 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len) 
		}



		if (hdr->hdr.gso_type & VIRTIO_NET_HDR_GSO_ECN)

			skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_ECN;

			gso_type |= SKB_GSO_TCP_ECN;



		skb_shinfo(skb)->gso_size = hdr->hdr.gso_size;

		if (skb_shinfo(skb)->gso_size == 0) {
@@ -405,6 +408,7 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len) 
			goto frame_err;

		}



		skb_shinfo(skb)->gso_type |= gso_type;

		/* Header must be checked, and gso_segs computed. */

		skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;

		skb_shinfo(skb)->gso_segs = 0;

include/linux/skbuff.h

+19 −0
Original line number Diff line number Diff line
@@ -307,6 +307,13 @@ enum { 
	SKB_GSO_TCPV6 = 1 << 4,



	SKB_GSO_FCOE = 1 << 5,



	/* This indicates at least one fragment might be overwritten

	 * (as in vmsplice(), sendfile() ...)

	 * If we need to compute a TX checksum, we'll need to copy

	 * all frags to avoid possible bad checksum

	 */

	SKB_GSO_SHARED_FRAG = 1 << 6,

};



#if BITS_PER_LONG > 32
@@ -2200,6 +2207,18 @@ static inline int skb_linearize(struct sk_buff *skb) 
	return skb_is_nonlinear(skb) ? __skb_linearize(skb) : 0;

}



/**

 * skb_has_shared_frag - can any frag be overwritten

 * @skb: buffer to test

 *

 * Return true if the skb has at least one frag that might be modified

 * by an external entity (as in vmsplice()/sendfile())

 */

static inline bool skb_has_shared_frag(const struct sk_buff *skb)

{

	return skb_shinfo(skb)->gso_type & SKB_GSO_SHARED_FRAG;

}



/**

 *	skb_linearize_cow - make sure skb is linear and writable

 *	@skb: buffer to process

net/core/dev.c

+9 −0
Original line number Diff line number Diff line
@@ -2271,6 +2271,15 @@ int skb_checksum_help(struct sk_buff *skb) 
		return -EINVAL;

	}



	/* Before computing a checksum, we should make sure no frag could

	 * be modified by an external entity : checksum could be wrong.

	 */

	if (skb_has_shared_frag(skb)) {

		ret = __skb_linearize(skb);

		if (ret)

			goto out;

	}



	offset = skb_checksum_start_offset(skb);

	BUG_ON(offset >= skb_headlen(skb));

	csum = skb_checksum(skb, offset, skb->len - offset, 0);

CRA Git | Maintained and supported by SUSTech CRA and CCSE