Skip to contentSkip to conversation

skb_partial_csum_set

PATCH In review Rusty Russell 3 participants 19 messages Jan 15, 2008 – Jan 22, 2008

Subsystems (5)
Networking Network drivers virtio core virtio net driver +1 more subsystems
lore
Share & tools
lore

Rusty Russell

author
#0 lore raw

PATCH 1/3 skb_partial_csum_set

Implement skb_partial_csum_set, for setting partial csums on untrusted packets.

Use it in virtio_net (replacing buggy version there), it's also going to be used by TAP for partial csum support.

Signed-off-by Rusty Russell <[email protected]>
---
 drivers/net/virtio_net.c |   11 +----------
 include/linux/skbuff.h   |    1 +
 net/core/skbuff.c        |   29 +++++++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 10 deletions(-)
include/linux/skbuff.h +1 -0
diff -r 72be3d596d31 include/linux/skbuff.h
@@ -1804,5 +1804,6 @@ static inline void skb_forward_csum(stru
 		skb->ip_summed = CHECKSUM_NONE;
 }
 
+bool skb_partial_csum_set(struct sk_buff *skb, u16 start, u16 off);
 #endif	/* __KERNEL__ */
 #endif	/* _LINUX_SKBUFF_H */
net/core/skbuff.c +29 -0
diff -r 72be3d596d31 net/core/skbuff.c
@@ -2214,6 +2214,34 @@ int skb_cow_data(struct sk_buff *skb, in
 	return elt;
 }
 
+/**
+ * skb_partial_csum_set - set up and verify partial csum values for packet
+ * @skb: the skb to set
+ * @start: the number of bytes after skb->data to start checksumming.
+ * @off: the offset from start to place the checksum.
+ *
+ * For untrusted partially-checksummed packets, we need to make sure the values
+ * for skb->csum_start and skb->csum_offset are valid so we don't oops.
+ *
+ * This function checks and sets those values and skb->ip_summed: if this
+ * returns false you should drop the packet.
+ */
+bool skb_partial_csum_set(struct sk_buff *skb, u16 start, u16 off)
+{
+	if (unlikely(start > skb->len - 2) || 
+	    unlikely((int)start + off > skb->len - 2)) {
+		if (net_ratelimit())
+			printk(KERN_WARNING
+			       "bad partial csum: csum=%u/%u len=%u\n",
+			       start, off, skb->len);
+		return false;
+	}
+	skb->ip_summed = CHECKSUM_PARTIAL;
+	skb->csum_start = skb_headroom(skb) + start;
+	skb->csum_offset = off;
+	return true;
+}
+
 EXPORT_SYMBOL(___pskb_trim);
 EXPORT_SYMBOL(__kfree_skb);
 EXPORT_SYMBOL(kfree_skb);

@@ -2250,3 +2278,4 @@ EXPORT_SYMBOL(skb_append_datato_frags);
 
 EXPORT_SYMBOL_GPL(skb_to_sgvec);
 EXPORT_SYMBOL_GPL(skb_cow_data);
+EXPORT_SYMBOL_GPL(skb_partial_csum_set);
drivers/net/virtio_net.c +1 -10
diff -r 72be3d596d31 drivers/net/virtio_net.c
@@ -89,17 +89,8 @@ static void receive_skb(struct net_devic
 
 	if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
 		pr_debug("Needs csum!\n");
-		skb->ip_summed = CHECKSUM_PARTIAL;
-		skb->csum_start = hdr->csum_start;
-		skb->csum_offset = hdr->csum_offset;
-		if (skb->csum_start > skb->len - 2
-		    || skb->csum_offset > skb->len - 2) {
-			if (net_ratelimit())
-				printk(KERN_WARNING "%s: csum=%u/%u len=%u\n",
-				       dev->name, skb->csum_start,
-				       skb->csum_offset, skb->len);
+		if (!skb_partial_csum_set(skb,hdr->csum_start,hdr->csum_offset))
 			goto frame_err;
-		}
 	}
 
 	if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
author
#1 lore raw

PATCH 2/3 virtio: Net header needs gso_hdr_len

It's far easier to deal with GSO if we don't have to parse the packet to figure out the header length. Add the field to the virtio_net_hdr struct (and fix the spaces that somehow crept in there).

Signed-off-by Rusty Russell <[email protected]>
---
 drivers/net/virtio_net.c   |    4 +++-
 include/linux/virtio_net.h |   11 ++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)
drivers/net/virtio_net.c +3 -1
diff -r 24ef33a4ab14 drivers/net/virtio_net.c
@@ -126,6 +126,7 @@ static void receive_skb(struct net_devic
 		/* Header must be checked, and gso_segs computed. */
 		skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
 		skb_shinfo(skb)->gso_segs = 0;
+		skb_set_transport_header(skb, hdr->gso_hdr_len);
 	}
 
 	netif_receive_skb(skb);

@@ -247,6 +248,7 @@ static int start_xmit(struct sk_buff *sk
 	}
 
 	if (skb_is_gso(skb)) {
+		hdr->gso_hdr_len = skb_transport_header(skb) - skb->data;
 		hdr->gso_size = skb_shinfo(skb)->gso_size;
 		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_ECN)
 			hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4_ECN;

@@ -260,7 +262,7 @@ static int start_xmit(struct sk_buff *sk
 			BUG();
 	} else {
 		hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE;
-		hdr->gso_size = 0;
+		hdr->gso_size = hdr->gso_hdr_len = 0;
 	}
 
 	vnet_hdr_to_sg(sg, skb);
