[PATCH net-next] net: Fix remote checksum offload with GUE

[PATCH net-next] net: Fix remote checksum offload with GUE

[Tom Herbert]

In skb_segment the check of whether or not to perform the checksum on
host was changed to not consider rather remote checksum offload is
in use. In the case that can_checksum_protocol fails the checksum
is computed regardless. __skb_udp_tunnel_segment was modified in a
related patch to add NETIF_F_HW_CSUM into features when grabbing
the enc_features and remote checksum offload is being done. The
problem is that this bit can be cleared in lower GSO layers that
are also doing tunneling (e.g. ipip, GRE when used with GUE),
so when we get to skb_segment that intent has been lost and
can_checksum_protocol fails.

This patch:

- Restores the check in skb_segment to look at remote checksum offload.
- Removes the code in __skb_udp_tunnel_segment to force the
  NETIF_F_HW_CSUM feature since this is no longer useful with above
  change.
- Removes check for remote checksum offload in gso_reset_checksum.
  A special case should not be needed here.

Tested: Single netperf STREAM over GUE-ipip

Before fix:
   5625 Mbps
After fix:
   6410 Mbps

Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 include/linux/skbuff.h |  4 ----
 net/core/skbuff.c      |  5 ++---
 net/ipv4/udp_offload.c | 10 ----------
 3 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 15d0df9..f6fe8a5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
 
 static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
 {
-	/* Do not update partial checksums if remote checksum is enabled. */
-	if (skb->remcsum_offload)
-		return;
-
 	SKB_GSO_CB(skb)->csum = res;
 	SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
 }
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f044f97..e4eb78d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3259,14 +3259,13 @@ skip_fraglist:
 		nskb->truesize += nskb->data_len;
 
 perform_csum_check:
