[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CANn89iLLWZ3v46KfCuHKzskQb58tW2mp0d-uibX_GV+=ZG9iUA@mail.gmail.com>
Date: Mon, 20 Jan 2025 15:55:26 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Thomas Bogendoerfer <tbogendoerfer@...e.de>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] gro_cells: Avoid packet re-ordering for cloned skbs
On Mon, Jan 20, 2025 at 3:32 PM Thomas Bogendoerfer
<tbogendoerfer@...e.de> wrote:
>
> On Mon, 13 Jan 2025 13:55:18 +0100
> Eric Dumazet <edumazet@...gle.com> wrote:
>
> > On Mon, Jan 13, 2025 at 12:28 PM Thomas Bogendoerfer
> > <tbogendoerfer@...e.de> wrote:
> > >
> > > On Thu, 9 Jan 2025 15:56:24 +0100
> > > Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > > On Thu, Jan 9, 2025 at 3:27 PM Thomas Bogendoerfer
> > > > <tbogendoerfer@...e.de> wrote:
> > > > >
> > > > > gro_cells_receive() passes a cloned skb directly up the stack and
> > > > > could cause re-ordering against segments still in GRO. To avoid
> > > > > this copy the skb and let GRO do it's work.
> > > > >
> > > > > Fixes: c9e6bc644e55 ("net: add gro_cells infrastructure")
> > > > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@...e.de>
> > > > > ---
> > > > > net/core/gro_cells.c | 11 ++++++++++-
> > > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c
> > > > > index ff8e5b64bf6b..2f8d688f9d82 100644
> > > > > --- a/net/core/gro_cells.c
> > > > > +++ b/net/core/gro_cells.c
> > > > > @@ -20,11 +20,20 @@ int gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb)
> > > > > if (unlikely(!(dev->flags & IFF_UP)))
> > > > > goto drop;
> > > > >
> > > > > - if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) {
> > > > > + if (!gcells->cells || netif_elide_gro(dev)) {
> > > > > +netif_rx:
> > > > > res = netif_rx(skb);
> > > > > goto unlock;
> > > > > }
> > > > > + if (skb_cloned(skb)) {
> > > > > + struct sk_buff *n;
> > > > >
> > > > > + n = skb_copy(skb, GFP_KERNEL);
> > > >
> > > > I do not think we want this skb_copy(). This is going to fail too often.
> > >
> > > ok
> > >
> > > > Can you remind us why we have this skb_cloned() check here ?
> > >
> > > some fields of the ip/tcp header are going to be changed in the first gro
> > > segment
> >
> > Presumably we should test skb_header_cloned()
> >
> > This means something like skb_cow_head(skb, 0) could be much more
> > reasonable than skb_copy().
>
> I don't think this will work, because at that point it's skb->data points
> at the IPv6 header in my test case (traffic between two namespaces connected
> via ip6 tunnel over ipvlan). Correct header offsets are set after later,
> when gro_cells napi routine runs.
>
> Do you see another option ?
Anything not attempting order-5 allocations will work :)
I would try something like that.
diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c
index ff8e5b64bf6b..74416194f148 100644
--- a/net/core/gro_cells.c
+++ b/net/core/gro_cells.c
@@ -4,6 +4,7 @@
#include <linux/netdevice.h>
#include <net/gro_cells.h>
#include <net/hotdata.h>
+#include <net/gro.h>
struct gro_cell {
struct sk_buff_head napi_skbs;
@@ -20,7 +21,7 @@ int gro_cells_receive(struct gro_cells *gcells,
struct sk_buff *skb)
if (unlikely(!(dev->flags & IFF_UP)))
goto drop;
- if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) {
+ if (!gcells->cells || netif_elide_gro(dev)) {
res = netif_rx(skb);
goto unlock;
}
@@ -58,7 +59,11 @@ static int gro_cell_poll(struct napi_struct *napi,
int budget)
skb = __skb_dequeue(&cell->napi_skbs);
if (!skb)
break;
- napi_gro_receive(napi, skb);
+ /* Core GRO stack does not play well with clones. */
+ if (skb_cloned(skb))
+ gro_normal_one(napi, skb, 1);
+ else
+ napi_gro_receive(napi, skb);
work_done++;
}
Powered by blists - more mailing lists