include/linux/virtio_net.h +6 -5
diff -r 24ef33a4ab14 include/linux/virtio_net.h
@@ -24,16 +24,17 @@ struct virtio_net_hdr
 struct virtio_net_hdr
 {
 #define VIRTIO_NET_HDR_F_NEEDS_CSUM	1	// Use csum_start, csum_offset
-      __u8 flags;
+	__u8 flags;
 #define VIRTIO_NET_HDR_GSO_NONE		0	// Not a GSO frame
 #define VIRTIO_NET_HDR_GSO_TCPV4	1	// GSO frame, IPv4 TCP (TSO)
 /* FIXME: Do we need this?  If they said they can handle ECN, do they care? */
 #define VIRTIO_NET_HDR_GSO_TCPV4_ECN	2	// GSO frame, IPv4 TCP w/ ECN
 #define VIRTIO_NET_HDR_GSO_UDP		3	// GSO frame, IPv4 UDP (UFO)
 #define VIRTIO_NET_HDR_GSO_TCPV6	4	// GSO frame, IPv6 TCP
-      __u8 gso_type;
-      __u16 gso_size;
-      __u16 csum_start;
-      __u16 csum_offset;
+	__u8 gso_type;
+	__u16 gso_hdr_len;	/* Ethernet + IP + tcp/udp hdrs */
+	__u16 gso_size;		/* Bytes to append to gso_hdr_len per frame */
+	__u16 csum_start;	/* Position to start checksumming from */
+	__u16 csum_offset;	/* Offset after that to place checksum */
 };
 #endif /* _LINUX_VIRTIO_NET_H */
author
#2 lore raw

PATCH 2/3 virtio: Net header needs gso_hdr_len

In reply to Rusty Russell

It's far easier to deal with GSO if we don't have to parse the packet to figure out the header length. Add the field to the virtio_net_hdr struct (and fix the spaces that somehow crept in there).

Signed-off-by Rusty Russell <[email protected]>
---
 drivers/net/virtio_net.c   |    4 +++-
 include/linux/virtio_net.h |   11 ++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)
drivers/net/virtio_net.c +3 -1
diff -r 24ef33a4ab14 drivers/net/virtio_net.c
@@ -126,6 +126,7 @@ static void receive_skb(struct net_devic
 		/* Header must be checked, and gso_segs computed. */
 		skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
 		skb_shinfo(skb)->gso_segs = 0;
+		skb_set_transport_header(skb, hdr->gso_hdr_len);
 	}
 
 	netif_receive_skb(skb);

@@ -247,6 +248,7 @@ static int start_xmit(struct sk_buff *sk
 	}
 
 	if (skb_is_gso(skb)) {
+		hdr->gso_hdr_len = skb_transport_header(skb) - skb->data;
 		hdr->gso_size = skb_shinfo(skb)->gso_size;
 		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_ECN)
 			hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4_ECN;

@@ -260,7 +262,7 @@ static int start_xmit(struct sk_buff *sk
 			BUG();
 	} else {
 		hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE;
-		hdr->gso_size = 0;
+		hdr->gso_size = hdr->gso_hdr_len = 0;
 	}
 
 	vnet_hdr_to_sg(sg, skb);
include/linux/virtio_net.h +6 -5
diff -r 24ef33a4ab14 include/linux/virtio_net.h
@@ -24,16 +24,17 @@ struct virtio_net_hdr
 struct virtio_net_hdr
 {
 #define VIRTIO_NET_HDR_F_NEEDS_CSUM	1	// Use csum_start, csum_offset
-      __u8 flags;
+	__u8 flags;
 #define VIRTIO_NET_HDR_GSO_NONE		0	// Not a GSO frame
 #define VIRTIO_NET_HDR_GSO_TCPV4	1	// GSO frame, IPv4 TCP (TSO)
 /* FIXME: Do we need this?  If they said they can handle ECN, do they care? */
 #define VIRTIO_NET_HDR_GSO_TCPV4_ECN	2	// GSO frame, IPv4 TCP w/ ECN
 #define VIRTIO_NET_HDR_GSO_UDP		3	// GSO frame, IPv4 UDP (UFO)
 #define VIRTIO_NET_HDR_GSO_TCPV6	4	// GSO frame, IPv6 TCP
-      __u8 gso_type;
-      __u16 gso_size;
-      __u16 csum_start;
-      __u16 csum_offset;
+	__u8 gso_type;
+	__u16 gso_hdr_len;	/* Ethernet + IP + tcp/udp hdrs */
+	__u16 gso_size;		/* Bytes to append to gso_hdr_len per frame */
+	__u16 csum_start;	/* Position to start checksumming from */
+	__u16 csum_offset;	/* Offset after that to place checksum */
 };
 #endif /* _LINUX_VIRTIO_NET_H */
author
#3 lore raw

PATCH 3/3 tun/tap GSO/partial csum support

This implements partial checksum and GSO support for tun/tap.

We use the virtio_net_hdr: it is an ABI already and designed to encapsulate such metadata as GSO and partial checksums.

lguest performance (160MB sendfile, worst/best/avg, 20 runs):

	Before: 5.06/3.39/3.82
	After:  4.69/0.84/2.84

Note that the way tun works, you have to use the TUNSETIFF ioctl to set this if you want to detect older kernels which don't have support.

Questions:
1) Should we rename/move virtio_net_hdr to something more generic?
2) Is this the right way to build a paged skb from user pages?
3) Do we need more checking for invalid GSO fields?

Signed-off-by Rusty Russell <[email protected]>
---
 drivers/net/tun.c        |  213 +++++++++++++++++++++++++++++++++++++++++------
 include/linux/if_tun.h   |    2
 3 files changed, 189 insertions(+), 28 deletions(-)
drivers/net/tun.c +222 -27
diff -r 1057851c060f drivers/net/tun.c
@@ -62,6 +62,7 @@
 #include <linux/if_ether.h>
 #include <linux/if_tun.h>
 #include <linux/crc32.h>
+#include <linux/virtio_net.h>
 #include <net/net_namespace.h>
 
 #include <asm/system.h>

@@ -238,35 +239,195 @@ static unsigned int tun_chr_poll(struct 
 	return mask;
 }
 
