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

[PATCH net v14] 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. Four 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, synchronously
freeing the lgr on the smc_lgr_cleanup_early() path without holding the
hash write_lock. A concurrent diag reader can load a non-NULL conn->lgr
and dereference lgr->is_smcd after the kfree. Skip the connection, link
group and DMB attributes in __smc_diag_dump() for sockets in SMC_INIT or
SMC_CLOSED, and for sockets that have fallen back to TCP, and do not
look at conn->lgr to determine diag_mode in SMC_INIT or SMC_CLOSED.

This changes the sock_diag output for such sockets. SMC_DIAG_CONNINFO,
SMC_DIAG_LGRINFO and SMC_DIAG_DMBINFO are not read while a socket is in
SMC_INIT or SMC_CLOSED or has fallen back to TCP, and diag_mode is
SMC_DIAG_MODE_SMCR for a non-fallback SMC-D socket in SMC_INIT or
SMC_CLOSED. smcss does not use these attributes for such sockets. A
fallback socket is a plain TCP socket with no SMC connection state to
report; the stale conn fields left behind by smc_conn_abort() must not
be read. SMC_DIAG_FALLBACK is still reported. Use smp_load_acquire() /
smp_store_release() on sk_state to order these checks on weakly ordered
CPUs.

Second, in smc_conn_kill() on SMC-D with dmb_nocopy, smcd_buf_detach()
NULLs and frees conn->sndbuf_desc while the socket is still hashed. A
concurrent diag reader entering the CONNINFO block can dereference the
freed descriptor. Unhash the socket right before smcd_buf_detach(). As a
result, killed ACTIVE and APPCLOSEWAIT sockets that are still open are
no longer visible to smcss on loopback-ism devices.

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. Sockets killed in the PEERCLOSEWAIT1/2, PEERFINCLOSEWAIT,
PROCESSABORT and APPFINCLOSEWAIT states are therefore unhashed, and
their inuse accounting dropped, before close(), even if the fd is still
open. Whether a killed socket remains visible to smcss thus depends on
its close state and, for SMC-D, on the device type.

Fourth, smc_listen_out_err() sets sk_state = SMC_CLOSED with a plain
store after smc_conn_abort() -> smc_conn_free() has already cleared
alert_token_local under write_lock_bh(). On weakly ordered CPUs a
concurrent diag reader using smp_load_acquire() on sk_state can observe
SMC_CLOSED while the preceding store to alert_token_local has not yet
become visible, causing it to dereference an lgr whose reference was
already dropped. Use smp_store_release() to order the alert_token_local
clear before the sk_state = SMC_CLOSED store. If smc_conn_free() skipped
the unregister because the link group was already unlinked by
smc_lgr_terminate_sched(), alert_token_local is still set; the
SMC_CLOSED check in __smc_diag_dump() keeps the reader from
dereferencing conn->lgr in that case.

Additionally, in __smc_diag_dump(), use the cached link->ibname instead
of link->smcibdev->ibdev->name to avoid a NULL pointer dereference or a
use-after-free of the IB device if __smcr_link_clear() zeroes the link
and the device is removed during concurrent SMC-R link failover. The
link data is still read without conns_lock or send_lock, so a dump
racing with a link failover may transiently report zeroed or mixed link
fields (ibname, ibport, link_id, GIDs). This does not affect memory
safety and the next dump reports the new link. Like the SMC netlink link
dump and the tracepoints, this reports the name at link creation time
and does not follow a later ib_device_rename().

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 v14:
- smc_diag.c: also exclude SMC_CLOSED before dereferencing conn->lgr to
  determine diag_mode; a server socket that fails in
  smc_listen_decline() with rc < 0 reaches SMC_CLOSED hashed, with a
  stale alert_token_local left by smc_conn_free() when
  smc_lgr_terminate_sched() had already emptied lgr->list, and the lgr
  can be freed by terminate_work while a diag reader still holds the
  hash read_lock
- commit message: document the diag_mode change for SMC_CLOSED and the
  limits of the smp_store_release() in smc_listen_out_err()

