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

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

On 2026-08-28 16:48:28, Mahanta Jambigi wrote:
>
>
>On 27/08/26 7:37 pm, Dust Li wrote:
>> 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
>
>For this patch specifically, the smc_accept_dequeue() path is a clear
>lock ordering violation that can cause a crash independently of the
>broader design issue — would you prefer I withdraw this patch and
>address everything together in the larger refactor, or keep this narrow
>fix and do the refactor separately?

I think you can keep this one for stable tree.

>
>> 
>>>
>>>>
>>>> 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() ?
>
>Yes, smc_getname(), smc_set_keepalive(), and all other unprotected
>clcsock dereferences would be fixed as part of this refactor, and in
>fact they are the strongest argument for doing it.
>
>The reason they become safe is structural, not incremental: once
>sock_release(clcsock) is deferred to the SMC socket destructor, clcsock
>is guaranteed non-NULL for the entire observable lifetime of the SMC
>socket. Any caller that holds a sock reference — whether it is a syscall
>path, a workqueue, or a timer callback — is by definition executing
>before the destructor runs. So smc_getname(), smc_set_keepalive(),
>smc_poll(), smc_diag_msg_common_fill(), and every other reader can
>dereference clcsock directly with no lock and no NULL check, and all the
>if (smc->clcsock) guards become dead code that can be removed.
>
>clcsock_release_lock itself would also be removed entirely, along with
>all its lock/unlock sites, since the mutex only exists to coordinate
>multiple sock_release() callers — a problem that disappears when there
>is only one.

Yeah, this is exactly what I expected for the refactor.


>
>>>>
>>>> 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 ?
>
>Sure, I can take that up. It will need a bit of careful work — the close
>state machine has multiple paths that currently call
>smc_clcsock_release() early, and the smc_tcp_listen_work() error path
>where kernel_accept() creates a clcsock that is never assigned to an SMC
>socket will need special handling. I would plan to send it as a small
>series rather than a single patch.

That's perfectly fine.

>
>I am currently occupied with a few other things so it may take some
>time, but I will pick it up once I have bandwidth. If you have any
>specific ideas on the approach or would like to co-develop it, I am very
>happy to collaborate — feel free to share your thoughts and we can work
>through the design together.

I shared my thoughts with my agent and had it implement a draft version for
me — let me send it over so you can take a look, and we can figure out how
to move forward together.

Best regards,
Dust