[PATCH net] udp: Compute L4 checksum as usual when not segmenting the skb

[PATCH net] udp: Compute L4 checksum as usual when not segmenting the skb

[Jakub Sitnicki]

If:

  1) the user requested USO, but
  2) there is not enough payload for GSO to kick in, and
  3) the egress device doesn't offer checksum offload, then

we want to compute the L4 checksum in software early on.

In the case when we taking the GSO path, but it has been requested, the
software checksum fallback in skb_segment doesn't get a chance to compute
the full checksum, if the egress device can't do it. As a result we end up
sending UDP datagrams with only a partial checksum filled in, which the
peer will discard.

Fixes: 10154dbded6d ("udp: Allow GSO transmit from devices with no checksum offload")
Reported-by: Ivan Babrou <ivan@cloudflare.com>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
Cc: stable@vger.kernel.org
---
This shouldn't have fallen through the cracks. I clearly need to extend the
net/udpgso selftests further to cover the whole TX path for software
USO+csum case. I will follow up with that but I wanted to get the fix out
in the meantime. Apologies for the oversight.
---
 net/ipv4/udp.c | 4 +++-
 net/ipv6/udp.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 8accbf4cb295..2849b273b131 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -951,8 +951,10 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
 			skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
 			skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(datalen,
 								 cork->gso_size);
+
+			/* Don't checksum the payload, skb will get segmented */
+			goto csum_partial;
 		}
-		goto csum_partial;
 	}
 
 	if (is_udplite)  				 /*     UDP-Lite      */
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 52dfbb2ff1a8..0cef8ae5d1ea 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1266,8 +1266,10 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
 			skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
 			skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(datalen,
 								 cork->gso_size);
+
+			/* Don't checksum the payload, skb will get segmented */
+			goto csum_partial;
 		}
-		goto csum_partial;
 	}
 
 	if (is_udplite)

Re: [PATCH net] udp: Compute L4 checksum as usual when not segmenting the skb

[Willem de Bruijn]

Jakub Sitnicki wrote:
> If:
> 
>   1) the user requested USO, but
>   2) there is not enough payload for GSO to kick in, and
>   3) the egress device doesn't offer checksum offload, then
> 
> we want to compute the L4 checksum in software early on.
> 
> In the case when we taking the GSO path, but it has been requested, the

What does it refers to here?

> software checksum fallback in skb_segment doesn't get a chance to compute
> the full checksum, if the egress device can't do it. As a result we end up
> sending UDP datagrams with only a partial checksum filled in, which the
> peer will discard.
> 
> Fixes: 10154dbded6d ("udp: Allow GSO transmit from devices with no checksum offload")
> Reported-by: Ivan Babrou <ivan@cloudflare.com>
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> Cc: stable@vger.kernel.org
> ---
> This shouldn't have fallen through the cracks. I clearly need to extend the
> net/udpgso selftests further to cover the whole TX path for software
> USO+csum case. I will follow up with that but I wanted to get the fix out
> in the meantime. Apologies for the oversight.
> ---
>  net/ipv4/udp.c | 4 +++-
>  net/ipv6/udp.c | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 8accbf4cb295..2849b273b131 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -951,8 +951,10 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>  			skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
>  			skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(datalen,
>  								 cork->gso_size);
> +
> +			/* Don't checksum the payload, skb will get segmented */
> +			goto csum_partial;
>  		}
> -		goto csum_partial;

The issue here is that GSO packets with CHECKSUM_NONE will get fixed
software checksummed in skb_segment, but no such fallback path is
entered for regular packets, right?

We could setup CHECKSUM_PARTIAL and rely on validate_xmit_skb. But
might as well do the software checksumming right here, as your
patch does.

If I follow this all, ACK from me. Just want to make sure.
>  	}
>  
>  	if (is_udplite)  				 /*     UDP-Lite      */
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 52dfbb2ff1a8..0cef8ae5d1ea 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1266,8 +1266,10 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
>  			skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
>  			skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(datalen,
>  								 cork->gso_size);
> +
> +			/* Don't checksum the payload, skb will get segmented */
> +			goto csum_partial;
>  		}
> -		goto csum_partial;
>  	}
>  
>  	if (is_udplite)
> 

Re: [PATCH net] udp: Compute L4 checksum as usual when not segmenting the skb

[Jakub Sitnicki]

On Thu, Oct 10, 2024 at 09:31 AM -04, Willem de Bruijn wrote:
> Jakub Sitnicki wrote:
>> If:
>> 
>>   1) the user requested USO, but
>>   2) there is not enough payload for GSO to kick in, and
>>   3) the egress device doesn't offer checksum offload, then
>> 
>> we want to compute the L4 checksum in software early on.
>> 
>> In the case when we taking the GSO path, but it has been requested, the
>
> What does it refers to here?

That's a typo there. Will fix. It should have said:

  In the case when we *not* taking the GSO path, but it has been
  requested, ...

Pseudo code-wise something like:

  s.setsockopt(SOL_UDP, UDP_SEGMENT, 1200)
  s.sendto(b"x", ("192.0.2.1", 9))


>> software checksum fallback in skb_segment doesn't get a chance to compute
>> the full checksum, if the egress device can't do it. As a result we end up
>> sending UDP datagrams with only a partial checksum filled in, which the
>> peer will discard.
>> 
>> Fixes: 10154dbded6d ("udp: Allow GSO transmit from devices with no checksum offload")
>> Reported-by: Ivan Babrou <ivan@cloudflare.com>
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> Cc: stable@vger.kernel.org
>> ---
>> This shouldn't have fallen through the cracks. I clearly need to extend the
>> net/udpgso selftests further to cover the whole TX path for software
>> USO+csum case. I will follow up with that but I wanted to get the fix out
>> in the meantime. Apologies for the oversight.
>> ---
>>  net/ipv4/udp.c | 4 +++-
>>  net/ipv6/udp.c | 4 +++-
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index 8accbf4cb295..2849b273b131 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -951,8 +951,10 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>>  			skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
>>  			skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(datalen,
>>  								 cork->gso_size);
>> +
>> +			/* Don't checksum the payload, skb will get segmented */
>> +			goto csum_partial;
>>  		}
>> -		goto csum_partial;
>
> The issue here is that GSO packets with CHECKSUM_NONE will get fixed
> software checksummed in skb_segment, but no such fallback path is
> entered for regular packets, right?
>
> We could setup CHECKSUM_PARTIAL and rely on validate_xmit_skb. But
> might as well do the software checksumming right here, as your
> patch does.

Yes, all correct.

To add to it - I figured that marking the skb with CHECKSUM_PARTIAL,
when device doesn't offer csum offload, would be more confusing.

>
> If I follow this all, ACK from me. Just want to make sure.

Thanks. I will carry it in v2.

Will wait a bit before respinning it.
Maybe Ivan can get a chance to this patch.

>>  	}
>>  
>>  	if (is_udplite)  				 /*     UDP-Lite      */
>> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>> index 52dfbb2ff1a8..0cef8ae5d1ea 100644
>> --- a/net/ipv6/udp.c
>> +++ b/net/ipv6/udp.c
>> @@ -1266,8 +1266,10 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
>>  			skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
>>  			skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(datalen,
>>  								 cork->gso_size);
>> +
>> +			/* Don't checksum the payload, skb will get segmented */
>> +			goto csum_partial;
>>  		}
>> -		goto csum_partial;
>>  	}
>>  
>>  	if (is_udplite)
>>