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: <CANn89iKDA4TMpQeQoxicd8rkph5+Am2iuoSDETvFn03CiQQV3g@mail.gmail.com>
Date:   Wed, 22 Dec 2021 03:58:13 -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 Wed, Dec 22, 2021 at 3:06 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> Hello,
>
> On Tue, 2021-12-21 at 20:31 -0800, Eric Dumazet 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.
>
> Thank you for the feedback.
>
> I double checked the above: in my test even skb_cloned() suffice.
>
> > > +                       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 :)
>
> Only vaguely related: I have a bunch of micro optimizations for the GRO
> engine. I did not submit the patches because I can observe the gain
> only in micro-benchmarks, but I'm wondering if that could be visible
> with very high speed TCP stream? I can share the code if that could be
> of general interest (after some rebasing, the patches predates gro.c)
>
> > 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 tested the above, and it works, too.
>
> I thought about something similar, but I overlooked possible OoO or
> behaviour changes when a packet socket is attached to the paired device
> (as it would disable GRO).

Have you tried a pskb_expand_head() instead of a full copy ?
Perhaps that would be enough, and keep all packets going through GRO to
make sure OOO is covered.

>
> It looks like tcpdump should have not ill-effects (the mmap rx-path
> releases the skb clone before the orig packet reaches the other end),
> so I guess the above is fine (and sure is better to avoid more
> timestamp related problem).
>
> Do you prefer to submit it formally, or do you prefer I'll send a v2
> with the latter code?

Sure, please submit a V2.

>
> Thanks!
>
> Paolo
>
>