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

[PATCH 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.

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 | 7 +++++++
 2 files changed, 7 insertions(+), 3 deletions(-)

-- 
1.9.0

[PATCH 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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index ff111a8..ba91084 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -322,6 +322,13 @@ 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.
+		 */
+		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 1/2] mactap: Fix checksum errors for non-gso packets in bridge mode

[Michael S. Tsirkin]

On Wed, Apr 23, 2014 at 12:51:40PM -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>
> ---
>  drivers/net/macvtap.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index ff111a8..ba91084 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -322,6 +322,13 @@ 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.
> +		 */
> +		if (skb->ip_summed == CHECKSUM_PARTIAL &&
> +		    !(features & NETIF_F_ALL_CSUM) &&
> +		    skb_checksum_help(skb))
> +			goto drop;

Hmm confused by NETIF_F_ALL_CSUM here.

features come from here:
                feature_mask = NETIF_F_HW_CSUM;

                if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
                        if (arg & TUN_F_TSO_ECN)
                                feature_mask |= NETIF_F_TSO_ECN;
                        if (arg & TUN_F_TSO4)
                                feature_mask |= NETIF_F_TSO;
                        if (arg & TUN_F_TSO6)
                                feature_mask |= NETIF_F_TSO6;
                }

                if (arg & TUN_F_UFO)
                        feature_mask |= NETIF_F_UFO;


okay so why not just check that NETIF_F_HW_CSUM is set?

Also does it matter whether specific offloads are enabled?


>  		skb_queue_tail(&q->sk.sk_receive_queue, skb);
>  	}
>  
> -- 
> 1.9.0

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

[Vlad Yasevich]

On 04/23/2014 03:20 PM, Michael S. Tsirkin wrote:
> On Wed, Apr 23, 2014 at 12:51:40PM -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>
>> ---
>>  drivers/net/macvtap.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>> index ff111a8..ba91084 100644
>> --- a/drivers/net/macvtap.c
>> +++ b/drivers/net/macvtap.c
>> @@ -322,6 +322,13 @@ 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.
>> +		 */
>> +		if (skb->ip_summed == CHECKSUM_PARTIAL &&
>> +		    !(features & NETIF_F_ALL_CSUM) &&
>> +		    skb_checksum_help(skb))
>> +			goto drop;
> 
> Hmm confused by NETIF_F_ALL_CSUM here.
> 
> features come from here:
>                 feature_mask = NETIF_F_HW_CSUM;
> 
>                 if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
>                         if (arg & TUN_F_TSO_ECN)
>                                 feature_mask |= NETIF_F_TSO_ECN;
>                         if (arg & TUN_F_TSO4)
>                                 feature_mask |= NETIF_F_TSO;
>                         if (arg & TUN_F_TSO6)
>                                 feature_mask |= NETIF_F_TSO6;
>                 }
> 
>                 if (arg & TUN_F_UFO)
>                         feature_mask |= NETIF_F_UFO;
> 
> 
> okay so why not just check that NETIF_F_HW_CSUM is set?

We can do that, but it doesn't make much difference.

> 
> Also does it matter whether specific offloads are enabled?
> 

No it doesn't matter at all.  The packet is not a GSO packet
so no other acceleration is used.
Also, other offloads are dependent on checksum.

-vlad

> 
>>  		skb_queue_tail(&q->sk.sk_receive_queue, skb);
>>  	}
>>  
>> -- 
>> 1.9.0

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

[Michael S. Tsirkin]

On Wed, Apr 23, 2014 at 03:39:44PM -0400, Vlad Yasevich wrote:
> On 04/23/2014 03:20 PM, Michael S. Tsirkin wrote:
> > On Wed, Apr 23, 2014 at 12:51:40PM -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>
> >> ---
> >>  drivers/net/macvtap.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >> index ff111a8..ba91084 100644
> >> --- a/drivers/net/macvtap.c
> >> +++ b/drivers/net/macvtap.c
> >> @@ -322,6 +322,13 @@ 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.
> >> +		 */
> >> +		if (skb->ip_summed == CHECKSUM_PARTIAL &&
> >> +		    !(features & NETIF_F_ALL_CSUM) &&
> >> +		    skb_checksum_help(skb))
> >> +			goto drop;
> > 
> > Hmm confused by NETIF_F_ALL_CSUM here.
> > 
> > features come from here:
> >                 feature_mask = NETIF_F_HW_CSUM;
> > 
> >                 if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
> >                         if (arg & TUN_F_TSO_ECN)
> >                                 feature_mask |= NETIF_F_TSO_ECN;
> >                         if (arg & TUN_F_TSO4)
> >                                 feature_mask |= NETIF_F_TSO;
> >                         if (arg & TUN_F_TSO6)
> >                                 feature_mask |= NETIF_F_TSO6;
> >                 }
> > 
> >                 if (arg & TUN_F_UFO)
> >                         feature_mask |= NETIF_F_UFO;
> > 
> > 
> > okay so why not just check that NETIF_F_HW_CSUM is set?
> 
> We can do that, but it doesn't make much difference.

