linux-kernel - Re: [PATCH v2 net] gro_cells: Avoid packet re-ordering for cloned skbs

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: <CANn89iLwOWvzZqN2VpUQ74a5BXRgvZH4_D2iesQBdnGWmZodcg@mail.gmail.com>
Date: Thu, 23 Jan 2025 11:07:07 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Thomas Bogendoerfer <tbogendoerfer@...e.de>, "David S. Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Simon Horman <horms@...nel.org>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 net] gro_cells: Avoid packet re-ordering for cloned skbs

On Thu, Jan 23, 2025 at 9:43 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 1/21/25 12:50 PM, Thomas Bogendoerfer 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 queue cloned skbs and use gro_normal_one() to pass it during
> > normal NAPI work.
> >
> > Fixes: c9e6bc644e55 ("net: add gro_cells infrastructure")
> > Suggested-by: Eric Dumazet <edumazet@...gle.com>
> > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@...e.de>
> > --
> > v2: don't use skb_copy(), but make decision how to pass cloned skbs in
> >     napi poll function (suggested by Eric)
> > v1: https://lore.kernel.org/lkml/20250109142724.29228-1-tbogendoerfer@suse.de/
> >
> >  net/core/gro_cells.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c
> > index ff8e5b64bf6b..762746d18486 100644
> > --- a/net/core/gro_cells.c
> > +++ b/net/core/gro_cells.c
> > @@ -2,6 +2,7 @@
> >  #include <linux/skbuff.h>
> >  #include <linux/slab.h>
> >  #include <linux/netdevice.h>
> > +#include <net/gro.h>
> >  #include <net/gro_cells.h>
> >  #include <net/hotdata.h>
> >
> > @@ -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);
>
> I must admit it's not clear to me how/why the above will avoid OoO. I
> assume OoO happens when we observe both cloned and uncloned packets
> belonging to the same connection/flow.
>
> What if we have a (uncloned) packet for the relevant flow in the GRO,
> 'rx_count - 1' packets already sitting in 'rx_list' and a cloned packet
> for the critical flow reaches gro_cells_receive()?
>
> Don't we need to unconditionally flush any packets belonging to the same
> flow?

It would only matter if we had 2 or more segments that would belong
to the same flow and packet train (potential 'GRO super packet'), with
the 'cloned'
status being of mixed value on various segments.

In practice, the cloned status will be the same for all segments.

Same issue would happen when/if dev->features NETIF_F_GRO is flipped
back and forth : We do not really care.