[PATCH net-next 0/2] tcp: annotate data-races around icsk_retransmits and icsk_probes_out

[PATCH net-next 0/2] tcp: annotate data-races around icsk_retransmits and icsk_probes_out

[Eric Dumazet]

icsk->icsk_retransmits is read locklessly from inet_sk_diag_fill(),
tcp_get_timestamping_opt_stats, get_tcp4_sock() and get_tcp6_sock().

icsk->icsk_probes_out is read locklessly from inet_sk_diag_fill(),
get_tcp4_sock() and get_tcp6_sock().

Add corresponding READ_ONCE()/WRITE_ONCE() annotations.

Eric Dumazet (2):
  tcp: annotate data-races around icsk->icsk_retransmits
  tcp: annotate data-races around icsk->icsk_probes_out

 net/ipv4/inet_diag.c  | 6 +++---
 net/ipv4/tcp.c        | 5 +++--
 net/ipv4/tcp_input.c  | 8 ++++----
 net/ipv4/tcp_ipv4.c   | 4 ++--
 net/ipv4/tcp_output.c | 6 +++---
 net/ipv4/tcp_timer.c  | 6 +++---
 net/ipv6/tcp_ipv6.c   | 4 ++--
 net/mptcp/protocol.c  | 3 ++-
 8 files changed, 22 insertions(+), 20 deletions(-)

-- 
2.51.0.rc2.233.g662b1ed5c5-goog

[PATCH net-next 1/2] tcp: annotate data-races around icsk->icsk_retransmits

[Eric Dumazet]

icsk->icsk_retransmits is read locklessly from inet_sk_diag_fill(),
tcp_get_timestamping_opt_stats, get_tcp4_sock() and get_tcp6_sock().

