Date: Fri, 2 Jun 2023 17:21:44 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Alexander H Duyck <alexander.duyck@...il.com>
Cc: Alexander Duyck <alexanderduyck@...a.com>, "David S . Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, Xin Long <lucien.xin@...il.com>, 
	David Ahern <dsahern@...nel.org>, "eric.dumazet@...il.com" <eric.dumazet@...il.com>
Subject: Re: [PATCH net] tcp: gso: really support BIG TCP

On Fri, Jun 2, 2023 at 4:24 PM Alexander H Duyck
<alexander.duyck@...il.com> wrote:
>
> On Fri, 2023-06-02 at 04:30 +0200, Eric Dumazet wrote:
> > On Thu, Jun 1, 2023 at 11:46 PM Alexander Duyck <alexanderduyck@...a.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Eric Dumazet <edumazet@...gle.com>
> > > > Sent: Thursday, June 1, 2023 2:18 PM
> > > > To: David S . Miller <davem@...emloft.net>; Jakub Kicinski
> > > > <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>
> > > > Cc: netdev@...r.kernel.org; Xin Long <lucien.xin@...il.com>; David Ahern
> > > > <dsahern@...nel.org>; eric.dumazet@...il.com; Eric Dumazet
> > > > <edumazet@...gle.com>; Alexander Duyck <alexanderduyck@...a.com>
> > > > Subject: [PATCH net] tcp: gso: really support BIG TCP
> > > >
> > > > >
> > > > We missed that tcp_gso_segment() was assuming skb->len was smaller than
> > > > 65535 :
> > > >
> > > > oldlen = (u16)~skb->len;
> > > >
> > > > This part came with commit 0718bcc09b35 ("[NET]: Fix CHECKSUM_HW GSO
> > > > problems.")
> > > >
> > > > This leads to wrong TCP checksum.
> > > >
> > > > Simply use csum_fold() to support 32bit packet lengthes.
> > > >
> > > > oldlen name is a bit misleading, as it is the contribution of skb->len on the
> > > > input skb TCP checksum. I added a comment to clarify this point.
> > > >
> > > > Fixes: 09f3d1a3a52c ("ipv6/gso: remove temporary HBH/jumbo header")
> > > > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > > > Cc: Alexander Duyck <alexanderduyck@...com>
> > > > ---
> > > >  net/ipv4/tcp_offload.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c index
> > > > 45dda788938704c3f762256266d9ea29b6ded4a5..5a1a163b2d859696df8f204b5
> > > > 0e3fc76c14b64e9 100644
> > > > --- a/net/ipv4/tcp_offload.c
> > > > +++ b/net/ipv4/tcp_offload.c
> > > > @@ -75,7 +75,8 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
> > > >       if (!pskb_may_pull(skb, thlen))
> > > >               goto out;
> > > >
> > > > -     oldlen = (u16)~skb->len;
> > > > +     /* Contribution of skb->len in current TCP checksum */
> > > > +     oldlen = (__force u32)csum_fold((__force __wsum)skb->len);
> > > >       __skb_pull(skb, thlen);
> > > >
> > > >       mss = skb_shinfo(skb)->gso_size;
> > >
> > > The only thing I am not sure about with this change is if we risk overflowing a u32 with all the math that may occur. The original code couldn't exceed a u16 for delta since we were essentially adding -oldlen + new header + mss. With us now allowing the use of a value larger than 16 bit we should be able to have the resultant value exceed 16b which means we might overflow when we add it to the current checksum.
> > >
> > > As such we may want to look at holding off on the csum_fold until after we have added the new header and mss and then store the folded value in delta.
> > >
> >
> > I think you missed that csum_fold() result is also a 16bit value.
>
> I saw that. My concern was more about delta versus the oldlen value
> itself though. Specifically your folded value is added to thlen + mss
> which can then overflow past a 16b value, and when byteswapped and then
> added to the original checksum there is a risk of potential overflow.

I do not think it matters. Herbert Xu said that what matters is that

oldlen + (thlen + mss) would not overflow a 32bit value./

>
> The general idea was that ~skb->len + (segment length) will always
> technically be less than 0 since the original skb->len should always be
> larger or equal to the new segmented length. So the code as it was
> would always generate a value 16 or less in length.
>
> This was important when we computed delta and added it to the original
> value since we were using htonl which would byteswap things so we could
> potentially generate a 32b value, but it couldn't overflow since the
> two addends consisted of the upper 16b and lower 16b.
>
> That is why I am thinking we are better off just dropping the "(u16)"
> cast and just passing ~skb->len as the old_len.
>
> To address this we then have a couple different approaches we could
> take:
> 1. use csum_fold on the "delta" value either before or after the htonl.
> 2. use csum_add instead "+" for the addition of (th->check + delta)
>
> I'm thinking option 2 may be the better way to go as it would just add
> 2 addc operations, one for newcheck and one at the end of the function
> for th->check.

Are you working on a patch yourself ? What would be the ETA ?

Thanks.

Powered by blists - more mailing lists