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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 23 Dec 2021 10:08:05 -0800
From:   Dave Taht <dave.taht@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Paolo Abeni <pabeni@...hat.com>, netdev <netdev@...r.kernel.org>,
        Ignat Korchagin <ignat@...udflare.com>
Subject: Re: [PATCH net] veth: ensure skb entering GRO are not cloned.

On Wed, Dec 22, 2021 at 5:17 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> 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've always liked the idea of being able to coherently timestamp from
packet ingress to egress.

> 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 :)

GRO is the bane of my sub-gbit existence. I've been wishing we had a
compile time option to always split it up or a righter path forward
using veth interfaces here: https://github.com/rchac/LibreQoS

> 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++;
>         }



-- 
I tried to build a better future, a few times:
https://wayforward.archive.org/?site=https%3A%2F%2Fwww.icei.org

Dave Täht CEO, TekLibre, LLC