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

On Wed, Dec 22, 2021 at 5:17 PM Eric Dumazet [off-list ref] wrote:

On Tue, Dec 21, 2021 at 1:34 PM Paolo Abeni [off-list ref] wrote:

quoted

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.

...

quoted

Address the issue checking for cloned skbs even in the GRO-without-XDP
input path.

Reported-and-tested-by: Ignat Korchagin <redacted>
Fixes: d3256efd8e8b ("veth: allow enabling NAPI even without XDP")
Signed-off-by: Paolo Abeni <pabeni@redhat.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.

quoted

+                       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

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