Re: [net PATCH] net: Segment CHECKSUM_UNNECESSARY GSO skbs other than fraglist

[Paolo Abeni <pabeni@redhat.com>]

On 11/4/26 03:51, Willem de Bruijn wrote:
> Alexander Duyck wrote:
>> From: Alexander Duyck <alexanderduyck@fb.com>
>>
>> The netif_needs_gso function lets a GSO skb reach the device unsegmented
>> when it is CHECKSUM_PARTIAL or CHECKSUM_UNNECESSARY. The second case was
>> added for FCoE, whose transmit path marked CRC offload that way at the
>> time. Commit 253aab0597d9 ("fcoe: Use CHECKSUM_PARTIAL to indicate CRC
>> offload") moved FCoE back to CHECKSUM_PARTIAL but left the exemption in
>> place, with nothing left that needed it. Fraglist GSO, added later, is
>> its only user today, as its segments already carry valid checksums.
>>
>> A TCP GSO skb can still arrive as CHECKSUM_UNNECESSARY without being
>> fraglist. Hardware LRO builds its aggregates that way, for example mlx5
>> sets SKB_GSO_TCPV4 or SKB_GSO_TCPV6 and CHECKSUM_UNNECESSARY, without
>> SKB_GSO_DODGY. IP forwarding and bridging turn LRO off, but a tc mirred
>> redirect does not, so such an skb can be sent out another device. It
>> has no csum_start and its TCP checksum field is whatever the aggregate
>> left there. Drivers that only set up TSO for CHECKSUM_PARTIAL, such as
>> fbnic, i40e and ice, then send the whole skb as one oversized frame
>> with a bad checksum.
>>
>> Have the validate_xmit_skb function mark any CHECKSUM_UNNECESSARY GSO
>> skb other than fraglist as CHECKSUM_NONE, which means the same on
>> transmit. The netif_needs_gso function then segments it in software,
> 
> tcp[46]_gso_segment convert to CHECKSUM_PARTIAL and then
> tcp_gso_segment will send it on to hardware if skb_gso_ok with
> NETIF_F_GSO_ROBUST.
> 
>> and the __skb_gso_segment function unshares the header before the
>> protocol code rewrites the L4 checksum field. The callers of the
>> netif_needs_gso function that decide on segmentation for themselves,
>> such as tap, are left as they are.
>>
>> Found by code inspection. In QEMU with an emulated fbnic, TCPv4 and
>> TCPv6 GSO skbs built this way each went out as a single 11 KB frame
>> with a bad TCP checksum. With this change they are segmented into
>> MSS-sized frames with valid checksums.
>>
>> Fixes: 253aab0597d9 ("fcoe: Use CHECKSUM_PARTIAL to indicate CRC offload")
>> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> 
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> 
> With the above aside. Not important enough to respin imho.
> 
>> ---
>>   net/core/dev.c |   11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index f32353b0467c..28b4a652d037 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -4207,6 +4207,17 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
>>   	if (unlikely(!skb))
>>   		goto out_null;
>>   
>> +	/* Only fraglist GSO, whose segments already carry valid checksums,
>> +	 * can be handed to a device as CHECKSUM_UNNECESSARY. Any other GSO
>> +	 * skb has no csum_start for the device to segment from. NONE means
>> +	 * the same on transmit, and has netif_needs_gso() segment it in
>> +	 * software.
>> +	 */
>> +	if (unlikely(skb->ip_summed == CHECKSUM_UNNECESSARY &&
>> +		     skb_is_gso(skb) &&
>> +		     !(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST)))
>> +		skb->ip_summed = CHECKSUM_NONE;
>> +
> 
> It's unfortunate that we need to add yet another branch in the hot path.
> But I also do not see a good alternative.
I understand fraglist GSO has some use-case, but I'm really wondering if
it's really worthy the pain?

I'll personally think we should consider removing it entirely.

/P