netdev - Re: [PATCH bpf-next v2 3/3] selftests/bpf: add rto max for bpf

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: <71d6c5e8-058b-470d-b411-347e2a1266a5@linux.dev>
Date: Tue, 18 Feb 2025 18:01:10 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, dsahern@...nel.org, kuniyu@...zon.com, ast@...nel.org,
 daniel@...earbox.net, andrii@...nel.org, eddyz87@...il.com, song@...nel.org,
 yonghong.song@...ux.dev, john.fastabend@...il.com, kpsingh@...nel.org,
 sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, shuah@...nel.org,
 ykolal@...com, bpf@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 3/3] selftests/bpf: add rto max for
 bpf_setsockopt test

On 2/16/25 7:42 PM, Jason Xing wrote:
> Add TCP_RTO_MAX_MS selftests for active and passive flows
> in various bpf callbacks. Even though the TCP_RTO_MAX_MS
> can be used in established phase, we highly discourage
> to do so because it may trigger unexpected behaviour.
> On the contrary, it's highly recommended that the maximum
> value of RTO is set before first time of transmission, such
> as BPF_SOCK_OPS_{PASSIVE|ACTIVE}_ESTABLISHED_CB,

s/,/./

What unexpected behavior when setting in BPF after the established state?

Setting it after the established state or not is not specific to BPF. syscall 
can choose to do it after the connection established also. The above makes it 
unclear what unexpected behavior that the BPF prog will cause if TCP_RTO_MAX_MS 
is used in BPF instead of syscall.

If there is subtle difference between calling TCP_RTO_MAX_MS from bpf and from 
syscall, please write it clearly what are the unexpected behaviors when calling 
in BPF after the established states.

Otherwise, the commit message can be just this:

Test the TCP_RTO_MAX_MS optname in the existing setget_sockopt test.

> 
> Signed-off-by: Jason Xing <kerneljasonxing@...il.com>
> ---
>   tools/include/uapi/linux/tcp.h                      | 1 +
>   tools/testing/selftests/bpf/progs/bpf_tracing_net.h | 1 +
>   tools/testing/selftests/bpf/progs/setget_sockopt.c  | 1 +
>   3 files changed, 3 insertions(+)
> 
> diff --git a/tools/include/uapi/linux/tcp.h b/tools/include/uapi/linux/tcp.h
> index 13ceeb395eb8..7989e3f34a58 100644
> --- a/tools/include/uapi/linux/tcp.h
> +++ b/tools/include/uapi/linux/tcp.h
> @@ -128,6 +128,7 @@ enum {
>   #define TCP_CM_INQ		TCP_INQ
>   
>   #define TCP_TX_DELAY		37	/* delay outgoing packets by XX usec */
> +#define TCP_RTO_MAX_MS		44	/* max rto time in ms */

Have you checked if this change is really needed?

>   
>   
>   #define TCP_REPAIR_ON		1
> diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> index 59843b430f76..eb6ed1b7b2ef 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> @@ -49,6 +49,7 @@
>   #define TCP_SAVED_SYN		28
>   #define TCP_CA_NAME_MAX		16
>   #define TCP_NAGLE_OFF		1
> +#define TCP_RTO_MAX_MS		44
>   
>   #define TCP_ECN_OK              1
>   #define TCP_ECN_QUEUE_CWR       2
> diff --git a/tools/testing/selftests/bpf/progs/setget_sockopt.c b/tools/testing/selftests/bpf/progs/setget_sockopt.c
> index 6dd4318debbf..106fe430f41b 100644
> --- a/tools/testing/selftests/bpf/progs/setget_sockopt.c
> +++ b/tools/testing/selftests/bpf/progs/setget_sockopt.c
> @@ -61,6 +61,7 @@ static const struct sockopt_test sol_tcp_tests[] = {
>   	{ .opt = TCP_NOTSENT_LOWAT, .new = 1314, .expected = 1314, },
>   	{ .opt = TCP_BPF_SOCK_OPS_CB_FLAGS, .new = BPF_SOCK_OPS_ALL_CB_FLAGS,
>   	  .expected = BPF_SOCK_OPS_ALL_CB_FLAGS, },
> +	{ .opt = TCP_RTO_MAX_MS, .new = 2000, .expected = 2000, },
>   	{ .opt = 0, },
>   };
>