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@xxxxxxxxxxxxxx>
>> Signed-off-by: Jakub Sitnicki <jakub@xxxxxxxxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>> ---
>> 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)
>>
