netdev - Re: [PATCH v3 net-next 2/2] udp_tunnel: use static call for GRO hooks when possible

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: <67d07324aff9a_2fa72c294d@willemb.c.googlers.com.notmuch>
Date: Tue, 11 Mar 2025 13:30:12 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Paolo Abeni <pabeni@...hat.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Simon Horman <horms@...nel.org>, 
 David Ahern <dsahern@...nel.org>, 
 kuniyu@...zon.com
Subject: Re: [PATCH v3 net-next 2/2] udp_tunnel: use static call for GRO hooks
 when possible

Paolo Abeni wrote:
> On 3/11/25 3:51 AM, Willem de Bruijn wrote:
> > Paolo Abeni wrote:
> [...]
> >> +void udp_tunnel_update_gro_rcv(struct sock *sk, bool add)
> >> +{
> >> +	struct udp_tunnel_type_entry *cur = NULL, *avail = NULL;
> >> +	struct udp_sock *up = udp_sk(sk);
> >> +	int i, old_gro_type_nr;
> >> +
> >> +	if (!up->gro_receive)
> >> +		return;
> >> +
> >> +	mutex_lock(&udp_tunnel_gro_type_lock);
> >> +	for (i = 0; i < UDP_MAX_TUNNEL_TYPES; i++) {
> >> +		if (!refcount_read(&udp_tunnel_gro_types[i].count))
> > 
> > Optionally: && !avail, to fill the list from the front. And on delete
> > avoid gaps. For instance, like __fanout_link/__fanout_unlink.
> > 
> > Can stop sooner then. And list length is then implicit as i once found
> > the first [i].count == zero.
> > 
> > Then again, this list is always short. I can imagine you prefer to
> > leave as is.
> 
> I avoided optimizations for this slow path, to keep the code simpler.
> Thinking again about it, avoiding gaps will simplify/cleanup the code a
> bit (no need to lookup the enabled tunnel on deletion and to use `avail`
> on addition), so I'll do it.
> 
> Note that I'll still need to explicitly track the number of enabled
> tunnel types, as an easy way to disable the static call in the unlikely
> udp_tunnel_gro_type_nr == UDP_MAX_TUNNEL_TYPES event.
> 
> [...]
> >> +	if (udp_tunnel_gro_type_nr == 1) {
> >> +		for (i = 0; i < UDP_MAX_TUNNEL_TYPES; i++) {
> >> +			cur = &udp_tunnel_gro_types[i];
> >> +			if (refcount_read(&cur->count)) {
> >> +				static_call_update(udp_tunnel_gro_rcv,
> >> +						   cur->gro_receive);
> >> +				static_branch_enable(&udp_tunnel_static_call);
> >> +			}
> >> +		}
> >> +	} else if (old_gro_type_nr == 1) {
> >> +		static_branch_disable(&udp_tunnel_static_call);
> >> +		static_call_update(udp_tunnel_gro_rcv, dummy_gro_rcv);
> > 
> > These operations must not be reorderd, or dummy_gro_rcv might get hit.
> > 
> > If static calls are not configured, the last call is just a
> > WRITE_ONCE. Similar for static_branch_disable if !CONFIG_JUMP_LABEL.
> 
> When both construct are disabled, I think a wmb/rmb pair would be needed
> to ensure no reordering, and that in turn looks overkill. I think it
> would be better just drop the WARN_ONCE in dummy_gro_rcv().

SGTM, thanks.