[PATCH 0/3] virtio_net GSO/partial csum fixes

[PATCH 0/3] virtio_net GSO/partial csum fixes

[Mark McLoughlin]

Hi Rusty,
        Here's a few virtio_net fixes related to partial csums
and GSO. These come about from playing with the tun/tap based
GSO patches you and Herbert posted earlier.

        3/3 is the signifcant one - I didn't actually reproduce
the problem with virtio_net, but rather observed it with the
tun/tap patch. In that case, packets which were sent by the
guest with a partial csum were ending up with an incorrect
csum when they were forwarded to a physical network.

Cheers,
Mark.

[PATCH 1/3] virtio: Fix typo in virtio_net_hdr comments

[Mark McLoughlin]

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 include/linux/virtio_net.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 9405aa6..38c0571 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -38,7 +38,7 @@ struct virtio_net_hdr
 #define VIRTIO_NET_HDR_GSO_ECN		0x80	// TCP has ECN set
 	__u8 gso_type;
 	__u16 hdr_len;		/* Ethernet + IP + tcp/udp hdrs */
-	__u16 gso_size;		/* Bytes to append to gso_hdr_len per frame */
+	__u16 gso_size;		/* Bytes to append to hdr_len per frame */
 	__u16 csum_start;	/* Position to start checksumming from */
 	__u16 csum_offset;	/* Offset after that to place checksum */
 };
-- 
1.5.4.1

[PATCH 2/3] virtio_net: Trivial coding style fix

[Mark McLoughlin]

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 drivers/net/virtio_net.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index fe7cdf2..50e6e59 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -94,7 +94,7 @@ static void receive_skb(struct net_device *dev, struct sk_buff *skb,
 
 	if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
 		pr_debug("Needs csum!\n");
-		if (!skb_partial_csum_set(skb,hdr->csum_start,hdr->csum_offset))
+		if (!skb_partial_csum_set(skb, hdr->csum_start, hdr->csum_offset))
 			goto frame_err;
 	}
 
-- 
1.5.4.1

[PATCH 3/3] virtio_net: Fix skb->csum_start computation

[Mark McLoughlin]

hdr->csum_start is the offset from the start of the ethernet
header to the transport layer checksum field. skb->csum_start
is the offset from skb->head.

skb_partial_csum_set() assumes that skb->data points to the
ethernet header - i.e. it computes skb->csum_start by adding
the headroom to hdr->csum_start.

Since eth_type_trans() skb_pull()s the ethernet header,
skb_partial_csum_set() should be called before
eth_type_trans().

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 drivers/net/virtio_net.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 50e6e59..503486f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -86,9 +86,7 @@ static void receive_skb(struct net_device *dev, struct sk_buff *skb,
 	BUG_ON(len > MAX_PACKET_LEN);
 
 	skb_trim(skb, len);
-	skb->protocol = eth_type_trans(skb, dev);
-	pr_debug("Receiving skb proto 0x%04x len %i type %i\n",
-		 ntohs(skb->protocol), skb->len, skb->pkt_type);
+
 	dev->stats.rx_bytes += skb->len;
 	dev->stats.rx_packets++;
 
@@ -98,6 +96,10 @@ static void receive_skb(struct net_device *dev, struct sk_buff *skb,
 			goto frame_err;
 	}
 
+	skb->protocol = eth_type_trans(skb, dev);
+	pr_debug("Receiving skb proto 0x%04x len %i type %i\n",
+		 ntohs(skb->protocol), skb->len, skb->pkt_type);
+
 	if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
 		pr_debug("GSO!\n");
 		switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
-- 
1.5.4.1

Re: [PATCH 2/3] virtio_net: Trivial coding style fix

[Rusty Russell]

On Tuesday 27 May 2008 21:20:46 Mark McLoughlin wrote:
> -		if (!skb_partial_csum_set(skb,hdr->csum_start,hdr->csum_offset))
> +		if (!skb_partial_csum_set(skb, hdr->csum_start, hdr->csum_offset))

I compressed the whitespace to avoid going over 80 cols.  A choice of lesser 
evils :)

Cheers,
Rusty.

Re: [PATCH 3/3] virtio_net: Fix skb->csum_start computation

[Herbert Xu]

On Tue, May 27, 2008 at 12:20:47PM +0100, Mark McLoughlin wrote:
> hdr->csum_start is the offset from the start of the ethernet
> header to the transport layer checksum field. skb->csum_start
> is the offset from skb->head.
> 
> skb_partial_csum_set() assumes that skb->data points to the
> ethernet header - i.e. it computes skb->csum_start by adding
> the headroom to hdr->csum_start.
> 
> Since eth_type_trans() skb_pull()s the ethernet header,
> skb_partial_csum_set() should be called before
> eth_type_trans().
> 
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>

Good catch! Clearly shows I never ran this across a real Ethernet
device :)

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 3/3] virtio_net: Fix skb->csum_start computation

[Rusty Russell]

On Tuesday 27 May 2008 21:20:47 Mark McLoughlin wrote:
> hdr->csum_start is the offset from the start of the ethernet
> header to the transport layer checksum field. skb->csum_start
> is the offset from skb->head.
>
> skb_partial_csum_set() assumes that skb->data points to the
> ethernet header - i.e. it computes skb->csum_start by adding
> the headroom to hdr->csum_start.
>
> Since eth_type_trans() skb_pull()s the ethernet header,
> skb_partial_csum_set() should be called before
> eth_type_trans().

As should the rx_bytes += skb->len.  Thanks for this!

Applied (and 1/3, the typo patch).

I'll backport this to -stable, as well.

Thanks,
Rusty.

Re: [PATCH 3/3] virtio_net: Fix skb->csum_start computation

[Rusty Russell]

On Tuesday 27 May 2008 21:20:47 Mark McLoughlin wrote:
> hdr->csum_start is the offset from the start of the ethernet
> header to the transport layer checksum field. skb->csum_start
> is the offset from skb->head.
>
> skb_partial_csum_set() assumes that skb->data points to the
> ethernet header - i.e. it computes skb->csum_start by adding
> the headroom to hdr->csum_start.
>
> Since eth_type_trans() skb_pull()s the ethernet header,
> skb_partial_csum_set() should be called before
> eth_type_trans().

As should the rx_bytes += skb->len.  Thanks for this!

Applied (and 1/3, the typo patch).

I'll backport this to -stable, as well.

Thanks,
Rusty.