netdev - Re: [PATCH net] veth: ensure skb entering GRO are not cloned.

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: <CANn89iKpiQzW1UnsQSYzULJ8d-QHsy7Wz=NtgvVXBqh-iuNptQ@mail.gmail.com>
Date:   Tue, 21 Dec 2021 20:31:53 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Ignat Korchagin <ignat@...udflare.com>
Subject: Re: [PATCH net] veth: ensure skb entering GRO are not cloned.

On Tue, Dec 21, 2021 at 1:34 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> After commit d3256efd8e8b ("veth: allow enabling NAPI even without XDP"),
> if GRO is enabled on a veth device and TSO is disabled on the peer
> device, TCP skbs will go through the NAPI callback. If there is no XDP
> program attached, the veth code does not perform any share check, and
> shared/cloned skbs could enter the GRO engine.
>
>

...

> Address the issue checking for cloned skbs even in the GRO-without-XDP
> input path.
>
> Reported-and-tested-by: Ignat Korchagin <ignat@...udflare.com>
> Fixes: d3256efd8e8b ("veth: allow enabling NAPI even without XDP")
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
>  drivers/net/veth.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index b78894c38933..abd1f949b2f5 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -718,6 +718,14 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
>         rcu_read_lock();
>         xdp_prog = rcu_dereference(rq->xdp_prog);
>         if (unlikely(!xdp_prog)) {
> +               if (unlikely(skb_shared(skb) || skb_head_is_locked(skb))) {

Why skb_head_is_locked() needed here ?
I would think skb_cloned() is enough for the problem we want to address.

> +                       struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC | __GFP_NOWARN);
> +
> +                       if (!nskb)
> +                               goto drop;
> +                       consume_skb(skb);
> +                       skb = nskb;
> +               }
>                 rcu_read_unlock();
>                 goto out;
>         }
> --
> 2.33.1
>

- It seems adding yet memory alloc/free and copies is defeating GRO purpose.
- After skb_copy(), GRO is forced to use the expensive frag_list way
for aggregation anyway.
- veth mtu could be set to 64KB, so we could have order-4 allocation
attempts here.

Would the following fix [1] be better maybe, in terms of efficiency,
and keeping around skb EDT/tstamp
information (see recent thread with Martin and Daniel )

I think it also focuses more on the problem (GRO is not capable of
dealing with cloned skb yet).
Who knows, maybe in the future we will _have_ to add more checks in
GRO fast path for some other reason,
since it is becoming the Swiss army knife of networking :)

Although I guess this whole case (disabling TSO) is moot, I have no
idea why anyone would do that :)

[1]
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 50eb43e5bf459bb998e264d399bc85d4e9d73594..fe7a4d2f7bfc834ea56d1da185c0f53bfbd22ad0
100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -879,8 +879,12 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,

                        stats->xdp_bytes += skb->len;
                        skb = veth_xdp_rcv_skb(rq, skb, bq, stats);
-                       if (skb)
-                               napi_gro_receive(&rq->xdp_napi, skb);
+                       if (skb) {
+                               if (skb_shared(skb) || skb_cloned(skb))
+                                       netif_receive_skb(skb);
+                               else
+                                       napi_gro_receive(&rq->xdp_napi, skb);
+                       }
                }
                done++;
        }