Re: [PATCH net] net/smc: serialize clcsock teardown in smc_accept_dequeue

[Dust Li <dust.li@linux.alibaba.com>]

On 2026-08-24 08:58:51, Mahanta Jambigi wrote:
>smc_accept_dequeue() open-codes clcsock teardown for SMC_CLOSED child sockets
>without taking clcsock_release_lock:
>
>  new_sk->sk_prot->unhash(new_sk);
>  if (isk->clcsock) {
>      sock_release(isk->clcsock);
>      isk->clcsock = NULL;
>  }
>
>This bypasses the clcsock_release_lock discipline used elsewhere in SMC clcsock
>lifetime handling. In particular, other paths serialize clcsock access and
>updates with clcsock_release_lock, but this local teardown path does not.
>
>Fix it by taking clcsock_release_lock around the local teardown and by storing
>NULL before sock_release(), matching the established ordering used by other
>clcsock teardown paths.
>
>Fixes: 127f49705823 ("net/smc: release clcsock from tcp_listen_worker")
>Reviewed-by: Hidayath Khan <hidayath@linux.ibm.com>
>Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
>---
> net/smc/af_smc.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
>diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>index 00403175b740..bbf8269876ee 100644
>--- a/net/smc/af_smc.c
>+++ b/net/smc/af_smc.c
>@@ -1833,10 +1833,14 @@ struct sock *smc_accept_dequeue(struct sock *parent,
> 		smc_accept_unlink(new_sk);
> 		if (new_sk->sk_state == SMC_CLOSED) {
> 			new_sk->sk_prot->unhash(new_sk);
>-			if (isk->clcsock) {
>-				sock_release(isk->clcsock);
>-				isk->clcsock = NULL;
>-			}
>+			mutex_lock(&isk->clcsock_release_lock);
>+			if (isk->clcsock) {
>+				struct socket *clcsock = isk->clcsock;
>+
>+				isk->clcsock = NULL;
>+				sock_release(clcsock);
>+			}
>+			mutex_unlock(&isk->clcsock_release_lock);

Why not call smc_clcsock_release() here ?

After looking deeper into this issue, I found smc_diag_msg_common_fill()/
smc_getname() and many other branches hasn't hold lock_sock() and may also
have the race issue ? For example, smc_getname() calling smc->clcsock->ops->getname()
while the other workqueue is releasing the clcsock.

Since SMC has long been plagued by this kind of locking issue, I think we
should consider a long-term solution to address it.

What about stop releasing the clcsock early and tie its lifetime to the SMC
socket itself. The early paths don't really need to release it ??? a
kernel_sock_shutdown()/tcp_abort() is enough to stop it, and calling them more
than once is safe because the TCP layer already handles that.  sock_release()
is different: it frees the socket, so it must happen exactly once. If we move
this single release to the final teardown of the SMC socket, no user can exist
at that point anymore (fd users are gone after smc_release(), and every
work/accept-queue context holds a sock reference), which makes both
clcsock_release_lock and all the if (smc->clcsock) NULL checks removable.

Ideas ?

Best regards,
Dust