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

[PATCH net v7] 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. Three paths expose a window where a hashed socket has
freed or partially freed conn/lgr/lnk state.

First, smc_conn_abort() calls smc_conn_free() during early handshake aborts
while the socket is still hashed in SMC_INIT. On the smc_lgr_cleanup_early()
path this frees the lgr synchronously without holding the hash write_lock. A
concurrent diag reader can therefore load a non-NULL conn->lgr and dereference
lgr->is_smcd after the kfree. Guard the conn->lgr->is_smcd load in
__smc_diag_dump() by skipping it when the socket is in SMC_INIT state. The same
guard also skips the CONNINFO/LGRINFO/DMBINFO blocks for SMC_CLOSED sockets,
which may briefly remain hashed after smc_conn_free() when the fd is still open;
smcss already suppresses those attributes for closed sockets, so there is no
observability regression.

Second, in smc_conn_kill() on SMC-D with dmb_nocopy, smcd_buf_detach() NULLs and
frees conn->sndbuf_desc before the socket is unhashed. A concurrent diag reader
entering the CONNINFO block can dereference the freed descriptor. Unhash the
socket at the top of smc_conn_kill(), before smcd_buf_detach().

Third, in the terminal teardown branches of smc_close_active_abort()
(PEERCLOSEWAIT and PROCESSABORT groups) and in smc_close_passive_work(),
smc_conn_free() drops lgr and lnk references while the socket is still hashed.
Unhash immediately before each smc_conn_free() call at those two sites.

Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported")
Cc: stable@vger.kernel.org
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
Changes in v7:
- no code changes; commit message only
- document the SMC_CLOSED leg of the early-exit guard and the rationale
  that smcss already skips CLOSED sockets on the userspace side; add
  Fixes: ae2be35cbed2 for the smc_conn_kill() hunk since
  smcd_buf_detach() was introduced by that commit

Changes in v6:
- smc_diag.c: inline sk->sk_state != SMC_INIT into the else-if condition
  so conn->lgr->is_smcd is never loaded for SMC_INIT sockets; retain the
  SMC_INIT || SMC_CLOSED early-exit guard and out: label from v5 to
  defensively skip the CONNINFO/LGRINFO/DMBINFO blocks for sockets with
  no live connection
- smc_close.c: remove the unconditional unhash from the top of
  smc_close_active_abort(); place it scoped immediately before each of
  the two smc_conn_free() calls in the PEERCLOSEWAIT and PROCESSABORT
  branches, and before smc_conn_free() in smc_close_passive_work();
  this avoids prematurely hiding live SMC_ACTIVE/APPCLOSEWAIT sockets
  from smcss
- smc_core.c: unhash at the top of smc_conn_kill(), before
  smcd_buf_detach(), so the socket is off the hash table before
  conn->sndbuf_desc is freed on the dmb_nocopy path

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 | 3 +++
 net/smc/smc_core.c  | 1 +
 net/smc/smc_diag.c  | 7 ++++++-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
index bb0313ef5f7c..aa6363abdd74 100644
--- a/net/smc/smc_close.c
+++ b/net/smc/smc_close.c
@@ -154,6 +154,7 @@ void smc_close_active_abort(struct smc_sock *smc)
 		if (sk->sk_state != SMC_PEERABORTWAIT)
 			break;
 		sk->sk_state = SMC_CLOSED;
+		sk->sk_prot->unhash(sk);
 		smc_conn_free(&smc->conn);
 		release_clcsock = true;
 		sock_put(sk); /* passive closing */
@@ -165,6 +166,7 @@ void smc_close_active_abort(struct smc_sock *smc)
 		if (sk->sk_state != SMC_PEERABORTWAIT)
 			break;
 		sk->sk_state = SMC_CLOSED;
+		sk->sk_prot->unhash(sk);
 		smc_conn_free(&smc->conn);
 		release_clcsock = true;
 		break;
