linux-kernel - Re: [PATCH net-next 2/2] tcp: propagate GSO to the new skb built in tcp collapse

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: <CANn89iKFH_F0_Ri76Zsez2Nn8JZ9obprsk+OE7i+enGC=n8Syw@mail.gmail.com>
Date:   Fri, 27 Jul 2018 20:13:19 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Yafang Shao <laoar.shao@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 2/2] tcp: propagate GSO to the new skb built in
 tcp collapse

On Fri, Jul 27, 2018 at 8:02 PM Yafang Shao <laoar.shao@...il.com> wrote:
>
> Currently the collapsed SKB doesn't propagate the GSO information to the
> new SKB.
> The GSO should be propagated for better tracking, i.e. when this SKB is
> dropped we could know how many network segments are dropped.

What is "the GSO"  ?

>
> Signed-off-by: Yafang Shao <laoar.shao@...il.com>
> ---
>  net/ipv4/tcp_input.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 90f83eb..af52e4e 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4893,6 +4893,8 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb)
>                 if (!nskb)
>                         break;
>
> +               skb_shinfo(nskb)->gso_size = skb_shinfo(skb)->gso_size;
> +               skb_shinfo(nskb)->gso_type = skb_shinfo(skb)->gso_type;

Why gso_size and gso_type are important ?

Where later in the stack these values are used ?

>                 memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
>  #ifdef CONFIG_TLS_DEVICE
>                 nskb->decrypted = skb->decrypted;
> @@ -4906,18 +4908,24 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb)
>
>                 /* Copy data, releasing collapsed skbs. */
>                 while (copy > 0) {
> -                       int offset = start - TCP_SKB_CB(skb)->seq;
>                         int size = TCP_SKB_CB(skb)->end_seq - start;
> +                       int offset = start - TCP_SKB_CB(skb)->seq;

>
>                         BUG_ON(offset < 0);
>                         if (size > 0) {
> -                               size = min(copy, size);
> +                               if (copy >= size)
> +                                       skb_shinfo(nskb)->gso_segs +=
> +                                               max_t(u16, 1, skb_shinfo(skb)->gso_segs);
> +                               else
> +                                       size = copy;
> +

So... what happens if copy was partial ?

Your patch does not really fix the uncertainty, it merely shifts it a bit.