Changes in v13:
- smc_diag.c: also skip the CONNINFO/LGRINFO/DMBINFO blocks for sockets
  with use_fallback set; the SMC_DIAG_FALLBACK attribute is still
  reported. After smc_lgr_terminate_sched() emptied lgr->list,
  smc_conn_free() skips smc_lgr_unregister_conn(), so a socket that then
  falls back and reaches SMC_ACTIVE can keep a stale alert_token_local
  and a diag reader could dereference the freed sndbuf/rmb/lgr; a
  fallback socket is a plain TCP socket with no SMC connection state to
  report
- smc_core.c: drop the v11 and v12 changes (unconditional
  smc_lgr_unregister_conn() in smc_conn_free() and the
  !lgr->terminating check in smc_lgr_schedule_free_work()); the v12
  check was racy against __smc_lgr_terminate() and the changes moved the
  patch away from the diag reader race, so smc_core.c is back to the v10
  change only; the Fixes tag for 2a0674fffb6b added in v11 is dropped
  with them
- commit message: state that using the cached link->ibname avoids the
  NULL pointer dereference and the IB device use-after-free, and that
  link fields may be transiently zeroed or mixed in a dump racing with a
  link failover, without affecting memory safety
- commit message: drop the claim that other kill paths leave the socket
  hashed; every smc_conn_kill() ends in smc_close_active_abort(), so
  sockets killed in the PEERCLOSEWAIT1/2, PEERFINCLOSEWAIT, PROCESSABORT
  and APPFINCLOSEWAIT states are unhashed before close() even if the fd
  is still open, and the dmb_nocopy unhash hides ACTIVE/APPCLOSEWAIT
  sockets on loopback-ism; smcss visibility thus depends on close state
  and device type; this supersedes the v10 entry below

Changes in v12:
- smc_core.c: guard smc_lgr_schedule_free_work() with !lgr->terminating
  in addition to !lgr->freeing, preventing free_work from being re-armed
  on a terminating LGR after smc_lgr_cleanup_early() has called
  __smc_lgr_terminate() (which sets terminating=1 and cancels free_work
  without setting freeing=1); without this a concurrent smc_conn_free()
  decrementing conns_num to 0 would re-arm free_work, and the subsequent
  kfree(lgr) in smc_lgr_free() would leave the armed work pointing at
  freed memory

Changes in v11:
- smc_core.c: unregister connection unconditionally in smc_conn_free()
  rather than gating it on !list_empty(&lgr->list), ensuring
  alert_token_local is always reset to 0
- commit message: add Fixes tag for 2a0674fffb6b and clarify that
  copying cached link->ibname eliminates the NULL dereference / UAF
  during concurrent link clearing

Changes in v10:
- smc_core.c: move the unhash out of the top of smc_conn_kill() into the
  smc_ism_support_dmb_nocopy() branch, right before smcd_buf_detach(); the
  other kill paths leave ACTIVE/APPCLOSEWAIT sockets open, so they must
  stay visible to smcss and keep their inuse accounting
- commit message: document the sock_diag output changes for sockets in
  SMC_INIT and SMC_CLOSED (extension attributes omitted, diag_mode of
  SMC-D in SMC_INIT) and that ibname is the name cached at link creation

Changes in v9:
- af_smc.c: change plain store in smc_listen_out_err() to
  smp_store_release(&newsmcsk->sk_state, SMC_CLOSED) to order the
  preceding alert_token_local clear (done under write_lock_bh() inside
  smc_conn_free()) before the sk_state write; pairs with the
  smp_load_acquire() in __smc_diag_dump() to close a use-after-free on
  weakly ordered CPUs where a diag reader could observe SMC_CLOSED while
  conn->lgr still appeared live

Changes in v8:
- smc_diag.c: use smp_load_acquire(&sk->sk_state) to prevent
  speculative load reordering on weakly ordered CPUs (such as arm64)
  during handshake abort fallback
