linux-kernel - Re: [PATCH v3 net-next v3 2/6] net: add support for segmenting TCP fraglist GSO packets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ca40fe27-30b5-4941-9235-6cc444eed1fd@nbd.name>
Date: Fri, 26 Apr 2024 13:35:57 +0200
From: Felix Fietkau <nbd@....name>
To: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 Eric Dumazet <edumazet@...gle.com>, "David S. Miller" <davem@...emloft.net>,
 David Ahern <dsahern@...nel.org>, Jakub Kicinski <kuba@...nel.org>
Cc: willemdebruijn.kernel@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 net-next v3 2/6] net: add support for segmenting TCP
 fraglist GSO packets

On 26.04.24 12:40, Paolo Abeni wrote:
> On Fri, 2024-04-26 at 11:39 +0200, Felix Fietkau wrote:
>> On 26.04.24 10:28, Paolo Abeni wrote:
>> > On Fri, 2024-04-26 at 08:51 +0200, Felix Fietkau wrote:
>> > > Preparation for adding TCP fraglist GRO support. It expects packets to be
>> > > combined in a similar way as UDP fraglist GSO packets.
>> > > For IPv4 packets, NAT is handled in the same way as UDP fraglist GSO.
>> > > 
>> > > Signed-off-by: Felix Fietkau <nbd@....name>
>> > > ---
>> > >  net/ipv4/tcp_offload.c   | 65 ++++++++++++++++++++++++++++++++++++++++
>> > >  net/ipv6/tcpv6_offload.c |  3 ++
>> > >  2 files changed, 68 insertions(+)
>> > > 
>> > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
>> > > index fab0973f995b..c493e95e09a5 100644
>> > > --- a/net/ipv4/tcp_offload.c
>> > > +++ b/net/ipv4/tcp_offload.c
>> > > @@ -28,6 +28,68 @@ static void tcp_gso_tstamp(struct sk_buff *skb, unsigned int ts_seq,
>> > >  	}
>> > >  }
>> > >  
>> > > +static void __tcpv4_gso_segment_csum(struct sk_buff *seg,
>> > > +				     __be32 *oldip, __be32 *newip,
>> > > +				     __be16 *oldport, __be16 *newport)
>> > > +{
>> > > +	struct tcphdr *th;
>> > > +	struct iphdr *iph;
>> > > +
>> > > +	if (*oldip == *newip && *oldport == *newport)
>> > > +		return;
>> > > +
>> > > +	th = tcp_hdr(seg);
>> > > +	iph = ip_hdr(seg);
>> > > +
>> > > +	inet_proto_csum_replace4(&th->check, seg, *oldip, *newip, true);
>> > > +	inet_proto_csum_replace2(&th->check, seg, *oldport, *newport, false);
>> > > +	*oldport = *newport;
>> > > +
>> > > +	csum_replace4(&iph->check, *oldip, *newip);
>> > > +	*oldip = *newip;
>> > > +}
>> > > +
>> > > +static struct sk_buff *__tcpv4_gso_segment_list_csum(struct sk_buff *segs)
>> > > +{
>> > > +	struct sk_buff *seg;
>> > > +	struct tcphdr *th, *th2;
>> > > +	struct iphdr *iph, *iph2;
>> > > +
>> > > +	seg = segs;
>> > > +	th = tcp_hdr(seg);
>> > > +	iph = ip_hdr(seg);
>> > > +	th2 = tcp_hdr(seg->next);
>> > > +	iph2 = ip_hdr(seg->next);
>> > > +
>> > > +	if (!(*(u32 *)&th->source ^ *(u32 *)&th2->source) &&
>> > > +	    iph->daddr == iph2->daddr && iph->saddr == iph2->saddr)
>> > > +		return segs;
>> > 
>> > As mentioned in previous revisions, I think a problem with this
>> > approach is that the stack could make other changes to the TCP header
>> > after the GRO stage, that are unnoticed here and could cause csum
>> > corruption, if the egress device does not recompute the packet csum.
>> 
>> On segmentation, each packet keeps its original TCP header and csum. If 
>> the stack makes changes, they apply to the first packet only. I don't 
>> see how we could get csum corruption.
> 
> You are right. I did not take in account that such changes (to the
> first skb) are not reflected to the frag_list at segmentation time. The
> end result could be different from what the user/admin is expecting,
> but at least should not impact drops.
> 
> Side note: alike UDP, this is not supporting IPv6 NAT...

I will add that for both in the next version.

Thanks,

- Felix