netdev - Re: [PATCH net-next 1/2] udp_tunnel: create a fast-path GRO lookup.

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: <840bec2f-51a6-4036-bf9a-f215a4db1be5@redhat.com>
Date: Thu, 6 Mar 2025 22:20:02 +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>
Subject: Re: [PATCH net-next 1/2] udp_tunnel: create a fast-path GRO lookup.

On 3/6/25 8:46 PM, Willem de Bruijn wrote:
> Paolo Abeni wrote:
>> Most UDP tunnels bind a socket to a local port, with ANY address, no
>> peer and no interface index specified.
>> Additionally it's quite common to have a single tunnel device per
>> namespace.
>>
>> Track in each namespace the UDP tunnel socket respecting the above.
>> When only a single one is present, store a reference in the netns.
>>
>> When such reference is not NULL, UDP tunnel GRO lookup just need to
>> match the incoming packet destination port vs the socket local port.
>>
>> The tunnel socket never set the reuse[port] flag[s], when bound to no
>> address and interface, no other socket can exist in the same netns
>> matching the specified local port.
>>
>> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> 
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index c1a85b300ee87..ac6dd2703190e 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -12,6 +12,38 @@
>>  #include <net/udp.h>
>>  #include <net/protocol.h>
>>  #include <net/inet_common.h>
>> +#include <net/udp_tunnel.h>
>> +
>> +#if IS_ENABLED(CONFIG_NET_UDP_TUNNEL)
>> +static DEFINE_SPINLOCK(udp_tunnel_gro_lock);
>> +
>> +void udp_tunnel_update_gro_lookup(struct net *net, struct sock *sk, bool add)
>> +{
>> +	bool is_ipv6 = sk->sk_family == AF_INET6;
>> +	struct udp_sock *tup, *up = udp_sk(sk);
>> +	struct udp_tunnel_gro *udp_tunnel_gro;
>> +
>> +	spin_lock(&udp_tunnel_gro_lock);
>> +	udp_tunnel_gro = &net->ipv4.udp_tunnel_gro[is_ipv6];
> 
> It's a bit odd to have an ipv6 member in netns.ipv4. Does it
> significantly simplify the code vs a separate entry in netns.ipv6?

The code complexity should not change much. I place both the ipv4 and
ipv6 data there to allow cache-line based optimization, as all the netns
fast-path fields are under struct netns_ipv4.

Currently the UDP tunnel related fields share the same cache-line of
`udp_table`.

>> @@ -631,8 +663,13 @@ static struct sock *udp4_gro_lookup_skb(struct sk_buff *skb, __be16 sport,
>>  {
>>  	const struct iphdr *iph = skb_gro_network_header(skb);
>>  	struct net *net = dev_net_rcu(skb->dev);
>> +	struct sock *sk;
>>  	int iif, sdif;
>>  
>> +	sk = udp_tunnel_sk(net, false);
>> +	if (sk && dport == htons(sk->sk_num))
>> +		return sk;
>> +
> 
> This improves tunnel performance at a slight cost to everything else,
> by having the tunnel check before the normal socket path.
> 
> Does a 5% best case gain warrant that? Not snark, I don't have a
> good answer.

We enter this function only when udp_encap_needed_key is true: ~an UDP
tunnel has been configured[1].

When tunnels are enabled, AFAIK the single tunnel device is the most
common and most relevant scenario, and in such setup this gives
measurable performance improvement. Other tunnel-based scenarios will
see the additional cost of a single conditional (using data on an
already hot cacheline, due to the above layout).

If you are concerned about such cost, I can add an additional static
branch protecting the above code chunk, so that the conditional will be
performed only when there is a single UDP tunnel configured. Please, let
me know.

Thanks,

Paolo

[1] to be more accurate: an UDP tunnel or an UDP socket with GRO enabled