[PATCH net 0/6] tcp_metrics: series of fixes

[PATCH net 0/6] tcp_metrics: series of fixes

[Eric Dumazet]

This series contains a fix for addr_same() and various
data-race annotations.

We still have to address races over tm->tcpm_saddr and
tm->tcpm_daddr later.

Eric Dumazet (6):
  tcp_metrics: fix addr_same() helper
  tcp_metrics: annotate data-races around tm->tcpm_stamp
  tcp_metrics: annotate data-races around tm->tcpm_lock
  tcp_metrics: annotate data-races around tm->tcpm_vals[]
  tcp_metrics: annotate data-races around tm->tcpm_net
  tcp_metrics: fix data-race in tcpm_suck_dst() vs fastopen

 net/ipv4/tcp_metrics.c | 70 ++++++++++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 26 deletions(-)

-- 
2.41.0.640.ga95def55d0-goog

[PATCH net 1/6] tcp_metrics: fix addr_same() helper

[Eric Dumazet]

Because v4 and v6 families use separate inetpeer trees (respectively
net->ipv4.peers and net->ipv6.peers), inetpeer_addr_cmp(a, b) assumes
a & b share the same family.

tcp_metrics use a common hash table, where entries can have different
families.

We must therefore make sure to not call inetpeer_addr_cmp()
if the families do not match.

Fixes: d39d14ffa24c ("net: Add helper function to compare inetpeer addresses")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: David Ahern <dsahern@kernel.org>
---
 net/ipv4/tcp_metrics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 82f4575f9cd90049a5ad4c7329ad1ddc28fc1aa0..c4daf0aa2d4d9695e128b67df571d91d647a254d 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -78,7 +78,7 @@ static void tcp_metric_set(struct tcp_metrics_block *tm,
 static bool addr_same(const struct inetpeer_addr *a,
 		      const struct inetpeer_addr *b)
 {
-	return inetpeer_addr_cmp(a, b) == 0;
+	return (a->family == b->family) && !inetpeer_addr_cmp(a, b);
 }
 
 struct tcpm_hash_bucket {
-- 
2.41.0.640.ga95def55d0-goog

[PATCH net 2/6] tcp_metrics: annotate data-races around tm->tcpm_stamp

[Eric Dumazet]

tm->tcpm_stamp can be read or written locklessly.

Add needed READ_ONCE()/WRITE_ONCE() to document this.

Also constify tcpm_check_stamp() dst argument.

Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_metrics.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index c4daf0aa2d4d9695e128b67df571d91d647a254d..83861658879638149d2746290a285a4f75fc3117 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -97,7 +97,7 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm,
 	u32 msval;
 	u32 val;
 
-	tm->tcpm_stamp = jiffies;
+	WRITE_ONCE(tm->tcpm_stamp, jiffies);
 
 	val = 0;
 	if (dst_metric_locked(dst, RTAX_RTT))
@@ -131,9 +131,15 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm,
 
 #define TCP_METRICS_TIMEOUT		(60 * 60 * HZ)
 
-static void tcpm_check_stamp(struct tcp_metrics_block *tm, struct dst_entry *dst)
+static void tcpm_check_stamp(struct tcp_metrics_block *tm,
+			     const struct dst_entry *dst)
 {
-	if (tm && unlikely(time_after(jiffies, tm->tcpm_stamp + TCP_METRICS_TIMEOUT)))
+	unsigned long limit;
+
+	if (!tm)
+		return;
+	limit = READ_ONCE(tm->tcpm_stamp) + TCP_METRICS_TIMEOUT;
+	if (unlikely(time_after(jiffies, limit)))
 		tcpm_suck_dst(tm, dst, false);
 }
 
@@ -174,7 +180,8 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
 		oldest = deref_locked(tcp_metrics_hash[hash].chain);
 		for (tm = deref_locked(oldest->tcpm_next); tm;
 		     tm = deref_locked(tm->tcpm_next)) {
-			if (time_before(tm->tcpm_stamp, oldest->tcpm_stamp))
+			if (time_before(READ_ONCE(tm->tcpm_stamp),
+					READ_ONCE(oldest->tcpm_stamp)))
 				oldest = tm;
 		}
 		tm = oldest;
@@ -434,7 +441,7 @@ void tcp_update_metrics(struct sock *sk)
 					       tp->reordering);
 		}
 	}
-	tm->tcpm_stamp = jiffies;
+	WRITE_ONCE(tm->tcpm_stamp, jiffies);
 out_unlock:
 	rcu_read_unlock();
 }