@@ -433,6 +435,7 @@ 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)) {
+			sk->sk_prot->unhash(sk);
 			smc_conn_free(conn);
 			if (smc->clcsock)
 				release_clcsock = true;
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 04aedd957543..d7bd6d7f428b 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1517,6 +1517,7 @@ static void smc_conn_kill(struct smc_connection *conn, bool soft)
 {
 	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);

+	smc->sk.sk_prot->unhash(&smc->sk);
 	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
 		conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
 	else
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index bf0beaa23bdb..b29bd34acf7b 100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -90,7 +90,8 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
 	r->diag_state = sk->sk_state;
 	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)
+	else if (sk->sk_state != SMC_INIT &&
+		 smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd)
 		r->diag_mode = SMC_DIAG_MODE_SMCD;
 	else
 		r->diag_mode = SMC_DIAG_MODE_SMCR;
@@ -103,6 +104,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;
+
 	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
 	    smc->conn.alert_token_local) {
 		struct smc_connection *conn = &smc->conn;
@@ -185,6 +189,7 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
 			goto errout;
 	}

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

-- 
2.50.1 (Apple Git-155)

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v7] 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. Three paths expose a window where a hashed socket has
> freed or partially freed conn/lgr/lnk state.
> 
> First, smc_conn_abort() calls smc_conn_free() during early handshake aborts
> while the socket is still hashed in SMC_INIT. On the smc_lgr_cleanup_early()
> path this frees the lgr synchronously without holding the hash write_lock. A
> concurrent diag reader can therefore load a non-NULL conn->lgr and dereference
> lgr->is_smcd after the kfree. Guard the conn->lgr->is_smcd load in
> __smc_diag_dump() by skipping it when the socket is in SMC_INIT state. The same
> guard also skips the CONNINFO/LGRINFO/DMBINFO blocks for SMC_CLOSED sockets,
> which may briefly remain hashed after smc_conn_free() when the fd is still open;
> smcss already suppresses those attributes for closed sockets, so there is no
> observability regression.
> 
> Second, in smc_conn_kill() on SMC-D with dmb_nocopy, smcd_buf_detach() NULLs and
> frees conn->sndbuf_desc before the socket is unhashed. A concurrent diag reader
> entering the CONNINFO block can dereference the freed descriptor. Unhash the
> socket at the top of smc_conn_kill(), before smcd_buf_detach().
> 
> Third, in the terminal teardown branches of smc_close_active_abort()
> (PEERCLOSEWAIT and PROCESSABORT groups) and in smc_close_passive_work(),
> smc_conn_free() drops lgr and lnk references while the socket is still hashed.
> Unhash immediately before each smc_conn_free() call at those two sites.
> 
> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
> Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
> Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported")
> Cc: stable@vger.kernel.org
> 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/20260930073029.1201202-1-mjambigi@linux.ibm.com?part=1

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

[Mahanta Jambigi]

On 30/09/26 1:03 pm, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

The issue was found by manual code inspection while reviewing the diag
dump path in net/smc/smc_diag.c.

> 
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

The races are theoretical — none of the three windows were triggered in
practice. The smc_conn_kill() / smcd_buf_detach() race requires an
active smcss poll to coincide with an SMC-D lgr termination on the
dmb_nocopy path; the smc_close_active_abort() and
smc_close_passive_work() races require a concurrent diag dump during the
narrow teardown window between sk_state = SMC_CLOSED and
smc_conn_free(). No stack trace or error message is available since the
bugs were not hit in a running system.

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

[Hidayath Khan]

