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] [day] [month] [year] [list]

Message-ID: <CAL+tcoC0jsCMie0Y5qgwGVb=MZ+gfMhBC3eodGp3G5Yr=dC4Dw@mail.gmail.com>
Date: Wed, 19 Feb 2025 10:17:52 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
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 Wed, Feb 19, 2025 at 10:01 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> 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.

I don't think there is any difference between them. For both of them,
It would be better to set before transmission.

>
> Otherwise, the commit message can be just this:
>
> Test the TCP_RTO_MAX_MS optname in the existing setget_sockopt test.

Got it. Will use this instead.

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

I thought we needed to sync it from include/uapi/linux/tcp.h. Will test it then.

Thanks,
Jason

>
> >
> >
> >   #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, },
> >   };
> >
>