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

[Felix Fietkau]

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@xxxxxxxx>
> > ---
> >  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.

- Felix