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

[Jakub Sitnicki <jakub@cloudflare.com>]

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)
>>