- af_smc.c: pair with smp_store_release(&sk->sk_state, SMC_ACTIVE)
  when transitioning out of SMC_INIT
- smc_diag.c: copy cached link->ibname instead of chasing
  link->smcibdev->ibdev->name to prevent NULL pointer dereference / UAF
  if __smcr_link_clear() zeroes the link during link failover

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
  a Fixes tag for 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 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

Changes in v2:
- 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


 net/smc/af_smc.c    | 10 +++++-----
 net/smc/smc_close.c |  3 +++
 net/smc/smc_core.c  |  4 +++-
 net/smc/smc_diag.c  | 13 +++++++++----
 4 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index XXXXXXX..XXXXXXX 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -976,7 +976,7 @@ static int smc_connect_fallback(struct smc_sock *smc, int reason_code)
 	smc_copy_sock_settings_to_clc(smc);
 	smc->connect_nonblock = 0;
 	if (smc->sk.sk_state == SMC_INIT)
-		smc->sk.sk_state = SMC_ACTIVE;
+		smp_store_release(&smc->sk.sk_state, SMC_ACTIVE);
 	return 0;
 }

@@ -1381,7 +1381,7 @@ static int smc_connect_rdma(struct smc_sock *smc,
 	smc_copy_sock_settings_to_clc(smc);
 	smc->connect_nonblock = 0;
 	if (smc->sk.sk_state == SMC_INIT)
-		smc->sk.sk_state = SMC_ACTIVE;
+		smp_store_release(&smc->sk.sk_state, SMC_ACTIVE);

 	return 0;
 connect_abort:
@@ -1484,7 +1484,7 @@ static int smc_connect_ism(struct smc_sock *smc,
 	smc_copy_sock_settings_to_clc(smc);
 	smc->connect_nonblock = 0;
 	if (smc->sk.sk_state == SMC_INIT)
-		smc->sk.sk_state = SMC_ACTIVE;
+		smp_store_release(&smc->sk.sk_state, SMC_ACTIVE);

 	return 0;
 connect_abort:
@@ -1951,7 +1951,7 @@ static void smc_listen_out_connected(struct smc_sock *new_smc)
 	struct sock *newsmcsk = &new_smc->sk;

 	if (newsmcsk->sk_state == SMC_INIT)
-		newsmcsk->sk_state = SMC_ACTIVE;
+		smp_store_release(&newsmcsk->sk_state, SMC_ACTIVE);

 	smc_listen_out(new_smc);
 }
@@ -1965,7 +1965,7 @@ static void smc_listen_out_err(struct smc_sock *new_smc)
 	this_cpu_inc(net->smc.smc_stats->srv_hshake_err_cnt);
 	if (newsmcsk->sk_state == SMC_INIT)
 		sock_put(&new_smc->sk); /* passive closing */
-	newsmcsk->sk_state = SMC_CLOSED;
+	smp_store_release(&newsmcsk->sk_state, SMC_CLOSED);

 	smc_listen_out(new_smc);
 }
diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
index XXXXXXX..XXXXXXX 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 @@ wakeup:
 		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 XXXXXXX..XXXXXXX 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1530,8 +1530,10 @@ static void smc_conn_kill(struct smc_connection *conn, bool soft)
 			tasklet_kill(&conn->rx_tsklet);
 		else
 			tasklet_unlock_wait(&conn->rx_tsklet);
-		if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
+		if (smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
+			smc->sk.sk_prot->unhash(&smc->sk);
 			smcd_buf_detach(conn);
+		}
 	} else {
 		smc_cdc_wait_pend_tx_wr(conn);
 	}
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index XXXXXXX..XXXXXXX 100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -87,10 +87,11 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,

 	r = nlmsg_data(nlh);
 	smc_diag_msg_common_fill(r, sk);
-	r->diag_state = sk->sk_state;
+	r->diag_state = smp_load_acquire(&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 (r->diag_state != SMC_INIT && r->diag_state != SMC_CLOSED &&
+		 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,10 @@ 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 (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED ||
+	    smc->use_fallback)
+		goto out;
+
 	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
 	    smc->conn.alert_token_local) {
 		struct smc_connection *conn = &smc->conn;
@@ -154,8 +159,7 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
 			.lnk[0].link_id = link->link_id,
 		};

-		memcpy(linfo.lnk[0].ibname, link->smcibdev->ibdev->name,
-		       sizeof(link->smcibdev->ibdev->name));
+		memcpy(linfo.lnk[0].ibname, link->ibname, sizeof(link->ibname));
 		smc_gid_be16_convert(linfo.lnk[0].gid, link->gid);
 		smc_gid_be16_convert(linfo.lnk[0].peer_gid, link->peer_gid);

@@ -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 v14] 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 v14] 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: 1 · Medium: 1 (2) · 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 fix is incomplete.
- [Medium] After this patch, __smc_diag_dump() (net/smc/smc_diag.c)
  reports SMC_DIAG_MODE_SMCR for every non-fallback socket in SMC_INIT…
- [Low] The commit message's 'Fourth' item says the
  smp_store_release(&newsmcsk->sk_state, SMC_CLOSED) in…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  smc_conn_free() (net/smc/smc_core.c:1281-1282) calls…
- [Medium] This problem wasn't introduced by this patch, but
  smc_diag_handler_dump() (net/smc/smc_diag.c) never checks that the…

--

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

This patch tries to close races between the SMC sock_diag dump and the
teardown of conn/lgr/lnk state. __smc_diag_dump() now skips the extension
attributes and the lgr based diag_mode test for sockets in SMC_INIT,
SMC_CLOSED or fallback, and several teardown paths unhash the socket
before smc_conn_free() or smcd_buf_detach(). It also adds acquire/release
ordering on sk_state and copies the cached link->ibname.

> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index d10244af5bd29..ec5980add7a9d 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c

[ ... ]

> @@ -1971,7 +1971,7 @@ static void smc_listen_out_err(struct smc_sock *new_smc)
>  	this_cpu_inc(net->smc.smc_stats->srv_hshake_err_cnt);
>  	if (newsmcsk->sk_state == SMC_INIT)
>  		sock_put(&new_smc->sk); /* passive closing */
> -	newsmcsk->sk_state = SMC_CLOSED;
> +	smp_store_release(&newsmcsk->sk_state, SMC_CLOSED);

[Severity: Low]

The commit message says:

  Fourth, smc_listen_out_err() sets sk_state = SMC_CLOSED with a plain
  store after smc_conn_abort() -> smc_conn_free() has already cleared
  alert_token_local under write_lock_bh(). On weakly ordered CPUs a
  concurrent diag reader using smp_load_acquire() on sk_state can observe
  SMC_CLOSED while the preceding store to alert_token_local has not yet
  become visible, causing it to dereference an lgr whose reference was
  already dropped.

Is this still accurate with the current __smc_diag_dump()?

A reader that sees SMC_CLOSED now skips the conn->lgr dereference in the
diag_mode test, and the new goto out skips the extension blocks.

Before this store the state is SMC_INIT. smc_listen_work() holds the
socket lock until smc_listen_out(), so close_work can't change it in
between, and SMC_INIT is skipped the same way.

So whichever value the reader sees, it never reads alert_token_local or
conn->lgr, and the release ordering doesn't seem to protect anything the
reader relies on. The commit message already says this for the
stale-token case ("the SMC_CLOSED check in __smc_diag_dump() keeps the
reader from dereferencing conn->lgr").

This reasoning seems to date from v9, before v14 added SMC_CLOSED to the
diag_mode test. Could the fourth item be described as hardening, or
dropped, so stable backporters don't treat the barrier as a required
standalone fix?

>  
>  	smc_listen_out(new_smc);
>  }
> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
> index a8d4b4b65f244..e26586114f802 100644
> --- a/net/smc/smc_close.c
> +++ b/net/smc/smc_close.c

[ ... ]

> @@ -437,6 +439,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);

[Severity: Medium]

This is a pre-existing issue and wasn't introduced by this patch. Does
smc_conn_free() leak the socket reference held by abort_work when it
cancels a pending abort_work?

smc_conn_free() in net/smc/smc_core.c does:

	} else {
		smc_cdc_wait_pend_tx_wr(conn);
		if (current_work() != &conn->abort_work)
			cancel_work_sync(&conn->abort_work);
	}

and ignores the return value. The only code that schedules abort_work is
smc_cdc_msg_validate(), and it hands a reference to the work:

		sock_hold(&smc->sk); /* sock_put in abort_work */
		if (!queue_work(smc_close_wq, &conn->abort_work))
			sock_put(&smc->sk);

smc_conn_abort_work() drops that reference when it runs. Suppose
abort_work is still pending when an SMC-R connection reaches
smc_conn_free() from outside abort_work, for example via __smc_release().
Then cancel_work_sync() returns true, nobody drops the reference, and the
smc_sock is never freed.

smc_close_cancel_work() handles close_work the same way with:

	if (cancel_work_sync(&smc->conn.close_work))
		sock_put(sk);

Should smc_conn_free() do the same for abort_work?

[ ... ]

> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb6..7b4656d6560a6 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
> @@ -87,10 +87,11 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>  
>  	r = nlmsg_data(nlh);
>  	smc_diag_msg_common_fill(r, sk);
> -	r->diag_state = sk->sk_state;
> +	r->diag_state = smp_load_acquire(&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 (r->diag_state != SMC_INIT && r->diag_state != SMC_CLOSED &&
> +		 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]

With this change every non-fallback socket in SMC_INIT or SMC_CLOSED is
reported as SMC_DIAG_MODE_SMCR. That includes SMC-D connections, which
used to report SMC_DIAG_MODE_SMCD.

include/uapi/linux/smc_diag.h defines diag_mode as:

/* Mode of a connection */
enum {
	SMC_DIAG_MODE_SMCR,
	SMC_DIAG_MODE_FALLBACK_TCP,
	SMC_DIAG_MODE_SMCD,
};

It has no "unknown" value. Is it intended that an SMC-D connection is
reported as SMC-R here?

Live SMC-D sockets can stay hashed in SMC_CLOSED with a valid lgr and
alert_token_local. smc_close_passive_work() or
smc_close_passive_abort_received() can move a socket to SMC_CLOSED while
the fd is still open. smc_close_passive_work() only unhashes and frees
when:

		if ((sk->sk_state == SMC_CLOSED) &&
		    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {

SMC-D sockets in SMC_INIT after smc_conn_create() are affected the same
way. A tool that filters by mode (smcss -D / -R style) would put these
sockets in the wrong group.

Could the race be avoided without misreporting? For example, is_smcd
could be cached in the smc_sock, or SMC-D could be inferred from
conn.lgr != NULL && conn.lnk == NULL, which only reads pointers and
doesn't dereference the lgr.

>  	user_ns = sk_user_ns(NETLINK_CB(cb->skb).sk);
>  	if (smc_diag_msg_attrs_fill(sk, skb, r, user_ns))
>  		goto errout;
> @@ -103,6 +104,10 @@ 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 (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED ||
> +	    smc->use_fallback)
> +		goto out;
> +

[Severity: High]

Can a socket whose connection was already released by smc_conn_abort()
still move into a state this guard doesn't cover?

The guard only checks the sk_state snapshot. Take a client handshake
that fails with a negative rc after smc_conn_create():

smc_connect_rdma()
  connect_abort:
    smc_conn_abort()
      smc_conn_free()   /* sets conn->freed, drops lgr/lnk refs */

smc_connect_decline_fallback() returns early without changing the state,
and smc_connect_work() only does:

	rc = __smc_connect(smc);
	if (rc < 0)
		smc->sk.sk_err = -rc;

So the socket stays hashed in SMC_INIT with use_fallback == false.

Suppose smc_lgr_terminate_sched() had already emptied lgr->list, which
is the stale-token case the changelog mentions. Then smc_conn_free()
skips the unregister:

	if (!list_empty(&lgr->list)) {
		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
		smc_lgr_unregister_conn(conn);
	}

alert_token_local stays non-zero and the conn stays in lgr->conns_all.

smc_cdc_rx_handler() doesn't check conn->freed:

	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
	if (!conn || conn->out_of_sync) {

So a close or peer-abort CDC can still queue close_work. Once the
handshake thread drops the socket lock, smc_close_passive_work() does:

	case SMC_INIT:
		sk->sk_state = SMC_APPCLOSEWAIT1;
		break;

Or smc_close_passive_abort_received() moves SMC_INIT to
SMC_PROCESSABORT. Both are plain stores, and neither path reaches the
new unhash plus smc_conn_free() branch.

A diag reader under read_lock(&h->lock) would then see APPCLOSEWAIT1 or
PROCESSABORT and pass both new checks. It would find
smc_conn_lgr_valid() true, because conn->lgr is never cleared. It then
dereferences smc->conn.lgr->is_smcd, conn->sndbuf_desc, conn->rmb_desc
and lgr->list.

Meanwhile terminate_work runs
__smc_lgr_terminate()->smc_conn_kill()->smc_close_active_abort(). Its
ACTIVE/APPCLOSEWAIT branch goes to SMC_CLOSED without unhashing:

	case SMC_ACTIVE:
	case SMC_APPCLOSEWAIT1:
	case SMC_APPCLOSEWAIT2:
		sk->sk_state = SMC_PEERABORTWAIT;
		smc_close_cancel_work(smc);
		if (sk->sk_state != SMC_PEERABORTWAIT)
			break;
		sk->sk_state = SMC_CLOSED;
		sock_put(sk); /* (postponed) passive closing */

So the hash write_lock is never taken before smc_lgr_free() drops the
last lgr reference. Wouldn't the reader then be dereferencing a freed
smc_link_group?

There is also a weaker variant, where the token is cleared normally and
the lgr is freed synchronously via smc_lgr_cleanup_early(). Safety then
depends on the token clear being visible to a reader that acquire-loads
an sk_state written with a plain store. This is probably ordered in
practice through lock_sock()/release_sock(), but the patch doesn't
establish it.

Separately, after a failed handshake the same SMC_INIT to
SMC_APPCLOSEWAIT1 transition later runs the "postponed passive closing"
sock_put() in smc_close_active() or smc_close_active_abort(). But
smc_connect_decline_fallback() has already dropped that reference.

Would one of these work better?

- unhash a socket whose conn was freed by smc_conn_abort(), or
  otherwise hide it from diag
- cancel close_work, or check conn->freed in smc_cdc_rx_handler() and
  in the diag reader
- pin the lgr for as long as stale conn state is visible

>  	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>  	    smc->conn.alert_token_local) {

[Severity: Medium]

This is a pre-existing issue and wasn't introduced by this patch. Is
req->diag_ext guaranteed to be inside the request payload here?

smc_diag_handler_dump() starts the dump after checking only:

	if (h->nlmsg_type == SOCK_DIAG_BY_FAMILY &&
	    h->nlmsg_flags & NLM_F_DUMP) {

The generic __sock_diag_cmd() only checks for a struct sock_diag_req:

	if (nlmsg_len(nlh) < sizeof(*req))
		return -EINVAL;

That struct is 2 bytes. So an unprivileged SOCK_DIAG_BY_FAMILY |
NLM_F_DUMP request for AF_SMC with a 2 byte payload is accepted. diag_ext
is at payload offset 3, past skb->len, in tailroom that
netlink_sendmsg() never initialized.

Those uninitialized bits then decide whether SMC_DIAG_CONNINFO,
SMC_DIAG_LGRINFO and SMC_DIAG_DMBINFO are emitted. That exposes the bits
to the requester, and KMSAN would report it as uninit-value.

inet_diag and unix_diag reject requests shorter than their family
request struct. Should smc_diag_handler_dump() also reject
nlmsg_len(h) < sizeof(struct smc_diag_req)?

[ ... ]

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