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

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: <CANn89iLTb_JgLAKk5omW82SH-h8qtZLs54nX5c9Y9GbKdmTFgg@mail.gmail.com>
Date: Tue, 25 Mar 2025 12:13:13 +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 2/2] tcp: support TCP_DELACK_MAX_US for
 set/getsockopt use

On Mon, Mar 17, 2025 at 1:03 PM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> Support adjusting/reading delayed ack max for socket level by using
> set/getsockopt().
>
> This option aligns with TCP_BPF_DELACK_MAX usage. Considering that bpf
> option was implemented before this patch, so we need to use a standalone
> new option for pure tcp set/getsockopt() use.
>
> Add WRITE_ONCE/READ_ONCE() to prevent data-race if setsockopt()
> happens to write one value to icsk_delack_max while icsk_delack_max is
> being read.
>
> Signed-off-by: Jason Xing <kerneljasonxing@...il.com>
> ---
>  include/uapi/linux/tcp.h |  1 +
>  net/ipv4/tcp.c           | 13 ++++++++++++-
>  net/ipv4/tcp_output.c    |  2 +-
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> index b2476cf7058e..2377e22f2c4b 100644
> --- a/include/uapi/linux/tcp.h
> +++ b/include/uapi/linux/tcp.h
> @@ -138,6 +138,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_DELACK_MAX_US      46      /* max delayed ack 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 b89c1b676b8e..578e79024955 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -3353,7 +3353,7 @@ int tcp_disconnect(struct sock *sk, int flags)
>         icsk->icsk_probes_tstamp = 0;
>         icsk->icsk_rto = TCP_TIMEOUT_INIT;
>         WRITE_ONCE(icsk->icsk_rto_min, TCP_RTO_MIN);
> -       icsk->icsk_delack_max = TCP_DELACK_MAX;
> +       WRITE_ONCE(icsk->icsk_delack_max, TCP_DELACK_MAX);

Same comment here as the first patch, I think we should not change
csk->icsk_delack_max in tcp_disconnect(),
otherwise a prior setsockopt() setting is erased.

Probably not a big deal, and if it is, could be fixed in a followup patch.

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