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

[PATCH net v6] 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 then
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.

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")
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
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,5 +189,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 v6] 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 then
> 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.
> 
> 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")
> 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/20260926065023.1629497-1-mjambigi@linux.ibm.com?part=1

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

[netdev-bot+sashiko]

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

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

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 leaves open another window of the class it says it
  closes ('a hashed socket has freed or partially freed conn/lgr/lnk…
- [Medium] The new early exit in __smc_diag_dump()
  (net/smc/smc_diag.c:107-108, `if (sk->sk_state == SMC_INIT ||…
- [Medium] For the smc_conn_abort() path, the patch's only protection
  against dereferencing a freed conn->lgr in __smc_diag_dump() is two…
- [Low] The second race the patch fixes is smcd_buf_detach() freeing
  conn->sndbuf_desc in smc_conn_kill() while the socket is hashed and…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  smc_diag_handler_dump() (net/smc/smc_diag.c:245-260) starts the dump…

--

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

This patch aims to close races between the SMC sock_diag dump and the
teardown of conn/lgr/lnk state. It unhashes the socket before
smc_conn_free() in the close paths and at the top of smc_conn_kill(). It
also makes __smc_diag_dump() skip the lgr->is_smcd load, and all extension
blocks, for SMC_INIT and SMC_CLOSED sockets.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 9974149659c2f..663926465ba7f 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

[Severity: Low]
Should this patch also have a Fixes: tag for ae2be35cbed2?

The commit message says this hunk is needed because smcd_buf_detach()
frees conn->sndbuf_desc in smc_conn_kill() on the dmb_nocopy path:

smcd_buf_detach() {
    ...
	buf_desc = conn->sndbuf_desc;
	conn->sndbuf_desc = NULL;
	kfree(buf_desc);
}

That path was added by ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer
DMB if supported"). It was moved to its current place by 36cdf5d48ca1
("net/smc: unregister the connection before draining the rx tasklet"),
which itself has Fixes: ae2be35cbed2.

Both tags in this patch, f16a7dd5cf27 and 9dbe086c69b8, are older than the
dmb_nocopy detach code. Without a tag for ae2be35cbed2, stable trees that
lack dmb_nocopy would get the unconditional unhash, but the stated reason
for it would not apply there.

> 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 read of sk->sk_state enough to stop this from reading a freed
lgr?

smc_conn_free() never clears conn->lgr. On the smc_conn_abort() path the
writer does the following under lock_sock(), using only plain stores:

smc_conn_abort()
  smc_conn_free()
    smc_lgr_unregister_conn()     /* alert_token_local = 0 */
  smc_lgr_cleanup_early(lgr)
    __smc_lgr_terminate()
      smc_lgr_free()              /* lgrA freed, conn->lgr dangling */

One of two things follows:

- smc_switch_to_fallback() and then sk_state = SMC_ACTIVE, for example in
  smc_connect_fallback().
- In the ISM/RDMA v2 server retry loops, smc_conn_create() sets
  conn->lgr = lgrB with a new token, and then
  newsmcsk->sk_state = SMC_ACTIVE.

The reader here holds only the hash read_lock. There is no READ_ONCE(), and
nothing orders the sk_state load before the later loads of
conn->alert_token_local, conn->lgr and lgr->is_smcd. The control dependency
on the sk_state test does not order those later loads.

On weakly ordered CPUs such as arm64 or POWER, could this see SMC_ACTIVE
with use_fallback still false, together with a stale token or the stale
lgrA pointer, and then read lgrA->is_smcd after it was freed?

This sk_state test and the second one below are separate reads, so within
one dump they can return different values. KCSAN would likely report both
as data races.

> @@ -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: Medium]
Does this drop CONNINFO, LGRINFO and DMBINFO for SMC_CLOSED sockets whose
connection is still registered and valid?

A hashed socket can reach SMC_CLOSED with its connection intact. For
example, after shutdown(SHUT_WR) in SMC_APPCLOSEWAIT1 the socket is in
SMC_APPCLOSEWAIT2, and then the peer aborts:

smc_close_passive_work()
  smc_close_passive_abort_received()
	case SMC_APPCLOSEWAIT2:
	case SMC_PEERFINCLOSEWAIT:
		sk->sk_state = SMC_CLOSED;

The fd is still open, so smc_close_passive_work() skips both the new
unhash and smc_conn_free():

		if ((sk->sk_state == SMC_CLOSED) &&
		    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {
			sk->sk_prot->unhash(sk);
			smc_conn_free(conn);

The socket therefore stays hashed with alert_token_local and conn->lgr
still valid.

With this patch, every caller of smc_conn_free() unhashes first:
smc_close_active_abort(), smc_close_passive_work() and __smc_release(). It
looks like any SMC_CLOSED socket still in the hash has valid connection
state. If so, the SMC_CLOSED half of this check only hides valid data that
smcss -D and smcss -R used to report when the matching diag_ext bits were
set.

The commit message describes only the narrower sk_state != SMC_INIT guard
on the is_smcd load. This early exit appears only in the v5 and v6
changelog below the --- line, which is dropped when the patch is applied.

Could the SMC_CLOSED part be removed, or described in the commit message if
it is intended?

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

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but is
req->diag_ext always inside the request payload here?

smc_diag_handler_dump() starts the dump without checking the request
length:

	if (h->nlmsg_type == SOCK_DIAG_BY_FAMILY &&
	    h->nlmsg_flags & NLM_F_DUMP) {
		{
			struct netlink_dump_control c = {
				.dump = smc_diag_dump,

The generic check in __sock_diag_cmd() only compares against struct
sock_diag_req (2 bytes), not struct smc_diag_req (52 bytes). inet_diag,
unix_diag, packet_diag, netlink_diag, tipc_diag and vsock_diag all check
nlmsg_len(h) against their own request size.

With a 2-byte payload, diag_ext at offset 3 falls in skb tailroom that
netlink_sendmsg() did not initialize. When a live SMC connection exists,
whether CONNINFO, LGRINFO and DMBINFO appear in the reply would reveal bits
of that uninitialized memory to an unprivileged local user. KMSAN would
likely report this as an uninit-value use.

Should smc_diag_handler_dump() reject requests where nlmsg_len(h) is less
than sizeof(struct smc_diag_req)?

[ ... ]

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

[Severity: High]
The commit message says the patch closes windows where "a hashed socket has
freed or partially freed conn/lgr/lnk state". It also cites 9dbe086c69b8
("net/smc: fix invalid link access in dumping SMC-R connections"). Is SMC-R
link failover still exposed to the same race?

The LGRINFO block above this label runs under only the hash read_lock:

	if (smc_conn_lgr_valid(&smc->conn) && !smc->conn.lgr->is_smcd &&
	    (req->diag_ext & (1 << (SMC_DIAG_LGRINFO - 1))) &&
	    !list_empty(&smc->conn.lgr->list)) {
		struct smc_link *link = smc->conn.lnk;
		...
		memcpy(linfo.lnk[0].ibname, link->smcibdev->ibdev->name,
		       sizeof(link->smcibdev->ibdev->name));

At the same time, smcr_link_down() or smc_llc_process_srv_delete_link() can
run:

smc_switch_conns()
  smc_switch_link_and_count()
    smcr_link_put(conn->lnk);
    conn->lnk = to_lnk;
smcr_link_clear(lnk)
  __smcr_link_clear()
    memset(lnk, 0, sizeof(struct smc_link));

The link switch takes only conn->send_lock. The socket stays SMC_ACTIVE and
hashed, and the lgr stays on its list. So neither the new
SMC_INIT/SMC_CLOSED check nor the list_empty() check skips this block.

If a reader has already loaded the old conn->lnk, could it dereference a
NULL link->smcibdev? Could it instead report fields from a different link
that has reused the lgr->lnk[] slot?

The v2 posting protected the conn->lnk swap in smc_switch_link_and_count()
with lgr_lnk_lock. v3 dropped that, and nothing has replaced it.

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

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

[Mahanta Jambigi]

On 30/09/26 6:21 am, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 2 (1) · Low: 1
> 
> 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 leaves open another window of the class it says it
>   closes ('a hashed socket has freed or partially freed conn/lgr/lnk…
> - [Medium] The new early exit in __smc_diag_dump()
>   (net/smc/smc_diag.c:107-108, `if (sk->sk_state == SMC_INIT ||…
> - [Medium] For the smc_conn_abort() path, the patch's only protection
>   against dereferencing a freed conn->lgr in __smc_diag_dump() is two…
> - [Low] The second race the patch fixes is smcd_buf_detach() freeing
>   conn->sndbuf_desc in smc_conn_kill() while the socket is hashed and…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but
>   smc_diag_handler_dump() (net/smc/smc_diag.c:245-260) starts the dump…
> 
> --
> 
> net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This patch aims to close races between the SMC sock_diag dump and the
> teardown of conn/lgr/lnk state. It unhashes the socket before
> smc_conn_free() in the close paths and at the top of smc_conn_kill(). It
> also makes __smc_diag_dump() skip the lgr->is_smcd load, and all extension
> blocks, for SMC_INIT and SMC_CLOSED sockets.
> 
>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>> index 9974149659c2f..663926465ba7f 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
> 
> [Severity: Low]
> Should this patch also have a Fixes: tag for ae2be35cbed2?

You are right! I'll add this in v7.

> 
> The commit message says this hunk is needed because smcd_buf_detach()
> frees conn->sndbuf_desc in smc_conn_kill() on the dmb_nocopy path:
> 
> smcd_buf_detach() {
>     ...
> 	buf_desc = conn->sndbuf_desc;
> 	conn->sndbuf_desc = NULL;
> 	kfree(buf_desc);
> }
> 
> That path was added by ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer
> DMB if supported"). It was moved to its current place by 36cdf5d48ca1
> ("net/smc: unregister the connection before draining the rx tasklet"),
> which itself has Fixes: ae2be35cbed2.
> 
> Both tags in this patch, f16a7dd5cf27 and 9dbe086c69b8, are older than the
> dmb_nocopy detach code. Without a tag for ae2be35cbed2, stable trees that
> lack dmb_nocopy would get the unconditional unhash, but the stated reason
> for it would not apply there.
> 
>> 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 read of sk->sk_state enough to stop this from reading a freed
> lgr?
> 
> smc_conn_free() never clears conn->lgr. On the smc_conn_abort() path the
> writer does the following under lock_sock(), using only plain stores:
> 
> smc_conn_abort()
>   smc_conn_free()
>     smc_lgr_unregister_conn()     /* alert_token_local = 0 */
>   smc_lgr_cleanup_early(lgr)
>     __smc_lgr_terminate()
>       smc_lgr_free()              /* lgrA freed, conn->lgr dangling */
> 
> One of two things follows:
> 
> - smc_switch_to_fallback() and then sk_state = SMC_ACTIVE, for example in
>   smc_connect_fallback().
> - In the ISM/RDMA v2 server retry loops, smc_conn_create() sets
>   conn->lgr = lgrB with a new token, and then
>   newsmcsk->sk_state = SMC_ACTIVE.
> 
> The reader here holds only the hash read_lock. There is no READ_ONCE(), and
> nothing orders the sk_state load before the later loads of
> conn->alert_token_local, conn->lgr and lgr->is_smcd. The control dependency
> on the sk_state test does not order those later loads.
> 
> On weakly ordered CPUs such as arm64 or POWER, could this see SMC_ACTIVE
> with use_fallback still false, together with a stale token or the stale
> lgrA pointer, and then read lgrA->is_smcd after it was freed?

On s390 arch, the hardware memory model provides strong ordering — all
loads are ordered with respect to prior loads, so the speculative load
scenario described in this comment cannot occur there.

For the race to be observable on arm64 or any other weakly ordered arch,
all of the following must be true simultaneously:

1) A diag dump is actively running (smcss or a monitoring tool).
2) A connection is in the narrow window between smc_conn_free() zeroing
alert_token_local and smc_lgr_cleanup_early() completing smc_lgr_free().
This path fires only on early handshake abort, before the CLC handshake
completes — a window measured in microseconds.
3) use_fallback is still false. The use_fallback check at line 91
precedes the sk_state check; if fallback has already been set the reader
never reaches the guarded code.
4) The arm64 CPU speculatively executes the conn->lgr->is_smcd load
before confirming the sk_state != SMC_INIT branch. This requires two
independent speculative loads to both produce stale values: first
conn->lgr (no data dependency on sk_state prevents its speculation), and
then lgr->is_smcd off the stale pointer.

The window requires all four conditions simultaneously, the practical
risk is negligible. Rather than addressing this in isolation here, a
proper audit and READ_ONCE/smp_load_acquire annotation pass across
net/smc/ for weak-ordering correctness would be better handled as a
separate patch.

> 
> This sk_state test and the second one below are separate reads, so within
> one dump they can return different values. KCSAN would likely report both
> as data races.
> 
>> @@ -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: Medium]
> Does this drop CONNINFO, LGRINFO and DMBINFO for SMC_CLOSED sockets whose
> connection is still registered and valid?
> 
> A hashed socket can reach SMC_CLOSED with its connection intact. For
> example, after shutdown(SHUT_WR) in SMC_APPCLOSEWAIT1 the socket is in
> SMC_APPCLOSEWAIT2, and then the peer aborts:
> 
> smc_close_passive_work()
>   smc_close_passive_abort_received()
> 	case SMC_APPCLOSEWAIT2:
> 	case SMC_PEERFINCLOSEWAIT:
> 		sk->sk_state = SMC_CLOSED;
> 
> The fd is still open, so smc_close_passive_work() skips both the new
> unhash and smc_conn_free():
> 
> 		if ((sk->sk_state == SMC_CLOSED) &&
> 		    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {
> 			sk->sk_prot->unhash(sk);
> 			smc_conn_free(conn);
> 
> The socket therefore stays hashed with alert_token_local and conn->lgr
> still valid.
> 
> With this patch, every caller of smc_conn_free() unhashes first:
> smc_close_active_abort(), smc_close_passive_work() and __smc_release(). It
> looks like any SMC_CLOSED socket still in the hash has valid connection
> state. If so, the SMC_CLOSED half of this check only hides valid data that
> smcss -D and smcss -R used to report when the matching diag_ext bits were
> set.
> 
> The commit message describes only the narrower sk_state != SMC_INIT guard
> on the is_smcd load. This early exit appears only in the v5 and v6
> changelog below the --- line, which is dropped when the patch is applied.
> 
> Could the SMC_CLOSED part be removed, or described in the commit message if
> it is intended?

The SMC_CLOSED guard is *intentional* — needed to prevent a UAF where
smc_conn_free() has already run but the socket is still hashed.

There is also no observability regression: smcss (from the separate
smc-tools userspace repository, not part of this kernel patch) already
does goto newline on diag_state == CLOSED and never reads CONNINFO,
LGRINFO or DMBINFO for closed sockets regardless of what the kernel
sends. The rationale is currently only in the v6 changelog below ---
which gets stripped on apply. Will add it to the commit message body in v7.

> 
>>  	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>>  	    smc->conn.alert_token_local) {
>>  		struct smc_connection *conn = &smc->conn;
> 
> [Severity: Medium]
> This is a pre-existing issue and was not introduced by this patch, but is
> req->diag_ext always inside the request payload here?> > smc_diag_handler_dump() starts the dump without checking the request
> length:
> 
> 	if (h->nlmsg_type == SOCK_DIAG_BY_FAMILY &&
> 	    h->nlmsg_flags & NLM_F_DUMP) {
> 		{
> 			struct netlink_dump_control c = {
> 				.dump = smc_diag_dump,
> 
> The generic check in __sock_diag_cmd() only compares against struct
> sock_diag_req (2 bytes), not struct smc_diag_req (52 bytes). inet_diag,
> unix_diag, packet_diag, netlink_diag, tipc_diag and vsock_diag all check
> nlmsg_len(h) against their own request size.
> 
> With a 2-byte payload, diag_ext at offset 3 falls in skb tailroom that
> netlink_sendmsg() did not initialize. When a live SMC connection exists,
> whether CONNINFO, LGRINFO and DMBINFO appear in the reply would reveal bits
> of that uninitialized memory to an unprivileged local user. KMSAN would
> likely report this as an uninit-value use.
> 
> Should smc_diag_handler_dump() reject requests where nlmsg_len(h) is less
> than sizeof(struct smc_diag_req)?

You are right! I'll address this issue via a seperate patch as its a
pre-existing issue.

> 
> [ ... ]
> 
>> @@ -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;
>>  
> 
> [Severity: High]
> The commit message says the patch closes windows where "a hashed socket has
> freed or partially freed conn/lgr/lnk state". It also cites 9dbe086c69b8
> ("net/smc: fix invalid link access in dumping SMC-R connections"). Is SMC-R
> link failover still exposed to the same race?
> 
> The LGRINFO block above this label runs under only the hash read_lock:
> 
> 	if (smc_conn_lgr_valid(&smc->conn) && !smc->conn.lgr->is_smcd &&
> 	    (req->diag_ext & (1 << (SMC_DIAG_LGRINFO - 1))) &&
> 	    !list_empty(&smc->conn.lgr->list)) {
> 		struct smc_link *link = smc->conn.lnk;
> 		...
> 		memcpy(linfo.lnk[0].ibname, link->smcibdev->ibdev->name,
> 		       sizeof(link->smcibdev->ibdev->name));
> 
> At the same time, smcr_link_down() or smc_llc_process_srv_delete_link() can
> run:
> 
> smc_switch_conns()
>   smc_switch_link_and_count()
>     smcr_link_put(conn->lnk);
>     conn->lnk = to_lnk;
> smcr_link_clear(lnk)
>   __smcr_link_clear()
>     memset(lnk, 0, sizeof(struct smc_link));
> 
> The link switch takes only conn->send_lock. The socket stays SMC_ACTIVE and
> hashed, and the lgr stays on its list. So neither the new
> SMC_INIT/SMC_CLOSED check nor the list_empty() check skips this block.
> 
> If a reader has already loaded the old conn->lnk, could it dereference a
> NULL link->smcibdev? Could it instead report fields from a different link
> that has reused the lgr->lnk[] slot?

The smcr_link_put() in smc_switch_link_and_count() drops only the
per-connection hold taken by smc_conn_create(). The link's base refcount
(initialised to 1 in smcr_link_init()) is only released by the
smcr_link_put() at the end of smcr_link_clear(). Therefore
__smcr_link_clear() and its memset(lnk, 0) are never triggered from the
smc_switch_conns() path, and the race described in this comment is *not*
reachable.

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

[Mahanta Jambigi]

On 30/09/26 6:21 am, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 2 (1) · Low: 1
> 
> 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 leaves open another window of the class it says it
>   closes ('a hashed socket has freed or partially freed conn/lgr/lnk…
> - [Medium] The new early exit in __smc_diag_dump()
>   (net/smc/smc_diag.c:107-108, `if (sk->sk_state == SMC_INIT ||…
> - [Medium] For the smc_conn_abort() path, the patch's only protection
>   against dereferencing a freed conn->lgr in __smc_diag_dump() is two…
> - [Low] The second race the patch fixes is smcd_buf_detach() freeing
>   conn->sndbuf_desc in smc_conn_kill() while the socket is hashed and…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but
>   smc_diag_handler_dump() (net/smc/smc_diag.c:245-260) starts the dump…

I'll address the applicable issues in v7. I missed the following line in
my previous reply:

pw-bot: cr