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

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:

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?

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