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

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

[Mahanta Jambigi]

smc_sk_init() calls sk->sk_prot->hash(sk) before several fields are
fully initialised: clcsock_release_lock, the saved clcsk_* callbacks,
use_fallback/fallback_rsn, and conn.close_work.  Once hash() returns the
socket is visible to concurrent hash walkers, which can then observe
uninitialised state.

Move hash(sk) to the end of smc_sk_init() so the socket is published
only after it is fully constructed.

Fixes: d0e35656d834 ("net/smc: refactoring initialization of smc sock")
Reviewed-by: Hidayath Khan <hidayath@linux.ibm.com>
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
Found by syzbot while testing a diag dump patch that drops the hash
read lock before calling mutex_lock(&smc->clcsock_release_lock).  That
change made the uninitialised mutex reachable for the first time and
triggered DEBUG_LOCKS_WARN_ON(lock->magic != lock) immediately.

syzbot report:
https://ci.syzbot.org/series/ab0d12fd-741a-4a0f-9f49-e52b84e94f4a

 net/smc/af_smc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 00403175b740..5e7560417ec7 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);
 }
 
 static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,
-- 
2.50.1

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

[sashiko-bot]

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 <mjambigi@linux.ibm.com>

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.

>  }
>  
>  static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813074315.554926-1-mjambigi@linux.ibm.com?part=1

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

[Mahanta Jambigi]

On 14/08/26 1:13 pm, sashiko-bot@kernel.org 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 <mjambigi@linux.ibm.com>
> 
> 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/20260807081606.3200128-1-mjambigi@linux.ibm.com/