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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 12 Oct 2023 19:14:35 +0200
From: Wenjia Zhang <wenjia@...ux.ibm.com>
To: "D. Wythe" <alibuda@...ux.alibaba.com>, kgraul@...ux.ibm.com,
        jaka@...ux.ibm.com, wintera@...ux.ibm.com
Cc: kuba@...nel.org, davem@...emloft.net, netdev@...r.kernel.org,
        linux-s390@...r.kernel.org, linux-rdma@...r.kernel.org
Subject: Re: [PATCH net 4/5] net/smc: protect connection state transitions in
 listen work



On 11.10.23 09:33, D. Wythe wrote:
> From: "D. Wythe" <alibuda@...ux.alibaba.com>
> 
> Consider the following scenario:
> 
> 				smc_close_passive_work
> smc_listen_out_connected
> 				lock_sock()
> if (state  == SMC_INIT)
> 				if (state  == SMC_INIT)
> 					state = SMC_APPCLOSEWAIT1;
> 	state = SMC_ACTIVE
> 				release_sock()
> 
> This would cause the state machine of the connection to be corrupted.
> Also, this issue can occur in smc_listen_out_err().
> 
> To solve this problem, we can protect the state transitions under
> the lock of sock to avoid collision.
> 
To this fix, I have to repeat the question from Alexandra.
Did the scenario occur in real life? Or is it just kind of potencial 
problem you found during the code review?

If it is the former one, could you please show us the corresponding 
message, e.g. from dmesg? If it is the latter one, I'd like to deal with 
it more carefully. Going from this scenario, I noticed that there could 
also be other similar places where we need to make sure that no race 
happens. Thus, it would make more sense to find a systematic approach.

> Fixes: 3b2dec2603d5 ("net/smc: restructure client and server code in af_smc")
> Signed-off-by: D. Wythe <alibuda@...ux.alibaba.com>
> ---
>   net/smc/af_smc.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 5ad2a9f..3bb8265 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1926,8 +1926,10 @@ static void smc_listen_out_connected(struct smc_sock *new_smc)
>   {
>   	struct sock *newsmcsk = &new_smc->sk;
>   
> +	lock_sock(newsmcsk);
>   	if (newsmcsk->sk_state == SMC_INIT)
>   		newsmcsk->sk_state = SMC_ACTIVE;
> +	release_sock(newsmcsk);
>   
>   	smc_listen_out(new_smc);
>   }
> @@ -1939,9 +1941,12 @@ static void smc_listen_out_err(struct smc_sock *new_smc)
>   	struct net *net = sock_net(newsmcsk);
>   
>   	this_cpu_inc(net->smc.smc_stats->srv_hshake_err_cnt);
> +
> +	lock_sock(newsmcsk);
>   	if (newsmcsk->sk_state == SMC_INIT)
>   		sock_put(&new_smc->sk); /* passive closing */
>   	newsmcsk->sk_state = SMC_CLOSED;
> +	release_sock(newsmcsk);
>   
>   	smc_listen_out(new_smc);
>   }

Powered by blists - more mailing lists