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: <8a47efb6-ee87-4287-b61b-eff37421609f@redhat.com>
Date: Tue, 11 Mar 2025 18:24:36 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: 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

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

/P