Re: [PATCH net] net/smc: hash socket only after full initialisation in smc_sk_init()

On 14/08/26 1:13 pm, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Data Race and Unsafe Publication leading to TOCTOU NULL pointer dereference in smc_diag_msg_common_fill
> --
> 
> --- Patch [1]: [PATCH net] net/smc: hash socket only after full initialisation in smc_sk_init() ---
> commit 86b5da4b01196842fbdd925890a17242339aa010
> Author: Mahanta Jambigi <[email protected]>
> 
> net/smc: hash socket only after full initialisation in smc_sk_init()
> 
> This commit delays hashing the SMC socket to the end of smc_sk_init() to
> prevent concurrent hash walkers from observing an uninitialized socket.
> 
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 00403175b7406..5e7560417ec70 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -409,13 +409,13 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
>>  				      "sk_lock-AF_SMC", &smc_key);
>>  	spin_lock_init(&smc->accept_q_lock);
>>  	spin_lock_init(&smc->conn.send_lock);
>> -	sk->sk_prot->hash(sk);
>>  	mutex_init(&smc->clcsock_release_lock);
>>  	smc_init_saved_callbacks(smc);
>>  	smc->limit_smc_hs = net->smc.limit_smc_hs;
>>  	smc->use_fallback = false; /* assume rdma capability first */
>>  	smc->fallback_rsn = 0;
>>  	smc_close_init(smc);
>> +	sk->sk_prot->hash(sk);
> 
> [Severity: High]
> This is a pre-existing issue, but does moving the hash here fully protect
> hash walkers from seeing uninitialized socket state?
> 
> Even with this change, the socket is still hashed inside smc_sk_init(),
> which is called by smc_sock_alloc() before caller-level initialization
> completes. For instance, in smc_create():
> 
> net/smc/af_smc.c:smc_create() {
>     ...
> 	sk = smc_sock_alloc(net, sock, protocol);
> 	if (!sk)
> 		goto out;
> 
> 	rc = smc_create_clcsk(net, sk, family);
>     ...
> }
> 
> Because smc_sock_alloc() hashes the socket, it becomes visible to hash
> walkers before smc_create_clcsk() assigns smc->clcsock.
> 
> A similar pattern occurs in smc_clcsock_accept():
> 
> net/smc/af_smc.c:smc_clcsock_accept() {
>     ...
> 	new_sk = smc_sock_alloc(sock_net(lsk), NULL, lsk->sk_protocol);
>     ...
> 	(*new_smc)->clcsock = new_clcsock;
>     ...
> }
> 
> Could this expose the socket to a TOCTOU race in smc_diag_msg_common_fill()?
> 
> That function accesses smc->clcsock locklessly without READ_ONCE():
> 
> net/smc/smc_diag.c:smc_diag_msg_common_fill() {
>     ...
> 	if (!smc->clcsock)
> 		return;
> 	r->id.idiag_sport = htons(smc->clcsock->sk->sk_num);
>     ...
> }
> 
> Since the compiler might reload the pointer after the NULL check, a
> concurrent update to smc->clcsock (either during initialization or when
> smc_clcsock_release() sets it to NULL) could cause a NULL pointer
> dereference.

The TOCTOU race in smc_diag_msg_common_fill() that you identified is
*addressed* by a companion patch ("net/smc: fix clcsock and lgr/lnk
races in smc_diag dump path"[1]). That patch wraps the NULL check and
all clcsock field reads inside mutex_lock(&smc->clcsock_release_lock),
which is the same mutex held by smc_clcsock_release() when it sets
clcsock = NULL. This makes the check and reads atomic with the writer,
closing the TOCTOU.

[1]
https://lore.kernel.org/netdev/[email protected]/