netdev - Re: [RFC net-next 1/9] skb: introduce gro

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: <6677dc5cb5cca_33522729474@willemb.c.googlers.com.notmuch>
Date: Sun, 23 Jun 2024 04:27:08 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Yan Zhai <yan@...udflare.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, 
 "David S. Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 Alexei Starovoitov <ast@...nel.org>, 
 Daniel Borkmann <daniel@...earbox.net>, 
 Jesper Dangaard Brouer <hawk@...nel.org>, 
 John Fastabend <john.fastabend@...il.com>, 
 Willem de Bruijn <willemb@...gle.com>, 
 Simon Horman <horms@...nel.org>, 
 Florian Westphal <fw@...len.de>, 
 Mina Almasry <almasrymina@...gle.com>, 
 Abhishek Chauhan <quic_abchauha@...cinc.com>, 
 David Howells <dhowells@...hat.com>, 
 Alexander Lobakin <aleksander.lobakin@...el.com>, 
 David Ahern <dsahern@...nel.org>, 
 Richard Gobert <richardbgobert@...il.com>, 
 Antoine Tenart <atenart@...nel.org>, 
 Felix Fietkau <nbd@....name>, 
 Soheil Hassas Yeganeh <soheil@...gle.com>, 
 Pavel Begunkov <asml.silence@...il.com>, 
 Lorenzo Bianconi <lorenzo@...nel.org>, 
 Thomas Weißschuh <linux@...ssschuh.net>, 
 linux-kernel@...r.kernel.org, 
 bpf@...r.kernel.org
Subject: Re: [RFC net-next 1/9] skb: introduce gro_disabled bit

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.

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

> 
> best
> Yan