[PATCH v2 net 0/2] Fix macvtap checksum errors in bridge mode

[PATCH v2 net 0/2] Fix macvtap checksum errors in bridge mode

[Vlad Yasevich]

The following is a problematic configuration:

 VM1: virtio-net device connected to macvtap0@eth0
 VM2: e1000 device connect to macvtap1@eth0

The problem is is that virtio-net supports checksum offloading
and thus sends the packets to the host with CHECKSUM_PARTIAL set.
On the other hand, e1000 does not support any acceleration.

For small TCP packets (and this includes the 3-way handshake),
e1000 end up receiving packets that only have a partial checksum
set.  This causes TCP to fail checksum validation and to drop
packets.  As a result tcp connections can not be established.

The following 2 patches resolve this issue. The first patch adds
a check to the non-gso code path to see if the checksum needs to
be computed.  The second patch reverts an old commit that set
ip_summed to CHECKSUM_UNNECESSARY.  Proper checksum update is
necessary under certain circumstances.

I wend through the old thread
  http://comments.gmane.org/gmane.linux.kernel.containers.lxc.general/1459
and tried the reproducers listed there, but could not cause
invalid checksum to trigger with this series.
Daniel and Andrian, if you have the time please try this patch
set to see if you still see the old checksum issues.

v1->v2: Updated comment from MST.

Vlad Yasevich (2):
  mactap: Fix checksum errors for non-gso packets in bridge mode
  Revert "macvlan : fix checksums error when we are in bridge mode"

 drivers/net/macvlan.c | 3 ---
 drivers/net/macvtap.c | 9 +++++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

-- 
1.9.0

[PATCH v2 net 1/2] mactap: Fix checksum errors for non-gso packets in bridge mode

[Vlad Yasevich]

The following is a problematic configuration:

 VM1: virtio-net device connected to macvtap0@eth0
 VM2: e1000 device connect to macvtap1@eth0

The problem is is that virtio-net supports checksum offloading
and thus sends the packets to the host with CHECKSUM_PARTIAL set.
On the other hand, e1000 does not support any acceleration.

For small TCP packets (and this includes the 3-way handshake),
e1000 ends up receiving packets that only have a partial checksum
set.  This causes TCP to fail checksum validation and to drop
packets.  As a result tcp connections can not be established.

Commit 3e4f8b787370978733ca6cae452720a4f0c296b8
	macvtap: Perform GSO on forwarding path.
fixes this issue for large packets wthat will end up undergoing GSO.
This commit adds a check for the non-GSO case and attempts to
compute the checksum for partially checksummed packets in the
non-GSO case.

CC: Daniel Lezcano <daniel.lezcano@free.fr>
CC: Patrick McHardy <kaber@trash.net>
CC: Andrian Nord <nightnord@gmail.com>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Jason Wang <jasowang@redhat.com>
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvtap.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index ff111a8..3381c4f 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -322,6 +322,15 @@ static rx_handler_result_t macvtap_handle_frame(struct sk_buff **pskb)
 			segs = nskb;
 		}
 	} else {
+		/* If we receive a partial checksum and the tap side
+		 * doesn't support checksum offload, compute the checksum.
+		 * Note: it doesn't matter which checksum feature to
+		 *        check, we either support them all or none.
+		 */
+		if (skb->ip_summed == CHECKSUM_PARTIAL &&
+		    !(features & NETIF_F_ALL_CSUM) &&
+		    skb_checksum_help(skb))
+			goto drop;
 		skb_queue_tail(&q->sk.sk_receive_queue, skb);
 	}
 
-- 
1.9.0

Re: [PATCH v2 net 1/2] mactap: Fix checksum errors for non-gso packets in bridge mode

[Michael S. Tsirkin]

