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: <CA+FuTScuTVjJLZFzMrufhp9+WzUnvmtsvShN6FWPx1R_Cau7Uw@mail.gmail.com>
Date: Thu, 6 Mar 2025 17:25:33 -0500
From: Willem de Bruijn <willemb@...gle.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>, netdev@...r.kernel.org, 
	"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 Thu, Mar 6, 2025 at 4:20 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> 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`.

That's reason enough. Since you have to respin, please add a comment
in the commit message. It looked surprising at first read.

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

Oh right, not all regular UDP GRO. That is okay then.