@@ -647,7 +654,7 @@ static int tcp_metrics_fill_info(struct sk_buff *msg,
 	}
 
 	if (nla_put_msecs(msg, TCP_METRICS_ATTR_AGE,
-			  jiffies - tm->tcpm_stamp,
+			  jiffies - READ_ONCE(tm->tcpm_stamp),
 			  TCP_METRICS_ATTR_PAD) < 0)
 		goto nla_put_failure;
 
-- 
2.41.0.640.ga95def55d0-goog

[PATCH net 3/6] tcp_metrics: annotate data-races around tm->tcpm_lock

[Eric Dumazet]

tm->tcpm_lock can be read or written locklessly.

Add needed READ_ONCE()/WRITE_ONCE() to document this.

Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_metrics.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 83861658879638149d2746290a285a4f75fc3117..131fa300496914f78c682182f0db480ceb71b6a0 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -59,7 +59,8 @@ static inline struct net *tm_net(struct tcp_metrics_block *tm)
 static bool tcp_metric_locked(struct tcp_metrics_block *tm,
 			      enum tcp_metric_index idx)
 {
-	return tm->tcpm_lock & (1 << idx);
+	/* Paired with WRITE_ONCE() in tcpm_suck_dst() */
+	return READ_ONCE(tm->tcpm_lock) & (1 << idx);
 }
 
 static u32 tcp_metric_get(struct tcp_metrics_block *tm,
@@ -110,7 +111,8 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm,
 		val |= 1 << TCP_METRIC_CWND;
 	if (dst_metric_locked(dst, RTAX_REORDERING))
 		val |= 1 << TCP_METRIC_REORDERING;
-	tm->tcpm_lock = val;
+	/* Paired with READ_ONCE() in tcp_metric_locked() */
+	WRITE_ONCE(tm->tcpm_lock, val);
 
 	msval = dst_metric_raw(dst, RTAX_RTT);
 	tm->tcpm_vals[TCP_METRIC_RTT] = msval * USEC_PER_MSEC;
-- 
2.41.0.640.ga95def55d0-goog

[PATCH net 4/6] tcp_metrics: annotate data-races around tm->tcpm_vals[]

[Eric Dumazet]

tm->tcpm_vals[] values can be read or written locklessly.

Add needed READ_ONCE()/WRITE_ONCE() to document this,
and force use of tcp_metric_get() and tcp_metric_set()

Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_metrics.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 131fa300496914f78c682182f0db480ceb71b6a0..fd4ab7a51cef210005146dfbc3235a2db717a44f 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -63,17 +63,19 @@ static bool tcp_metric_locked(struct tcp_metrics_block *tm,
 	return READ_ONCE(tm->tcpm_lock) & (1 << idx);
 }
 
-static u32 tcp_metric_get(struct tcp_metrics_block *tm,
+static u32 tcp_metric_get(const struct tcp_metrics_block *tm,
 			  enum tcp_metric_index idx)
 {
-	return tm->tcpm_vals[idx];
+	/* Paired with WRITE_ONCE() in tcp_metric_set() */
+	return READ_ONCE(tm->tcpm_vals[idx]);
 }
 
 static void tcp_metric_set(struct tcp_metrics_block *tm,
 			   enum tcp_metric_index idx,
 			   u32 val)
 {
-	tm->tcpm_vals[idx] = val;
+	/* Paired with READ_ONCE() in tcp_metric_get() */
+	WRITE_ONCE(tm->tcpm_vals[idx], val);
 }
 
 static bool addr_same(const struct inetpeer_addr *a,
@@ -115,13 +117,16 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm,
 	WRITE_ONCE(tm->tcpm_lock, val);
 
 	msval = dst_metric_raw(dst, RTAX_RTT);
-	tm->tcpm_vals[TCP_METRIC_RTT] = msval * USEC_PER_MSEC;
+	tcp_metric_set(tm, TCP_METRIC_RTT, msval * USEC_PER_MSEC);
 
 	msval = dst_metric_raw(dst, RTAX_RTTVAR);
-	tm->tcpm_vals[TCP_METRIC_RTTVAR] = msval * USEC_PER_MSEC;
-	tm->tcpm_vals[TCP_METRIC_SSTHRESH] = dst_metric_raw(dst, RTAX_SSTHRESH);
-	tm->tcpm_vals[TCP_METRIC_CWND] = dst_metric_raw(dst, RTAX_CWND);
-	tm->tcpm_vals[TCP_METRIC_REORDERING] = dst_metric_raw(dst, RTAX_REORDERING);
+	tcp_metric_set(tm, TCP_METRIC_RTTVAR, msval * USEC_PER_MSEC);
+	tcp_metric_set(tm, TCP_METRIC_SSTHRESH,
+		       dst_metric_raw(dst, RTAX_SSTHRESH));
+	tcp_metric_set(tm, TCP_METRIC_CWND,
+		       dst_metric_raw(dst, RTAX_CWND));
+	tcp_metric_set(tm, TCP_METRIC_REORDERING,
+		       dst_metric_raw(dst, RTAX_REORDERING));
 	if (fastopen_clear) {
 		tm->tcpm_fastopen.mss = 0;
 		tm->tcpm_fastopen.syn_loss = 0;
@@ -667,7 +672,7 @@ static int tcp_metrics_fill_info(struct sk_buff *msg,
 		if (!nest)
 			goto nla_put_failure;
 		for (i = 0; i < TCP_METRIC_MAX_KERNEL + 1; i++) {
-			u32 val = tm->tcpm_vals[i];
+			u32 val = tcp_metric_get(tm, i);
 
 			if (!val)
 				continue;
-- 
2.41.0.640.ga95def55d0-goog

[PATCH net 5/6] tcp_metrics: annotate data-races around tm->tcpm_net

[Eric Dumazet]

tm->tcpm_net can be read or written locklessly.

Instead of changing write_pnet() and read_pnet() and potentially
hurt performance, add the needed READ_ONCE()/WRITE_ONCE()
in tm_net() and tcpm_new().

Fixes: 849e8a0ca8d5 ("tcp_metrics: Add a field tcpm_net and verify it matches on lookup")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_metrics.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index fd4ab7a51cef210005146dfbc3235a2db717a44f..4fd274836a48f73d0b1206adfa14c17c3b28bc30 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -40,7 +40,7 @@ struct tcp_fastopen_metrics {
 
 struct tcp_metrics_block {
 	struct tcp_metrics_block __rcu	*tcpm_next;
-	possible_net_t			tcpm_net;
+	struct net			*tcpm_net;
 	struct inetpeer_addr		tcpm_saddr;
 	struct inetpeer_addr		tcpm_daddr;
 	unsigned long			tcpm_stamp;
@@ -51,9 +51,10 @@ struct tcp_metrics_block {
 	struct rcu_head			rcu_head;
 };
 
-static inline struct net *tm_net(struct tcp_metrics_block *tm)
+static inline struct net *tm_net(const struct tcp_metrics_block *tm)
 {
-	return read_pnet(&tm->tcpm_net);
+	/* Paired with the WRITE_ONCE() in tcpm_new() */
+	return READ_ONCE(tm->tcpm_net);
 }
 
 static bool tcp_metric_locked(struct tcp_metrics_block *tm,
@@ -197,7 +198,9 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
 		if (!tm)
 			goto out_unlock;
 	}
-	write_pnet(&tm->tcpm_net, net);
+	/* Paired with the READ_ONCE() in tm_net() */
+	WRITE_ONCE(tm->tcpm_net, net);
+
 	tm->tcpm_saddr = *saddr;
 	tm->tcpm_daddr = *daddr;
 
-- 
2.41.0.640.ga95def55d0-goog

[PATCH net 6/6] tcp_metrics: fix data-race in tcpm_suck_dst() vs fastopen

[Eric Dumazet]

Whenever tcpm_new() reclaims an old entry, tcpm_suck_dst()
would overwrite data that could be read from tcp_fastopen_cache_get()
or tcp_metrics_fill_info().

We need to acquire fastopen_seqlock to maintain consistency.

For newly allocated objects, tcpm_new() can switch to kzalloc()
to avoid an extra fastopen_seqlock acquisition.

Fixes: 1fe4c481ba63 ("net-tcp: Fast Open client - cookie cache")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
 net/ipv4/tcp_metrics.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 4fd274836a48f73d0b1206adfa14c17c3b28bc30..99ac5efe244d3c654deaa8f8c0fffeeb5d5597b1 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -93,6 +93,7 @@ static struct tcpm_hash_bucket	*tcp_metrics_hash __read_mostly;
 static unsigned int		tcp_metrics_hash_log __read_mostly;
 
 static DEFINE_SPINLOCK(tcp_metrics_lock);
+static DEFINE_SEQLOCK(fastopen_seqlock);
 
 static void tcpm_suck_dst(struct tcp_metrics_block *tm,
 			  const struct dst_entry *dst,
@@ -129,11 +130,13 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm,
 	tcp_metric_set(tm, TCP_METRIC_REORDERING,
 		       dst_metric_raw(dst, RTAX_REORDERING));
 	if (fastopen_clear) {
+		write_seqlock(&fastopen_seqlock);
 		tm->tcpm_fastopen.mss = 0;
 		tm->tcpm_fastopen.syn_loss = 0;
 		tm->tcpm_fastopen.try_exp = 0;
 		tm->tcpm_fastopen.cookie.exp = false;
 		tm->tcpm_fastopen.cookie.len = 0;
+		write_sequnlock(&fastopen_seqlock);
 	}
 }
 
@@ -194,7 +197,7 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
 		}
 		tm = oldest;
 	} else {
-		tm = kmalloc(sizeof(*tm), GFP_ATOMIC);
+		tm = kzalloc(sizeof(*tm), GFP_ATOMIC);
 		if (!tm)
 			goto out_unlock;
 	}
@@ -204,7 +207,7 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
 	tm->tcpm_saddr = *saddr;
 	tm->tcpm_daddr = *daddr;
 
-	tcpm_suck_dst(tm, dst, true);
+	tcpm_suck_dst(tm, dst, reclaim);
 
 	if (likely(!reclaim)) {
 		tm->tcpm_next = tcp_metrics_hash[hash].chain;
@@ -556,8 +559,6 @@ bool tcp_peer_is_proven(struct request_sock *req, struct dst_entry *dst)
 	return ret;
 }
 
-static DEFINE_SEQLOCK(fastopen_seqlock);
-
 void tcp_fastopen_cache_get(struct sock *sk, u16 *mss,
 			    struct tcp_fastopen_cookie *cookie)
 {
-- 
2.41.0.640.ga95def55d0-goog

Re: [PATCH net 1/6] tcp_metrics: fix addr_same() helper

[David Ahern]

On 8/2/23 7:14 AM, Eric Dumazet wrote:
> Because v4 and v6 families use separate inetpeer trees (respectively
> net->ipv4.peers and net->ipv6.peers), inetpeer_addr_cmp(a, b) assumes
> a & b share the same family.
> 
> tcp_metrics use a common hash table, where entries can have different
> families.
> 
> We must therefore make sure to not call inetpeer_addr_cmp()
> if the families do not match.
> 
> Fixes: d39d14ffa24c ("net: Add helper function to compare inetpeer addresses")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Ahern <dsahern@kernel.org>
> ---
>  net/ipv4/tcp_metrics.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 2/6] tcp_metrics: annotate data-races around tm->tcpm_stamp

[David Ahern]

On 8/2/23 7:14 AM, Eric Dumazet wrote:
> tm->tcpm_stamp can be read or written locklessly.
> 
> Add needed READ_ONCE()/WRITE_ONCE() to document this.
> 
> Also constify tcpm_check_stamp() dst argument.
> 
> Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/tcp_metrics.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 3/6] tcp_metrics: annotate data-races around tm->tcpm_lock

[David Ahern]

On 8/2/23 7:14 AM, Eric Dumazet wrote:
> tm->tcpm_lock can be read or written locklessly.
> 
> Add needed READ_ONCE()/WRITE_ONCE() to document this.
> 
> Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/tcp_metrics.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 4/6] tcp_metrics: annotate data-races around tm->tcpm_vals[]

[David Ahern]

On 8/2/23 7:14 AM, Eric Dumazet wrote:
> tm->tcpm_vals[] values can be read or written locklessly.
> 
> Add needed READ_ONCE()/WRITE_ONCE() to document this,
> and force use of tcp_metric_get() and tcp_metric_set()
> 
> Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/tcp_metrics.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 5/6] tcp_metrics: annotate data-races around tm->tcpm_net

[David Ahern]

On 8/2/23 7:14 AM, Eric Dumazet wrote:
> tm->tcpm_net can be read or written locklessly.
> 
> Instead of changing write_pnet() and read_pnet() and potentially
> hurt performance, add the needed READ_ONCE()/WRITE_ONCE()
> in tm_net() and tcpm_new().
> 
> Fixes: 849e8a0ca8d5 ("tcp_metrics: Add a field tcpm_net and verify it matches on lookup")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/tcp_metrics.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 1/6] tcp_metrics: fix addr_same() helper

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Wed,  2 Aug 2023 13:14:55 +0000
> Because v4 and v6 families use separate inetpeer trees (respectively
> net->ipv4.peers and net->ipv6.peers), inetpeer_addr_cmp(a, b) assumes
> a & b share the same family.
> 
> tcp_metrics use a common hash table, where entries can have different
> families.
> 
> We must therefore make sure to not call inetpeer_addr_cmp()
> if the families do not match.
> 
> Fixes: d39d14ffa24c ("net: Add helper function to compare inetpeer addresses")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Ahern <dsahern@kernel.org>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>


> ---
>  net/ipv4/tcp_metrics.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> index 82f4575f9cd90049a5ad4c7329ad1ddc28fc1aa0..c4daf0aa2d4d9695e128b67df571d91d647a254d 100644
> --- a/net/ipv4/tcp_metrics.c
> +++ b/net/ipv4/tcp_metrics.c
> @@ -78,7 +78,7 @@ static void tcp_metric_set(struct tcp_metrics_block *tm,
>  static bool addr_same(const struct inetpeer_addr *a,
>  		      const struct inetpeer_addr *b)
>  {
> -	return inetpeer_addr_cmp(a, b) == 0;
> +	return (a->family == b->family) && !inetpeer_addr_cmp(a, b);
>  }
>  
>  struct tcpm_hash_bucket {
> -- 
> 2.41.0.640.ga95def55d0-goog

Re: [PATCH net 2/6] tcp_metrics: annotate data-races around tm->tcpm_stamp

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Wed,  2 Aug 2023 13:14:56 +0000
> tm->tcpm_stamp can be read or written locklessly.
> 
> Add needed READ_ONCE()/WRITE_ONCE() to document this.
> 
> Also constify tcpm_check_stamp() dst argument.
> 
> Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>


> ---
>  net/ipv4/tcp_metrics.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> index c4daf0aa2d4d9695e128b67df571d91d647a254d..83861658879638149d2746290a285a4f75fc3117 100644
> --- a/net/ipv4/tcp_metrics.c
> +++ b/net/ipv4/tcp_metrics.c
> @@ -97,7 +97,7 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm,
>  	u32 msval;
>  	u32 val;
>  
> -	tm->tcpm_stamp = jiffies;
> +	WRITE_ONCE(tm->tcpm_stamp, jiffies);
>  
>  	val = 0;
>  	if (dst_metric_locked(dst, RTAX_RTT))
> @@ -131,9 +131,15 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm,
>  
>  #define TCP_METRICS_TIMEOUT		(60 * 60 * HZ)
>  
> -static void tcpm_check_stamp(struct tcp_metrics_block *tm, struct dst_entry *dst)
> +static void tcpm_check_stamp(struct tcp_metrics_block *tm,
> +			     const struct dst_entry *dst)
>  {
> -	if (tm && unlikely(time_after(jiffies, tm->tcpm_stamp + TCP_METRICS_TIMEOUT)))
> +	unsigned long limit;
> +
> +	if (!tm)
> +		return;
> +	limit = READ_ONCE(tm->tcpm_stamp) + TCP_METRICS_TIMEOUT;
> +	if (unlikely(time_after(jiffies, limit)))
>  		tcpm_suck_dst(tm, dst, false);
>  }
>  
> @@ -174,7 +180,8 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
>  		oldest = deref_locked(tcp_metrics_hash[hash].chain);
>  		for (tm = deref_locked(oldest->tcpm_next); tm;
>  		     tm = deref_locked(tm->tcpm_next)) {
> -			if (time_before(tm->tcpm_stamp, oldest->tcpm_stamp))
> +			if (time_before(READ_ONCE(tm->tcpm_stamp),
> +					READ_ONCE(oldest->tcpm_stamp)))
>  				oldest = tm;
>  		}
>  		tm = oldest;
> @@ -434,7 +441,7 @@ void tcp_update_metrics(struct sock *sk)
>  					       tp->reordering);
>  		}
>  	}
> -	tm->tcpm_stamp = jiffies;
> +	WRITE_ONCE(tm->tcpm_stamp, jiffies);
>  out_unlock:
>  	rcu_read_unlock();
>  }
> @@ -647,7 +654,7 @@ static int tcp_metrics_fill_info(struct sk_buff *msg,
>  	}
>  
>  	if (nla_put_msecs(msg, TCP_METRICS_ATTR_AGE,
> -			  jiffies - tm->tcpm_stamp,
> +			  jiffies - READ_ONCE(tm->tcpm_stamp),
>  			  TCP_METRICS_ATTR_PAD) < 0)
>  		goto nla_put_failure;
>  
> -- 
> 2.41.0.640.ga95def55d0-goog