Seems cleaner to test a single bit otherwise one is left
wondering what happens if only one bit matches.

> > 
> > Also does it matter whether specific offloads are enabled?
> > 
> 
> No it doesn't matter at all.  The packet is not a GSO packet
> so no other acceleration is used.

Hmm how do we know it's not a gso packet?
All I see is need_gso test which means it needs segmentation.


> Also, other offloads are dependent on checksum.
> 
> -vlad

Right so what if checksum is on, but segmentation is off?
Not the case with e1000 today but can be with other userspace.


> > 
> >>  		skb_queue_tail(&q->sk.sk_receive_queue, skb);
> >>  	}
> >>  
> >> -- 
> >> 1.9.0

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

[Vlad Yasevich]

On 04/23/2014 04:10 PM, Michael S. Tsirkin wrote:
> On Wed, Apr 23, 2014 at 03:39:44PM -0400, Vlad Yasevich wrote:
>> On 04/23/2014 03:20 PM, Michael S. Tsirkin wrote:
>>> On Wed, Apr 23, 2014 at 12:51:40PM -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>
>>>> ---
>>>>  drivers/net/macvtap.c | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>> index ff111a8..ba91084 100644
>>>> --- a/drivers/net/macvtap.c
>>>> +++ b/drivers/net/macvtap.c
>>>> @@ -322,6 +322,13 @@ 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.
>>>> +		 */
>>>> +		if (skb->ip_summed == CHECKSUM_PARTIAL &&
>>>> +		    !(features & NETIF_F_ALL_CSUM) &&
>>>> +		    skb_checksum_help(skb))
>>>> +			goto drop;
>>>
>>> Hmm confused by NETIF_F_ALL_CSUM here.
>>>
>>> features come from here:
>>>                 feature_mask = NETIF_F_HW_CSUM;
>>>
>>>                 if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
>>>                         if (arg & TUN_F_TSO_ECN)
>>>                                 feature_mask |= NETIF_F_TSO_ECN;
>>>                         if (arg & TUN_F_TSO4)
>>>                                 feature_mask |= NETIF_F_TSO;
>>>                         if (arg & TUN_F_TSO6)
>>>                                 feature_mask |= NETIF_F_TSO6;
>>>                 }
>>>
>>>                 if (arg & TUN_F_UFO)
>>>                         feature_mask |= NETIF_F_UFO;
>>>
>>>
>>> okay so why not just check that NETIF_F_HW_CSUM is set?
>>
>> We can do that, but it doesn't make much difference.
> 
> Seems cleaner to test a single bit otherwise one is left
> wondering what happens if only one bit matches.

I can certainly do a single test, but if we ever change it,
this will be another palace that would have to change.

The above is also what dev_start_hard_xmit() does.

> 
>>>
>>> Also does it matter whether specific offloads are enabled?
>>>
>>
>> No it doesn't matter at all.  The packet is not a GSO packet
>> so no other acceleration is used.
> 
> Hmm how do we know it's not a gso packet?
> All I see is need_gso test which means it needs segmentation.

Part of netif_needs_gso() is a test for skb_is_gso().  So it
it's gso and doesn't need segmentation (meaning the guest can
receive large packets), then partial checksum is OK.

> 
> 
>> Also, other offloads are dependent on checksum.
>>
>> -vlad
> 
> Right so what if checksum is on, but segmentation is off?
> Not the case with e1000 today but can be with other userspace.
> 

In this case, the skb will be in need to segmentation and will take
a different branch.

-vlad
> 
>>>
>>>>  		skb_queue_tail(&q->sk.sk_receive_queue, skb);
>>>>  	}
>>>>  
>>>> -- 
>>>> 1.9.0

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

[Michael S. Tsirkin]