On 30/09/26 1:00 pm, Mahanta Jambigi wrote:
> The diag dump walks the socket hash table under a read_lock and dereferences
> conn->lgr and conn->lnk. Three paths expose a window where a hashed socket has
> freed or partially freed conn/lgr/lnk state.
>
> First, smc_conn_abort() calls smc_conn_free() during early handshake aborts
> while the socket is still hashed in SMC_INIT. On the smc_lgr_cleanup_early()
> path this frees the lgr synchronously without holding the hash write_lock. A
> concurrent diag reader can therefore load a non-NULL conn->lgr and dereference
> lgr->is_smcd after the kfree. Guard the conn->lgr->is_smcd load in
> __smc_diag_dump() by skipping it when the socket is in SMC_INIT state. The same
> guard also skips the CONNINFO/LGRINFO/DMBINFO blocks for SMC_CLOSED sockets,
> which may briefly remain hashed after smc_conn_free() when the fd is still open;
> smcss already suppresses those attributes for closed sockets, so there is no
> observability regression.
>
> Second, in smc_conn_kill() on SMC-D with dmb_nocopy, smcd_buf_detach() NULLs and
> frees conn->sndbuf_desc before the socket is unhashed. A concurrent diag reader
> entering the CONNINFO block can dereference the freed descriptor. Unhash the
> socket at the top of smc_conn_kill(), before smcd_buf_detach().
>
> Third, in the terminal teardown branches of smc_close_active_abort()
> (PEERCLOSEWAIT and PROCESSABORT groups) and in smc_close_passive_work(),
> smc_conn_free() drops lgr and lnk references while the socket is still hashed.
> Unhash immediately before each smc_conn_free() call at those two sites.
>
> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
> Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
> Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
> ---
> Changes in v7:
> - no code changes; commit message only
> - document the SMC_CLOSED leg of the early-exit guard and the rationale
>    that smcss already skips CLOSED sockets on the userspace side; add
>    Fixes: ae2be35cbed2 for the smc_conn_kill() hunk since
>    smcd_buf_detach() was introduced by that commit
>
> Changes in v6:
> - smc_diag.c: inline sk->sk_state != SMC_INIT into the else-if condition
>    so conn->lgr->is_smcd is never loaded for SMC_INIT sockets; retain the
>    SMC_INIT || SMC_CLOSED early-exit guard and out: label from v5 to
>    defensively skip the CONNINFO/LGRINFO/DMBINFO blocks for sockets with
>    no live connection
> - smc_close.c: remove the unconditional unhash from the top of
>    smc_close_active_abort(); place it scoped immediately before each of
>    the two smc_conn_free() calls in the PEERCLOSEWAIT and PROCESSABORT
>    branches, and before smc_conn_free() in smc_close_passive_work();
>    this avoids prematurely hiding live SMC_ACTIVE/APPCLOSEWAIT sockets
>    from smcss
> - smc_core.c: unhash at the top of smc_conn_kill(), before
>    smcd_buf_detach(), so the socket is off the hash table before
>    conn->sndbuf_desc is freed on the dmb_nocopy path
>
> 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 | 3 +++
>   net/smc/smc_core.c  | 1 +
>   net/smc/smc_diag.c  | 7 ++++++-
>   3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
> index bb0313ef5f7c..aa6363abdd74 100644
> --- a/net/smc/smc_close.c
> +++ b/net/smc/smc_close.c
> @@ -154,6 +154,7 @@ void smc_close_active_abort(struct smc_sock *smc)
>   		if (sk->sk_state != SMC_PEERABORTWAIT)
>   			break;
>   		sk->sk_state = SMC_CLOSED;
> +		sk->sk_prot->unhash(sk);
>   		smc_conn_free(&smc->conn);
>   		release_clcsock = true;
>   		sock_put(sk); /* passive closing */
> @@ -165,6 +166,7 @@ void smc_close_active_abort(struct smc_sock *smc)
>   		if (sk->sk_state != SMC_PEERABORTWAIT)
>   			break;
>   		sk->sk_state = SMC_CLOSED;
> +		sk->sk_prot->unhash(sk);
>   		smc_conn_free(&smc->conn);
>   		release_clcsock = true;
>   		break;
> @@ -433,6 +435,7 @@ 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)) {
> +			sk->sk_prot->unhash(sk);
>   			smc_conn_free(conn);
>   			if (smc->clcsock)
>   				release_clcsock = true;
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 04aedd957543..d7bd6d7f428b 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1517,6 +1517,7 @@ static void smc_conn_kill(struct smc_connection *conn, bool soft)
>   {
>   	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);
>
> +	smc->sk.sk_prot->unhash(&smc->sk);
>   	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
>   		conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
>   	else
> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb..b29bd34acf7b 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
> @@ -90,7 +90,8 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>   	r->diag_state = sk->sk_state;
>   	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)
> +	else if (sk->sk_state != SMC_INIT &&
> +		 smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd)
>   		r->diag_mode = SMC_DIAG_MODE_SMCD;
>   	else
>   		r->diag_mode = SMC_DIAG_MODE_SMCR;
> @@ -103,6 +104,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;
> +
>   	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>   	    smc->conn.alert_token_local) {
>   		struct smc_connection *conn = &smc->conn;
> @@ -185,6 +189,7 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>   			goto errout;
>   	}
>
> +out:
>   	nlmsg_end(skb, nlh);
>   	return 0;
Reviewed-by: Hidayath Khan <hidayath@linux.ibm.com>
>

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

[Sidraya Jayagond]

On 30/09/26 1:00 pm, Mahanta Jambigi wrote:
> The diag dump walks the socket hash table under a read_lock and dereferences
> conn->lgr and conn->lnk. Three paths expose a window where a hashed socket has
> freed or partially freed conn/lgr/lnk state.
> 
> First, smc_conn_abort() calls smc_conn_free() during early handshake aborts
> while the socket is still hashed in SMC_INIT. On the smc_lgr_cleanup_early()
> path this frees the lgr synchronously without holding the hash write_lock. A
> concurrent diag reader can therefore load a non-NULL conn->lgr and dereference
> lgr->is_smcd after the kfree. Guard the conn->lgr->is_smcd load in
> __smc_diag_dump() by skipping it when the socket is in SMC_INIT state. The same
> guard also skips the CONNINFO/LGRINFO/DMBINFO blocks for SMC_CLOSED sockets,
> which may briefly remain hashed after smc_conn_free() when the fd is still open;
> smcss already suppresses those attributes for closed sockets, so there is no
> observability regression.
> 
> Second, in smc_conn_kill() on SMC-D with dmb_nocopy, smcd_buf_detach() NULLs and
> frees conn->sndbuf_desc before the socket is unhashed. A concurrent diag reader
> entering the CONNINFO block can dereference the freed descriptor. Unhash the
> socket at the top of smc_conn_kill(), before smcd_buf_detach().
> 
> Third, in the terminal teardown branches of smc_close_active_abort()
> (PEERCLOSEWAIT and PROCESSABORT groups) and in smc_close_passive_work(),
> smc_conn_free() drops lgr and lnk references while the socket is still hashed.
> Unhash immediately before each smc_conn_free() call at those two sites.
> 
> Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
> Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
> Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
> ---
> Changes in v7:
> - no code changes; commit message only
> - document the SMC_CLOSED leg of the early-exit guard and the rationale
>   that smcss already skips CLOSED sockets on the userspace side; add
>   Fixes: ae2be35cbed2 for the smc_conn_kill() hunk since
>   smcd_buf_detach() was introduced by that commit
> 
> Changes in v6:
> - smc_diag.c: inline sk->sk_state != SMC_INIT into the else-if condition
>   so conn->lgr->is_smcd is never loaded for SMC_INIT sockets; retain the
>   SMC_INIT || SMC_CLOSED early-exit guard and out: label from v5 to
>   defensively skip the CONNINFO/LGRINFO/DMBINFO blocks for sockets with
>   no live connection
> - smc_close.c: remove the unconditional unhash from the top of
>   smc_close_active_abort(); place it scoped immediately before each of
>   the two smc_conn_free() calls in the PEERCLOSEWAIT and PROCESSABORT
>   branches, and before smc_conn_free() in smc_close_passive_work();
>   this avoids prematurely hiding live SMC_ACTIVE/APPCLOSEWAIT sockets
>   from smcss
> - smc_core.c: unhash at the top of smc_conn_kill(), before
>   smcd_buf_detach(), so the socket is off the hash table before
>   conn->sndbuf_desc is freed on the dmb_nocopy path
> 
> 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 | 3 +++
>  net/smc/smc_core.c  | 1 +
>  net/smc/smc_diag.c  | 7 ++++++-
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
> index bb0313ef5f7c..aa6363abdd74 100644
> --- a/net/smc/smc_close.c
> +++ b/net/smc/smc_close.c
> @@ -154,6 +154,7 @@ void smc_close_active_abort(struct smc_sock *smc)
>  		if (sk->sk_state != SMC_PEERABORTWAIT)
>  			break;
>  		sk->sk_state = SMC_CLOSED;
> +		sk->sk_prot->unhash(sk);
>  		smc_conn_free(&smc->conn);
>  		release_clcsock = true;
>  		sock_put(sk); /* passive closing */
> @@ -165,6 +166,7 @@ void smc_close_active_abort(struct smc_sock *smc)
>  		if (sk->sk_state != SMC_PEERABORTWAIT)
>  			break;
>  		sk->sk_state = SMC_CLOSED;
> +		sk->sk_prot->unhash(sk);
>  		smc_conn_free(&smc->conn);
>  		release_clcsock = true;
>  		break;
> @@ -433,6 +435,7 @@ 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)) {
> +			sk->sk_prot->unhash(sk);
>  			smc_conn_free(conn);
>  			if (smc->clcsock)
>  				release_clcsock = true;
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 04aedd957543..d7bd6d7f428b 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1517,6 +1517,7 @@ static void smc_conn_kill(struct smc_connection *conn, bool soft)
>  {
>  	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);
> 
> +	smc->sk.sk_prot->unhash(&smc->sk);
>  	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
>  		conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
>  	else
> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb..b29bd34acf7b 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
> @@ -90,7 +90,8 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>  	r->diag_state = sk->sk_state;
>  	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)
> +	else if (sk->sk_state != SMC_INIT &&
> +		 smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd)
>  		r->diag_mode = SMC_DIAG_MODE_SMCD;
>  	else
>  		r->diag_mode = SMC_DIAG_MODE_SMCR;
> @@ -103,6 +104,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;
> +
>  	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>  	    smc->conn.alert_token_local) {
>  		struct smc_connection *conn = &smc->conn;
> @@ -185,6 +189,7 @@ 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 v7] net/smc: fix lgr/lnk lifetime vs diag reader race

[netdev-bot+sashiko]

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

Critical: 0 · High: 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] The patch does not fully close the diag-reader vs. link-lifetime
  race it says it fixes.
- [Medium] The new safety gates in __smc_diag_dump() depend on a plain,
  lockless load of sk->sk_state.

--

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

This patch tries to close races between the SMC sock_diag dump and
teardown paths. The dump walks the socket hash under read_lock, while the
teardown paths free lgr, lnk or sndbuf_desc state. The patch adds sk_state
based guards for SMC_INIT and SMC_CLOSED sockets to __smc_diag_dump(), and
unhashes the socket earlier in smc_conn_kill(), smc_close_active_abort()
and smc_close_passive_work().

> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb6..b29bd34acf7b5 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
> @@ -90,7 +90,8 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>  	r->diag_state = sk->sk_state;
>  	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)
> +	else if (sk->sk_state != SMC_INIT &&
> +		 smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd)
>  		r->diag_mode = SMC_DIAG_MODE_SMCD;
>  	else
>  		r->diag_mode = SMC_DIAG_MODE_SMCR;

[Severity: Medium]
Is a plain load of sk->sk_state enough to order the later loads of
conn->alert_token_local, conn->lgr->is_smcd and conn->sndbuf_desc->len?

This check, and the SMC_INIT/SMC_CLOSED guard added below, share no lock
with the writer. Nothing sits between the sk_state load and the conn field
loads: no READ_ONCE(), no smp_load_acquire(), no smp_rmb(). Only a control
dependency links them, and that doesn't order load->load on weakly ordered
CPUs.

On the early abort path the writer runs under lock_sock():

smc_conn_abort()
  smc_conn_free()
    smc_lgr_unregister_conn()    /* alert_token_local = 0 */
  smc_lgr_cleanup_early()
    __smc_lgr_terminate()
      smc_lgr_free()             /* lgr and buffers freed, conn->lgr and
                                    conn->sndbuf_desc left set */
smc_switch_to_fallback()
...
sk->sk_state = SMC_ACTIVE

On arm64, could the reader see sk_state == SMC_ACTIVE together with a
stale non-zero alert_token_local? It would then read lgr->is_smcd or
sndbuf_desc->len from freed memory and copy the value into the netlink
reply.

This can't happen on x86 or s390, which keep load-load order, and the
writer's window is long. The Linux kernel memory model still allows it.

Would READ_ONCE() or smp_load_acquire() on sk_state (or an smp_rmb()
before the conn field loads), paired with release-ordered state changes,
be needed here?

> @@ -103,6 +104,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 leave the conn->lnk replacement during SMC-R link failover still
racing with the diag reader?

For SMC_ACTIVE and closing-state sockets, the LGRINFO block further down
still does this with only read_lock(&smc_hash->lock) held:

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));
    ...
}

The failover side doesn't take the hash lock and doesn't unhash the
socket. For example, when the peer sends DELETE_LINK:

smc_llc_process_cli_delete_link()
  smc_switch_conns(lgr, lnk_del, false)
    smc_switch_link_and_count(conn, to_lnk)
      smcr_link_put(conn->lnk);
      conn->lnk = to_lnk;
  smcr_link_clear(lnk_del, true)
    ...
    __smcr_link_clear()
      put_device(&lnk->smcibdev->ibdev->dev);
      smcibdev = lnk->smcibdev;
      memset(lnk, 0, sizeof(struct smc_link));
      ...
      atomic_dec_return(&smcibdev->lnk_cnt)

A local RNIC or port going down reaches smc_switch_conns() the same way,
through smcr_link_down().

smc_switch_link_and_count() runs only under lgr->conns_lock and
conn->send_lock. The diag reader takes neither lock.

Suppose the reader loaded the old conn->lnk before the switch and was
delayed before the later loads. Could it then dereference a NULL
link->smcibdev after the memset()? Could it instead read a freed smcibdev
or ibdev and copy those bytes to userspace?

An unprivileged SOCK_DIAG dump that asks for SMC_DIAG_LGRINFO (for
example smcss -R) can reach this.

The socket stays hashed in SMC_ACTIVE or a closing state. So neither this
guard nor the !list_empty(&smc->conn.lgr->list) check from 9dbe086c69b8
filters it out.

v2 of this patch protected the swap in smc_switch_link_and_count() with
lgr_lnk_lock, but v3 dropped that. The patch still carries Fixes:
9dbe086c69b8 and is titled "fix lgr/lnk lifetime vs diag reader race".
Should the conn->lnk replacement also be covered, for example with a lock
or RCU pairing on the link swap?

[ ... ]

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

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

[Mahanta Jambigi]

On 04/10/26 1:18 pm, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 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] The patch does not fully close the diag-reader vs. link-lifetime
>   race it says it fixes.
> - [Medium] The new safety gates in __smc_diag_dump() depend on a plain,
>   lockless load of sk->sk_state.
> 
> --
> 
> net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This patch tries to close races between the SMC sock_diag dump and
> teardown paths. The dump walks the socket hash under read_lock, while the
> teardown paths free lgr, lnk or sndbuf_desc state. The patch adds sk_state
> based guards for SMC_INIT and SMC_CLOSED sockets to __smc_diag_dump(), and
> unhashes the socket earlier in smc_conn_kill(), smc_close_active_abort()
> and smc_close_passive_work().
> 
>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>> index bf0beaa23bdb6..b29bd34acf7b5 100644
>> --- a/net/smc/smc_diag.c
>> +++ b/net/smc/smc_diag.c
>> @@ -90,7 +90,8 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>>  	r->diag_state = sk->sk_state;
>>  	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)
>> +	else if (sk->sk_state != SMC_INIT &&
>> +		 smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd)
>>  		r->diag_mode = SMC_DIAG_MODE_SMCD;
>>  	else
>>  		r->diag_mode = SMC_DIAG_MODE_SMCR;
> 
> [Severity: Medium]
> Is a plain load of sk->sk_state enough to order the later loads of
> conn->alert_token_local, conn->lgr->is_smcd and conn->sndbuf_desc->len?
> 
> This check, and the SMC_INIT/SMC_CLOSED guard added below, share no lock
> with the writer. Nothing sits between the sk_state load and the conn field
> loads: no READ_ONCE(), no smp_load_acquire(), no smp_rmb(). Only a control
> dependency links them, and that doesn't order load->load on weakly ordered
> CPUs.
> 
> On the early abort path the writer runs under lock_sock():
> 
> smc_conn_abort()
>   smc_conn_free()
>     smc_lgr_unregister_conn()    /* alert_token_local = 0 */
>   smc_lgr_cleanup_early()
>     __smc_lgr_terminate()
>       smc_lgr_free()             /* lgr and buffers freed, conn->lgr and
>                                     conn->sndbuf_desc left set */
> smc_switch_to_fallback()
> ...
> sk->sk_state = SMC_ACTIVE
> 
> On arm64, could the reader see sk_state == SMC_ACTIVE together with a
> stale non-zero alert_token_local? It would then read lgr->is_smcd or
> sndbuf_desc->len from freed memory and copy the value into the netlink
> reply.
> 
> This can't happen on x86 or s390, which keep load-load order, and the
> writer's window is long. The Linux kernel memory model still allows it.
> 
> Would READ_ONCE() or smp_load_acquire() on sk_state (or an smp_rmb()
> before the conn field loads), paired with release-ordered state changes,
> be needed here?

Agreed. Under the LKMM, a control dependency does not order load-load on
weakly ordered architectures (such as ARM64).

In v8, I will address this with acquire-release ordering:
- In __smc_diag_dump(), I use smp_load_acquire(&sk->sk_state) and reuse
  the acquired state (in r->diag_state) across the subsequent mode/state
  checks.
- In af_smc.c, I paired it with smp_store_release(&sk->sk_state,
SMC_ACTIVE) when transitioning out of SMC_INIT.

> 
>> @@ -103,6 +104,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 leave the conn->lnk replacement during SMC-R link failover still
> racing with the diag reader?
> 
> For SMC_ACTIVE and closing-state sockets, the LGRINFO block further down
> still does this with only read_lock(&smc_hash->lock) held:
> 
> 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));
>     ...
> }
> 
> The failover side doesn't take the hash lock and doesn't unhash the
> socket. For example, when the peer sends DELETE_LINK:
> 
> smc_llc_process_cli_delete_link()
>   smc_switch_conns(lgr, lnk_del, false)
>     smc_switch_link_and_count(conn, to_lnk)
>       smcr_link_put(conn->lnk);
>       conn->lnk = to_lnk;
>   smcr_link_clear(lnk_del, true)
>     ...
>     __smcr_link_clear()
>       put_device(&lnk->smcibdev->ibdev->dev);
>       smcibdev = lnk->smcibdev;
>       memset(lnk, 0, sizeof(struct smc_link));
>       ...
>       atomic_dec_return(&smcibdev->lnk_cnt)
> 
> A local RNIC or port going down reaches smc_switch_conns() the same way,
> through smcr_link_down().
> 
> smc_switch_link_and_count() runs only under lgr->conns_lock and
> conn->send_lock. The diag reader takes neither lock.
> 
> Suppose the reader loaded the old conn->lnk before the switch and was
> delayed before the later loads. Could it then dereference a NULL
> link->smcibdev after the memset()? Could it instead read a freed smcibdev
> or ibdev and copy those bytes to userspace?
> 
> An unprivileged SOCK_DIAG dump that asks for SMC_DIAG_LGRINFO (for
> example smcss -R) can reach this.
> 
> The socket stays hashed in SMC_ACTIVE or a closing state. So neither this
> guard nor the !list_empty(&smc->conn.lgr->list) check from 9dbe086c69b8
> filters it out
Good catch. When smcr_link_clear() drops the final base refcount after
smc_switch_conns(), __smcr_link_clear() can execute memset() and
put_device() while a concurrent diag reader holds a pointer to the old link.

To eliminate the pointer dereference and external device lifetime
dependency entirely, in v8 I switched from chasing
link->smcibdev->ibdev->name to copying directly from the cached
link->ibname (which is embedded in struct smc_link and initialized once
at link creation). Since struct smc_link is embedded in the link group
(pinned while the socket is hashed), this avoids the NULL dereference
and UAF on smcibdev/ibdev without introducing extra locking on the
critical TX path.

I'll post v8 shortly with these 2 changes.

pw-bot: cr