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

[Mahanta Jambigi <mjambigi@linux.ibm.com>]

On 26/08/26 8:48 am, Dust Li wrote:
> 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 ?

I initially considered *smc_clcsock_release()*, but it unconditionally
calls cancel_work_sync() for child sockets (since listen_smc is always
set and current_work() != &smc->smc_listen_work is always true here).
Even though smc_listen_work has already completed before
smc_accept_enqueue() is called, cancel_work_sync() is not truly free on
that path — it still acquires the worker pool spinlock and scans the
executing-worker list before determining the work is idle. The inline
open-coding avoids that overhead entirely.

> 
> 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.

I am addressing the smc_diag_msg_common_fill() issue via a separate
patch[1].

> 
> 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 ?

I agree, but it is a significant refactor touching every early-release
path in af_smc.c, smc_close.c etc & it warrants its own patch series
with careful ordering of changes.

[1] -
https://lore.kernel.org/netdev/20260807081606.3200128-1-mjambigi@linux.ibm.com/