On Wed, Apr 23, 2014 at 04:30:22PM -0400, Vlad Yasevich wrote:
> On 04/23/2014 04:10 PM, Michael S. Tsirkin wrote:
> > On Wed, Apr 23, 2014 at 03:39:44PM -0400, Vlad Yasevich wrote:
> >> On 04/23/2014 03:20 PM, Michael S. Tsirkin wrote:
> >>> On Wed, Apr 23, 2014 at 12:51:40PM -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>
> >>>> ---
> >>>>  drivers/net/macvtap.c | 7 +++++++
> >>>>  1 file changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>>> index ff111a8..ba91084 100644
> >>>> --- a/drivers/net/macvtap.c
> >>>> +++ b/drivers/net/macvtap.c
> >>>> @@ -322,6 +322,13 @@ 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.
> >>>> +		 */
> >>>> +		if (skb->ip_summed == CHECKSUM_PARTIAL &&
> >>>> +		    !(features & NETIF_F_ALL_CSUM) &&
> >>>> +		    skb_checksum_help(skb))
> >>>> +			goto drop;
> >>>
> >>> Hmm confused by NETIF_F_ALL_CSUM here.
> >>>
> >>> features come from here:
> >>>                 feature_mask = NETIF_F_HW_CSUM;
> >>>
> >>>                 if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
> >>>                         if (arg & TUN_F_TSO_ECN)
> >>>                                 feature_mask |= NETIF_F_TSO_ECN;
> >>>                         if (arg & TUN_F_TSO4)
> >>>                                 feature_mask |= NETIF_F_TSO;
> >>>                         if (arg & TUN_F_TSO6)
> >>>                                 feature_mask |= NETIF_F_TSO6;
> >>>                 }
> >>>
> >>>                 if (arg & TUN_F_UFO)
> >>>                         feature_mask |= NETIF_F_UFO;
> >>>
> >>>
> >>> okay so why not just check that NETIF_F_HW_CSUM is set?
> >>
> >> We can do that, but it doesn't make much difference.
> > 
> > Seems cleaner to test a single bit otherwise one is left
> > wondering what happens if only one bit matches.
> 
> I can certainly do a single test, but if we ever change it,
> this will be another palace that would have to change.

Hmm change what exactly? Add support for selectively
disabling checksum for specific protocols?

> The above is also what dev_start_hard_xmit() does.

Yes and I was wondering about that too, but check it out: that one
calls: netif_skb_dev_features which in turn calls harmonize_features.
And there we have:
        if (skb->ip_summed != CHECKSUM_NONE &&
            !can_checksum_protocol(features, skb_network_protocol(skb, &tmp))) {
                features &= ~NETIF_F_ALL_CSUM;
        } else if (illegal_highdma(dev, skb)) {
                features &= ~NETIF_F_SG;
        }       

So NETIF_F_HW_CSUM is tested because it's cleared by a per-protocol
handling here which is not there in your patch.

Your patch is still correct - the reason harmonize_features is not
necessary is because tap either sets HW_CSUM or nothing,
can_checksum_protocol is always true or always false. But since we rely
on this anyway, isn't it better to make this explicit?

Alternatively let's clarify the comment here:

> >>>> +		/* If we receive a partial checksum and the tap side
> >>>> +		 * doesn't support checksum offload, compute the checksum.

Add:

       +                 * Note: it doesn't matter which checksum feature to
       +                 * check, we either support them all or none.

> >>>> +		 */

Fine?

> > 
> >>>
> >>> Also does it matter whether specific offloads are enabled?
> >>>
> >>
> >> No it doesn't matter at all.  The packet is not a GSO packet
> >> so no other acceleration is used.
> > 
> > Hmm how do we know it's not a gso packet?
> > All I see is need_gso test which means it needs segmentation.
> 
> Part of netif_needs_gso() is a test for skb_is_gso().  So it
> it's gso and doesn't need segmentation (meaning the guest can
> receive large packets), then partial checksum is OK.

That is correct- thanks for the clarification.


> > 
> > 
> >> Also, other offloads are dependent on checksum.
> >>
> >> -vlad
> > 
> > Right so what if checksum is on, but segmentation is off?
> > Not the case with e1000 today but can be with other userspace.
> > 
> 
> In this case, the skb will be in need to segmentation and will take
> a different branch.
> 
> -vlad
> > 
> >>>
> >>>>  		skb_queue_tail(&q->sk.sk_receive_queue, skb);
> >>>>  	}
> >>>>  
> >>>> -- 
> >>>> 1.9.0

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

[Vlad Yasevich]

