Re: [PATCH v3] net/smc: release the internal TCP sock on IPPROTO_SMC socket creation failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A preexisting race condition in `smc_close_active_abort` and `smc_close_passive_work` can cause a Use-After-Free or NULL pointer dereference when racing with a concurrent `smc_diag` dump.
--

--- Patch [1]: [PATCH v3] net/smc: release the internal TCP sock on IPPROTO_SMC socket creation failure ---
commit e59fabfdc9a39ca0071e05ed30e01dc2a6a78c65
Author: Chuyf26 <Chuyf26@linux.alibaba.com>

net/smc: release the internal TCP sock on IPPROTO_SMC socket creation failure

This commit adds a .destroy hook to SMC protocols to release the internal TCP
clcsock when socket creation fails. It specifically unhashes the socket before
releasing the clcsock to prevent concurrent smc_diag dumps from dereferencing
a freed socket.

> diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
> index a94084b4a498e..520b666fdd8fa 100644
> --- a/net/smc/smc_inet.c
> +++ b/net/smc/smc_inet.c

[ ... ]

> @@ -116,6 +120,18 @@ static int smc_inet_init_sock(struct sock *sk)
>  	return smc_create_clcsk(net, sk, sk->sk_family);
>  }
>  
> +static void smc_inet_destroy_sock(struct sock *sk)
> +{
> +	/* The sock is hashed and smc_diag dumps dereference smc->clcsock
> +	 * without clcsock_release_lock, while sk_common_release() calls
> +	 * .destroy before .unhash. Unhash first, as __smc_release() does,
> +	 * so no dump can observe the clcsock being released; the second
> +	 * unhash is a no-op.
> +	 */
> +	sk->sk_prot->unhash(sk);
> +	smc_clcsock_release(smc_sk(sk));
> +}
> +

[Severity: High]
This is a pre-existing issue, but since the comments mention that smc_diag
dumps dereference smc->clcsock without locks, could this same race occur
in smc_close_active_abort() and smc_close_passive_work()?

In net/smc/smc_close.c:smc_close_active_abort(), clcsock is released:

    if (release_clcsock) {
        release_sock(sk);
        smc_clcsock_release(smc);
        lock_sock(sk);
    }

And similarly in net/smc/smc_close.c:smc_close_passive_work():

    release_sock(sk);
    if (release_clcsock)
        smc_clcsock_release(smc);
    sock_put(sk);

In these paths, smc_clcsock_release() is called, but it appears the socket
is still in the hash table as unhashing typically happens later.

Concurrently, a dump in net/smc/smc_diag.c:smc_diag_msg_common_fill() does:

    if (!smc->clcsock)
        return;
    r->id.idiag_sport = htons(smc->clcsock->sk->sk_num);

Could a concurrent smc_diag dump hit a use-after-free or NULL pointer
dereference here if a link group failure or peer disconnect triggers socket
closure while the dump iterates the hash table?

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