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

[Kuniyuki Iwashima <kuniyu@amazon.com>]

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