netdev - Re: [PATCH net 1/2] rfs: annotate lockless accesses to sk->sk

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230602202028.49984-1-kuniyu@amazon.com>
Date: Fri, 2 Jun 2023 13:20:28 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <edumazet@...gle.com>
CC: <davem@...emloft.net>, <eric.dumazet@...il.com>, <kuba@...nel.org>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>, <kuniyu@...zon.com>
Subject: Re: [PATCH net 1/2] rfs: annotate lockless accesses to sk->sk_rxhash

From: Eric Dumazet <edumazet@...gle.com>
Date: Fri,  2 Jun 2023 16:31:40 +0000
> Add READ_ONCE()/WRITE_ONCE() on accesses to sk->sk_rxhash.
> 
> This also prevents a (smart ?) compiler to remove the condition in:
> 
> if (sk->sk_rxhash != newval)
> 	sk->sk_rxhash = newval;
> 
> We need the condition to avoid dirtying a shared cache line.
> 
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---
>  include/net/sock.h | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index b418425d7230c8cee81df34fcc66d771ea5085e9..bf71855d47feccda716b3cabf259d6055b764a3c 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1152,8 +1152,12 @@ static inline void sock_rps_record_flow(const struct sock *sk)
>  		 * OR	an additional socket flag
>  		 * [1] : sk_state and sk_prot are in the same cache line.
>  		 */
> -		if (sk->sk_state == TCP_ESTABLISHED)
> -			sock_rps_record_flow_hash(sk->sk_rxhash);
> +		if (sk->sk_state == TCP_ESTABLISHED) {
> +			/* This READ_ONCE() is paired with the WRITE_ONCE()
> +			 * from sock_rps_save_rxhash() and sock_rps_reset_rxhash().
> +			 */
> +			sock_rps_record_flow_hash(READ_ONCE(sk->sk_rxhash));
> +			}

nit: unnecessary \t here

other than that

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

Thanks!

>  	}
>  #endif
>  }
> @@ -1162,15 +1166,19 @@ static inline void sock_rps_save_rxhash(struct sock *sk,
>  					const struct sk_buff *skb)
>  {
>  #ifdef CONFIG_RPS
> -	if (unlikely(sk->sk_rxhash != skb->hash))
> -		sk->sk_rxhash = skb->hash;
> +	/* The following WRITE_ONCE() is paired with the READ_ONCE()
> +	 * here, and another one in sock_rps_record_flow().
> +	 */
> +	if (unlikely(READ_ONCE(sk->sk_rxhash) != skb->hash))
> +		WRITE_ONCE(sk->sk_rxhash, skb->hash);
>  #endif
>  }
>  
>  static inline void sock_rps_reset_rxhash(struct sock *sk)
>  {
>  #ifdef CONFIG_RPS
> -	sk->sk_rxhash = 0;
> +	/* Paired with READ_ONCE() in sock_rps_record_flow() */
> +	WRITE_ONCE(sk->sk_rxhash, 0);
>  #endif
>  }
>  
> -- 
> 2.41.0.rc0.172.g3f132b7071-goog