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

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

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?

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

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

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.

> 
> Best regards,
> Dust