On Tue, Apr 29, 2014 at 10:09:50AM -0400, Vlad Yasevich wrote:
> The following is a problematic configuration:
> 
>  VM1: virtio-net device connected to macvtap0@eth0
>  VM2: e1000 device connect to macvtap1@eth0
> 
> The problem is is that virtio-net supports checksum offloading
> and thus sends the packets to the host with CHECKSUM_PARTIAL set.
> On the other hand, e1000 does not support any acceleration.
> 
> For small TCP packets (and this includes the 3-way handshake),
> e1000 ends up receiving packets that only have a partial checksum
> set.  This causes TCP to fail checksum validation and to drop
> packets.  As a result tcp connections can not be established.
> 
> Commit 3e4f8b787370978733ca6cae452720a4f0c296b8
> 	macvtap: Perform GSO on forwarding path.
> fixes this issue for large packets wthat will end up undergoing GSO.
> This commit adds a check for the non-GSO case and attempts to
> compute the checksum for partially checksummed packets in the
> non-GSO case.
> 
> CC: Daniel Lezcano <daniel.lezcano@free.fr>
> CC: Patrick McHardy <kaber@trash.net>
> CC: Andrian Nord <nightnord@gmail.com>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/net/macvtap.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index ff111a8..3381c4f 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -322,6 +322,15 @@ static rx_handler_result_t macvtap_handle_frame(struct sk_buff **pskb)
>  			segs = nskb;
>  		}
>  	} else {
> +		/* If we receive a partial checksum and the tap side
> +		 * doesn't support checksum offload, compute the checksum.
> +		 * Note: it doesn't matter which checksum feature to
> +		 *        check, we either support them all or none.
> +		 */
> +		if (skb->ip_summed == CHECKSUM_PARTIAL &&
> +		    !(features & NETIF_F_ALL_CSUM) &&
> +		    skb_checksum_help(skb))
> +			goto drop;
>  		skb_queue_tail(&q->sk.sk_receive_queue, skb);
>  	}
>  
> -- 
> 1.9.0

Re: [PATCH v2 net 1/2] mactap: Fix checksum errors for non-gso packets in bridge mode

[Jason Wang]

On 04/29/2014 10:09 PM, Vlad Yasevich wrote:
> The following is a problematic configuration:
>
>  VM1: virtio-net device connected to macvtap0@eth0
>  VM2: e1000 device connect to macvtap1@eth0
>
> The problem is is that virtio-net supports checksum offloading
> and thus sends the packets to the host with CHECKSUM_PARTIAL set.
> On the other hand, e1000 does not support any acceleration.
>
> For small TCP packets (and this includes the 3-way handshake),
> e1000 ends up receiving packets that only have a partial checksum
> set.  This causes TCP to fail checksum validation and to drop
> packets.  As a result tcp connections can not be established.
>
> Commit 3e4f8b787370978733ca6cae452720a4f0c296b8
> 	macvtap: Perform GSO on forwarding path.
> fixes this issue for large packets wthat will end up undergoing GSO.
> This commit adds a check for the non-GSO case and attempts to
> compute the checksum for partially checksummed packets in the
> non-GSO case.
>
> CC: Daniel Lezcano <daniel.lezcano@free.fr>
> CC: Patrick McHardy <kaber@trash.net>
> CC: Andrian Nord <nightnord@gmail.com>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  drivers/net/macvtap.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index ff111a8..3381c4f 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -322,6 +322,15 @@ static rx_handler_result_t macvtap_handle_frame(struct sk_buff **pskb)
>  			segs = nskb;
>  		}
>  	} else {
> +		/* If we receive a partial checksum and the tap side
> +		 * doesn't support checksum offload, compute the checksum.
> +		 * Note: it doesn't matter which checksum feature to
> +		 *        check, we either support them all or none.
> +		 */
> +		if (skb->ip_summed == CHECKSUM_PARTIAL &&
> +		    !(features & NETIF_F_ALL_CSUM) &&
> +		    skb_checksum_help(skb))
> +			goto drop;
>  		skb_queue_tail(&q->sk.sk_receive_queue, skb);
>  	}
>  

Acked-by: Jason Wang <jasowang@redhat.com>

[PATCH v2 net 2/2] Revert "macvlan : fix checksums error when we are in bridge mode"

[Vlad Yasevich]

This reverts commit 12a2856b604476c27d85a5f9a57ae1661fc46019.
The commit above doesn't appear to be necessary any more as the
checksums appear to be correctly computed/validated.

Additionally the above commit breaks kvm configurations where
one VM is using a device that support checksum offload (virtio) and
the other VM does not.
In this case, packets leaving virtio device will have CHECKSUM_PARTIAL
set.  The packets is forwarded to a macvtap that has offload features
turned off.  Since we use CHECKSUM_UNNECESSARY, the host does does not
update the checksum and thus a bad checksum is passed up to
the guest.

CC: Daniel Lezcano <daniel.lezcano@free.fr>
CC: Patrick McHardy <kaber@trash.net>
CC: Andrian Nord <nightnord@gmail.com>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Jason Wang <jasowang@redhat.com>
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvlan.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 1831fb7..33b6cf8 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -263,11 +263,9 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
 	const struct macvlan_dev *vlan = netdev_priv(dev);
 	const struct macvlan_port *port = vlan->port;
 	const struct macvlan_dev *dest;
