netdev - Re: [PATCH bpf-next v3 1/7] udp: re-score reuseport groups when connected sockets are present

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: <20230626172537.57064-1-kuniyu@amazon.com>
Date: Mon, 26 Jun 2023 10:25:37 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <lmb@...valent.com>
CC: <andrii@...nel.org>, <ast@...nel.org>, <bpf@...r.kernel.org>,
	<daniel@...earbox.net>, <davem@...emloft.net>, <dsahern@...nel.org>,
	<edumazet@...gle.com>, <haoluo@...gle.com>, <hemanthmalla@...il.com>,
	<joe@...d.net.nz>, <john.fastabend@...il.com>, <jolsa@...nel.org>,
	<kpsingh@...nel.org>, <kuba@...nel.org>, <kuniyu@...zon.com>,
	<linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
	<martin.lau@...ux.dev>, <mykolal@...com>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>, <sdf@...gle.com>, <shuah@...nel.org>, <song@...nel.org>,
	<willemdebruijn.kernel@...il.com>, <yhs@...com>
Subject: Re: [PATCH bpf-next v3 1/7] udp: re-score reuseport groups when connected sockets are present

From: Lorenz Bauer <lmb@...valent.com>
Date: Mon, 26 Jun 2023 16:08:58 +0100
> Contrary to TCP, UDP reuseport groups can contain TCP_ESTABLISHED
> sockets. To support these properly we remember whether a group has
> a connected socket and skip the fast reuseport early-return. In
> effect we continue scoring all reuseport sockets and then choose the
> one with the highest score.
> 
> The current code fails to re-calculate the score for the result of
> lookup_reuseport. According to Kuniyuki Iwashima:
> 
>     1) SO_INCOMING_CPU is set
>        -> selected sk might have +1 score
> 
>     2) BPF prog returns ESTABLISHED and/or SO_INCOMING_CPU sk
>        -> selected sk will have more than 8
> 
>   Using the old score could trigger more lookups depending on the
>   order that sockets are created.
> 
>     sk -> sk (SO_INCOMING_CPU) -> sk (ESTABLISHED)
>     |     |
>     `-> select the next SO_INCOMING_CPU sk
>           |
>           `-> select itself (We should save this lookup)
> 
> Fixes: efc6b6f6c311 ("udp: Improve load balancing for SO_REUSEPORT.")
> Signed-off-by: Lorenz Bauer <lmb@...valent.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@...zon.com>

1 minor nit below.


> ---
>  net/ipv4/udp.c | 20 +++++++++++++++-----
>  net/ipv6/udp.c | 19 ++++++++++++++-----
>  2 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index fd3dae081f3a..5ef478d2c408 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -450,14 +450,24 @@ static struct sock *udp4_lib_lookup2(struct net *net,
>  		score = compute_score(sk, net, saddr, sport,
>  				      daddr, hnum, dif, sdif);
>  		if (score > badness) {
> -			result = lookup_reuseport(net, sk, skb,
> -						  saddr, sport, daddr, hnum);
> +			badness = score;
> +			result = lookup_reuseport(net, sk, skb, saddr, sport, daddr, hnum);
> +			if (!result) {
> +				result = sk;
> +				continue;
> +			}
> +
>  			/* Fall back to scoring if group has connections */
> -			if (result && !reuseport_has_conns(sk))
> +			if (!reuseport_has_conns(sk))
>  				return result;
>  
> -			result = result ? : sk;
> -			badness = score;
> +			/* Reuseport logic returned an error, keep original score. */
> +			if (IS_ERR(result))
> +				continue;
> +
> +			badness = compute_score(result, net, saddr, sport,
> +						daddr, hnum, dif, sdif);
> +

Unnecessary blank line here.


>  		}
>  	}
>  	return result;
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index e5a337e6b970..8b3cb1d7da7c 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -193,14 +193,23 @@ static struct sock *udp6_lib_lookup2(struct net *net,
>  		score = compute_score(sk, net, saddr, sport,
>  				      daddr, hnum, dif, sdif);
>  		if (score > badness) {
> -			result = lookup_reuseport(net, sk, skb,
> -						  saddr, sport, daddr, hnum);
> +			badness = score;
> +			result = lookup_reuseport(net, sk, skb, saddr, sport, daddr, hnum);
> +			if (!result) {
> +				result = sk;
> +				continue;
> +			}
> +
>  			/* Fall back to scoring if group has connections */
> -			if (result && !reuseport_has_conns(sk))
> +			if (!reuseport_has_conns(sk))
>  				return result;
>  
> -			result = result ? : sk;
> -			badness = score;
> +			/* Reuseport logic returned an error, keep original score. */
> +			if (IS_ERR(result))
> +				continue;
> +
> +			badness = compute_score(sk, net, saddr, sport,
> +						daddr, hnum, dif, sdif);
>  		}
>  	}
>  	return result;
> 
> -- 
> 2.40.1