+static struct sk_buff *copy_user_skb(size_t align, struct iovec *iv, size_t len)
+{
+	struct sk_buff *skb;
+
+	if (!(skb = alloc_skb(len + align, GFP_KERNEL)))
+		return ERR_PTR(-ENOMEM);
+
+	if (align)
+		skb_reserve(skb, align);
+
+	if (memcpy_fromiovec(skb_put(skb, len), iv, len)) {
+		kfree_skb(skb);
+		return ERR_PTR(-EFAULT);
+	}
+	return skb;
+}
+
+/* This will fail if they give us a crazy iovec, but that's their own fault. */
+static int get_user_skb_frags(const struct iovec *iv, size_t count,
+			      struct skb_frag_struct *f)
+{
+	unsigned int i, j, num_pg = 0;
+	int err;
+	struct page *pages[MAX_SKB_FRAGS];
+
+	down_read(&current->mm->mmap_sem);
+	for (i = 0; i < count; i++) {
+		int n, npages;
+		unsigned long base, len;
+		base = (unsigned long)iv[i].iov_base;
+		len = (unsigned long)iv[i].iov_len;
+
+		if (len == 0)
+			continue;
+
+		/* How many pages will this take? */
+		npages = 1 + (base + len - 1)/PAGE_SIZE - base/PAGE_SIZE;
+		if (unlikely(num_pg + npages > MAX_SKB_FRAGS)) {
+			err = -ENOSPC;
+			goto fail;
+		}
+		n = get_user_pages(current, current->mm, base, npages,
+				   0, 0, pages, NULL);
+		if (unlikely(n < 0)) {
+			err = n;
+			goto fail;
+		}
+
+		/* Transfer pages to the frag array */
+		for (j = 0; j < n; j++) {
+			f[num_pg].page = pages[j];
+			if (j == 0) {
+				f[num_pg].page_offset = offset_in_page(base);
+				f[num_pg].size = min(len, PAGE_SIZE -
+						     f[num_pg].page_offset);
+			} else {
+				f[num_pg].page_offset = 0;
+				f[num_pg].size = min(len, PAGE_SIZE);
+			}
+			len -= f[num_pg].size;
+			base += f[num_pg].size;
+			num_pg++;
+		}
+
+		if (unlikely(n != npages)) {
+			err = -EFAULT;
+			goto fail;
+		}
+	}
+	up_read(&current->mm->mmap_sem);
+	return num_pg;
+
+fail:
+	for (i = 0; i < num_pg; i++)
+		put_page(f[i].page);
+	up_read(&current->mm->mmap_sem);
+	return err;
+}
+
+
+static struct sk_buff *map_user_skb(const struct virtio_net_hdr *gso,
+				    size_t align, struct iovec *iv,
+				    size_t count, size_t len)
+{
+	struct sk_buff *skb;
+	struct skb_shared_info *sinfo;
+	int err;
+
+	if (gso->gso_hdr_len > len)
+		return ERR_PTR(-EINVAL);
+
+	if (!(skb = alloc_skb(gso->gso_hdr_len + align, GFP_KERNEL)))
+		return ERR_PTR(-ENOMEM);
+
+	if (align)
+		skb_reserve(skb, align);
+
+	sinfo = skb_shinfo(skb);
+	sinfo->gso_size = gso->gso_size;
+	sinfo->gso_type = SKB_GSO_DODGY;
+	switch (gso->gso_type) {
+	case VIRTIO_NET_HDR_GSO_TCPV4_ECN:
+		sinfo->gso_type |= SKB_GSO_TCP_ECN;
+		/* fall through */
+	case VIRTIO_NET_HDR_GSO_TCPV4:
+		sinfo->gso_type |= SKB_GSO_TCPV4;
+		break;
+	case VIRTIO_NET_HDR_GSO_TCPV6:
+		sinfo->gso_type |= SKB_GSO_TCPV6;
+		break;
+	case VIRTIO_NET_HDR_GSO_UDP:
+		sinfo->gso_type |= SKB_GSO_UDP;
+		break;
+	default:
+		err = -EINVAL;
+		goto fail;
+	}
+
+	/* Copy in the header. */
+	if (memcpy_fromiovec(skb_put(skb, gso->gso_hdr_len), iv,
+			     gso->gso_hdr_len)) {
+		err = -EFAULT;
+		goto fail;
+	}
+
+	err = get_user_skb_frags(iv, count, sinfo->frags);
+	if (err < 0)
+		goto fail;
+
+	sinfo->nr_frags = err;
+	skb->len += len;
+	skb->data_len += len;
+	
+	/* GSO code expects transport header set up */
+	skb_set_transport_header(skb, gso->gso_hdr_len);
+
+	return skb;
+
+fail:
+	kfree_skb(skb);
+	return ERR_PTR(err);
+}
+
+static inline size_t iov_total(const struct iovec *iv, unsigned long count)
+{
+	unsigned long i;
+	size_t len;
+
+	for (i = 0, len = 0; i < count; i++)
+		len += iv[i].iov_len;
+
+	return len;
+}
+
 /* Get packet from user space buffer */
-static __inline__ ssize_t tun_get_user(struct tun_struct *tun, struct iovec *iv, size_t count)
+static __inline__ ssize_t tun_get_user(struct tun_struct *tun, struct iovec *iv, size_t num)
 {
 	struct tun_pi pi = { 0, __constant_htons(ETH_P_IP) };
+	struct virtio_net_hdr gso = { 0, VIRTIO_NET_HDR_GSO_NONE };
 	struct sk_buff *skb;
-	size_t len = count, align = 0;
+	size_t tot_len = iov_total(iv, num);
+	size_t len = tot_len, align = 0;
 
 	if (!(tun->flags & TUN_NO_PI)) {
-		if ((len -= sizeof(pi)) > count)
+		if ((len -= sizeof(pi)) > tot_len)
 			return -EINVAL;
 
 		if(memcpy_fromiovec((void *)&pi, iv, sizeof(pi)))
+			return -EFAULT;
+	}
+	if (tun->flags & TUN_GSO_HDR) {
+		if ((len -= sizeof(gso)) > tot_len)
+			return -EINVAL;
+
+		if (memcpy_fromiovec((void *)&gso, iv, sizeof(gso)))
 			return -EFAULT;
 	}
 
 	if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV)
 		align = NET_IP_ALIGN;
 
