netdev - Re: [PATCH net] tcp: gso: really support BIG TCP

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKgT0Uffq97JF7cpkyQPmFh0mreHx+yKxgExnOGn6KzEoQ0HyA@mail.gmail.com>
Date: Fri, 2 Jun 2023 08:38:52 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Eric Dumazet <edumazet@...gle.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 8:25 AM Eric Dumazet <edumazet@...gle.com> wrote:

<...>

> This was the WIP (and not working at all)

This is way more than what I was thinking Some of what I had stated
was just pseudocode. No need for the addition of newlen for instance.

> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index 3568607588b107f06b8fb7b1143d5417bb2a3ac2..19bc2d9ae10d45aa5cbb35add4aa8e9f6b46a6ab
> 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -60,11 +60,11 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
>  {
>         struct sk_buff *segs = ERR_PTR(-EINVAL);
>         unsigned int sum_truesize = 0;
> +       unsigned int oldlen, newlen;
>         struct tcphdr *th;
>         unsigned int thlen;
>         unsigned int seq;
> -       __be32 delta;
> -       unsigned int oldlen;
> +       __wsum delta;
>         unsigned int mss;
>         struct sk_buff *gso_skb = skb;
>         __sum16 newcheck;

I wouldn't even bother with any of these changes.

> @@ -78,7 +78,7 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
>         if (!pskb_may_pull(skb, thlen))
>                 goto out;
>
> -       oldlen = (u16)~skb->len;
> +       oldlen = skb->len;
>         __skb_pull(skb, thlen);
>
>         mss = skb_shinfo(skb)->gso_size;

As I stated before I would just drop the "(u16)". We are expanding the
oldlen to a u32, but don't have to worry about it overflowing because
skb->len should always be greater than or equal to our segmented
length. So the most it could reach is ~0 if skb->len == segmented
length.

> @@ -113,7 +113,9 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
>         if (skb_is_gso(segs))
>                 mss *= skb_shinfo(segs)->gso_segs;
>
> -       delta = htonl(oldlen + (thlen + mss));
> +       newlen = thlen + mss;
> +       delta = csum_sub(htonl(newlen), htonl(oldlen));
> +       newcheck = csum_fold(csum_add(csum_unfold(th->check), delta));
>
>         skb = segs;
>         th = tcp_hdr(skb)

I think all of this can just be dropped.

> @@ -122,8 +124,6 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
>         if (unlikely(skb_shinfo(gso_skb)->tx_flags & SKBTX_SW_TSTAMP))
>                 tcp_gso_tstamp(segs, skb_shinfo(gso_skb)->tskey, seq, mss);
>
> -       newcheck = ~csum_fold((__force __wsum)((__force u32)th->check +
> -                                              (__force u32)delta));
>
>         while (skb->next) {
>                 th->fin = th->psh = 0;

So the change I would make here is:
newcheck = ~csum_fold(csum_add(csum_unfold(th->check, ()), (__force
__wsum)delta);


> @@ -168,11 +168,11 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
>                         WARN_ON_ONCE(refcount_sub_and_test(-delta,
> &skb->sk->sk_wmem_alloc));
>         }
>
> -       delta = htonl(oldlen + (skb_tail_pointer(skb) -
> -                               skb_transport_header(skb)) +
> -                     skb->data_len);
> -       th->check = ~csum_fold((__force __wsum)((__force u32)th->check +
> -                               (__force u32)delta));
> +       newlen = skb_tail_pointer(skb) - skb_transport_header(skb) +
> +                skb->data_len;
> +       delta = csum_sub(htonl(newlen), htonl(oldlen));
> +       th->check = csum_fold(csum_add(csum_unfold(th->check), delta));
> +
>         if (skb->ip_summed == CHECKSUM_PARTIAL)
>                 gso_reset_checksum(skb, ~th->check);
>         else

Likewise here, the only thing that has to be replaced is the th->check
line so it would be:
th->check = ~csum_fold(csum_add(csum_unfold(th->check, ()), (__force
__wsum)delta);