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

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

On 2026-08-26 14:42:31, Mahanta Jambigi wrote:
>
>
>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.

OK

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

What about other places that dereference clcsock, like smc_getname()/smc_set_keepalive() ?


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

Yes, that should be a bit refactor, are you interested in doing that ?

Best regards,
Dust