-	if (!(skb = alloc_skb(len + align, GFP_KERNEL))) {
+	if (gso.gso_type != VIRTIO_NET_HDR_GSO_NONE)
+		skb = map_user_skb(&gso, align, iv, num, len);
+	else
+		skb = copy_user_skb(align, iv, len);
+
+	if (IS_ERR(skb)) {
 		tun->dev->stats.rx_dropped++;
-		return -ENOMEM;
-	}
-
-	if (align)
-		skb_reserve(skb, align);
-	if (memcpy_fromiovec(skb_put(skb, len), iv, len)) {
-		tun->dev->stats.rx_dropped++;
-		kfree_skb(skb);
-		return -EFAULT;
+		return PTR_ERR(skb);
 	}
 
 	switch (tun->flags & TUN_TYPE_MASK) {

@@ -280,7 +441,13 @@ static __inline__ ssize_t tun_get_user(s
 		break;
 	};
 
-	if (tun->flags & TUN_NOCHECKSUM)
+	if (gso.flags & (1 << VIRTIO_NET_F_NO_CSUM)) {
+		if (!skb_partial_csum_set(skb,gso.csum_start,gso.csum_offset)) {
+			tun->dev->stats.rx_dropped++;
+			kfree_skb(skb);
+			return -EINVAL;
+		}
+	} else if (tun->flags & TUN_NOCHECKSUM)
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 	netif_rx_ni(skb);

@@ -289,18 +456,7 @@ static __inline__ ssize_t tun_get_user(s
 	tun->dev->stats.rx_packets++;
 	tun->dev->stats.rx_bytes += len;
 
-	return count;
-}
-
-static inline size_t iov_total(const struct iovec *iv, unsigned long count)
-{
-	unsigned long i;
-	size_t len;
-
-	for (i = 0, len = 0; i < count; i++)
-		len += iv[i].iov_len;
-
-	return len;
+	return tot_len;
 }
 
 static ssize_t tun_chr_aio_write(struct kiocb *iocb, const struct iovec *iv,

@@ -313,7 +469,7 @@ static ssize_t tun_chr_aio_write(struct 
 
 	DBG(KERN_INFO "%s: tun_chr_write %ld\n", tun->dev->name, count);
 
-	return tun_get_user(tun, (struct iovec *) iv, iov_total(iv, count));
+	return tun_get_user(tun, (struct iovec *) iv, count);
 }
 
 /* Put packet to the user space buffer */

@@ -336,6 +492,42 @@ static __inline__ ssize_t tun_put_user(s
 		if (memcpy_toiovec(iv, (void *) &pi, sizeof(pi)))
 			return -EFAULT;
 		total += sizeof(pi);
+	}
+	if (tun->flags & TUN_GSO_HDR) {
+		struct virtio_net_hdr gso;
+		struct skb_shared_info *sinfo = skb_shinfo(skb);
+
+		if (skb_is_gso(skb)) {
+			gso.gso_hdr_len = skb_transport_header(skb) - skb->data;
+			gso.gso_size = sinfo->gso_size;
+			if (sinfo->gso_type & SKB_GSO_TCP_ECN)
+				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4_ECN;
+			else if (sinfo->gso_type & SKB_GSO_TCPV4)
+				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
+			else if (sinfo->gso_type & SKB_GSO_TCPV6)
+				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
+			else if (sinfo->gso_type & SKB_GSO_UDP)
+				gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
+			else
+				BUG();
+		} else
+			gso.gso_type = VIRTIO_NET_HDR_GSO_NONE;
+		
+		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			gso.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
+			gso.csum_start = skb->csum_start - skb_headroom(skb);
+			gso.csum_offset = skb->csum_offset;
+		} else {
+			gso.flags = 0;
+			gso.csum_offset = gso.csum_start = 0;
+		}
+
+		if ((len -= sizeof(gso)) < 0)
+			return -EINVAL;
+
+		if (memcpy_toiovec(iv, (void *)&gso, sizeof(gso)))
+			return -EFAULT;
+		total += sizeof(gso);
 	}
 
 	len = min_t(int, skb->len, len);

@@ -543,6 +735,9 @@ static int tun_set_iff(struct file *file
 
 	if (ifr->ifr_flags & IFF_ONE_QUEUE)
 		tun->flags |= TUN_ONE_QUEUE;
+
+	if (ifr->ifr_flags & IFF_GSO_HDR)
+		tun->flags |= TUN_GSO_HDR;
 
 	file->private_data = tun;
 	tun->attached = 1;
include/linux/if_tun.h +2 -0
diff -r 1057851c060f include/linux/if_tun.h
@@ -70,6 +70,7 @@ struct tun_struct {
 #define TUN_NO_PI	0x0040
 #define TUN_ONE_QUEUE	0x0080
 #define TUN_PERSIST 	0x0100	
+#define TUN_GSO_HDR 	0x0200	
 
 /* Ioctl defines */
 #define TUNSETNOCSUM  _IOW('T', 200, int) 

@@ -85,6 +86,7 @@ struct tun_struct {
 #define IFF_TAP		0x0002
 #define IFF_NO_PI	0x1000
 #define IFF_ONE_QUEUE	0x2000
+#define IFF_GSO_HDR	0x4000
 
 struct tun_pi {
 	unsigned short flags;
author
#4 lore raw

PATCH 3/3 tun/tap GSO/partial csum support

In reply to Rusty Russell

This implements partial checksum and GSO support for tun/tap.

We use the virtio_net_hdr: it is an ABI already and designed to encapsulate such metadata as GSO and partial checksums.

lguest performance (160MB sendfile, worst/best/avg, 20 runs):

	Before: 5.06/3.39/3.82
	After:  4.69/0.84/2.84

Note that the way tun works, you have to use the TUNSETIFF ioctl to set this if you want to detect older kernels which don't have support.

Questions:
1) Should we rename/move virtio_net_hdr to something more generic?
2) Is this the right way to build a paged skb from user pages?
3) Do we need more checking for invalid GSO fields?

Signed-off-by Rusty Russell <[email protected]>
---
 drivers/net/tun.c        |  213 +++++++++++++++++++++++++++++++++++++++++------
 include/linux/if_tun.h   |    2
 3 files changed, 189 insertions(+), 28 deletions(-)
drivers/net/tun.c +222 -27
diff -r 1057851c060f drivers/net/tun.c
@@ -62,6 +62,7 @@
 #include <linux/if_ether.h>
 #include <linux/if_tun.h>
 #include <linux/crc32.h>
+#include <linux/virtio_net.h>
 #include <net/net_namespace.h>
 
 #include <asm/system.h>

@@ -238,35 +239,195 @@ static unsigned int tun_chr_poll(struct 
 	return mask;
 }
 
+static struct sk_buff *copy_user_skb(size_t align, struct iovec *iv, size_t len)
+{
+	struct sk_buff *skb;
+
+	if (!(skb = alloc_skb(len + align, GFP_KERNEL)))
+		return ERR_PTR(-ENOMEM);
+
+	if (align)
+		skb_reserve(skb, align);
+
+	if (memcpy_fromiovec(skb_put(skb, len), iv, len)) {
+		kfree_skb(skb);
+		return ERR_PTR(-EFAULT);
+	}
+	return skb;
+}
+
+/* This will fail if they give us a crazy iovec, but that's their own fault. */
+static int get_user_skb_frags(const struct iovec *iv, size_t count,
+			      struct skb_frag_struct *f)
+{
+	unsigned int i, j, num_pg = 0;
+	int err;
+	struct page *pages[MAX_SKB_FRAGS];
+
+	down_read(&current->mm->mmap_sem);
+	for (i = 0; i < count; i++) {
+		int n, npages;
+		unsigned long base, len;
+		base = (unsigned long)iv[i].iov_base;
+		len = (unsigned long)iv[i].iov_len;
+
+		if (len == 0)
+			continue;
+
+		/* How many pages will this take? */
+		npages = 1 + (base + len - 1)/PAGE_SIZE - base/PAGE_SIZE;
+		if (unlikely(num_pg + npages > MAX_SKB_FRAGS)) {
+			err = -ENOSPC;
+			goto fail;
+		}
+		n = get_user_pages(current, current->mm, base, npages,
+				   0, 0, pages, NULL);
+		if (unlikely(n < 0)) {
+			err = n;
+			goto fail;
+		}
+
+		/* Transfer pages to the frag array */
+		for (j = 0; j < n; j++) {
+			f[num_pg].page = pages[j];
+			if (j == 0) {
+				f[num_pg].page_offset = offset_in_page(base);
+				f[num_pg].size = min(len, PAGE_SIZE -
+						     f[num_pg].page_offset);
+			} else {
+				f[num_pg].page_offset = 0;
+				f[num_pg].size = min(len, PAGE_SIZE);
+			}
+			len -= f[num_pg].size;
+			base += f[num_pg].size;
+			num_pg++;
+		}
+
+		if (unlikely(n != npages)) {
+			err = -EFAULT;
+			goto fail;
+		}
+	}
+	up_read(&current->mm->mmap_sem);
+	return num_pg;
+
+fail:
+	for (i = 0; i < num_pg; i++)
+		put_page(f[i].page);
+	up_read(&current->mm->mmap_sem);
+	return err;
+}
+
+
+static struct sk_buff *map_user_skb(const struct virtio_net_hdr *gso,
+				    size_t align, struct iovec *iv,
+				    size_t count, size_t len)
+{
+	struct sk_buff *skb;
+	struct skb_shared_info *sinfo;
+	int err;
+
+	if (gso->gso_hdr_len > len)
+		return ERR_PTR(-EINVAL);
+
+	if (!(skb = alloc_skb(gso->gso_hdr_len + align, GFP_KERNEL)))
+		return ERR_PTR(-ENOMEM);
+
+	if (align)
+		skb_reserve(skb, align);
+
+	sinfo = skb_shinfo(skb);
+	sinfo->gso_size = gso->gso_size;
+	sinfo->gso_type = SKB_GSO_DODGY;
+	switch (gso->gso_type) {
+	case VIRTIO_NET_HDR_GSO_TCPV4_ECN:
+		sinfo->gso_type |= SKB_GSO_TCP_ECN;
+		/* fall through */
+	case VIRTIO_NET_HDR_GSO_TCPV4:
+		sinfo->gso_type |= SKB_GSO_TCPV4;
+		break;
+	case VIRTIO_NET_HDR_GSO_TCPV6:
+		sinfo->gso_type |= SKB_GSO_TCPV6;
+		break;
+	case VIRTIO_NET_HDR_GSO_UDP:
+		sinfo->gso_type |= SKB_GSO_UDP;
+		break;
+	default:
+		err = -EINVAL;
+		goto fail;
+	}
+
+	/* Copy in the header. */
+	if (memcpy_fromiovec(skb_put(skb, gso->gso_hdr_len), iv,
+			     gso->gso_hdr_len)) {
+		err = -EFAULT;
+		goto fail;
+	}
+
+	err = get_user_skb_frags(iv, count, sinfo->frags);
+	if (err < 0)
+		goto fail;
+
+	sinfo->nr_frags = err;
+	skb->len += len;
+	skb->data_len += len;
+	
+	/* GSO code expects transport header set up */
+	skb_set_transport_header(skb, gso->gso_hdr_len);
+
+	return skb;
+
+fail:
+	kfree_skb(skb);
+	return ERR_PTR(err);
+}
+
+static inline size_t iov_total(const struct iovec *iv, unsigned long count)
+{
+	unsigned long i;
+	size_t len;
+
+	for (i = 0, len = 0; i < count; i++)
+		len += iv[i].iov_len;
+
+	return len;
+}
+
 /* Get packet from user space buffer */
-static __inline__ ssize_t tun_get_user(struct tun_struct *tun, struct iovec *iv, size_t count)
+static __inline__ ssize_t tun_get_user(struct tun_struct *tun, struct iovec *iv, size_t num)
 {
 	struct tun_pi pi = { 0, __constant_htons(ETH_P_IP) };
+	struct virtio_net_hdr gso = { 0, VIRTIO_NET_HDR_GSO_NONE };
 	struct sk_buff *skb;
-	size_t len = count, align = 0;
+	size_t tot_len = iov_total(iv, num);
+	size_t len = tot_len, align = 0;
 
 	if (!(tun->flags & TUN_NO_PI)) {
-		if ((len -= sizeof(pi)) > count)
+		if ((len -= sizeof(pi)) > tot_len)
 			return -EINVAL;
 
 		if(memcpy_fromiovec((void *)&pi, iv, sizeof(pi)))
+			return -EFAULT;
+	}
+	if (tun->flags & TUN_GSO_HDR) {
+		if ((len -= sizeof(gso)) > tot_len)
+			return -EINVAL;
+
+		if (memcpy_fromiovec((void *)&gso, iv, sizeof(gso)))
 			return -EFAULT;
 	}
 
 	if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV)
 		align = NET_IP_ALIGN;
 
-	if (!(skb = alloc_skb(len + align, GFP_KERNEL))) {
+	if (gso.gso_type != VIRTIO_NET_HDR_GSO_NONE)
+		skb = map_user_skb(&gso, align, iv, num, len);
+	else
+		skb = copy_user_skb(align, iv, len);
+
+	if (IS_ERR(skb)) {
 		tun->dev->stats.rx_dropped++;
-		return -ENOMEM;
-	}
-
-	if (align)
-		skb_reserve(skb, align);
-	if (memcpy_fromiovec(skb_put(skb, len), iv, len)) {
-		tun->dev->stats.rx_dropped++;
-		kfree_skb(skb);
-		return -EFAULT;
+		return PTR_ERR(skb);
 	}
 
 	switch (tun->flags & TUN_TYPE_MASK) {

@@ -280,7 +441,13 @@ static __inline__ ssize_t tun_get_user(s
 		break;
 	};
 
-	if (tun->flags & TUN_NOCHECKSUM)
+	if (gso.flags & (1 << VIRTIO_NET_F_NO_CSUM)) {
+		if (!skb_partial_csum_set(skb,gso.csum_start,gso.csum_offset)) {
+			tun->dev->stats.rx_dropped++;
+			kfree_skb(skb);
+			return -EINVAL;
+		}
+	} else if (tun->flags & TUN_NOCHECKSUM)
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 	netif_rx_ni(skb);

@@ -289,18 +456,7 @@ static __inline__ ssize_t tun_get_user(s
 	tun->dev->stats.rx_packets++;
 	tun->dev->stats.rx_bytes += len;
 
-	return count;
-}
-
-static inline size_t iov_total(const struct iovec *iv, unsigned long count)
-{
-	unsigned long i;
-	size_t len;
-
-	for (i = 0, len = 0; i < count; i++)
-		len += iv[i].iov_len;
-
-	return len;
+	return tot_len;
 }
 
 static ssize_t tun_chr_aio_write(struct kiocb *iocb, const struct iovec *iv,

@@ -313,7 +469,7 @@ static ssize_t tun_chr_aio_write(struct 
 
 	DBG(KERN_INFO "%s: tun_chr_write %ld\n", tun->dev->name, count);
 
-	return tun_get_user(tun, (struct iovec *) iv, iov_total(iv, count));
+	return tun_get_user(tun, (struct iovec *) iv, count);
 }
 
 /* Put packet to the user space buffer */

@@ -336,6 +492,42 @@ static __inline__ ssize_t tun_put_user(s
 		if (memcpy_toiovec(iv, (void *) &pi, sizeof(pi)))
 			return -EFAULT;
 		total += sizeof(pi);
+	}
+	if (tun->flags & TUN_GSO_HDR) {
+		struct virtio_net_hdr gso;
+		struct skb_shared_info *sinfo = skb_shinfo(skb);
+
+		if (skb_is_gso(skb)) {
+			gso.gso_hdr_len = skb_transport_header(skb) - skb->data;
+			gso.gso_size = sinfo->gso_size;
+			if (sinfo->gso_type & SKB_GSO_TCP_ECN)
+				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4_ECN;
+			else if (sinfo->gso_type & SKB_GSO_TCPV4)
+				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
+			else if (sinfo->gso_type & SKB_GSO_TCPV6)
+				gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
+			else if (sinfo->gso_type & SKB_GSO_UDP)
+				gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
+			else
+				BUG();
+		} else
+			gso.gso_type = VIRTIO_NET_HDR_GSO_NONE;
+		
+		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			gso.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
+			gso.csum_start = skb->csum_start - skb_headroom(skb);
+			gso.csum_offset = skb->csum_offset;
+		} else {
+			gso.flags = 0;
+			gso.csum_offset = gso.csum_start = 0;
+		}
+
+		if ((len -= sizeof(gso)) < 0)
+			return -EINVAL;
+
+		if (memcpy_toiovec(iv, (void *)&gso, sizeof(gso)))
+			return -EFAULT;
+		total += sizeof(gso);
 	}
 
 	len = min_t(int, skb->len, len);

@@ -543,6 +735,9 @@ static int tun_set_iff(struct file *file
 
 	if (ifr->ifr_flags & IFF_ONE_QUEUE)
 		tun->flags |= TUN_ONE_QUEUE;
+
+	if (ifr->ifr_flags & IFF_GSO_HDR)
+		tun->flags |= TUN_GSO_HDR;
 
 	file->private_data = tun;
 	tun->attached = 1;
include/linux/if_tun.h +2 -0
diff -r 1057851c060f include/linux/if_tun.h
@@ -70,6 +70,7 @@ struct tun_struct {
 #define TUN_NO_PI	0x0040
 #define TUN_ONE_QUEUE	0x0080
 #define TUN_PERSIST 	0x0100	
+#define TUN_GSO_HDR 	0x0200	
 
 /* Ioctl defines */
 #define TUNSETNOCSUM  _IOW('T', 200, int) 

@@ -85,6 +86,7 @@ struct tun_struct {
 #define IFF_TAP		0x0002
 #define IFF_NO_PI	0x1000
 #define IFF_ONE_QUEUE	0x2000
+#define IFF_GSO_HDR	0x4000
 
 struct tun_pi {
 	unsigned short flags;
maintainer Acked-by on v1 1/3
#15 lore raw
In reply to Rusty Russell

From: Rusty Russell <[email protected]>
Date: Tue, 15 Jan 2008 21:41:55 +1100

Quoted 6 lines

Implement skb_partial_csum_set, for setting partial csums on untrusted packets.

Use it in virtio_net (replacing buggy version there), it's also going to be used by TAP for partial csum support.

Signed-off-by Rusty Russell <[email protected]>

Looks fine to me.

Acked-by David S. Miller <[email protected]>

If you like I can merge this into my net-2.6.25 tree, or alternatively if it makes your life easier you then you can handle it yourself.

maintainer Acked-by on v1 1/3
#18 lore raw
In reply to Rusty Russell

From: Rusty Russell <[email protected]>
Date: Tue, 15 Jan 2008 21:41:55 +1100

Quoted 6 lines

Implement skb_partial_csum_set, for setting partial csums on untrusted packets.

Use it in virtio_net (replacing buggy version there), it's also going to be used by TAP for partial csum support.

Signed-off-by Rusty Russell <[email protected]>

Looks fine to me.

Acked-by David S. Miller <[email protected]>

If you like I can merge this into my net-2.6.25 tree, or alternatively if it makes your life easier you then you can handle it yourself.

author
#16 lore raw
In reply to David Miller
Quoted 9 lines · On Tuesday 15 January 2008 22:14:22 David Miller wrote:

From: Rusty Russell <[email protected]>
Date: Tue, 15 Jan 2008 21:41:55 +1100

Quoted 6 lines

Implement skb_partial_csum_set, for setting partial csums on untrusted packets.

Use it in virtio_net (replacing buggy version there), it's also going to be used by TAP for partial csum support.

Quoted 8 lines
Quoted 1 line
Signed-off-by Rusty Russell <[email protected]>

Looks fine to me.

Acked-by David S. Miller <[email protected]>

If you like I can merge this into my net-2.6.25 tree, or alternatively if it makes your life easier you then you can handle it yourself.

Thanks, that will reduce coordination pain.

Cheers,
Rusty.

author
#17 lore raw
In reply to David Miller
Quoted 9 lines · On Tuesday 15 January 2008 22:14:22 David Miller wrote:

From: Rusty Russell <[email protected]>
Date: Tue, 15 Jan 2008 21:41:55 +1100

Quoted 6 lines

Implement skb_partial_csum_set, for setting partial csums on untrusted packets.

Use it in virtio_net (replacing buggy version there), it's also going to be used by TAP for partial csum support.

Quoted 8 lines
Quoted 1 line
Signed-off-by Rusty Russell <[email protected]>

Looks fine to me.

Acked-by David S. Miller <[email protected]>

If you like I can merge this into my net-2.6.25 tree, or alternatively if it makes your life easier you then you can handle it yourself.

Thanks, that will reduce coordination pain.

Cheers,
Rusty.

#5 lore raw

Re: [PATCH 2/3] virtio: Net header needs gso_hdr_len

In reply to Rusty Russell
Quoted 14 lines · Rusty Russell

It's far easier to deal with GSO if we don't have to parse the packet to figure out the header length. Add the field to the virtio_net_hdr struct (and fix the spaces that somehow crept in there).

Signed-off-by Rusty Russell <[email protected]>

---
drivers/net/virtio_net.c | 4 +++-
include/linux/virtio_net.h | 11 ++++++-----

2 files changed, 9 insertions(+), 6 deletions(-)

diff -r 24ef33a4ab14 drivers/net/virtio_net.c

drivers/net/virtio_net.c Tue Jan 15 21:21:40 2008 +1100 static void receive_skb(struct net_devic · line 129

Quoted 4 lines
drivers/net/virtio_net.c Tue Jan 15 21:21:40 2008 +1100 renamed from drivers/net/virtio_net.c Tue Jan 15 16:59:58 2008 +1100 +1 -0
@@ -126,6 +126,7 @@ static void receive_skb(struct net_devic
               /* Header must be checked, and gso_segs computed. */
               skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
               skb_shinfo(skb)->gso_segs = 0;
+               skb_set_transport_header(skb, hdr->gso_hdr_len);

Why do we need this? When receiving GSO packets from an untrusted source the network stack will fill in the transport header offset after verifying that the headers are sane.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#6 lore raw

Re: [PATCH 2/3] virtio: Net header needs gso_hdr_len

In reply to Rusty Russell
Quoted 14 lines · Rusty Russell

It's far easier to deal with GSO if we don't have to parse the packet to figure out the header length. Add the field to the virtio_net_hdr struct (and fix the spaces that somehow crept in there).

Signed-off-by Rusty Russell <[email protected]>

---
drivers/net/virtio_net.c | 4 +++-
include/linux/virtio_net.h | 11 ++++++-----

2 files changed, 9 insertions(+), 6 deletions(-)

diff -r 24ef33a4ab14 drivers/net/virtio_net.c

drivers/net/virtio_net.c Tue Jan 15 21:21:40 2008 +1100 static void receive_skb(struct net_devic · line 129

Quoted 4 lines
drivers/net/virtio_net.c Tue Jan 15 21:21:40 2008 +1100 renamed from drivers/net/virtio_net.c Tue Jan 15 16:59:58 2008 +1100 +1 -0
@@ -126,6 +126,7 @@ static void receive_skb(struct net_devic
               /* Header must be checked, and gso_segs computed. */
               skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
               skb_shinfo(skb)->gso_segs = 0;
+               skb_set_transport_header(skb, hdr->gso_hdr_len);

Why do we need this? When receiving GSO packets from an untrusted source the network stack will fill in the transport header offset after verifying that the headers are sane.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#7 lore raw

Re: [PATCH 2/3] virtio: Net header needs gso_hdr_len

Quoted 11 lines · On Wednesday 16 January 2008 11:06:21 Herbert Xu wrote:
Quoted 10 lines · Rusty Russell

It's far easier to deal with GSO if we don't have to parse the packet to figure out the header length. Add the field to the virtio_net_hdr struct (and fix the spaces that somehow crept in there).

Signed-off-by Rusty Russell <[email protected]>

---
drivers/net/virtio_net.c | 4 +++-
include/linux/virtio_net.h | 11 ++++++-----

2 files changed, 9 insertions(+), 6 deletions(-)
Quoted 12 lines

drivers/net/virtio_net.c Tue Jan 15 21:21:40 2008 +1100 static void receive_skb(struct net_devic · line 129

Quoted 8 lines

diff -r 24ef33a4ab14 drivers/net/virtio_net.c

drivers/net/virtio_net.c Tue Jan 15 21:21:40 2008 +1100 renamed from drivers/net/virtio_net.c Tue Jan 15 16:59:58 2008 +1100 +1 -0
@@ -126,6 +126,7 @@ static void receive_skb(struct net_devic
               /* Header must be checked, and gso_segs computed. */
               skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
               skb_shinfo(skb)->gso_segs = 0;
+               skb_set_transport_header(skb, hdr->gso_hdr_len);

Why do we need this? When receiving GSO packets from an untrusted source the network stack will fill in the transport header offset after verifying that the headers are sane.

Thanks for clarifying; it simplifies things.

I'll re-test and resend.
Rusty.

author
#8 lore raw

Re: [PATCH 2/3] virtio: Net header needs gso_hdr_len

In reply to Herbert Xu
Quoted 11 lines · On Wednesday 16 January 2008 11:06:21 Herbert Xu wrote:
Quoted 10 lines · Rusty Russell

It's far easier to deal with GSO if we don't have to parse the packet to figure out the header length. Add the field to the virtio_net_hdr struct (and fix the spaces that somehow crept in there).

Signed-off-by Rusty Russell <[email protected]>

---
drivers/net/virtio_net.c | 4 +++-
include/linux/virtio_net.h | 11 ++++++-----

2 files changed, 9 insertions(+), 6 deletions(-)
Quoted 12 lines

drivers/net/virtio_net.c Tue Jan 15 21:21:40 2008 +1100 static void receive_skb(struct net_devic · line 129

Quoted 8 lines

diff -r 24ef33a4ab14 drivers/net/virtio_net.c

drivers/net/virtio_net.c Tue Jan 15 21:21:40 2008 +1100 renamed from drivers/net/virtio_net.c Tue Jan 15 16:59:58 2008 +1100 +1 -0
@@ -126,6 +126,7 @@ static void receive_skb(struct net_devic
               /* Header must be checked, and gso_segs computed. */
               skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
               skb_shinfo(skb)->gso_segs = 0;
+               skb_set_transport_header(skb, hdr->gso_hdr_len);

Why do we need this? When receiving GSO packets from an untrusted source the network stack will fill in the transport header offset after verifying that the headers are sane.

Thanks for clarifying; it simplifies things.

I'll re-test and resend.
Rusty.

#9 lore raw

Re: [PATCH 2/3] virtio: Net header needs gso_hdr_len

Quoted 9 lines · Rusty Russell
Quoted 7 lines
Quoted 3 lines

It's far easier to deal with GSO if we don't have to parse the packet to figure out the header length. Add the field to the virtio_net_hdr struct (and fix the spaces that somehow crept in there).

Why do we need this? When receiving GSO packets from an untrusted source the network stack will fill in the transport header offset after verifying that the headers are sane.

Thanks for clarifying; it simplifies things.

Actually now that I've tried your test program I can see that this field exists not because of GSO, but because of SG. It tells you how many bytes you want to put in the skb head as opposed to the frag array.

So this field is fine with me as long as it is named as such to avoid confusion since it really has nothing to do with GSO as you also need it for SG with large MTUs.

I think this is more flexible than the Xen approach where this is essentially hard-coded to 64 bytes.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#10 lore raw

Re: [PATCH 2/3] virtio: Net header needs gso_hdr_len

In reply to Rusty Russell
Quoted 9 lines · Rusty Russell
Quoted 7 lines
Quoted 3 lines

It's far easier to deal with GSO if we don't have to parse the packet to figure out the header length. Add the field to the virtio_net_hdr struct (and fix the spaces that somehow crept in there).

Why do we need this? When receiving GSO packets from an untrusted source the network stack will fill in the transport header offset after verifying that the headers are sane.

Thanks for clarifying; it simplifies things.

Actually now that I've tried your test program I can see that this field exists not because of GSO, but because of SG. It tells you how many bytes you want to put in the skb head as opposed to the frag array.

So this field is fine with me as long as it is named as such to avoid confusion since it really has nothing to do with GSO as you also need it for SG with large MTUs.

I think this is more flexible than the Xen approach where this is essentially hard-coded to 64 bytes.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#11 lore raw

Re: [PATCH 2/3] virtio: Net header needs gso_hdr_len

Quoted 9 lines · On Tuesday 22 January 2008 21:36:30 Herbert Xu wrote:
Quoted 8 lines · Rusty Russell
Quoted 7 lines
Quoted 3 lines

It's far easier to deal with GSO if we don't have to parse the packet to figure out the header length. Add the field to the virtio_net_hdr struct (and fix the spaces that somehow crept in there).

Why do we need this? When receiving GSO packets from an untrusted source the network stack will fill in the transport header offset after verifying that the headers are sane.

Quoted 6 lines
Quoted 1 line

Thanks for clarifying; it simplifies things.

Actually now that I've tried your test program I can see that this field exists not because of GSO, but because of SG. It tells you how many bytes you want to put in the skb head as opposed to the frag array.

Yes, I took it out after your comments, then realized I needed it and put it back.

Quoted 3 lines

So this field is fine with me as long as it is named as such to avoid confusion since it really has nothing to do with GSO as you also need it for SG with large MTUs.

Hmm, how about just "hdr_len" rather than "gso_hdr_len"?

Thanks,
Rusty.

author
#12 lore raw

Re: [PATCH 2/3] virtio: Net header needs gso_hdr_len

In reply to Herbert Xu
Quoted 9 lines · On Tuesday 22 January 2008 21:36:30 Herbert Xu wrote:
Quoted 8 lines · Rusty Russell
Quoted 7 lines
Quoted 3 lines

It's far easier to deal with GSO if we don't have to parse the packet to figure out the header length. Add the field to the virtio_net_hdr struct (and fix the spaces that somehow crept in there).

Why do we need this? When receiving GSO packets from an untrusted source the network stack will fill in the transport header offset after verifying that the headers are sane.

Quoted 6 lines
Quoted 1 line

Thanks for clarifying; it simplifies things.

Actually now that I've tried your test program I can see that this field exists not because of GSO, but because of SG. It tells you how many bytes you want to put in the skb head as opposed to the frag array.

Yes, I took it out after your comments, then realized I needed it and put it back.

Quoted 3 lines

So this field is fine with me as long as it is named as such to avoid confusion since it really has nothing to do with GSO as you also need it for SG with large MTUs.

Hmm, how about just "hdr_len" rather than "gso_hdr_len"?

Thanks,
Rusty.

#13 lore raw

Re: [PATCH 2/3] virtio: Net header needs gso_hdr_len

Quoted 6 lines · Rusty Russell
Quoted 4 lines

So this field is fine with me as long as it is named as such to avoid confusion since it really has nothing to do with GSO as you also need it for SG with large MTUs.

Hmm, how about just "hdr_len" rather than "gso_hdr_len"?

Sounds fine to me.

Thanks,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#14 lore raw

Re: [PATCH 2/3] virtio: Net header needs gso_hdr_len

In reply to Rusty Russell
Quoted 6 lines · Rusty Russell
Quoted 4 lines

So this field is fine with me as long as it is named as such to avoid confusion since it really has nothing to do with GSO as you also need it for SG with large MTUs.

Hmm, how about just "hdr_len" rather than "gso_hdr_len"?

Sounds fine to me.

Thanks,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt