Re: [PATCH 4/4] lguest: Use GSO/IFF_VNET_HDR extensions on tun/tap

[Rusty Russell <rusty@rustcorp.com.au>]

On Thursday 26 June 2008 05:07:18 Anthony Liguori wrote:
> Rusty Russell wrote:
> > @@ -1563,6 +1561,16 @@ static void setup_tun_net(char *arg)
> >  	/* Tell Guest what MAC address to use. */
> >  	add_feature(dev, VIRTIO_NET_F_MAC);
> >  	add_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY);
> > +	/* Expect Guest to handle everything except UFO */
> > +	add_feature(dev, VIRTIO_NET_F_CSUM);
>
> You're setting this feature twice.

Hmm, not in the version here?

> > +	add_feature(dev, VIRTIO_NET_F_GUEST_CSUM);
>
> You set this feature, but I never see the virtio-net driver acknowledge
> the feature.  Curiously, my implementation with KVM is struggling
> because UDP packet checksums are not correct so the DHCP client is
> ignoring them.  If I disable CSUM offload, things it works fine (using
> the virtio-net header).  The problem is only host=>guest, guest=>host is
> fine.

OK, found this: wrong args to skb_partial_csum_set.  It was found by Mark 
McLoughlin before, I just lost the fix when I extracted this into a separate 
patch.  I chose to move the call to skb_partial_csum_set(), rather than use 
his fix (which assumed a tap not tun device).

Here's two fixes on top of previous patch:

diff -u b/drivers/net/tun.c b/drivers/net/tun.c
--- b/drivers/net/tun.c	Thu Jun 26 00:21:59 2008 +1000
+++ b/drivers/net/tun.c	Thu Jun 26 14:35:03 2008 +1000
@@ -298,11 +298,11 @@
 		if ((len -= sizeof(gso)) > count)
 			return -EINVAL;
 
-		if (gso.hdr_len > len)
-			return -EINVAL;
-
 		if (memcpy_fromiovec((void *)&gso, iv, sizeof(gso)))
 			return -EFAULT;
+
+		if (gso.hdr_len > len)
+			return -EINVAL;
 	}
 
 	if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV) {
@@ -324,6 +324,16 @@
 		return -EFAULT;
 	}
 
+	if (gso.flags & VIRTIO_NET_HDR_F_NEEDS_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;
+
 	switch (tun->flags & TUN_TYPE_MASK) {
 	case TUN_TUN_DEV:
 		skb_reset_mac_header(skb);
@@ -335,16 +345,6 @@
 		break;
 	};
 
-	if (gso.flags & VIRTIO_NET_HDR_F_NEEDS_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;
-
 	if (gso.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
 		pr_debug("GSO!\n");
 		switch (gso.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {