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

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

[Mahanta Jambigi]

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);
 			sock_put(new_sk); /* final */
 			continue;
 		}
-- 
2.50.1 (Apple Git-155)

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

[sashiko-bot]

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

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824065851.1070988-1-mjambigi@linux.ibm.com?part=1

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

[Dust Li]

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

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

[Mahanta Jambigi]

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/

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

[Dust Li]

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