Re: [PATCH net] net: ipv6: fix TCP GSO segmentation with NAT

[Paolo Abeni <pabeni@redhat.com>]

On 2/24/25 2:00 PM, Eric Dumazet wrote:
> On Mon, Feb 24, 2025 at 12:21 PM Felix Fietkau <nbd@nbd.name> wrote:
>>
>> When updating the source/destination address, the TCP/UDP checksum needs to
>> be updated as well.
>>
>> Fixes: bee88cd5bd83 ("net: add support for segmenting TCP fraglist GSO packets")
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>  net/ipv6/tcpv6_offload.c | 20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
>> index a45bf17cb2a1..5d0fcdbf57a1 100644
>> --- a/net/ipv6/tcpv6_offload.c
>> +++ b/net/ipv6/tcpv6_offload.c
>> @@ -113,24 +113,36 @@ static struct sk_buff *__tcpv6_gso_segment_list_csum(struct sk_buff *segs)
>>         struct sk_buff *seg;
>>         struct tcphdr *th2;
>>         struct ipv6hdr *iph2;
>> +       bool addr_equal;
>>
>>         seg = segs;
>>         th = tcp_hdr(seg);
>>         iph = ipv6_hdr(seg);
>>         th2 = tcp_hdr(seg->next);
>>         iph2 = ipv6_hdr(seg->next);
>> +       addr_equal = ipv6_addr_equal(&iph->saddr, &iph2->saddr) &&
>> +                    ipv6_addr_equal(&iph->daddr, &iph2->daddr);
>>
>>         if (!(*(const u32 *)&th->source ^ *(const u32 *)&th2->source) &&
>> -           ipv6_addr_equal(&iph->saddr, &iph2->saddr) &&
>> -           ipv6_addr_equal(&iph->daddr, &iph2->daddr))
>> +           addr_equal)
>>                 return segs;
>>
>>         while ((seg = seg->next)) {
>>                 th2 = tcp_hdr(seg);
>>                 iph2 = ipv6_hdr(seg);
>>
>> -               iph2->saddr = iph->saddr;
>> -               iph2->daddr = iph->daddr;
>> +               if (!addr_equal) {
>> +                       inet_proto_csum_replace16(&th2->check, seg,
>> +                                                 iph2->saddr.s6_addr32,
>> +                                                 iph->saddr.s6_addr32,
>> +                                                 true);
>> +                       inet_proto_csum_replace16(&th2->check, seg,
>> +                                                 iph2->daddr.s6_addr32,
>> +                                                 iph->daddr.s6_addr32,
>> +                                                 true);
>> +                       iph2->saddr = iph->saddr;
>> +                       iph2->daddr = iph->daddr;
>> +               }
>>                 __tcpv6_gso_segment_csum(seg, &th2->source, th->source);
>>                 __tcpv6_gso_segment_csum(seg, &th2->dest, th->dest);
> 
> Good catch !
> 
> I note that __tcpv4_gso_segment_csum() does the needed csum changes
> for both ports and address components.
> 
> Have you considered using the same logic for IPv6, to keep
> __tcpv6_gso_segment_list_csum()
> and __tcpv4_gso_segment_list_csum() similar ?

I agree with Eric, I think we should try to keep ipv4 and ipv6 code and
behavior similar, where/when it makes sense (like here).

@Felix, could you please update the patch accordingly?

Thanks,

Paolo