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: <dad55584ad20723f1579475a09ef7b3a3607e087.camel@redhat.com>
Date:   Wed, 22 Dec 2021 12:06:31 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Ignat Korchagin <ignat@...udflare.com>
Subject: Re: [PATCH net] veth: ensure skb entering GRO are not cloned.

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

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?

Thanks!

Paolo