-		if (!csum) {
+		if (!csum && !nskb->remcsum_offload) {
 			if (skb_has_shared_frag(nskb)) {
 				err = __skb_linearize(nskb);
 				if (err)
 					goto err;
 			}
-			if (!nskb->remcsum_offload)
-				nskb->ip_summed = CHECKSUM_NONE;
+			nskb->ip_summed = CHECKSUM_NONE;
 			SKB_GSO_CB(nskb)->csum =
 				skb_checksum(nskb, doffset,
 					     nskb->len - doffset, 0);
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 8a3405a..f86f1e1 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
 
 	features &= skb->dev->hw_enc_features;
 
-	/* The only checksum offload we care about from here on out is the
-	 * outer one so strip the existing checksum feature flags and
-	 * instead set the flag based on our outer checksum offload value.
-	 */
-	if (remcsum || ufo) {
-		features &= ~NETIF_F_CSUM_MASK;
-		if (!need_csum || offload_csum)
-			features |= NETIF_F_HW_CSUM;
-	}
-
 	/* segment inner packet. */
 	segs = gso_inner_segment(skb, features);
 	if (IS_ERR_OR_NULL(segs)) {
-- 
2.8.0.rc2

Re: [PATCH net-next] net: Fix remote checksum offload with GUE

[Alexander Duyck]

On Tue, Mar 22, 2016 at 12:19 PM, Tom Herbert <tom@herbertland.com> wrote:
> In skb_segment the check of whether or not to perform the checksum on
> host was changed to not consider rather remote checksum offload is
> in use. In the case that can_checksum_protocol fails the checksum
> is computed regardless. __skb_udp_tunnel_segment was modified in a
> related patch to add NETIF_F_HW_CSUM into features when grabbing
> the enc_features and remote checksum offload is being done. The
> problem is that this bit can be cleared in lower GSO layers that
> are also doing tunneling (e.g. ipip, GRE when used with GUE),
> so when we get to skb_segment that intent has been lost and
> can_checksum_protocol fails.

So what you are describing sounds like a tunnel in tunnel scenario.
It might work better to just skip masking the features if
skb->remcsum_offload is set rather than trying to change how we
perform the offload.

I'm pretty sure this will cause data corruption and maybe a kernel
panic if Tx checksum offload is disabled.

> This patch:
>
> - Restores the check in skb_segment to look at remote checksum offload.
> - Removes the code in __skb_udp_tunnel_segment to force the
>   NETIF_F_HW_CSUM feature since this is no longer useful with above
>   change.
> - Removes check for remote checksum offload in gso_reset_checksum.
>   A special case should not be needed here.
>
> Tested: Single netperf STREAM over GUE-ipip
>
> Before fix:
>    5625 Mbps
> After fix:
>    6410 Mbps
>
> Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
> Signed-off-by: Tom Herbert <tom@herbertland.com>
> ---
>  include/linux/skbuff.h |  4 ----
>  net/core/skbuff.c      |  5 ++---
>  net/ipv4/udp_offload.c | 10 ----------
>  3 files changed, 2 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 15d0df9..f6fe8a5 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
>
>  static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
>  {
> -       /* Do not update partial checksums if remote checksum is enabled. */
> -       if (skb->remcsum_offload)
> -               return;
> -
>         SKB_GSO_CB(skb)->csum = res;
>         SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
>  }

I'm pretty sure this part here will break things when you don't have
an outer offload enabled.  What NIC did you test this on?  Did you try
disabling the Tx checksum support in the hardware to see what would
happen?

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index f044f97..e4eb78d 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3259,14 +3259,13 @@ skip_fraglist:
>                 nskb->truesize += nskb->data_len;
>
>  perform_csum_check:
> -               if (!csum) {
> +               if (!csum && !nskb->remcsum_offload) {
>                         if (skb_has_shared_frag(nskb)) {
>                                 err = __skb_linearize(nskb);
>                                 if (err)
>                                         goto err;
>                         }
> -                       if (!nskb->remcsum_offload)
> -                               nskb->ip_summed = CHECKSUM_NONE;
> +                       nskb->ip_summed = CHECKSUM_NONE;
>                         SKB_GSO_CB(nskb)->csum =
>                                 skb_checksum(nskb, doffset,
>                                              nskb->len - doffset, 0);

I'm pretty sure this is going to cause a huge mess if you are
requesting remote checksum but cannot perform an outer checksum.  One
of the reasons I merged these features together the way I did was
because we needed to perform the initial checksum to avoid causing a
kernel panic later on.  Otherwise we don't have the SKB_GSO_CB()->csum
and SKB_GSO_CB()->csum_start fields populated.

> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 8a3405a..f86f1e1 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>
>         features &= skb->dev->hw_enc_features;
>
> -       /* The only checksum offload we care about from here on out is the
> -        * outer one so strip the existing checksum feature flags and
> -        * instead set the flag based on our outer checksum offload value.
> -        */
> -       if (remcsum || ufo) {
> -               features &= ~NETIF_F_CSUM_MASK;
> -               if (!need_csum || offload_csum)
> -                       features |= NETIF_F_HW_CSUM;
> -       }
> -
>         /* segment inner packet. */
>         segs = gso_inner_segment(skb, features);
>         if (IS_ERR_OR_NULL(segs)) {

This part breaks UDP fragmentation I am pretty sure.  We need this bit
for UFO to avoid having to perform a checksum over the data twice if
we are offloading the outer UDP checksum.

Re: [PATCH net-next] net: Fix remote checksum offload with GUE

[Tom Herbert]

On Tue, Mar 22, 2016 at 1:20 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Tue, Mar 22, 2016 at 12:19 PM, Tom Herbert <tom@herbertland.com> wrote:
>> In skb_segment the check of whether or not to perform the checksum on
>> host was changed to not consider rather remote checksum offload is
>> in use. In the case that can_checksum_protocol fails the checksum
>> is computed regardless. __skb_udp_tunnel_segment was modified in a
>> related patch to add NETIF_F_HW_CSUM into features when grabbing
>> the enc_features and remote checksum offload is being done. The
>> problem is that this bit can be cleared in lower GSO layers that
>> are also doing tunneling (e.g. ipip, GRE when used with GUE),
>> so when we get to skb_segment that intent has been lost and
>> can_checksum_protocol fails.
>
> So what you are describing sounds like a tunnel in tunnel scenario.
> It might work better to just skip masking the features if
> skb->remcsum_offload is set rather than trying to change how we
> perform the offload.
>
To be clear, my patch is restoring the old behavior not implementing a new one.

> I'm pretty sure this will cause data corruption and maybe a kernel
> panic if Tx checksum offload is disabled.
>
Nope, working fine for me.

The outer checksum would be computed in
>> This patch:
>>
>> - Restores the check in skb_segment to look at remote checksum offload.
>> - Removes the code in __skb_udp_tunnel_segment to force the
>>   NETIF_F_HW_CSUM feature since this is no longer useful with above
>>   change.
>> - Removes check for remote checksum offload in gso_reset_checksum.
>>   A special case should not be needed here.
>>
>> Tested: Single netperf STREAM over GUE-ipip
>>
>> Before fix:
>>    5625 Mbps
>> After fix:
>>    6410 Mbps
>>
>> Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
>> Signed-off-by: Tom Herbert <tom@herbertland.com>
>> ---
>>  include/linux/skbuff.h |  4 ----
>>  net/core/skbuff.c      |  5 ++---
>>  net/ipv4/udp_offload.c | 10 ----------
>>  3 files changed, 2 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 15d0df9..f6fe8a5 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
>>
>>  static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
>>  {
>> -       /* Do not update partial checksums if remote checksum is enabled. */
>> -       if (skb->remcsum_offload)
>> -               return;
>> -
>>         SKB_GSO_CB(skb)->csum = res;
>>         SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
>>  }
>
> I'm pretty sure this part here will break things when you don't have
> an outer offload enabled.  What NIC did you test this on?  Did you try
> disabling the Tx checksum support in the hardware to see what would
> happen?
>
Mellanox.

When TX checksum is disabled the outer checksum is computed in
__skb_udp_tunnel_segment.

>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index f044f97..e4eb78d 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -3259,14 +3259,13 @@ skip_fraglist:
>>                 nskb->truesize += nskb->data_len;
>>
>>  perform_csum_check:
>> -               if (!csum) {
>> +               if (!csum && !nskb->remcsum_offload) {
>>                         if (skb_has_shared_frag(nskb)) {
>>                                 err = __skb_linearize(nskb);
>>                                 if (err)
>>                                         goto err;
>>                         }
>> -                       if (!nskb->remcsum_offload)
>> -                               nskb->ip_summed = CHECKSUM_NONE;
>> +                       nskb->ip_summed = CHECKSUM_NONE;
>>                         SKB_GSO_CB(nskb)->csum =
>>                                 skb_checksum(nskb, doffset,
>>                                              nskb->len - doffset, 0);
>
> I'm pretty sure this is going to cause a huge mess if you are
> requesting remote checksum but cannot perform an outer checksum.  One
> of the reasons I merged these features together the way I did was
> because we needed to perform the initial checksum to avoid causing a
> kernel panic later on.  Otherwise we don't have the SKB_GSO_CB()->csum
> and SKB_GSO_CB()->csum_start fields populated.
>
Nope, if we can't perform outer checksum it is done in the host. I'm
not seeing any problem.

>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index 8a3405a..f86f1e1 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>>
>>         features &= skb->dev->hw_enc_features;
>>
>> -       /* The only checksum offload we care about from here on out is the
>> -        * outer one so strip the existing checksum feature flags and
>> -        * instead set the flag based on our outer checksum offload value.
>> -        */
>> -       if (remcsum || ufo) {
>> -               features &= ~NETIF_F_CSUM_MASK;
>> -               if (!need_csum || offload_csum)
>> -                       features |= NETIF_F_HW_CSUM;
>> -       }
>> -
>>         /* segment inner packet. */
>>         segs = gso_inner_segment(skb, features);
>>         if (IS_ERR_OR_NULL(segs)) {
>
> This part breaks UDP fragmentation I am pretty sure.  We need this bit
> for UFO to avoid having to perform a checksum over the data twice if
> we are offloading the outer UDP checksum.

The maybe leave this for ufo if it is a problem (please test your
suppositions!), but this is not good for remcsum offload.

Tom

Re: [PATCH net-next] net: Fix remote checksum offload with GUE

[Alexander Duyck]

On Tue, Mar 22, 2016 at 2:10 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Tue, Mar 22, 2016 at 1:20 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Tue, Mar 22, 2016 at 12:19 PM, Tom Herbert <tom@herbertland.com> wrote:
>>> In skb_segment the check of whether or not to perform the checksum on
>>> host was changed to not consider rather remote checksum offload is
>>> in use. In the case that can_checksum_protocol fails the checksum
>>> is computed regardless. __skb_udp_tunnel_segment was modified in a
>>> related patch to add NETIF_F_HW_CSUM into features when grabbing
>>> the enc_features and remote checksum offload is being done. The
>>> problem is that this bit can be cleared in lower GSO layers that
>>> are also doing tunneling (e.g. ipip, GRE when used with GUE),
>>> so when we get to skb_segment that intent has been lost and
>>> can_checksum_protocol fails.
>>
>> So what you are describing sounds like a tunnel in tunnel scenario.
>> It might work better to just skip masking the features if
>> skb->remcsum_offload is set rather than trying to change how we
>> perform the offload.
>>
> To be clear, my patch is restoring the old behavior not implementing a new one.

Yes, but the old behavior could lead to kernel panics under certain
circumstances.

>> I'm pretty sure this will cause data corruption and maybe a kernel
>> panic if Tx checksum offload is disabled.
>>
> Nope, working fine for me.

What are the options you used to create the tunnel?  Did you enable
both remcsum and udpcsum?

> The outer checksum would be computed in
>>> This patch:
>>>
>>> - Restores the check in skb_segment to look at remote checksum offload.
>>> - Removes the code in __skb_udp_tunnel_segment to force the
>>>   NETIF_F_HW_CSUM feature since this is no longer useful with above
>>>   change.
>>> - Removes check for remote checksum offload in gso_reset_checksum.
>>>   A special case should not be needed here.
>>>
>>> Tested: Single netperf STREAM over GUE-ipip
>>>
>>> Before fix:
>>>    5625 Mbps
>>> After fix:
>>>    6410 Mbps
>>>
>>> Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
>>> Signed-off-by: Tom Herbert <tom@herbertland.com>
>>> ---
>>>  include/linux/skbuff.h |  4 ----
>>>  net/core/skbuff.c      |  5 ++---
>>>  net/ipv4/udp_offload.c | 10 ----------
>>>  3 files changed, 2 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>> index 15d0df9..f6fe8a5 100644
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
>>>
>>>  static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
>>>  {
>>> -       /* Do not update partial checksums if remote checksum is enabled. */
>>> -       if (skb->remcsum_offload)
>>> -               return;
>>> -
>>>         SKB_GSO_CB(skb)->csum = res;
>>>         SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
>>>  }
>>
>> I'm pretty sure this part here will break things when you don't have
>> an outer offload enabled.  What NIC did you test this on?  Did you try
>> disabling the Tx checksum support in the hardware to see what would
>> happen?
>>
> Mellanox.
>
> When TX checksum is disabled the outer checksum is computed in
> __skb_udp_tunnel_segment.

Yes, but that uses gso_make_checksum which requires the csum and
csum_start fields be initialized in the SKB_GSO_CB().

>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>> index f044f97..e4eb78d 100644
>>> --- a/net/core/skbuff.c
>>> +++ b/net/core/skbuff.c
>>> @@ -3259,14 +3259,13 @@ skip_fraglist:
>>>                 nskb->truesize += nskb->data_len;
>>>
>>>  perform_csum_check:
>>> -               if (!csum) {
>>> +               if (!csum && !nskb->remcsum_offload) {
>>>                         if (skb_has_shared_frag(nskb)) {
>>>                                 err = __skb_linearize(nskb);
>>>                                 if (err)
>>>                                         goto err;
>>>                         }
>>> -                       if (!nskb->remcsum_offload)
>>> -                               nskb->ip_summed = CHECKSUM_NONE;
>>> +                       nskb->ip_summed = CHECKSUM_NONE;
>>>                         SKB_GSO_CB(nskb)->csum =
>>>                                 skb_checksum(nskb, doffset,
>>>                                              nskb->len - doffset, 0);
>>
>> I'm pretty sure this is going to cause a huge mess if you are
>> requesting remote checksum but cannot perform an outer checksum.  One
>> of the reasons I merged these features together the way I did was
>> because we needed to perform the initial checksum to avoid causing a
>> kernel panic later on.  Otherwise we don't have the SKB_GSO_CB()->csum
>> and SKB_GSO_CB()->csum_start fields populated.
>>
> Nope, if we can't perform outer checksum it is done in the host. I'm
> not seeing any problem.

Are you running remcsum with udpcsum set or is udpcsum not set?  If
you don't set udpcsum it doesn't actually compute an outer checksum
value.

>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>> index 8a3405a..f86f1e1 100644
>>> --- a/net/ipv4/udp_offload.c
>>> +++ b/net/ipv4/udp_offload.c
>>> @@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>>>
>>>         features &= skb->dev->hw_enc_features;
>>>
>>> -       /* The only checksum offload we care about from here on out is the
>>> -        * outer one so strip the existing checksum feature flags and
>>> -        * instead set the flag based on our outer checksum offload value.
>>> -        */
>>> -       if (remcsum || ufo) {
>>> -               features &= ~NETIF_F_CSUM_MASK;
>>> -               if (!need_csum || offload_csum)
>>> -                       features |= NETIF_F_HW_CSUM;
>>> -       }
>>> -
>>>         /* segment inner packet. */
>>>         segs = gso_inner_segment(skb, features);
>>>         if (IS_ERR_OR_NULL(segs)) {
>>
>> This part breaks UDP fragmentation I am pretty sure.  We need this bit
>> for UFO to avoid having to perform a checksum over the data twice if
>> we are offloading the outer UDP checksum.
>
> The maybe leave this for ufo if it is a problem (please test your
> suppositions!), but this is not good for remcsum offload.

I suspect we are supposed to be setting encap_level to 0 when we set
skb->encapsulation to 0 before processing inner headers in the case of
UDP and GRE tunnel offloads.  I'll try to test and submit something in
the next hour or two.

- Alex

Re: [PATCH net-next] net: Fix remote checksum offload with GUE

[Tom Herbert]

On Tue, Mar 22, 2016 at 2:45 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Tue, Mar 22, 2016 at 2:10 PM, Tom Herbert <tom@herbertland.com> wrote:
>> On Tue, Mar 22, 2016 at 1:20 PM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Tue, Mar 22, 2016 at 12:19 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>> In skb_segment the check of whether or not to perform the checksum on
>>>> host was changed to not consider rather remote checksum offload is
>>>> in use. In the case that can_checksum_protocol fails the checksum
>>>> is computed regardless. __skb_udp_tunnel_segment was modified in a
>>>> related patch to add NETIF_F_HW_CSUM into features when grabbing
>>>> the enc_features and remote checksum offload is being done. The
>>>> problem is that this bit can be cleared in lower GSO layers that
>>>> are also doing tunneling (e.g. ipip, GRE when used with GUE),
>>>> so when we get to skb_segment that intent has been lost and
>>>> can_checksum_protocol fails.
>>>
>>> So what you are describing sounds like a tunnel in tunnel scenario.
>>> It might work better to just skip masking the features if
>>> skb->remcsum_offload is set rather than trying to change how we
>>> perform the offload.
>>>
>> To be clear, my patch is restoring the old behavior not implementing a new one.
>
> Yes, but the old behavior could lead to kernel panics under certain
> circumstances.
>
>>> I'm pretty sure this will cause data corruption and maybe a kernel
>>> panic if Tx checksum offload is disabled.
>>>
>> Nope, working fine for me.
>
> What are the options you used to create the tunnel?  Did you enable
> both remcsum and udpcsum?
>
modprobe fou
./ip fou add port 6080 gue
./ip link add name tun1 type ipip remote 10.1.1.2 local 10.1.1.2 ttl
225 encap gue encap-sport auto encap-dport 6080 encap-csum
encap-remcsum
ifconfig tun1 192.168.1.1
ip route add 192.168.1.0/24 dev tun1

Thanks,
Tom

>> The outer checksum would be computed in
>>>> This patch:
>>>>
>>>> - Restores the check in skb_segment to look at remote checksum offload.
>>>> - Removes the code in __skb_udp_tunnel_segment to force the
>>>>   NETIF_F_HW_CSUM feature since this is no longer useful with above
>>>>   change.
>>>> - Removes check for remote checksum offload in gso_reset_checksum.
>>>>   A special case should not be needed here.
>>>>
>>>> Tested: Single netperf STREAM over GUE-ipip
>>>>
>>>> Before fix:
>>>>    5625 Mbps
>>>> After fix:
>>>>    6410 Mbps
>>>>
>>>> Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
>>>> Signed-off-by: Tom Herbert <tom@herbertland.com>
>>>> ---
>>>>  include/linux/skbuff.h |  4 ----
>>>>  net/core/skbuff.c      |  5 ++---
>>>>  net/ipv4/udp_offload.c | 10 ----------
>>>>  3 files changed, 2 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>> index 15d0df9..f6fe8a5 100644
>>>> --- a/include/linux/skbuff.h
>>>> +++ b/include/linux/skbuff.h
>>>> @@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
>>>>
>>>>  static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
>>>>  {
>>>> -       /* Do not update partial checksums if remote checksum is enabled. */
>>>> -       if (skb->remcsum_offload)
>>>> -               return;
>>>> -
>>>>         SKB_GSO_CB(skb)->csum = res;
>>>>         SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
>>>>  }
>>>
>>> I'm pretty sure this part here will break things when you don't have
>>> an outer offload enabled.  What NIC did you test this on?  Did you try
>>> disabling the Tx checksum support in the hardware to see what would
>>> happen?
>>>
>> Mellanox.
>>
>> When TX checksum is disabled the outer checksum is computed in
>> __skb_udp_tunnel_segment.
>
> Yes, but that uses gso_make_checksum which requires the csum and
> csum_start fields be initialized in the SKB_GSO_CB().
>
>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>> index f044f97..e4eb78d 100644
>>>> --- a/net/core/skbuff.c
>>>> +++ b/net/core/skbuff.c
>>>> @@ -3259,14 +3259,13 @@ skip_fraglist:
>>>>                 nskb->truesize += nskb->data_len;
>>>>
>>>>  perform_csum_check:
>>>> -               if (!csum) {
>>>> +               if (!csum && !nskb->remcsum_offload) {
>>>>                         if (skb_has_shared_frag(nskb)) {
>>>>                                 err = __skb_linearize(nskb);
>>>>                                 if (err)
>>>>                                         goto err;
>>>>                         }
>>>> -                       if (!nskb->remcsum_offload)
>>>> -                               nskb->ip_summed = CHECKSUM_NONE;
>>>> +                       nskb->ip_summed = CHECKSUM_NONE;
>>>>                         SKB_GSO_CB(nskb)->csum =
>>>>                                 skb_checksum(nskb, doffset,
>>>>                                              nskb->len - doffset, 0);
>>>
>>> I'm pretty sure this is going to cause a huge mess if you are
>>> requesting remote checksum but cannot perform an outer checksum.  One
>>> of the reasons I merged these features together the way I did was
>>> because we needed to perform the initial checksum to avoid causing a
>>> kernel panic later on.  Otherwise we don't have the SKB_GSO_CB()->csum
>>> and SKB_GSO_CB()->csum_start fields populated.
>>>
>> Nope, if we can't perform outer checksum it is done in the host. I'm
>> not seeing any problem.
>
> Are you running remcsum with udpcsum set or is udpcsum not set?  If
> you don't set udpcsum it doesn't actually compute an outer checksum
> value.
>
>>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>>> index 8a3405a..f86f1e1 100644
>>>> --- a/net/ipv4/udp_offload.c
>>>> +++ b/net/ipv4/udp_offload.c
>>>> @@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>>>>
>>>>         features &= skb->dev->hw_enc_features;
>>>>
>>>> -       /* The only checksum offload we care about from here on out is the
>>>> -        * outer one so strip the existing checksum feature flags and
>>>> -        * instead set the flag based on our outer checksum offload value.
>>>> -        */
>>>> -       if (remcsum || ufo) {
>>>> -               features &= ~NETIF_F_CSUM_MASK;
>>>> -               if (!need_csum || offload_csum)
>>>> -                       features |= NETIF_F_HW_CSUM;
>>>> -       }
>>>> -
>>>>         /* segment inner packet. */
>>>>         segs = gso_inner_segment(skb, features);
>>>>         if (IS_ERR_OR_NULL(segs)) {
>>>
>>> This part breaks UDP fragmentation I am pretty sure.  We need this bit
>>> for UFO to avoid having to perform a checksum over the data twice if
>>> we are offloading the outer UDP checksum.
>>
>> The maybe leave this for ufo if it is a problem (please test your
>> suppositions!), but this is not good for remcsum offload.
>
> I suspect we are supposed to be setting encap_level to 0 when we set
> skb->encapsulation to 0 before processing inner headers in the case of
> UDP and GRE tunnel offloads.  I'll try to test and submit something in
> the next hour or two.
>
> - Alex

Re: [PATCH net-next] net: Fix remote checksum offload with GUE

[Tom Herbert]

On Tue, Mar 22, 2016 at 3:09 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Tue, Mar 22, 2016 at 2:45 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Tue, Mar 22, 2016 at 2:10 PM, Tom Herbert <tom@herbertland.com> wrote:
>>> On Tue, Mar 22, 2016 at 1:20 PM, Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>> On Tue, Mar 22, 2016 at 12:19 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>> In skb_segment the check of whether or not to perform the checksum on
>>>>> host was changed to not consider rather remote checksum offload is
>>>>> in use. In the case that can_checksum_protocol fails the checksum
>>>>> is computed regardless. __skb_udp_tunnel_segment was modified in a
>>>>> related patch to add NETIF_F_HW_CSUM into features when grabbing
>>>>> the enc_features and remote checksum offload is being done. The
>>>>> problem is that this bit can be cleared in lower GSO layers that
>>>>> are also doing tunneling (e.g. ipip, GRE when used with GUE),
>>>>> so when we get to skb_segment that intent has been lost and
>>>>> can_checksum_protocol fails.
>>>>
>>>> So what you are describing sounds like a tunnel in tunnel scenario.
>>>> It might work better to just skip masking the features if
>>>> skb->remcsum_offload is set rather than trying to change how we
>>>> perform the offload.
>>>>
>>> To be clear, my patch is restoring the old behavior not implementing a new one.
>>
>> Yes, but the old behavior could lead to kernel panics under certain
>> circumstances.
>>
>>>> I'm pretty sure this will cause data corruption and maybe a kernel
>>>> panic if Tx checksum offload is disabled.
>>>>
>>> Nope, working fine for me.
>>
>> What are the options you used to create the tunnel?  Did you enable
>> both remcsum and udpcsum?
>>
> modprobe fou
> ./ip fou add port 6080 gue
> ./ip link add name tun1 type ipip remote 10.1.1.2 local 10.1.1.2 ttl
> 225 encap gue encap-sport auto encap-dport 6080 encap-csum
> encap-remcsum
> ifconfig tun1 192.168.1.1
> ip route add 192.168.1.0/24 dev tun1
>
Here are the command I use for VXLAN for reference:

./ip link add vxlan0 type vxlan id 10 group 224.10.10.10 ttl 10 dev
eth0 udpcsum remcsumtx remcsumrx
ifconfig vxlan0 192.168.111.1
ip route add 192.168.111.0/24 dev vxlan0

When remcsum offload is working properly (enabled and outer checksum
is offloaded) then csum_partial gets very few cycles as shown by perf.

Tom

> Thanks,
> Tom
>
>>> The outer checksum would be computed in
>>>>> This patch:
>>>>>
>>>>> - Restores the check in skb_segment to look at remote checksum offload.
>>>>> - Removes the code in __skb_udp_tunnel_segment to force the
>>>>>   NETIF_F_HW_CSUM feature since this is no longer useful with above
>>>>>   change.
>>>>> - Removes check for remote checksum offload in gso_reset_checksum.
>>>>>   A special case should not be needed here.
>>>>>
>>>>> Tested: Single netperf STREAM over GUE-ipip
>>>>>
>>>>> Before fix:
>>>>>    5625 Mbps
>>>>> After fix:
>>>>>    6410 Mbps
>>>>>
>>>>> Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
>>>>> Signed-off-by: Tom Herbert <tom@herbertland.com>
>>>>> ---
>>>>>  include/linux/skbuff.h |  4 ----
>>>>>  net/core/skbuff.c      |  5 ++---
>>>>>  net/ipv4/udp_offload.c | 10 ----------
>>>>>  3 files changed, 2 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>>> index 15d0df9..f6fe8a5 100644
>>>>> --- a/include/linux/skbuff.h
>>>>> +++ b/include/linux/skbuff.h
>>>>> @@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
>>>>>
>>>>>  static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
>>>>>  {
>>>>> -       /* Do not update partial checksums if remote checksum is enabled. */
>>>>> -       if (skb->remcsum_offload)
>>>>> -               return;
>>>>> -
>>>>>         SKB_GSO_CB(skb)->csum = res;
>>>>>         SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
>>>>>  }
>>>>
>>>> I'm pretty sure this part here will break things when you don't have
>>>> an outer offload enabled.  What NIC did you test this on?  Did you try
>>>> disabling the Tx checksum support in the hardware to see what would
>>>> happen?
>>>>
>>> Mellanox.
>>>
>>> When TX checksum is disabled the outer checksum is computed in
>>> __skb_udp_tunnel_segment.
>>
>> Yes, but that uses gso_make_checksum which requires the csum and
>> csum_start fields be initialized in the SKB_GSO_CB().
>>
>>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>>> index f044f97..e4eb78d 100644
>>>>> --- a/net/core/skbuff.c
>>>>> +++ b/net/core/skbuff.c
>>>>> @@ -3259,14 +3259,13 @@ skip_fraglist:
>>>>>                 nskb->truesize += nskb->data_len;
>>>>>
>>>>>  perform_csum_check:
>>>>> -               if (!csum) {
>>>>> +               if (!csum && !nskb->remcsum_offload) {
>>>>>                         if (skb_has_shared_frag(nskb)) {
>>>>>                                 err = __skb_linearize(nskb);
>>>>>                                 if (err)
>>>>>                                         goto err;
>>>>>                         }
>>>>> -                       if (!nskb->remcsum_offload)
>>>>> -                               nskb->ip_summed = CHECKSUM_NONE;
>>>>> +                       nskb->ip_summed = CHECKSUM_NONE;
>>>>>                         SKB_GSO_CB(nskb)->csum =
>>>>>                                 skb_checksum(nskb, doffset,
>>>>>                                              nskb->len - doffset, 0);
>>>>
>>>> I'm pretty sure this is going to cause a huge mess if you are
>>>> requesting remote checksum but cannot perform an outer checksum.  One
>>>> of the reasons I merged these features together the way I did was
>>>> because we needed to perform the initial checksum to avoid causing a
>>>> kernel panic later on.  Otherwise we don't have the SKB_GSO_CB()->csum
>>>> and SKB_GSO_CB()->csum_start fields populated.
>>>>
>>> Nope, if we can't perform outer checksum it is done in the host. I'm
>>> not seeing any problem.
>>
>> Are you running remcsum with udpcsum set or is udpcsum not set?  If
>> you don't set udpcsum it doesn't actually compute an outer checksum
>> value.
>>
>>>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>>>> index 8a3405a..f86f1e1 100644
>>>>> --- a/net/ipv4/udp_offload.c
>>>>> +++ b/net/ipv4/udp_offload.c
>>>>> @@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>>>>>
>>>>>         features &= skb->dev->hw_enc_features;
>>>>>
>>>>> -       /* The only checksum offload we care about from here on out is the
>>>>> -        * outer one so strip the existing checksum feature flags and
>>>>> -        * instead set the flag based on our outer checksum offload value.
>>>>> -        */
>>>>> -       if (remcsum || ufo) {
>>>>> -               features &= ~NETIF_F_CSUM_MASK;
>>>>> -               if (!need_csum || offload_csum)
>>>>> -                       features |= NETIF_F_HW_CSUM;
>>>>> -       }
>>>>> -
>>>>>         /* segment inner packet. */
>>>>>         segs = gso_inner_segment(skb, features);
>>>>>         if (IS_ERR_OR_NULL(segs)) {
>>>>
>>>> This part breaks UDP fragmentation I am pretty sure.  We need this bit
>>>> for UFO to avoid having to perform a checksum over the data twice if
>>>> we are offloading the outer UDP checksum.
>>>
>>> The maybe leave this for ufo if it is a problem (please test your
>>> suppositions!), but this is not good for remcsum offload.
>>
>> I suspect we are supposed to be setting encap_level to 0 when we set
>> skb->encapsulation to 0 before processing inner headers in the case of
>> UDP and GRE tunnel offloads.  I'll try to test and submit something in
>> the next hour or two.
>>
>> - Alex