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

Thank you for the feedback.

I double checked the above: in my test even skb_cloned() suffice.

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

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