Add corresponding READ_ONCE()/WRITE_ONCE() annotations.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/inet_diag.c  | 2 +-
 net/ipv4/tcp.c        | 3 ++-
 net/ipv4/tcp_input.c  | 6 +++---
 net/ipv4/tcp_ipv4.c   | 2 +-
 net/ipv4/tcp_output.c | 2 +-
 net/ipv4/tcp_timer.c  | 2 +-
 net/ipv6/tcp_ipv6.c   | 2 +-
 net/mptcp/protocol.c  | 3 ++-
 8 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 2fa53b16fe7788eed9796c8476157a77eced096c..35c1579e5bd4cef4a8c44d03ed1af16fe1735ac0 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -313,7 +313,7 @@ int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk,
 	    icsk_pending == ICSK_TIME_REO_TIMEOUT ||
 	    icsk_pending == ICSK_TIME_LOSS_PROBE) {
 		r->idiag_timer = 1;
-		r->idiag_retrans = icsk->icsk_retransmits;
+		r->idiag_retrans = READ_ONCE(icsk->icsk_retransmits);
 		r->idiag_expires =
 			jiffies_delta_to_msecs(icsk_timeout(icsk) - jiffies);
 	} else if (icsk_pending == ICSK_TIME_PROBE0) {
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 71a956fbfc5533224ee00e792de2cfdccd4d40aa..463281ad076fd3baeaee5f9131f0e2ffd947e90b 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4348,7 +4348,8 @@ struct sk_buff *tcp_get_timestamping_opt_stats(const struct sock *sk,
 	nla_put_u32(stats, TCP_NLA_REORDERING, tp->reordering);
 	nla_put_u32(stats, TCP_NLA_MIN_RTT, tcp_min_rtt(tp));
 
-	nla_put_u8(stats, TCP_NLA_RECUR_RETRANS, inet_csk(sk)->icsk_retransmits);
+	nla_put_u8(stats, TCP_NLA_RECUR_RETRANS,
+		   READ_ONCE(inet_csk(sk)->icsk_retransmits));
 	nla_put_u8(stats, TCP_NLA_DELIVERY_RATE_APP_LMT, !!tp->rate_app_limited);
 	nla_put_u32(stats, TCP_NLA_SND_SSTHRESH, tp->snd_ssthresh);
 	nla_put_u32(stats, TCP_NLA_DELIVERED, tp->delivered);
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 71b76e98371a667b6e8263b32c242363672d7c5a..87f06e5acfe86411f5ea68e565f0c2bf4794ed53 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2569,7 +2569,7 @@ static bool tcp_try_undo_loss(struct sock *sk, bool frto_undo)
 		if (frto_undo)
 			NET_INC_STATS(sock_net(sk),
 					LINUX_MIB_TCPSPURIOUSRTOS);
-		inet_csk(sk)->icsk_retransmits = 0;
+		WRITE_ONCE(inet_csk(sk)->icsk_retransmits, 0);
 		if (tcp_is_non_sack_preventing_reopen(sk))
 			return true;
 		if (frto_undo || tcp_is_sack(tp)) {
@@ -3851,7 +3851,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
 
 	if (after(ack, prior_snd_una)) {
 		flag |= FLAG_SND_UNA_ADVANCED;
-		icsk->icsk_retransmits = 0;
+		WRITE_ONCE(icsk->icsk_retransmits, 0);
 
 #if IS_ENABLED(CONFIG_TLS_DEVICE)
 		if (static_branch_unlikely(&clean_acked_data_enabled.key))
@@ -6636,7 +6636,7 @@ static void tcp_rcv_synrecv_state_fastopen(struct sock *sk)
 		tcp_try_undo_recovery(sk);
 
 	tcp_update_rto_time(tp);
-	inet_csk(sk)->icsk_retransmits = 0;
+	WRITE_ONCE(inet_csk(sk)->icsk_retransmits, 0);
 	/* In tcp_fastopen_synack_timer() on the first SYNACK RTO we set
 	 * retrans_stamp but don't enter CA_Loss, so in case that happened we
 	 * need to zero retrans_stamp here to prevent spurious
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 84d3d556ed8062d07fe7019bc0dadd90d3b80d96..5d549dfd4e601eedb7c5cc3f3e503a5bef7ed4a4 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2958,7 +2958,7 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i)
 		rx_queue,
 		timer_active,
 		jiffies_delta_to_clock_t(timer_expires - jiffies),
-		icsk->icsk_retransmits,
+		READ_ONCE(icsk->icsk_retransmits),
 		from_kuid_munged(seq_user_ns(f), sk_uid(sk)),
 		icsk->icsk_probes_out,
 		sock_i_ino(sk),
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index dfbac0876d96ee6b556fff5b6c9ec8fe2e04aa05..8623def6f88921130e1944f03eb5408e9e26120d 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3954,7 +3954,7 @@ static void tcp_connect_init(struct sock *sk)
 	WRITE_ONCE(tp->copied_seq, tp->rcv_nxt);
 
 	inet_csk(sk)->icsk_rto = tcp_timeout_init(sk);
-	inet_csk(sk)->icsk_retransmits = 0;
+	WRITE_ONCE(inet_csk(sk)->icsk_retransmits, 0);
 	tcp_clear_retrans(tp);
 }
 
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index a207877270fbdef6f86f61093aa476b6cd6f8706..8b11ab4cc9524f0e0066f474044a4c585115ec29 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -444,7 +444,7 @@ static void tcp_update_rto_stats(struct sock *sk)
 		tp->total_rto_recoveries++;
 		tp->rto_stamp = tcp_time_stamp_ms(tp);
 	}
-	icsk->icsk_retransmits++;
+	WRITE_ONCE(icsk->icsk_retransmits, icsk->icsk_retransmits + 1);
 	tp->total_rto++;
 }
 
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 7577e7eb2c97b821826f633a11dd5567dde7b7cb..7b177054452b983d3f49a06869950565c03c13ee 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -2230,7 +2230,7 @@ static void get_tcp6_sock(struct seq_file *seq, struct sock *sp, int i)
 		   rx_queue,
 		   timer_active,
 		   jiffies_delta_to_clock_t(timer_expires - jiffies),
-		   icsk->icsk_retransmits,
+		   READ_ONCE(icsk->icsk_retransmits),
 		   from_kuid_munged(seq_user_ns(seq), sk_uid(sp)),
 		   icsk->icsk_probes_out,
 		   sock_i_ino(sp),
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 9a287b75c1b31bac9c35581db996e39e594872e0..f2e728239480444ffdb297efc35303848d4c4a31 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2587,7 +2587,8 @@ static void __mptcp_retrans(struct sock *sk)
 		if (mptcp_data_fin_enabled(msk)) {
 			struct inet_connection_sock *icsk = inet_csk(sk);
 
-			icsk->icsk_retransmits++;
+			WRITE_ONCE(icsk->icsk_retransmits,
+				   icsk->icsk_retransmits + 1);
 			mptcp_set_datafin_timeout(sk);
 			mptcp_send_ack(msk);
 
-- 
2.51.0.rc2.233.g662b1ed5c5-goog

[PATCH net-next 2/2] tcp: annotate data-races around icsk->icsk_probes_out

[Eric Dumazet]

icsk->icsk_probes_out is read locklessly from inet_sk_diag_fill(),
get_tcp4_sock() and get_tcp6_sock().

Add corresponding READ_ONCE()/WRITE_ONCE() annotations.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/inet_diag.c  | 4 ++--
 net/ipv4/tcp.c        | 2 +-
 net/ipv4/tcp_input.c  | 2 +-
 net/ipv4/tcp_ipv4.c   | 2 +-
 net/ipv4/tcp_output.c | 4 ++--
 net/ipv4/tcp_timer.c  | 4 ++--
 net/ipv6/tcp_ipv6.c   | 2 +-
 7 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 35c1579e5bd4cef4a8c44d03ed1af16fe1735ac0..549f1f521f4f6f9e5fe347767ed0f98f4aa9b62a 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -318,12 +318,12 @@ int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk,
 			jiffies_delta_to_msecs(icsk_timeout(icsk) - jiffies);
 	} else if (icsk_pending == ICSK_TIME_PROBE0) {
 		r->idiag_timer = 4;
-		r->idiag_retrans = icsk->icsk_probes_out;
+		r->idiag_retrans = READ_ONCE(icsk->icsk_probes_out);
 		r->idiag_expires =
 			jiffies_delta_to_msecs(icsk_timeout(icsk) - jiffies);
 	} else if (timer_pending(&sk->sk_timer)) {
 		r->idiag_timer = 2;
-		r->idiag_retrans = icsk->icsk_probes_out;
+		r->idiag_retrans = READ_ONCE(icsk->icsk_probes_out);
 		r->idiag_expires =
 			jiffies_delta_to_msecs(sk->sk_timer.expires - jiffies);
 	}
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 463281ad076fd3baeaee5f9131f0e2ffd947e90b..676b23d6fef9198361afd75bb161454e99aaefdc 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3376,7 +3376,7 @@ int tcp_disconnect(struct sock *sk, int flags)
 	WRITE_ONCE(tp->write_seq, seq);
 
 	icsk->icsk_backoff = 0;
-	icsk->icsk_probes_out = 0;
+	WRITE_ONCE(icsk->icsk_probes_out, 0);
 	icsk->icsk_probes_tstamp = 0;
 	icsk->icsk_rto = TCP_TIMEOUT_INIT;
 	WRITE_ONCE(icsk->icsk_rto_min, TCP_RTO_MIN);
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 87f06e5acfe86411f5ea68e565f0c2bf4794ed53..29b1ab0bde59fd5e0237c233f5a50258ce7ab39f 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3913,7 +3913,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
 	 * log. Something worked...
 	 */
 	WRITE_ONCE(sk->sk_err_soft, 0);
-	icsk->icsk_probes_out = 0;
+	WRITE_ONCE(icsk->icsk_probes_out, 0);
 	tp->rcv_tstamp = tcp_jiffies32;
 	if (!prior_packets)
 		goto no_queue;
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 5d549dfd4e601eedb7c5cc3f3e503a5bef7ed4a4..9543f153835908286384defa787e2a707fb8dbda 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2960,7 +2960,7 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i)
 		jiffies_delta_to_clock_t(timer_expires - jiffies),
 		READ_ONCE(icsk->icsk_retransmits),
 		from_kuid_munged(seq_user_ns(f), sk_uid(sk)),
-		icsk->icsk_probes_out,
+		READ_ONCE(icsk->icsk_probes_out),
 		sock_i_ino(sk),
 		refcount_read(&sk->sk_refcnt), sk,
 		jiffies_to_clock_t(icsk->icsk_rto),
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 8623def6f88921130e1944f03eb5408e9e26120d..5bd4e52c397fe5a9a070c06e0cce20a767718550 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -4392,13 +4392,13 @@ void tcp_send_probe0(struct sock *sk)
 
 	if (tp->packets_out || tcp_write_queue_empty(sk)) {
 		/* Cancel probe timer, if it is not required. */
-		icsk->icsk_probes_out = 0;
+		WRITE_ONCE(icsk->icsk_probes_out, 0);
 		icsk->icsk_backoff = 0;
 		icsk->icsk_probes_tstamp = 0;
 		return;
 	}
 
-	icsk->icsk_probes_out++;
+	WRITE_ONCE(icsk->icsk_probes_out, icsk->icsk_probes_out + 1);
 	if (err <= 0) {
 		if (icsk->icsk_backoff < READ_ONCE(net->ipv4.sysctl_tcp_retries2))
 			icsk->icsk_backoff++;
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 8b11ab4cc9524f0e0066f474044a4c585115ec29..2dd73a4e8e517ab46d69ab655a6b1a9cbcaf7c1e 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -392,7 +392,7 @@ static void tcp_probe_timer(struct sock *sk)
 	int max_probes;
 
 	if (tp->packets_out || !skb) {
-		icsk->icsk_probes_out = 0;
+		WRITE_ONCE(icsk->icsk_probes_out, 0);
 		icsk->icsk_probes_tstamp = 0;
 		return;
 	}
@@ -839,7 +839,7 @@ static void tcp_keepalive_timer(struct timer_list *t)
 			goto out;
 		}
 		if (tcp_write_wakeup(sk, LINUX_MIB_TCPKEEPALIVE) <= 0) {
-			icsk->icsk_probes_out++;
+			WRITE_ONCE(icsk->icsk_probes_out, icsk->icsk_probes_out + 1);
 			elapsed = keepalive_intvl_when(tp);
 		} else {
 			/* If keepalive was lost due to local congestion,
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 7b177054452b983d3f49a06869950565c03c13ee..5620d9e50e195894fb0289d5bb8963579e14429e 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -2232,7 +2232,7 @@ static void get_tcp6_sock(struct seq_file *seq, struct sock *sp, int i)
 		   jiffies_delta_to_clock_t(timer_expires - jiffies),
 		   READ_ONCE(icsk->icsk_retransmits),
 		   from_kuid_munged(seq_user_ns(seq), sk_uid(sp)),
-		   icsk->icsk_probes_out,
+		   READ_ONCE(icsk->icsk_probes_out),
 		   sock_i_ino(sp),
 		   refcount_read(&sp->sk_refcnt), sp,
 		   jiffies_to_clock_t(icsk->icsk_rto),
-- 
2.51.0.rc2.233.g662b1ed5c5-goog

Re: [PATCH net-next 2/2] tcp: annotate data-races around icsk->icsk_probes_out

[Neal Cardwell]

On Fri, Aug 22, 2025 at 5:17 AM Eric Dumazet <edumazet@google.com> wrote:
>
> icsk->icsk_probes_out is read locklessly from inet_sk_diag_fill(),
> get_tcp4_sock() and get_tcp6_sock().
>
> Add corresponding READ_ONCE()/WRITE_ONCE() annotations.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

> +       WRITE_ONCE(icsk->icsk_probes_out, icsk->icsk_probes_out + 1);

> +                       WRITE_ONCE(icsk->icsk_probes_out, icsk->icsk_probes_out + 1);

Do we want a READ_ONCE as well for those 2 cases? Like:

WRITE_ONCE(icsk->icsk_probes_out, READ_ONCE (icsk->icsk_probes_out) + 1);

Perhaps it's not strictly necessary, though I see several places in
the code base that use this approach for increments...

neal

Re: [PATCH net-next 2/2] tcp: annotate data-races around icsk->icsk_probes_out

[Eric Dumazet]

On Fri, Aug 22, 2025 at 6:22 AM Neal Cardwell <ncardwell@google.com> wrote:
>
> On Fri, Aug 22, 2025 at 5:17 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > icsk->icsk_probes_out is read locklessly from inet_sk_diag_fill(),
> > get_tcp4_sock() and get_tcp6_sock().
> >
> > Add corresponding READ_ONCE()/WRITE_ONCE() annotations.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> > +       WRITE_ONCE(icsk->icsk_probes_out, icsk->icsk_probes_out + 1);
>
> > +                       WRITE_ONCE(icsk->icsk_probes_out, icsk->icsk_probes_out + 1);
>
> Do we want a READ_ONCE as well for those 2 cases? Like:
>
> WRITE_ONCE(icsk->icsk_probes_out, READ_ONCE (icsk->icsk_probes_out) + 1);
>

READ_ONCE() here is not needed, because we own the socket lock.


> Perhaps it's not strictly necessary, though I see several places in
> the code base that use this approach for increments...

Maybe in contexts where the read can conflict with another parallel write

__inet_hash_connect
...
WRITE_ONCE(table_perturb[index], READ_ONCE(table_perturb[index]) + i + step);

In this case, we do not own a lock on table_perturb[index]

Re: [PATCH net-next 2/2] tcp: annotate data-races around icsk->icsk_probes_out

[Neal Cardwell]

On Fri, Aug 22, 2025 at 5:17 AM Eric Dumazet <edumazet@google.com> wrote:
>
> icsk->icsk_probes_out is read locklessly from inet_sk_diag_fill(),
> get_tcp4_sock() and get_tcp6_sock().
>
> Add corresponding READ_ONCE()/WRITE_ONCE() annotations.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Neal Cardwell <ncardwell@google.com>

Thanks, Eric!

neal

Re: [PATCH net-next 1/2] tcp: annotate data-races around icsk->icsk_retransmits

[Neal Cardwell]

On Fri, Aug 22, 2025 at 5:17 AM Eric Dumazet <edumazet@google.com> wrote:
>
> icsk->icsk_retransmits is read locklessly from inet_sk_diag_fill(),
> tcp_get_timestamping_opt_stats, get_tcp4_sock() and get_tcp6_sock().
>
> Add corresponding READ_ONCE()/WRITE_ONCE() annotations.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Neal Cardwell <ncardwell@google.com>

Thanks, Eric!

neal

Re: [PATCH net-next 0/2] tcp: annotate data-races around icsk_retransmits and icsk_probes_out

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 22 Aug 2025 09:17:24 +0000 you wrote:
> icsk->icsk_retransmits is read locklessly from inet_sk_diag_fill(),
> tcp_get_timestamping_opt_stats, get_tcp4_sock() and get_tcp6_sock().
> 
> icsk->icsk_probes_out is read locklessly from inet_sk_diag_fill(),
> get_tcp4_sock() and get_tcp6_sock().
> 
> Add corresponding READ_ONCE()/WRITE_ONCE() annotations.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] tcp: annotate data-races around icsk->icsk_retransmits
    https://git.kernel.org/netdev/net-next/c/e6f178be3c12
  - [net-next,2/2] tcp: annotate data-races around icsk->icsk_probes_out
    https://git.kernel.org/netdev/net-next/c/9bd999eb35cf

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html