On 04/24/2014 03:26 AM, Michael S. Tsirkin wrote:
> On Wed, Apr 23, 2014 at 04:30:22PM -0400, Vlad Yasevich wrote:
>> On 04/23/2014 04:10 PM, Michael S. Tsirkin wrote:
>>> On Wed, Apr 23, 2014 at 03:39:44PM -0400, Vlad Yasevich wrote:
>>>> On 04/23/2014 03:20 PM, Michael S. Tsirkin wrote:
>>>>> On Wed, Apr 23, 2014 at 12:51:40PM -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>
>>>>>> ---
>>>>>>  drivers/net/macvtap.c | 7 +++++++
>>>>>>  1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>>>> index ff111a8..ba91084 100644
>>>>>> --- a/drivers/net/macvtap.c
>>>>>> +++ b/drivers/net/macvtap.c
>>>>>> @@ -322,6 +322,13 @@ 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.
>>>>>> +		 */
>>>>>> +		if (skb->ip_summed == CHECKSUM_PARTIAL &&
>>>>>> +		    !(features & NETIF_F_ALL_CSUM) &&
>>>>>> +		    skb_checksum_help(skb))
>>>>>> +			goto drop;
>>>>>
>>>>> Hmm confused by NETIF_F_ALL_CSUM here.
>>>>>
>>>>> features come from here:
>>>>>                 feature_mask = NETIF_F_HW_CSUM;
>>>>>
>>>>>                 if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
>>>>>                         if (arg & TUN_F_TSO_ECN)
>>>>>                                 feature_mask |= NETIF_F_TSO_ECN;
>>>>>                         if (arg & TUN_F_TSO4)
>>>>>                                 feature_mask |= NETIF_F_TSO;
>>>>>                         if (arg & TUN_F_TSO6)
>>>>>                                 feature_mask |= NETIF_F_TSO6;
>>>>>                 }
>>>>>
>>>>>                 if (arg & TUN_F_UFO)
>>>>>                         feature_mask |= NETIF_F_UFO;
>>>>>
>>>>>
>>>>> okay so why not just check that NETIF_F_HW_CSUM is set?
>>>>
>>>> We can do that, but it doesn't make much difference.
>>>
>>> Seems cleaner to test a single bit otherwise one is left
>>> wondering what happens if only one bit matches.
>>
>> I can certainly do a single test, but if we ever change it,
>> this will be another palace that would have to change.
> 
> Hmm change what exactly? Add support for selectively
> disabling checksum for specific protocols?

Yes.  Right now, we kind-of lump them all together, but there
are some protocols that are not accounted for in HW_CSUM.

For instance, I am looking to add SCTP checksum offload.  It
would be very useful if the host has SCTP-capable nic.

> 
>> The above is also what dev_start_hard_xmit() does.
> 
> Yes and I was wondering about that too, but check it out: that one
> calls: netif_skb_dev_features which in turn calls harmonize_features.
> And there we have:
>         if (skb->ip_summed != CHECKSUM_NONE &&
>             !can_checksum_protocol(features, skb_network_protocol(skb, &tmp))) {
>                 features &= ~NETIF_F_ALL_CSUM;
>         } else if (illegal_highdma(dev, skb)) {
>                 features &= ~NETIF_F_SG;
>         }       
> 
> So NETIF_F_HW_CSUM is tested because it's cleared by a per-protocol
> handling here which is not there in your patch.
> 
> Your patch is still correct - the reason harmonize_features is not
> necessary is because tap either sets HW_CSUM or nothing,
> can_checksum_protocol is always true or always false. But since we rely
> on this anyway, isn't it better to make this explicit?
> 
> Alternatively let's clarify the comment here:
> 
>>>>>> +		/* If we receive a partial checksum and the tap side
>>>>>> +		 * doesn't support checksum offload, compute the checksum.
> 
> Add:
> 
>        +                 * Note: it doesn't matter which checksum feature to
>        +                 * check, we either support them all or none.
> 
>>>>>> +		 */
> 
> Fine?

Looks good to me.  I'll update and re-submit.

Thanks
-vlad

> 
>>>
>>>>>
>>>>> Also does it matter whether specific offloads are enabled?
>>>>>
>>>>
>>>> No it doesn't matter at all.  The packet is not a GSO packet
>>>> so no other acceleration is used.
>>>
>>> Hmm how do we know it's not a gso packet?
>>> All I see is need_gso test which means it needs segmentation.
>>
>> Part of netif_needs_gso() is a test for skb_is_gso().  So it
>> it's gso and doesn't need segmentation (meaning the guest can
>> receive large packets), then partial checksum is OK.
> 
> That is correct- thanks for the clarification.
> 
> 
>>>
>>>
>>>> Also, other offloads are dependent on checksum.
>>>>
>>>> -vlad
>>>
>>> Right so what if checksum is on, but segmentation is off?
>>> Not the case with e1000 today but can be with other userspace.
>>>
>>
>> In this case, the skb will be in need to segmentation and will take
>> a different branch.
>>
>> -vlad
>>>
>>>>>
>>>>>>  		skb_queue_tail(&q->sk.sk_receive_queue, skb);
>>>>>>  	}
>>>>>>  
>>>>>> -- 
>>>>>> 1.9.0

[PATCH 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