netdev - Re: [PATCH net-next v4 1/2] tcp: support TCP_RTO_MIN

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: <CANn89iL3E6TtmoNfHziZJbg-8nTZRg8-D3LyFg6g5GLvTmDfaA@mail.gmail.com>
Date: Tue, 25 Mar 2025 12:09:42 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, horms@...nel.org, 
	ncardwell@...gle.com, kuniyu@...zon.com, dsahern@...nel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v4 1/2] tcp: support TCP_RTO_MIN_US for
 set/getsockopt use

On Mon, Mar 17, 2025 at 1:03 PM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> Support adjusting/reading RTO MIN for socket level by using set/getsockopt().
>
> This new option has the same effect as TCP_BPF_RTO_MIN, which means it
> doesn't affect RTAX_RTO_MIN usage (by using ip route...). Considering that
> bpf option was implemented before this patch, so we need to use a standalone
> new option for pure tcp set/getsockopt() use.
>
> When the socket is created, its icsk_rto_min is set to the default
> value that is controlled by sysctl_tcp_rto_min_us. Then if application
> calls setsockopt() with TCP_RTO_MIN_US flag to pass a valid value, then
> icsk_rto_min will be overridden in jiffies unit.
>
> This patch adds WRITE_ONCE/READ_ONCE to avoid data-race around
> icsk_rto_min.
>
> Signed-off-by: Jason Xing <kerneljasonxing@...il.com>
> ---
>  Documentation/networking/ip-sysctl.rst |  4 ++--
>  include/net/tcp.h                      |  2 +-
>  include/uapi/linux/tcp.h               |  1 +
>  net/ipv4/tcp.c                         | 13 ++++++++++++-
>  4 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index 054561f8dcae..5c63ab928b97 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -1229,8 +1229,8 @@ tcp_pingpong_thresh - INTEGER
>  tcp_rto_min_us - INTEGER
>         Minimal TCP retransmission timeout (in microseconds). Note that the
>         rto_min route option has the highest precedence for configuring this
> -       setting, followed by the TCP_BPF_RTO_MIN socket option, followed by
> -       this tcp_rto_min_us sysctl.
> +       setting, followed by the TCP_BPF_RTO_MIN and TCP_RTO_MIN_US socket
> +       options, followed by this tcp_rto_min_us sysctl.
>
>         The recommended practice is to use a value less or equal to 200000
>         microseconds.
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 7207c52b1fc9..6a7aab854b86 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -806,7 +806,7 @@ u32 tcp_delack_max(const struct sock *sk);
>  static inline u32 tcp_rto_min(const struct sock *sk)
>  {
>         const struct dst_entry *dst = __sk_dst_get(sk);
> -       u32 rto_min = inet_csk(sk)->icsk_rto_min;
> +       u32 rto_min = READ_ONCE(inet_csk(sk)->icsk_rto_min);
>
>         if (dst && dst_metric_locked(dst, RTAX_RTO_MIN))
>                 rto_min = dst_metric_rtt(dst, RTAX_RTO_MIN);
> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> index 32a27b4a5020..b2476cf7058e 100644
> --- a/include/uapi/linux/tcp.h
> +++ b/include/uapi/linux/tcp.h
> @@ -137,6 +137,7 @@ enum {
>
>  #define TCP_IS_MPTCP           43      /* Is MPTCP being used? */
>  #define TCP_RTO_MAX_MS         44      /* max rto time in ms */
> +#define TCP_RTO_MIN_US         45      /* min rto time in us */
>
>  #define TCP_REPAIR_ON          1
>  #define TCP_REPAIR_OFF         0
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 46951e749308..b89c1b676b8e 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -3352,7 +3352,7 @@ int tcp_disconnect(struct sock *sk, int flags)
>         icsk->icsk_probes_out = 0;
>         icsk->icsk_probes_tstamp = 0;
>         icsk->icsk_rto = TCP_TIMEOUT_INIT;
> -       icsk->icsk_rto_min = TCP_RTO_MIN;
> +       WRITE_ONCE(icsk->icsk_rto_min, TCP_RTO_MIN);

Semi-orthogonal to your patch, apparently at disconnect() we throw
away user/eBPF choice.

Most socket options, once set via setsockopt() should stay even if the
socket is re-used.

Reviewed-by: Eric Dumazet <edumazet@...gle.com>