Re: [RFC net-next 1/9] skb: introduce gro_disabled bit

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Yan Zhai wrote:
> On Sun, Jun 23, 2024 at 3:27 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Yan Zhai wrote:
> > > > > -static inline bool netif_elide_gro(const struct net_device *dev)
> > > > > +static inline bool netif_elide_gro(const struct sk_buff *skb)
> > > > >  {
> > > > > -     if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
> > > > > +     if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
> > > > >               return true;
> > > > > +
> > > > > +#ifdef CONFIG_SKB_GRO_CONTROL
> > > > > +     return skb->gro_disabled;
> > > > > +#else
> > > > >       return false;
> > > > > +#endif
> > > >
> > > > Yet more branches in the hot path.
> > > >
> > > > Compile time configurability does not help, as that will be
> > > > enabled by distros.
> > > >
> > > > For a fairly niche use case. Where functionality of GRO already
> > > > works. So just a performance for a very rare case at the cost of a
> > > > regression in the common case. A small regression perhaps, but death
> > > > by a thousand cuts.
> > > >
> > >
> > > I share your concern on operating on this hotpath. Will a
> > > static_branch + sysctl make it less aggressive?
> >
> > That is always a possibility. But we have to use it judiciously,
> > cannot add a sysctl for every branch.
> >
> > I'm still of the opinion that Paolo shared that this seems a lot of
> > complexity for a fairly minor performance optimization for a rare
> > case.
> >
> Actually combining the discussion in this thread, I think it would be
> more than the corner cases that we encounter. Let me elaborate below.
> 
> > > Speaking of
> > > performance, I'd hope this can give us more control so we can achieve
> > > the best of two worlds: for TCP and some UDP traffic, we can enable
> > > GRO, while for some other classes that we know GRO does no good or
> > > even harm, let's disable GRO to save more cycles. The key observation
> > > is that developers may already know which traffic is blessed by GRO,
> > > but lack a way to realize it.
> >
> > Following up also on Daniel's point on using BPF as GRO engine. Even
> > earlier I tried to add an option to selectively enable GRO protocols
> > without BPF. Definitely worthwhile to be able to disable GRO handlers
> > to reduce attack surface to bad input.
> >
> I was probably staring too hard at my own things, which is indeed a
> corner case. But reducing the attack surface is indeed a good
> motivation for this patch. I checked briefly with our DoS team today,
> the DoS scenario will definitely benefit from skipping GRO, for
> example on SYN/RST floods. XDP is our main weapon to drop attack
> traffic today, but it does not always drop 100% of the floods, and
> time by time it does need to fall back to iptables due to the delay of
> XDP program assembly or the BPF limitation on analyzing the packet. I
> did an ad hoc measurement just now on a mostly idle server, with
> ~1.3Mpps SYN flood concentrated on one CPU and dropped them early in
> raw-PREROUTING. w/ GRO this would consume about 35-41% of the CPU
> time, while w/o GRO the time dropped to 9-12%. This seems a pretty
> significant breath room under heavy attacks.

A GRO opt-out might make sense.

A long time ago I sent a patch that configured GRO protocols using
syscalls, selectively (un)registering handlers. The interface was not
very nice, so I did not pursue it further. On the upside, the datapath
did not introduce any extra code. The intent was to reduce attack
surface of packet parsing code.

A few concerns with an XDP based opt-out. It is more work to enable:
requires compiling and load an XDP program. It adds cycles in the
hot path. And I do not entirely understand when an XDP program will be
able to detect that a packet should not enter the GRO engine, but
cannot drop the packet (your netfilter example above).

> But I am not sure I understand "BPF as GRO engine" here, it seems to
> me that being able to disable GRO by XDP is already good enough. Any
> more motivations to do more complex work here?

FWIW, we looked into this a few years ago. Analogous to the BPF flow
dissector: if the BPF program is loaded, use that instead of the C
code path. But we did not arrive at a practical implementation at the
time. Things may have changed, but one issue is how to store and
access the list (or table) of outstanding GRO skbs.

> best
> Yan
> 
> >
> > >
> > > best
> > > Yan
> >
> >