[PATCH net v5] net/smc: fix lgr/lnk lifetime vs diag reader race

[PATCH net v5] net/smc: fix lgr/lnk lifetime vs diag reader race

[Mahanta Jambigi]

The diag dump walks the socket hash table under a read_lock and dereferences
conn->lgr and conn->lnk. Two terminal teardown paths drop those references via
smc_conn_free() while the socket is still hashed:

  - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free()
  - smc_close_passive_work() -> smc_conn_free()

This allows the diag reader to dereference a freed lgr or lnk.

Fix it by unhashing the socket before smc_conn_free() is called at the two
terminal teardown sites in smc_close.c.

Additionally, smc_conn_abort() calls smc_conn_free() during early handshake
aborts while the socket remains hashed in SMC_INIT state. The
smc_listen_out_err() path leaves the socket hashed in SMC_CLOSED state after
smc_conn_abort() returns. Guard the conn/lgr/lnk inspection blocks in
__smc_diag_dump() by skipping them when the socket is in SMC_INIT or SMC_CLOSED
state.

Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
Changes in v5:
- guard conn/lgr/lnk blocks in __smc_diag_dump() with an
  (SMC_INIT || SMC_CLOSED) state check placed after the
  SMC_DIAG_FALLBACK nla_put; SMC_INIT can coexist with fallback
  (smc_sendmsg() MSG_FASTOPEN path), so the guard must not suppress
  the fallback reason

Changes in v4:
- dropped smc_conn_unhash() wrapper, conn->unhashed flag, and all
  changes to af_smc.c, smc.h, smc_core.c and smc_core.h;
  smc_unhash_sk() is already idempotent via sk_hashed(), so direct
  calls at the two teardown sites in smc_close.c are sufficient
- dropped the __smc_release() hunk: it needs no change since the
  subsequent unhash there is already a safe no-op
- fixed premature-unhash issue present in v3: smc_conn_free() must not
  unhash because smc_conn_abort() calls it before
  smc_switch_to_fallback() in both smc_listen_decline() and
  smc_connect_rdma() error paths; unhashing there would make live
  fallback sockets invisible to smcss
- likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(),
  smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed
  attempt and then smc_conn_create() on the next device; unhashing in
  smc_conn_free() would permanently hide the established connection from
  smc_diag since smc_conn_create() does not re-hash the socket

Changes in v3:
- redesigned as a single patch; dropped the lgr_lnk_lock spinlock
  approach and the 2-patch split
- fix is now at the socket hash layer: introduce smc_conn_unhash() with
  a per-connection unhashed flag; smc_conn_free() unhashes before dropping
  lgr/lnk refs, so any socket visible to the diag reader under the hash
  read_lock has valid conn->lgr and conn->lnk pointers
- __smc_release() updated to call smc_conn_unhash() for non-fallback
  sockets so the flag is honoured when smc_conn_free() already ran first
- smc_diag.c needs no changes; the hash read_lock invariant is
  sufficient without any per-connection lock in the dump path
- dropped the clcsock/mutex_trylock fix as that will be addressed separately

Changes in v2:
- this is v2 of the 2-patch series; the earlier submission was mislabelled
  [PATCH v3] but was in fact the first version sent to the list
- split into a 2-patch series; patch 1/2 adds per-connection lgr_lnk_lock
  infrastructure to smc_core, patch 2/2 fixes the diag dump path using it
- dropped lock_sock()/release_sock() from __smc_diag_dump(); v1 held the
  socket lock across all lgr/lnk dereferences, requiring the hash read_lock
  to be dropped and re-acquired around each socket
- dropped the restart-from-head loop in smc_diag_dump_proto(); the new
  design does not drop the hash read_lock mid-walk so the hlist truncation
  concern no longer applies
- dropped refcount_inc_not_zero() socket pinning from the dump loop for the
  same reason: the hash read_lock is now held for the full walk
- added per-connection lgr_lnk_lock spinlock to struct smc_connection;
  conn->lgr and conn->lnk are NULLed under this lock in smc_conn_free()
  before borrowed references are released, establishing the invariant: a
  non-NULL conn->lgr seen under lgr_lnk_lock guarantees the lgr is alive
- added lgr_lnk_lock to smc_switch_link_and_count() to protect the conn->lnk
  pointer swap from concurrent diag readers
- replaced mutex_lock() on clcsock_release_lock in smc_diag_msg_common_fill()
  with mutex_trylock(); mutex_lock() was valid in v1 because the hash
  spinlock had been dropped, but the new design holds the hash read_lock
  throughout so only a non-sleeping trylock is safe; a failed trylock leaves
  address fields zeroed, which is acceptable for a monitoring tool