-	__u8 ip_summed = skb->ip_summed;
 
 	if (vlan->mode == MACVLAN_MODE_BRIDGE) {
 		const struct ethhdr *eth = (void *)skb->data;
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 		/* send to other bridge ports directly */
 		if (is_multicast_ether_addr(eth->h_dest)) {
@@ -285,7 +283,6 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 xmit_world:
-	skb->ip_summed = ip_summed;
 	skb->dev = vlan->lowerdev;
 	return dev_queue_xmit(skb);
 }
-- 
1.9.0

Re: [PATCH v2 net 2/2] Revert "macvlan : fix checksums error when we are in bridge mode"

[Michael S. Tsirkin]

On Tue, Apr 29, 2014 at 10:09:51AM -0400, Vlad Yasevich wrote:
> This reverts commit 12a2856b604476c27d85a5f9a57ae1661fc46019.
> The commit above doesn't appear to be necessary any more as the
> checksums appear to be correctly computed/validated.
> 
> Additionally the above commit breaks kvm configurations where
> one VM is using a device that support checksum offload (virtio) and
> the other VM does not.
> In this case, packets leaving virtio device will have CHECKSUM_PARTIAL
> set.  The packets is forwarded to a macvtap that has offload features
> turned off.  Since we use CHECKSUM_UNNECESSARY, the host does does not
> update the checksum and thus a bad checksum is passed up to
> the guest.
> 
> CC: Daniel Lezcano <daniel.lezcano@free.fr>
> CC: Patrick McHardy <kaber@trash.net>
> CC: Andrian Nord <nightnord@gmail.com>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/net/macvlan.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 1831fb7..33b6cf8 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -263,11 +263,9 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
>  	const struct macvlan_dev *vlan = netdev_priv(dev);
>  	const struct macvlan_port *port = vlan->port;
>  	const struct macvlan_dev *dest;
> -	__u8 ip_summed = skb->ip_summed;
>  
>  	if (vlan->mode == MACVLAN_MODE_BRIDGE) {
>  		const struct ethhdr *eth = (void *)skb->data;
> -		skb->ip_summed = CHECKSUM_UNNECESSARY;
>  
>  		/* send to other bridge ports directly */
>  		if (is_multicast_ether_addr(eth->h_dest)) {
> @@ -285,7 +283,6 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
>  	}
>  
>  xmit_world:
> -	skb->ip_summed = ip_summed;
>  	skb->dev = vlan->lowerdev;
>  	return dev_queue_xmit(skb);
>  }
> -- 
> 1.9.0

Re: [PATCH v2 net 2/2] Revert "macvlan : fix checksums error when we are in bridge mode"

[Jason Wang]

On 04/29/2014 10:09 PM, Vlad Yasevich wrote:
> This reverts commit 12a2856b604476c27d85a5f9a57ae1661fc46019.
> The commit above doesn't appear to be necessary any more as the
> checksums appear to be correctly computed/validated.
>
> Additionally the above commit breaks kvm configurations where
> one VM is using a device that support checksum offload (virtio) and
> the other VM does not.
> In this case, packets leaving virtio device will have CHECKSUM_PARTIAL
> set.  The packets is forwarded to a macvtap that has offload features
> turned off.  Since we use CHECKSUM_UNNECESSARY, the host does does not
> update the checksum and thus a bad checksum is passed up to
> the guest.
>
> CC: Daniel Lezcano <daniel.lezcano@free.fr>
> CC: Patrick McHardy <kaber@trash.net>
> CC: Andrian Nord <nightnord@gmail.com>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  drivers/net/macvlan.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 1831fb7..33b6cf8 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -263,11 +263,9 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
>  	const struct macvlan_dev *vlan = netdev_priv(dev);
>  	const struct macvlan_port *port = vlan->port;
>  	const struct macvlan_dev *dest;
> -	__u8 ip_summed = skb->ip_summed;
>  
>  	if (vlan->mode == MACVLAN_MODE_BRIDGE) {
>  		const struct ethhdr *eth = (void *)skb->data;
> -		skb->ip_summed = CHECKSUM_UNNECESSARY;
>  
>  		/* send to other bridge ports directly */
>  		if (is_multicast_ether_addr(eth->h_dest)) {
> @@ -285,7 +283,6 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
>  	}
>  
>  xmit_world:
> -	skb->ip_summed = ip_summed;
>  	skb->dev = vlan->lowerdev;
>  	return dev_queue_xmit(skb);
>  }

Acked-by: Jason Wang <jasowang@redhat.com>

Re: [PATCH v2 net 0/2] Fix macvtap checksum errors in bridge mode

[David Miller]

Series applied and queued up for -stable, thanks!