Re: [PATCH net 3/6] tcp_metrics: annotate data-races around tm->tcpm_lock

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Wed,  2 Aug 2023 13:14:57 +0000
> tm->tcpm_lock can be read or written locklessly.
> 
> Add needed READ_ONCE()/WRITE_ONCE() to document this.
> 
> Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>


> ---
>  net/ipv4/tcp_metrics.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> index 83861658879638149d2746290a285a4f75fc3117..131fa300496914f78c682182f0db480ceb71b6a0 100644
> --- a/net/ipv4/tcp_metrics.c
> +++ b/net/ipv4/tcp_metrics.c
> @@ -59,7 +59,8 @@ static inline struct net *tm_net(struct tcp_metrics_block *tm)
>  static bool tcp_metric_locked(struct tcp_metrics_block *tm,
>  			      enum tcp_metric_index idx)
>  {
> -	return tm->tcpm_lock & (1 << idx);
> +	/* Paired with WRITE_ONCE() in tcpm_suck_dst() */
> +	return READ_ONCE(tm->tcpm_lock) & (1 << idx);
>  }
>  
>  static u32 tcp_metric_get(struct tcp_metrics_block *tm,
> @@ -110,7 +111,8 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm,
>  		val |= 1 << TCP_METRIC_CWND;
>  	if (dst_metric_locked(dst, RTAX_REORDERING))
>  		val |= 1 << TCP_METRIC_REORDERING;
> -	tm->tcpm_lock = val;
> +	/* Paired with READ_ONCE() in tcp_metric_locked() */
> +	WRITE_ONCE(tm->tcpm_lock, val);
>  
>  	msval = dst_metric_raw(dst, RTAX_RTT);
>  	tm->tcpm_vals[TCP_METRIC_RTT] = msval * USEC_PER_MSEC;
> -- 
> 2.41.0.640.ga95def55d0-goog

Re: [PATCH net 4/6] tcp_metrics: annotate data-races around tm->tcpm_vals[]

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Wed,  2 Aug 2023 13:14:58 +0000
> tm->tcpm_vals[] values can be read or written locklessly.
> 
> Add needed READ_ONCE()/WRITE_ONCE() to document this,
> and force use of tcp_metric_get() and tcp_metric_set()
> 
> Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>


> ---
>  net/ipv4/tcp_metrics.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> index 131fa300496914f78c682182f0db480ceb71b6a0..fd4ab7a51cef210005146dfbc3235a2db717a44f 100644
> --- a/net/ipv4/tcp_metrics.c
> +++ b/net/ipv4/tcp_metrics.c
> @@ -63,17 +63,19 @@ static bool tcp_metric_locked(struct tcp_metrics_block *tm,
>  	return READ_ONCE(tm->tcpm_lock) & (1 << idx);
>  }
>  
> -static u32 tcp_metric_get(struct tcp_metrics_block *tm,
> +static u32 tcp_metric_get(const struct tcp_metrics_block *tm,
>  			  enum tcp_metric_index idx)
>  {
> -	return tm->tcpm_vals[idx];
> +	/* Paired with WRITE_ONCE() in tcp_metric_set() */
> +	return READ_ONCE(tm->tcpm_vals[idx]);
>  }
>  
>  static void tcp_metric_set(struct tcp_metrics_block *tm,
>  			   enum tcp_metric_index idx,
>  			   u32 val)
>  {
> -	tm->tcpm_vals[idx] = val;
> +	/* Paired with READ_ONCE() in tcp_metric_get() */
> +	WRITE_ONCE(tm->tcpm_vals[idx], val);
>  }
>  
>  static bool addr_same(const struct inetpeer_addr *a,
> @@ -115,13 +117,16 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm,
>  	WRITE_ONCE(tm->tcpm_lock, val);
>  
>  	msval = dst_metric_raw(dst, RTAX_RTT);
> -	tm->tcpm_vals[TCP_METRIC_RTT] = msval * USEC_PER_MSEC;
> +	tcp_metric_set(tm, TCP_METRIC_RTT, msval * USEC_PER_MSEC);
>  
>  	msval = dst_metric_raw(dst, RTAX_RTTVAR);
> -	tm->tcpm_vals[TCP_METRIC_RTTVAR] = msval * USEC_PER_MSEC;
> -	tm->tcpm_vals[TCP_METRIC_SSTHRESH] = dst_metric_raw(dst, RTAX_SSTHRESH);
> -	tm->tcpm_vals[TCP_METRIC_CWND] = dst_metric_raw(dst, RTAX_CWND);
> -	tm->tcpm_vals[TCP_METRIC_REORDERING] = dst_metric_raw(dst, RTAX_REORDERING);
> +	tcp_metric_set(tm, TCP_METRIC_RTTVAR, msval * USEC_PER_MSEC);
> +	tcp_metric_set(tm, TCP_METRIC_SSTHRESH,
> +		       dst_metric_raw(dst, RTAX_SSTHRESH));
> +	tcp_metric_set(tm, TCP_METRIC_CWND,
> +		       dst_metric_raw(dst, RTAX_CWND));
> +	tcp_metric_set(tm, TCP_METRIC_REORDERING,
> +		       dst_metric_raw(dst, RTAX_REORDERING));
>  	if (fastopen_clear) {
>  		tm->tcpm_fastopen.mss = 0;
>  		tm->tcpm_fastopen.syn_loss = 0;
> @@ -667,7 +672,7 @@ static int tcp_metrics_fill_info(struct sk_buff *msg,
>  		if (!nest)
>  			goto nla_put_failure;
>  		for (i = 0; i < TCP_METRIC_MAX_KERNEL + 1; i++) {
> -			u32 val = tm->tcpm_vals[i];
> +			u32 val = tcp_metric_get(tm, i);
>  
>  			if (!val)
>  				continue;
> -- 
> 2.41.0.640.ga95def55d0-goog

Re: [PATCH net 5/6] tcp_metrics: annotate data-races around tm->tcpm_net

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Wed,  2 Aug 2023 13:14:59 +0000
> tm->tcpm_net can be read or written locklessly.
> 
> Instead of changing write_pnet() and read_pnet() and potentially
> hurt performance, add the needed READ_ONCE()/WRITE_ONCE()
> in tm_net() and tcpm_new().
> 
> Fixes: 849e8a0ca8d5 ("tcp_metrics: Add a field tcpm_net and verify it matches on lookup")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>


> ---
>  net/ipv4/tcp_metrics.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> index fd4ab7a51cef210005146dfbc3235a2db717a44f..4fd274836a48f73d0b1206adfa14c17c3b28bc30 100644
> --- a/net/ipv4/tcp_metrics.c
> +++ b/net/ipv4/tcp_metrics.c
> @@ -40,7 +40,7 @@ struct tcp_fastopen_metrics {
>  
>  struct tcp_metrics_block {
>  	struct tcp_metrics_block __rcu	*tcpm_next;
> -	possible_net_t			tcpm_net;
> +	struct net			*tcpm_net;
>  	struct inetpeer_addr		tcpm_saddr;
>  	struct inetpeer_addr		tcpm_daddr;
>  	unsigned long			tcpm_stamp;
> @@ -51,9 +51,10 @@ struct tcp_metrics_block {
>  	struct rcu_head			rcu_head;
>  };
>  
> -static inline struct net *tm_net(struct tcp_metrics_block *tm)
> +static inline struct net *tm_net(const struct tcp_metrics_block *tm)
>  {
> -	return read_pnet(&tm->tcpm_net);
> +	/* Paired with the WRITE_ONCE() in tcpm_new() */
> +	return READ_ONCE(tm->tcpm_net);
>  }
>  
>  static bool tcp_metric_locked(struct tcp_metrics_block *tm,
> @@ -197,7 +198,9 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
>  		if (!tm)
>  			goto out_unlock;
>  	}
> -	write_pnet(&tm->tcpm_net, net);
> +	/* Paired with the READ_ONCE() in tm_net() */
> +	WRITE_ONCE(tm->tcpm_net, net);
> +
>  	tm->tcpm_saddr = *saddr;
>  	tm->tcpm_daddr = *daddr;
>  
> -- 
> 2.41.0.640.ga95def55d0-goog

Re: [PATCH net 6/6] tcp_metrics: fix data-race in tcpm_suck_dst() vs fastopen

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Wed,  2 Aug 2023 13:15:00 +0000
> Whenever tcpm_new() reclaims an old entry, tcpm_suck_dst()
> would overwrite data that could be read from tcp_fastopen_cache_get()
> or tcp_metrics_fill_info().
> 
> We need to acquire fastopen_seqlock to maintain consistency.
> 
> For newly allocated objects, tcpm_new() can switch to kzalloc()
> to avoid an extra fastopen_seqlock acquisition.
> 
> Fixes: 1fe4c481ba63 ("net-tcp: Fast Open client - cookie cache")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>


> ---
>  net/ipv4/tcp_metrics.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> index 4fd274836a48f73d0b1206adfa14c17c3b28bc30..99ac5efe244d3c654deaa8f8c0fffeeb5d5597b1 100644
> --- a/net/ipv4/tcp_metrics.c
> +++ b/net/ipv4/tcp_metrics.c
> @@ -93,6 +93,7 @@ static struct tcpm_hash_bucket	*tcp_metrics_hash __read_mostly;
>  static unsigned int		tcp_metrics_hash_log __read_mostly;
>  
>  static DEFINE_SPINLOCK(tcp_metrics_lock);
> +static DEFINE_SEQLOCK(fastopen_seqlock);
>  
>  static void tcpm_suck_dst(struct tcp_metrics_block *tm,
>  			  const struct dst_entry *dst,
> @@ -129,11 +130,13 @@ static void tcpm_suck_dst(struct tcp_metrics_block *tm,
>  	tcp_metric_set(tm, TCP_METRIC_REORDERING,
>  		       dst_metric_raw(dst, RTAX_REORDERING));
>  	if (fastopen_clear) {
> +		write_seqlock(&fastopen_seqlock);
>  		tm->tcpm_fastopen.mss = 0;
>  		tm->tcpm_fastopen.syn_loss = 0;
>  		tm->tcpm_fastopen.try_exp = 0;
>  		tm->tcpm_fastopen.cookie.exp = false;
>  		tm->tcpm_fastopen.cookie.len = 0;
> +		write_sequnlock(&fastopen_seqlock);
>  	}
>  }
>  
> @@ -194,7 +197,7 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
>  		}
>  		tm = oldest;
>  	} else {
> -		tm = kmalloc(sizeof(*tm), GFP_ATOMIC);
> +		tm = kzalloc(sizeof(*tm), GFP_ATOMIC);
>  		if (!tm)
>  			goto out_unlock;
>  	}
> @@ -204,7 +207,7 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
>  	tm->tcpm_saddr = *saddr;
>  	tm->tcpm_daddr = *daddr;
>  
> -	tcpm_suck_dst(tm, dst, true);
> +	tcpm_suck_dst(tm, dst, reclaim);
>  
>  	if (likely(!reclaim)) {
>  		tm->tcpm_next = tcp_metrics_hash[hash].chain;
> @@ -556,8 +559,6 @@ bool tcp_peer_is_proven(struct request_sock *req, struct dst_entry *dst)
>  	return ret;
>  }
>  
> -static DEFINE_SEQLOCK(fastopen_seqlock);
> -
>  void tcp_fastopen_cache_get(struct sock *sk, u16 *mss,
>  			    struct tcp_fastopen_cookie *cookie)
>  {
> -- 
> 2.41.0.640.ga95def55d0-goog

Re: [PATCH net 0/6] tcp_metrics: series of fixes

[patchwork-bot+netdevbpf]

Hello:

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

On Wed,  2 Aug 2023 13:14:54 +0000 you wrote:
> This series contains a fix for addr_same() and various
> data-race annotations.
> 
> We still have to address races over tm->tcpm_saddr and
> tm->tcpm_daddr later.
> 
> Eric Dumazet (6):
>   tcp_metrics: fix addr_same() helper
>   tcp_metrics: annotate data-races around tm->tcpm_stamp
>   tcp_metrics: annotate data-races around tm->tcpm_lock
>   tcp_metrics: annotate data-races around tm->tcpm_vals[]
>   tcp_metrics: annotate data-races around tm->tcpm_net
>   tcp_metrics: fix data-race in tcpm_suck_dst() vs fastopen
> 
> [...]

Here is the summary with links:
  - [net,1/6] tcp_metrics: fix addr_same() helper
    https://git.kernel.org/netdev/net/c/e6638094d7af
  - [net,2/6] tcp_metrics: annotate data-races around tm->tcpm_stamp
    https://git.kernel.org/netdev/net/c/949ad62a5d53
  - [net,3/6] tcp_metrics: annotate data-races around tm->tcpm_lock
    https://git.kernel.org/netdev/net/c/285ce119a3c6
  - [net,4/6] tcp_metrics: annotate data-races around tm->tcpm_vals[]
    https://git.kernel.org/netdev/net/c/8c4d04f6b443
  - [net,5/6] tcp_metrics: annotate data-races around tm->tcpm_net
    https://git.kernel.org/netdev/net/c/d5d986ce42c7
  - [net,6/6] tcp_metrics: fix data-race in tcpm_suck_dst() vs fastopen
    https://git.kernel.org/netdev/net/c/ddf251fa2bc1

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