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

[Kuniyuki Iwashima <kuniyu@amazon.com>]

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