Re: [PATCH net v3] net/smc: fix clcsock and lgr/lnk races in smc_diag dump path

On 08/08/26 1:46 pm, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Dropping the hash lock per-socket and unconditionally restarting the list walk from the head using a logical index introduces O(N^2) list traversal complexity and causes active sockets to be skipped in the diag dump.
> 
> Pre-existing issues:
> - [Critical] The patch attempts to fix a Use-After-Free race on `smc->clcsock` by adding `clcsock_release_lock` in `smc_diag_msg_common_fill()`, but misses a concurrent teardown path in `smc_accept_dequeue()` that frees the socket without acquiring the mutex.
> --
> 
> --- Patch [1]: [PATCH net v3] net/smc: fix clcsock and lgr/lnk races in smc_diag dump path ---
> commit a16d6de2c1851166c340508a037e5292d66a0907
> Author: Mahanta Jambigi <[email protected]>
> 
> net/smc: fix clcsock and lgr/lnk races in smc_diag dump path
> 
> This commit attempts to fix two races in the SMC diag dump path by holding
> clcsock_release_lock in smc_diag_msg_common_fill() and lock_sock() in
> __smc_diag_dump(). It also modifies the dump iteration to pin sockets, drop
> the hash lock, and restart the traversal from the head of the list.
> 
>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>> index bf0beaa23bdb6..72d77360e504f 100644
>> --- a/net/smc/smc_diag.c
>> +++ b/net/smc/smc_diag.c
>> @@ -39,22 +39,34 @@ static void smc_diag_msg_common_fill(struct smc_diag_msg *r, struct sock *sk)
>>  	memset(r, 0, sizeof(*r));
>>  	r->diag_family = sk->sk_family;
>>  	sock_diag_save_cookie(sk, r->id.idiag_cookie);
>> -	if (!smc->clcsock)
>> -		return;
>> -	r->id.idiag_sport = htons(smc->clcsock->sk->sk_num);
>> -	r->id.idiag_dst[0] = smc->clcsock->sk->sk_daddr;
>> +	/*
>> +	 * smc_clcsock_release() sets smc->clcsock = NULL under
>> +	 * clcsock_release_lock before freeing the socket. Hold the same
>> +	 * mutex here to make the NULL check and all field reads atomic
>> +	 * with that writer. mutex_lock() is safe: this function is called
>> +	 * only after the hash spinlock has been dropped by
>> +	 * smc_diag_dump_proto().
>> +	 */
>> +	mutex_lock(&smc->clcsock_release_lock);
>> +	if (smc->clcsock) {
>> +		r->id.idiag_sport = htons(smc->clcsock->sk->sk_num);
> 
> [Severity: Critical]
> This is a pre-existing issue, but does this mutex fully protect against
> concurrent teardowns?
> 
> Looking at smc_accept_dequeue() in net/smc/af_smc.c, if an unaccepted socket
> is closed (SMC_CLOSED), it unhashes it and frees isk->clcsock without holding
> clcsock_release_lock or the child's socket lock:

I'll address this issue separately as this is a pre-existing one.

> 
> net/smc/af_smc.c:smc_accept_dequeue() {
>     ...
>     if (new_sk->sk_state == SMC_CLOSED) {
>         new_sk->sk_prot->unhash(new_sk);
>         if (isk->clcsock) {
>             sock_release(isk->clcsock);
>             isk->clcsock = NULL;
>         }
>     ...
> }
> 
> If the netlink diag dump thread concurrently iterates the hash table, grabs
> the socket reference, and calls smc_diag_msg_common_fill(), could it acquire
> the mutex, see a non-NULL clcsock, and dereference it while it is being freed
> by smc_accept_dequeue()?
> 
> [ ... ]
> 
>> @@ -204,25 +228,43 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb,
>>  	int rc = 0, num = 0;
>>  	struct sock *sk;
>>  
>> -	read_lock(&prot->h.smc_hash->lock);
>>  	head = &prot->h.smc_hash->ht;
>> +restart:
>> +	num = 0;
>> +	read_lock(&prot->h.smc_hash->lock);
>>  	if (hlist_empty(head))
>>  		goto out;
>> -
>>  	sk_for_each(sk, head) {
>>  		if (!net_eq(sock_net(sk), net))
>>  			continue;
>>  		if (num < snum)
>>  			goto next;
>> +		/*
>> +		 * Pin sk before dropping the lock. refcount_inc_not_zero()
>> +		 * skips sockets already past their last reference.
>> +		 * smc_unhash_sk() nulls sk->sk_node.next via sk_del_node_init()
>> +		 * so resuming an interrupted sk_for_each() would terminate
>> +		 * early if a socket is unhashed while the lock is dropped.
>> +		 * Restart from head after each unlock, using snum to skip
>> +		 * already-dumped entries.
>> +		 */
>> +		if (!refcount_inc_not_zero(&sk->sk_refcnt))
>> +			goto next;
>> +		read_unlock(&prot->h.smc_hash->lock);
>> +
>>  		rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh), bc);
>> +		sock_put(sk);
>> +
>>  		if (rc < 0)
>> -			goto out;
>> +			goto out_nolock;
>> +		snum = num + 1;
>> +		goto restart;
> 
> [Severity: High]
> Does restarting the list walk from the head using a logical index (snum)
> introduce an O(N^2) list traversal complexity?
> 
> If any previously seen sockets are concurrently unhashed while the lock is
> dropped, could stepping over (num < snum) elements skip active sockets that
> shifted to earlier positions in the list?

I got couple of reviews from Sashiko AI & I have addressed them here.

Q1: O(N²) complexity?

Yes, this is O(N²) in the worst case — on each restart we skip
already-dumped entries from the head. This is an accepted trade-off: the
same pattern is used in inet_diag and unix_diag. The lock cannot be held
across __smc_diag_dump() since it now takes sleeping locks (mutex_lock,
lock_sock), so drop-and-restart is unavoidable. The skip itself is cheap
(counter comparison only), and the SMC hash is small in practice.

Q2: Can sockets shift to earlier positions and get skipped?

No. SMC uses an hlist where new sockets are always inserted at the head
via hlist_add_head(). Removal does not reorder remaining nodes. So a
socket that existed before an unlock cannot move to an earlier position
— its ordinal index across restarts is stable. A socket inserted during
the unlock will appear at position 0 on the next restart and will be
skipped by num < snum, but that is correct and consistent behaviour —
netlink dumps are not guaranteed to be atomic snapshots.


Sashiko AI review ·
https://sashiko.dev/#/patchset/[email protected]?part=1