- all conn->lgr and conn->lnk accesses in __smc_diag_dump() use a
  snapshot-then-use pattern: fields are copied into local stack variables
  under lgr_lnk_lock and nla_put() is called after releasing the lock,
  avoiding any sleeping operation under the spinlock

 net/smc/smc_close.c | 10 ++++++++++
 net/smc/smc_diag.c  |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
index bb0313ef5f7c..3940098c651d 100644
--- a/net/smc/smc_close.c
+++ b/net/smc/smc_close.c
@@ -130,6 +130,11 @@ void smc_close_active_abort(struct smc_sock *smc)
 	struct sock *sk = &smc->sk;
 	bool release_clcsock = false;

+	/*
+	 * Unhash before any branch calls smc_conn_free() and drops lgr/lnk refs;
+	 * the diag reader must not see a hashed socket with freed lgr/lnk.
+	 */
+	sk->sk_prot->unhash(sk);
 	if (sk->sk_state != SMC_INIT && smc->clcsock && smc->clcsock->sk) {
 		sk->sk_err = ECONNABORTED;
 		if (smc->clcsock && smc->clcsock->sk)
@@ -433,6 +438,11 @@ static void smc_close_passive_work(struct work_struct *work)
 		sk->sk_state_change(sk);
 		if ((sk->sk_state == SMC_CLOSED) &&
 		    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {
+			/*
+			 * Unhash before smc_conn_free() drops lgr/lnk refs so the diag
+			 * reader cannot see a hashed socket with freed lgr/lnk.
+			 */
+			sk->sk_prot->unhash(sk);
 			smc_conn_free(conn);
 			if (smc->clcsock)
 				release_clcsock = true;
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index bf0beaa23bdb..fb615d3af058 100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -103,5 +103,8 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
 	if (nla_put(skb, SMC_DIAG_FALLBACK, sizeof(fallback), &fallback) < 0)
 		goto errout;

+	if (sk->sk_state == SMC_INIT || sk->sk_state == SMC_CLOSED)
+		goto out;
+
 	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
 	    smc->conn.alert_token_local) {
@@ -185,5 +188,6 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
 			goto errout;
 	}

+out:
 	nlmsg_end(skb, nlh);
 	return 0;
-- 
2.43.7

Re: [PATCH net v5] net/smc: fix lgr/lnk lifetime vs diag reader race

[sashiko-bot]

> The diag dump walks the socket hash table under a read_lock and dereferences
> conn->lgr and conn->lnk. Two terminal teardown paths drop those references via
> smc_conn_free() while the socket is still hashed:
> 
>   - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free()
>   - smc_close_passive_work() -> smc_conn_free()
> 
> This allows the diag reader to dereference a freed lgr or lnk.
> 
> Fix it by unhashing the socket before smc_conn_free() is called at the two
> terminal teardown sites in smc_close.c.
> 
> Additionally, smc_conn_abort() calls smc_conn_free() during early handshake
> aborts while the socket remains hashed in SMC_INIT state. The
> smc_listen_out_err() path leaves the socket hashed in SMC_CLOSED state after
> smc_conn_abort() returns. Guard the conn/lgr/lnk inspection blocks in
> __smc_diag_dump() by skipping them when the socket is in SMC_INIT or SMC_CLOSED
> state.
> 
> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
> Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
> 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/20260923061716.1970059-1-mjambigi@linux.ibm.com?part=1

Re: [PATCH net v5] net/smc: fix lgr/lnk lifetime vs diag reader race

[Dust Li]

On 2026-09-23 08:17:16, Mahanta Jambigi wrote:
>The diag dump walks the socket hash table under a read_lock and dereferences
>conn->lgr and conn->lnk. Two terminal teardown paths drop those references via
>smc_conn_free() while the socket is still hashed:
>
>  - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free()
>  - smc_close_passive_work() -> smc_conn_free()
>
>This allows the diag reader to dereference a freed lgr or lnk.
>
>Fix it by unhashing the socket before smc_conn_free() is called at the two
>terminal teardown sites in smc_close.c.
>
>Additionally, smc_conn_abort() calls smc_conn_free() during early handshake
>aborts while the socket remains hashed in SMC_INIT state. The
>smc_listen_out_err() path leaves the socket hashed in SMC_CLOSED state after
>smc_conn_abort() returns. Guard the conn/lgr/lnk inspection blocks in
>__smc_diag_dump() by skipping them when the socket is in SMC_INIT or SMC_CLOSED
>state.
>
>Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
>Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
>Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>


Reviewed-by: Dust Li <dust.li@linux.alibaba.com>

Best regards,
Dust

Re: [PATCH net v5] net/smc: fix lgr/lnk lifetime vs diag reader race

[Sidraya Jayagond]

On 23/09/26 11:47 am, Mahanta Jambigi wrote:
> The diag dump walks the socket hash table under a read_lock and dereferences
> conn->lgr and conn->lnk. Two terminal teardown paths drop those references via
> smc_conn_free() while the socket is still hashed:
> 
>   - smc_conn_kill() -> smc_close_active_abort() -> smc_conn_free()
>   - smc_close_passive_work() -> smc_conn_free()
> 
> This allows the diag reader to dereference a freed lgr or lnk.
> 
> Fix it by unhashing the socket before smc_conn_free() is called at the two
> terminal teardown sites in smc_close.c.
> 
> Additionally, smc_conn_abort() calls smc_conn_free() during early handshake
> aborts while the socket remains hashed in SMC_INIT state. The
> smc_listen_out_err() path leaves the socket hashed in SMC_CLOSED state after
> smc_conn_abort() returns. Guard the conn/lgr/lnk inspection blocks in
> __smc_diag_dump() by skipping them when the socket is in SMC_INIT or SMC_CLOSED
> state.
> 
> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
> Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
> Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
> ---
> Changes in v5:
> - guard conn/lgr/lnk blocks in __smc_diag_dump() with an
>   (SMC_INIT || SMC_CLOSED) state check placed after the
>   SMC_DIAG_FALLBACK nla_put; SMC_INIT can coexist with fallback
>   (smc_sendmsg() MSG_FASTOPEN path), so the guard must not suppress
>   the fallback reason
> 
> Changes in v4:
> - dropped smc_conn_unhash() wrapper, conn->unhashed flag, and all
>   changes to af_smc.c, smc.h, smc_core.c and smc_core.h;
>   smc_unhash_sk() is already idempotent via sk_hashed(), so direct
>   calls at the two teardown sites in smc_close.c are sufficient
> - dropped the __smc_release() hunk: it needs no change since the
>   subsequent unhash there is already a safe no-op
> - fixed premature-unhash issue present in v3: smc_conn_free() must not
>   unhash because smc_conn_abort() calls it before
>   smc_switch_to_fallback() in both smc_listen_decline() and
>   smc_connect_rdma() error paths; unhashing there would make live
>   fallback sockets invisible to smcss
> - likewise, the ISM/RDMA retry loops (smc_find_ism_v2_device_serv(),
>   smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed
>   attempt and then smc_conn_create() on the next device; unhashing in
>   smc_conn_free() would permanently hide the established connection from
>   smc_diag since smc_conn_create() does not re-hash the socket
> 
> Changes in v3:
> - redesigned as a single patch; dropped the lgr_lnk_lock spinlock
>   approach and the 2-patch split
> - fix is now at the socket hash layer: introduce smc_conn_unhash() with
>   a per-connection unhashed flag; smc_conn_free() unhashes before dropping
>   lgr/lnk refs, so any socket visible to the diag reader under the hash
>   read_lock has valid conn->lgr and conn->lnk pointers
> - __smc_release() updated to call smc_conn_unhash() for non-fallback
>   sockets so the flag is honoured when smc_conn_free() already ran first
> - smc_diag.c needs no changes; the hash read_lock invariant is
>   sufficient without any per-connection lock in the dump path
> - dropped the clcsock/mutex_trylock fix as that will be addressed separately
> 
> Changes in v2:
> - this is v2 of the 2-patch series; the earlier submission was mislabelled
>   [PATCH v3] but was in fact the first version sent to the list
> - split into a 2-patch series; patch 1/2 adds per-connection lgr_lnk_lock
>   infrastructure to smc_core, patch 2/2 fixes the diag dump path using it
> - dropped lock_sock()/release_sock() from __smc_diag_dump(); v1 held the
>   socket lock across all lgr/lnk dereferences, requiring the hash read_lock
>   to be dropped and re-acquired around each socket
> - dropped the restart-from-head loop in smc_diag_dump_proto(); the new
>   design does not drop the hash read_lock mid-walk so the hlist truncation
>   concern no longer applies
> - dropped refcount_inc_not_zero() socket pinning from the dump loop for the
>   same reason: the hash read_lock is now held for the full walk
> - added per-connection lgr_lnk_lock spinlock to struct smc_connection;
>   conn->lgr and conn->lnk are NULLed under this lock in smc_conn_free()
>   before borrowed references are released, establishing the invariant: a
>   non-NULL conn->lgr seen under lgr_lnk_lock guarantees the lgr is alive
> - added lgr_lnk_lock to smc_switch_link_and_count() to protect the conn->lnk
>   pointer swap from concurrent diag readers
> - replaced mutex_lock() on clcsock_release_lock in smc_diag_msg_common_fill()
>   with mutex_trylock(); mutex_lock() was valid in v1 because the hash
>   spinlock had been dropped, but the new design holds the hash read_lock
>   throughout so only a non-sleeping trylock is safe; a failed trylock leaves
>   address fields zeroed, which is acceptable for a monitoring tool
> - all conn->lgr and conn->lnk accesses in __smc_diag_dump() use a
>   snapshot-then-use pattern: fields are copied into local stack variables
>   under lgr_lnk_lock and nla_put() is called after releasing the lock,
>   avoiding any sleeping operation under the spinlock
> 
>  net/smc/smc_close.c | 10 ++++++++++
>  net/smc/smc_diag.c  |  4 ++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
> index bb0313ef5f7c..3940098c651d 100644
> --- a/net/smc/smc_close.c
> +++ b/net/smc/smc_close.c
> @@ -130,6 +130,11 @@ void smc_close_active_abort(struct smc_sock *smc)
>  	struct sock *sk = &smc->sk;
>  	bool release_clcsock = false;
> 
> +	/*
> +	 * Unhash before any branch calls smc_conn_free() and drops lgr/lnk refs;
> +	 * the diag reader must not see a hashed socket with freed lgr/lnk.
> +	 */
> +	sk->sk_prot->unhash(sk);
>  	if (sk->sk_state != SMC_INIT && smc->clcsock && smc->clcsock->sk) {
>  		sk->sk_err = ECONNABORTED;
>  		if (smc->clcsock && smc->clcsock->sk)
> @@ -433,6 +438,11 @@ static void smc_close_passive_work(struct work_struct *work)
>  		sk->sk_state_change(sk);
>  		if ((sk->sk_state == SMC_CLOSED) &&
>  		    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {
> +			/*
> +			 * Unhash before smc_conn_free() drops lgr/lnk refs so the diag
> +			 * reader cannot see a hashed socket with freed lgr/lnk.
> +			 */
> +			sk->sk_prot->unhash(sk);
>  			smc_conn_free(conn);
>  			if (smc->clcsock)
>  				release_clcsock = true;
> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb..fb615d3af058 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
> @@ -103,5 +103,8 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>  	if (nla_put(skb, SMC_DIAG_FALLBACK, sizeof(fallback), &fallback) < 0)
>  		goto errout;
> 
> +	if (sk->sk_state == SMC_INIT || sk->sk_state == SMC_CLOSED)
> +		goto out;
> +
>  	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>  	    smc->conn.alert_token_local) {
> @@ -185,5 +188,6 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>  			goto errout;
>  	}
> 
> +out:
>  	nlmsg_end(skb, nlh);
>  	return 0;

Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>

Re: [PATCH net v5] net/smc: fix lgr/lnk lifetime vs diag reader race

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 2 (1) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] __smc_diag_dump() (net/smc/smc_diag.c:91-96) still reads
  `smc->conn.lgr->is_smcd` to work out r->diag_mode.
- [High] In the SMC_DIAG_LGRINFO block, __smc_diag_dump() does `struct
  smc_link *link = smc->conn.lnk;` and then dereferences…
- [Medium] The commit message says the socket is unhashed "before
  smc_conn_free() is called at the two terminal teardown sites".

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  leaves it open while adding an unhash that runs too late on the same…

--

net/smc: fix lgr/lnk lifetime vs diag reader race

This patch unhashes SMC sockets in smc_close_active_abort() and
smc_close_passive_work() before smc_conn_free() drops the lgr/lnk
references. It also makes __smc_diag_dump() skip the conn/lgr/lnk
attribute blocks when the socket is in SMC_INIT or SMC_CLOSED state. The
goal is to keep the diag reader from dereferencing a freed link group or
link.

> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
> index bb0313ef5f7c1..3940098c651d9 100644
> --- a/net/smc/smc_close.c
> +++ b/net/smc/smc_close.c
> @@ -130,6 +130,11 @@ void smc_close_active_abort(struct smc_sock *smc)
>  	struct sock *sk = &smc->sk;
>  	bool release_clcsock = false;
>  
> +	/*
> +	 * Unhash before any branch calls smc_conn_free() and drops lgr/lnk refs;
> +	 * the diag reader must not see a hashed socket with freed lgr/lnk.
> +	 */
> +	sk->sk_prot->unhash(sk);

[Severity: Medium]
The commit message says the socket is unhashed "before smc_conn_free() is
called at the two terminal teardown sites". This unhash, though, runs
unconditionally for every state. Is that intended?

The SMC_ACTIVE, SMC_APPCLOSEWAIT1 and SMC_APPCLOSEWAIT2 branches never call
smc_conn_free(). They only set SMC_CLOSED and call sock_put(). SMC_INIT,
SMC_PEERABORTWAIT and SMC_CLOSED don't call it either. Neither does the early
break taken when smc_close_cancel_work() changes the state.

On link group termination, smc_conn_kill() calls smc_close_active_abort()
for every connection:

net/smc/smc_core.c:smc_conn_kill() {
    ...
    smc_lgr_unregister_conn(conn);
    smc_close_active_abort(smc);
}

Doesn't this make killed SMC_ACTIVE sockets vanish from smc_diag/smcss
right away instead of at close? The application still has them open, and
their lgr/lnk references are still held.

These sockets reach smc_conn_free() later in __smc_release(), which already
unhashes first. smc_release() is also a third caller of
smc_close_active_abort(), and the commit message doesn't mention it.

Would it be narrower to unhash only right before the two smc_conn_free()
calls in the SMC_PEERCLOSEWAIT1/2/SMC_PEERFINCLOSEWAIT and
SMC_PROCESSABORT/SMC_APPFINCLOSEWAIT branches?

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Still, on
the smc_conn_kill() path this new unhash runs too late to protect against
it.

On SMC-D with dmb_nocopy (for example loopback-ism), smc_conn_kill() frees
the sndbuf descriptor before it unregisters the conn and before it calls
smc_close_active_abort():

net/smc/smc_core.c:smc_conn_kill() {
    ...
    if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
        smcd_buf_detach(conn);
    ...
    smc_lgr_unregister_conn(conn);
    smc_close_active_abort(smc);
}

smcd_buf_detach() does this:

    buf_desc = conn->sndbuf_desc;
    conn->sndbuf_desc = NULL;
    kfree(buf_desc);

During that window the socket is still hashed and still in SMC_ACTIVE, so
it passes the new diag guard. alert_token_local is also still set. That
means __smc_diag_dump() enters the SMC_DIAG_CONNINFO block and evaluates:

    .sndbuf_size = conn->sndbuf_desc ?
        conn->sndbuf_desc->len : 0,

These are plain loads, and only the hash read_lock is held. Could this
dereference NULL, or read ->len from an smc_buf_desc that has been kfree'd?
The lock_sock() in __smc_lgr_terminate() does not exclude the diag reader.

Would unhashing at the start of smc_conn_kill(), before smcd_buf_detach(),
close this as well?

>  	if (sk->sk_state != SMC_INIT && smc->clcsock && smc->clcsock->sk) {
>  		sk->sk_err = ECONNABORTED;
>  		if (smc->clcsock && smc->clcsock->sk)

[ ... ]

> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb6..fe32091e37dab 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
> @@ -103,6 +103,9 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>  	if (nla_put(skb, SMC_DIAG_FALLBACK, sizeof(fallback), &fallback) < 0)
>  		goto errout;
>  
> +	if (sk->sk_state == SMC_INIT || sk->sk_state == SMC_CLOSED)
> +		goto out;
> +

[Severity: High]
Does this guard cover the lgr dereference earlier in __smc_diag_dump() that
sets r->diag_mode?

net/smc/smc_diag.c:__smc_diag_dump() {
    ...
    if (smc->use_fallback)
        r->diag_mode = SMC_DIAG_MODE_FALLBACK_TCP;
    else if (smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd)
        r->diag_mode = SMC_DIAG_MODE_SMCD;
    ...
}

That load happens before the new SMC_INIT/SMC_CLOSED check, and only the
smc hash read_lock is held.

The commit message says this guard handles the smc_conn_abort() SMC_INIT
case. Take a handshake failure such as smc_listen_decline() ->
smc_conn_abort(new_smc, local_first). The socket is still hashed in
SMC_INIT and use_fallback has not been set yet:

net/smc/af_smc.c:smc_conn_abort() {
    ...
    smc_conn_free(conn);
    if (local_first && lgr_valid)
        smc_lgr_cleanup_early(lgr);
}

The following chain then frees the lgr synchronously:

smc_lgr_cleanup_early()
  __smc_lgr_terminate()
    smc_lgr_free()
      smc_lgr_put()
        __smc_lgr_free()
          kfree(lgr)

Nothing on this path takes the hash write_lock, and conn->lgr is never
cleared. smc_conn_lgr_valid() is only plain loads of conn->lgr and
conn->alert_token_local.

Can a diag reader pass smc_conn_lgr_valid() just before the conn is
unregistered, and then read lgr->is_smcd after the kfree()?

smc_diag_handler_dump() has no capability check, and a remote peer can
cause the handshake failure. Would checking the state (or use_fallback)
before any conn.lgr access close this?

>  	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>  	    smc->conn.alert_token_local) {
>  		struct smc_connection *conn = &smc->conn;

[Severity: High]
Sockets in SMC_ACTIVE and the closing states still get past this guard. Is
the SMC_DIAG_LGRINFO block further down still exposed to a concurrent link
switch?

net/smc/smc_diag.c:__smc_diag_dump() {
    ...
    struct smc_link *link = smc->conn.lnk;
    ...
    memcpy(linfo.lnk[0].ibname, link->smcibdev->ibdev->name,
           sizeof(link->smcibdev->ibdev->name));
    ...
}

Only the hash read_lock is held here. During SMC-R link failover,
smcr_link_down() -> smc_switch_conns() -> smc_switch_link_and_count()
replaces conn->lnk under conn->send_lock and drops the conn's reference on
the old link:

net/smc/smc_core.c:smc_switch_link_and_count() {
    ...
    smcr_link_put(conn->lnk);
    conn->lnk = to_lnk;
    ...
}

When smcr_link_clear() drops the last reference, __smcr_link_clear() calls
put_device() on the ibdev and then memset(lnk, 0). The lgr stays on its
list during failover, so the !list_empty(&smc->conn.lgr->list) check does
not stop the reader.

Suppose a reader loads the old link pointer just before the switch. Can it
then see link->smcibdev == NULL and oops? That is the same signature that
9dbe086c69b8 fixed. It could also read the name of an ibdev that has
already been released.

Unhashing before smc_conn_free() does not cover this path. The v2
serialisation of the conn->lnk swap against the reader was dropped in this
version, and nothing replaces it.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923061716.1970059-1-mjambigi%40linux.ibm.com

Re: [PATCH net v5] net/smc: fix lgr/lnk lifetime vs diag reader race

[Mahanta Jambigi]

On 25/09/26 2:50 am, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 2 (1) · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] __smc_diag_dump() (net/smc/smc_diag.c:91-96) still reads
>   `smc->conn.lgr->is_smcd` to work out r->diag_mode.
> - [High] In the SMC_DIAG_LGRINFO block, __smc_diag_dump() does `struct
>   smc_link *link = smc->conn.lnk;` and then dereferences…
> - [Medium] The commit message says the socket is unhashed "before
>   smc_conn_free() is called at the two terminal teardown sites".
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the patch
>   leaves it open while adding an unhash that runs too late on the same…
> 
> --
> 
> net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This patch unhashes SMC sockets in smc_close_active_abort() and
> smc_close_passive_work() before smc_conn_free() drops the lgr/lnk
> references. It also makes __smc_diag_dump() skip the conn/lgr/lnk
> attribute blocks when the socket is in SMC_INIT or SMC_CLOSED state. The
> goal is to keep the diag reader from dereferencing a freed link group or
> link.
> 
>> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
>> index bb0313ef5f7c1..3940098c651d9 100644
>> --- a/net/smc/smc_close.c
>> +++ b/net/smc/smc_close.c
>> @@ -130,6 +130,11 @@ void smc_close_active_abort(struct smc_sock *smc)
>>  	struct sock *sk = &smc->sk;
>>  	bool release_clcsock = false;
>>  
>> +	/*
>> +	 * Unhash before any branch calls smc_conn_free() and drops lgr/lnk refs;
>> +	 * the diag reader must not see a hashed socket with freed lgr/lnk.
>> +	 */
>> +	sk->sk_prot->unhash(sk);
> 
> [Severity: Medium]
> The commit message says the socket is unhashed "before smc_conn_free() is
> called at the two terminal teardown sites". This unhash, though, runs
> unconditionally for every state. Is that intended?
> 
> The SMC_ACTIVE, SMC_APPCLOSEWAIT1 and SMC_APPCLOSEWAIT2 branches never call
> smc_conn_free(). They only set SMC_CLOSED and call sock_put(). SMC_INIT,
> SMC_PEERABORTWAIT and SMC_CLOSED don't call it either. Neither does the early
> break taken when smc_close_cancel_work() changes the state.
> 
> On link group termination, smc_conn_kill() calls smc_close_active_abort()
> for every connection:
> 
> net/smc/smc_core.c:smc_conn_kill() {
>     ...
>     smc_lgr_unregister_conn(conn);
>     smc_close_active_abort(smc);
> }
> 
> Doesn't this make killed SMC_ACTIVE sockets vanish from smc_diag/smcss
> right away instead of at close? The application still has them open, and
> their lgr/lnk references are still held.
> 
> These sockets reach smc_conn_free() later in __smc_release(), which already
> unhashes first. smc_release() is also a third caller of
> smc_close_active_abort(), and the commit message doesn't mention it.
> 
> Would it be narrower to unhash only right before the two smc_conn_free()
> calls in the SMC_PEERCLOSEWAIT1/2/SMC_PEERFINCLOSEWAIT and
> SMC_PROCESSABORT/SMC_APPFINCLOSEWAIT branches?

You are right. The unconditional unhash at the top of
smc_close_active_abort() was placed there because v5 traced the
smc_conn_kill() → smc_close_active_abort() → smc_conn_free() chain and
stopped at smc_close_active_abort() as the insertion point. This was too
broad — it also fires for the SMC_ACTIVE/APPCLOSEWAIT branches which
never call smc_conn_free(), and for the smc_release() call site where
the socket is a live connection the application still has open.

Fix in v6: remove the unconditional unhash from the top of
smc_close_active_abort() and place it scoped, immediately before each of
the two smc_conn_free() calls — in the PEERCLOSEWAIT1/2/PEERFINCLOSEWAIT
branch and the PROCESSABORT/APPFINCLOSEWAIT branch.

> 
> [Severity: High]
> This is a pre-existing issue, not one introduced by this patch. Still, on
> the smc_conn_kill() path this new unhash runs too late to protect against
> it.
> 
> On SMC-D with dmb_nocopy (for example loopback-ism), smc_conn_kill() frees
> the sndbuf descriptor before it unregisters the conn and before it calls
> smc_close_active_abort():
> 
> net/smc/smc_core.c:smc_conn_kill() {
>     ...
>     if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
>         smcd_buf_detach(conn);
>     ...
>     smc_lgr_unregister_conn(conn);
>     smc_close_active_abort(smc);
> }
> 
> smcd_buf_detach() does this:
> 
>     buf_desc = conn->sndbuf_desc;
>     conn->sndbuf_desc = NULL;
>     kfree(buf_desc);
> 
> During that window the socket is still hashed and still in SMC_ACTIVE, so
> it passes the new diag guard. alert_token_local is also still set. That
> means __smc_diag_dump() enters the SMC_DIAG_CONNINFO block and evaluates:
> 
>     .sndbuf_size = conn->sndbuf_desc ?
>         conn->sndbuf_desc->len : 0,
> 
> These are plain loads, and only the hash read_lock is held. Could this
> dereference NULL, or read ->len from an smc_buf_desc that has been kfree'd?
> The lock_sock() in __smc_lgr_terminate() does not exclude the diag reader.
> 
> Would unhashing at the start of smc_conn_kill(), before smcd_buf_detach(),
> close this as well?

You are right. In smc_conn_kill(), smcd_buf_detach() NULLs and frees
conn->sndbuf_desc before smc_close_active_abort() is called, but the
socket is still hashed during that window. The v5 unhash inside
smc_close_active_abort() is too late.

Fix in v6: move the unhash to the top of smc_conn_kill(), before
smcd_buf_detach(). Both call sites (__smc_lgr_terminate() and
smc_conn_abort_work()) hold lock_sock and have no expectation that the
socket remains hashed after smc_conn_kill() returns. smc_unhash_sk() is
idempotent so the subsequent unhash attempts further down become safe
no-ops.

> 
>>  	if (sk->sk_state != SMC_INIT && smc->clcsock && smc->clcsock->sk) {
>>  		sk->sk_err = ECONNABORTED;
>>  		if (smc->clcsock && smc->clcsock->sk)
> 
> [ ... ]
> 
>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>> index bf0beaa23bdb6..fe32091e37dab 100644
>> --- a/net/smc/smc_diag.c
>> +++ b/net/smc/smc_diag.c
>> @@ -103,6 +103,9 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>>  	if (nla_put(skb, SMC_DIAG_FALLBACK, sizeof(fallback), &fallback) < 0)
>>  		goto errout;
>>  
>> +	if (sk->sk_state == SMC_INIT || sk->sk_state == SMC_CLOSED)
>> +		goto out;
>> +
> 
> [Severity: High]
> Does this guard cover the lgr dereference earlier in __smc_diag_dump() that
> sets r->diag_mode?
> 
> net/smc/smc_diag.c:__smc_diag_dump() {
>     ...
>     if (smc->use_fallback)
>         r->diag_mode = SMC_DIAG_MODE_FALLBACK_TCP;
>     else if (smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd)
>         r->diag_mode = SMC_DIAG_MODE_SMCD;
>     ...
> }
> 
> That load happens before the new SMC_INIT/SMC_CLOSED check, and only the
> smc hash read_lock is held.
> 
> The commit message says this guard handles the smc_conn_abort() SMC_INIT
> case. Take a handshake failure such as smc_listen_decline() ->
> smc_conn_abort(new_smc, local_first). The socket is still hashed in
> SMC_INIT and use_fallback has not been set yet:
> 
> net/smc/af_smc.c:smc_conn_abort() {
>     ...
>     smc_conn_free(conn);
>     if (local_first && lgr_valid)
>         smc_lgr_cleanup_early(lgr);
> }
> 
> The following chain then frees the lgr synchronously:
> 
> smc_lgr_cleanup_early()
>   __smc_lgr_terminate()
>     smc_lgr_free()
>       smc_lgr_put()
>         __smc_lgr_free()
>           kfree(lgr)
> 
> Nothing on this path takes the hash write_lock, and conn->lgr is never
> cleared. smc_conn_lgr_valid() is only plain loads of conn->lgr and
> conn->alert_token_local.
> 
> Can a diag reader pass smc_conn_lgr_valid() just before the conn is
> unregistered, and then read lgr->is_smcd after the kfree()?
> 
> smc_diag_handler_dump() has no capability check, and a remote peer can
> cause the handshake failure. Would checking the state (or use_fallback)
> before any conn.lgr access close this?

You are right. The conn->lgr->is_smcd load in the r->diag_mode
assignment happens before the SMC_INIT guard, leaving a window where a
concurrent smc_lgr_cleanup_early() → kfree(lgr) on another CPU can race
with the reader. This is remotely triggerable since
smc_diag_handler_dump() has no capability check and a remote peer can
cause the handshake failure that leads to smc_conn_abort().

Fix in v6: inline an sk->sk_state != SMC_INIT check directly into the
else if condition, so conn->lgr->is_smcd is never loaded for SMC_INIT
sockets. The SMC_CLOSED check is not needed at that line — on every path
that sets SMC_CLOSED, either the socket is unhashed before
smc_conn_free() runs (__smc_release(), passive work), or conn->lgr is
still live and the dereference is safe.

> 
>>  	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>>  	    smc->conn.alert_token_local) {
>>  		struct smc_connection *conn = &smc->conn;
> 
> [Severity: High]
> Sockets in SMC_ACTIVE and the closing states still get past this guard. Is
> the SMC_DIAG_LGRINFO block further down still exposed to a concurrent link
> switch?
> 
> net/smc/smc_diag.c:__smc_diag_dump() {
>     ...
>     struct smc_link *link = smc->conn.lnk;
>     ...
>     memcpy(linfo.lnk[0].ibname, link->smcibdev->ibdev->name,
>            sizeof(link->smcibdev->ibdev->name));
>     ...
> }
> 
> Only the hash read_lock is held here. During SMC-R link failover,
> smcr_link_down() -> smc_switch_conns() -> smc_switch_link_and_count()
> replaces conn->lnk under conn->send_lock and drops the conn's reference on
> the old link:
> 
> net/smc/smc_core.c:smc_switch_link_and_count() {
>     ...
>     smcr_link_put(conn->lnk);
>     conn->lnk = to_lnk;
>     ...
> }
> 
> When smcr_link_clear() drops the last reference, __smcr_link_clear() calls
> put_device() on the ibdev and then memset(lnk, 0). The lgr stays on its

After tracing the refcount accounting I believe this race is not
reachable. The smcr_link_put() in smc_switch_link_and_count() only drops
the per-connection hold taken by smc_conn_create(). The link's base ref
(initialised to 1 in smcr_link_init()) is only released by
smcr_link_clear(), which is called exclusively from the lgr termination
path. Hence __smcr_link_clear() is not called in this path.

By that point the lgr has already been removed from the global list, so
the diag reader's !list_empty(&smc->conn.lgr->list) check already gates
it out of the lgrinfo block.

Will post v6 shortly.

pw-bot: cr