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

[PATCH net v9] 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. Guard the conn/lgr/lnk blocks in __smc_diag_dump()
for SMC_INIT and SMC_CLOSED sockets. As a side effect the diag_mode field is
reported as SMC_DIAG_MODE_SMCR for SMC-D sockets still in SMC_INIT; smcss
suppresses those rows before printing the mode string so there is no rendered
regression. 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 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.

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.

Additionally, in __smc_diag_dump(), use cached link->ibname instead of chasing
link->smcibdev->ibdev->name to avoid racing with link clearing during SMC-R link
failover.

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
Reviewed-by: Hidayath Khan <hidayath@linux.ibm.com>
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
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
  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 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  |  1 +
 net/smc/smc_diag.c  | 12 ++++++++----
 4 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index e9f93b3ab435..2f8308aa7daf 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 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 9974149659c2..663926465ba7 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..36a3b8f31f28 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 &&
+		 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 (r->diag_state == SMC_INIT || r->diag_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;
@@ -154,8 +158,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 +188,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 v9] 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 v9] net/smc: fix lgr/lnk lifetime vs diag reader race

[Mahanta Jambigi]

On 12/10/26 1:34 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 four 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 v9] 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: 0 · Medium: 2 (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).

- [Medium] The patch adds `smc->sk.sk_prot->unhash(&smc->sk)` as the
  first statement of smc_conn_kill() (net/smc/smc_core.c:1520), so it…
- [Medium] The new sk_state-based guards in __smc_diag_dump()
  (net/smc/smc_diag.c) change the sock_diag uAPI output for sockets…
- [Low] __smc_diag_dump() (net/smc/smc_diag.c:161) now copies
  link->ibname into smc_diag_linkinfo.ibname instead of…

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

--

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

This patch closes several windows where the SMC sock_diag dump can
dereference freed conn/lgr/lnk state of a socket that is still hashed. It
adds sk_state guards and acquire/release ordering in __smc_diag_dump(), and
unhashes the socket before smc_conn_free() in the close paths and at the
top of smc_conn_kill(). It also reports the cached link->ibname instead of
link->smcibdev->ibdev->name.

> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
> index bb0313ef5f7c1..aa6363abdd747 100644
> --- a/net/smc/smc_close.c
> +++ b/net/smc/smc_close.c

[ ... ]

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

[Severity: Medium]
This isn't a bug introduced by this patch, but it came up while looking at
the smc_conn_free() calls here. Can smc_conn_free() leak the smc_sock
reference that a pending abort_work holds?

On SMC-R, smc_cdc_msg_validate() takes a reference for the work:

net/smc/smc_cdc.c:smc_cdc_msg_validate() {
    ...
		sock_hold(&smc->sk); /* sock_put in abort_work */
		if (!queue_work(smc_close_wq, &conn->abort_work))
			sock_put(&smc->sk);
    ...
}

That reference is normally dropped at the end of smc_conn_abort_work():

	sock_put(&smc->sk); /* sock_hold done by schedulers of abort_work */

smc_conn_free() ignores the return value of the cancel:

net/smc/smc_core.c:smc_conn_free() {
    ...
		if (current_work() != &conn->abort_work)
			cancel_work_sync(&conn->abort_work);
    ...
}

Suppose smc_conn_free() runs from __smc_release() or
smc_close_passive_work() while abort_work is still pending. Then
cancel_work_sync() returns true, and nothing drops the reference the work
was holding.

smc_close_cancel_work() handles the same pattern for close_work:

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

Should smc_conn_free() do the same for abort_work?

>  			if (smc->clcsock)
>  				release_clcsock = true;
> 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);

[Severity: Medium]
Does this hide sockets that stay open from sock_diag for the rest of their
lifetime?

The unhash runs for every SMC-R and SMC-D kill. Only the SMC-D dmb_nocopy
smcd_buf_detach() further down needs it.

smc_conn_kill() is called from __smc_lgr_terminate() for every connection
of a terminating link group (link down, device removal, admin
termination). It is also called from smc_conn_abort_work().

For SMC_ACTIVE, SMC_APPCLOSEWAIT1 and SMC_APPCLOSEWAIT2,
smc_close_active_abort() does not free the conn afterwards:

net/smc/smc_close.c:smc_close_active_abort() {
    ...
	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 */
		break;
    ...
}

The early "if (sk->sk_state != SMC_PEERABORTWAIT) break;" exits also leave
the socket alive. The application keeps its fd until close().

SMC sockets are hashed only once, in smc_sk_init(), so they never go back
into the hash table.

smc_unhash_sk() also calls sock_prot_inuse_add(..., -1). The inuse count in
/proc/net/protocols then drops while the socket is still alive.

On SMC-R this doesn't seem to add any safety:
- smc_lgr_unregister_conn() clears alert_token_local
- the conn keeps its lgr and link references until smc_conn_free()
- the free inside smc_close_active_abort() now has its own unhash

The v6 changelog says the per-site placement "avoids prematurely hiding
live SMC_ACTIVE/APPCLOSEWAIT sockets from smcss". The commit message
describes this hunk only as an SMC-D dmb_nocopy reorder.

Would it be enough to move the unhash into the
smc_ism_support_dmb_nocopy() branch, right before smcd_buf_detach()?

>  	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 bf0beaa23bdb6..36a3b8f31f28d 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 &&
> +		 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 (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
> +		goto out;
> +

[Severity: Medium]
Do these two sk_state checks change the sock_diag uAPI output for sockets
whose connection is still live?

With the diag_mode change, every non-fallback SMC-D socket in SMC_INIT now
reports SMC_DIAG_MODE_SMCR. In include/uapi/linux/smc_diag.h, SMCR is 0
and SMCD has its own value, so there is no "unknown" value to fall back on.

SMC_INIT is not limited to the abort window:
- smc_connect_ism() and smc_connect_rdma() run smc_conn_create(),
  smc_buf_create(), smcd_buf_attach() and the whole CLC confirm exchange
  while in SMC_INIT
- on the server side the socket stays in SMC_INIT through
  smc_listen_work() until smc_listen_out_connected()

During these windows the lgr, alert_token_local and the buffers are all
valid, and the socket is hashed and visible to diag.

The new goto out also skips SMC_DIAG_CONNINFO, SMC_DIAG_LGRINFO and
SMC_DIAG_DMBINFO even when req->diag_ext asks for them. That bypasses the
existing alert_token_local, smc_conn_lgr_valid(), list_empty(&lgr->list)
and rmb_desc checks.

SMC_CLOSED doesn't mean the conn was freed either:
- smc_close_active() reached through shutdown(SHUT_RDWR) sets SMC_CLOSED in
  the APPCLOSEWAIT/APPFINCLOSEWAIT/PROCESSABORT cases without calling
  smc_conn_free()
- the SMC_ACTIVE branch of smc_close_active_abort() does the same
- smc_close_passive_work() frees the conn only when SOCK_DEAD is set or
  sk->sk_socket is NULL:

net/smc/smc_close.c:smc_close_passive_work() {
    ...
		if ((sk->sk_state == SMC_CLOSED) &&
		    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {
    ...
}

These sockets keep their conn until close(), but now report no extension
data.

The commit message says:

    As a side effect the diag_mode field is
    reported as SMC_DIAG_MODE_SMCR for SMC-D sockets still in SMC_INIT; smcss
    suppresses those rows before printing the mode string so there is no rendered
    regression.

That covers only one consumer, smcss. It also doesn't mention the dropped
extensions for live SMC_CLOSED sockets.

Is this change in uAPI output intended for sockets that still have a live
connection?

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

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Can
req->diag_ext be read past the end of the request payload here?

smc_diag_handler_dump() calls netlink_dump_start() without checking
nlmsg_len(h) against sizeof(struct smc_diag_req). The only length check
before that is in __sock_diag_cmd():

net/core/sock_diag.c:__sock_diag_cmd() {
    ...
	if (nlmsg_len(nlh) < sizeof(*req))
		return -EINVAL;
    ...
}

That checks only the 2-byte struct sock_diag_req, but diag_ext is at
payload offset 3.

An unprivileged user can send a SOCK_DIAG_BY_FAMILY/NLM_F_DUMP request with
nlmsg_len = NLMSG_HDRLEN + 2. This read then lands in uninitialized
tailroom of the request skb.

Whether CONNINFO, LGRINFO and DMBINFO appear in the reply would then reveal
bits of that byte, and KMSAN would report uninit-value.

inet_diag and unix_diag both check nlmsg_len against their full request
struct. Should smc_diag_handler_dump() return -EINVAL when
nlmsg_len(h) < sizeof(struct smc_diag_req)?

[ ... ]

> @@ -154,8 +158,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));

[Severity: Low]
Can this report a stale RDMA device name after the device is renamed?

link->ibname is copied only once, when the link is initialized:

net/smc/smc_core.c:smcr_copy_dev_info_to_link() {
    ...
	snprintf(link->ibname, sizeof(link->ibname), "%s",
		 smcibdev->ibdev->name);
    ...
}

ib_device_rename() updates ibdev->name and notifies clients through their
->rename callback. smc_ib_client doesn't have one:

static struct ib_client smc_ib_client = {
	.name	= "smc_ib",
	.add	= smc_ib_add_dev,
	.remove = smc_ib_remove_dev,
};

If the device is renamed (for example with "rdma dev set ... name ...")
while an SMC-R link is up, SMC_DIAG_LGRINFO would show the old name. The
SMC device dump in smc_nl_handle_smcr_dev() would show the current
ibdev->name. Before this patch, smc_diag reported the current name.

The SMC genl link dump (smc_nl_fill_lgr_link()) and the tracepoints already
use the cached name, so this follows an existing convention that goes stale
in the same way.

Would a full fix need a rename callback that refreshes link->ibname?

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

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

[Mahanta Jambigi]

On 13/10/26 1:33 pm, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 (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).
> 
> - [Medium] The patch adds `smc->sk.sk_prot->unhash(&smc->sk)` as the
>   first statement of smc_conn_kill() (net/smc/smc_core.c:1520), so it…
> - [Medium] The new sk_state-based guards in __smc_diag_dump()
>   (net/smc/smc_diag.c) change the sock_diag uAPI output for sockets…
> - [Low] __smc_diag_dump() (net/smc/smc_diag.c:161) now copies
>   link->ibname into smc_diag_linkinfo.ibname instead of…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but
>   smc_conn_free() (net/smc/smc_core.c:1279-1280) calls…
> - [Medium] This problem wasn't introduced by this patch, but
>   smc_diag_handler_dump() (net/smc/smc_diag.c:244) starts the dump…
> 
> --
> 
> net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This patch closes several windows where the SMC sock_diag dump can
> dereference freed conn/lgr/lnk state of a socket that is still hashed. It
> adds sk_state guards and acquire/release ordering in __smc_diag_dump(), and
> unhashes the socket before smc_conn_free() in the close paths and at the
> top of smc_conn_kill(). It also reports the cached link->ibname instead of
> link->smcibdev->ibdev->name.
> 
>> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
>> index bb0313ef5f7c1..aa6363abdd747 100644
>> --- a/net/smc/smc_close.c
>> +++ b/net/smc/smc_close.c
> 
> [ ... ]
> 
>> @@ -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);
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but it came up while looking at
> the smc_conn_free() calls here. Can smc_conn_free() leak the smc_sock
> reference that a pending abort_work holds?

Agreed, and it is a pre-existing bug, not something introduced by this
patch. smc_cdc_msg_validate() takes the reference with sock_hold() for
abort_work, and smc_conn_free() drops the return value of
cancel_work_sync(), so a still-pending work leaks the socket. I will
fix it in a separate patch, using the same pattern as
smc_close_cancel_work():

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

with Fixes: b286a0651e44 ("net/smc: handle incoming CDC validation
message"). It is unrelated to the diag lifetime race, so I would like
to keep it out of this patch.

> 
> On SMC-R, smc_cdc_msg_validate() takes a reference for the work:
> 
> net/smc/smc_cdc.c:smc_cdc_msg_validate() {
>     ...
> 		sock_hold(&smc->sk); /* sock_put in abort_work */
> 		if (!queue_work(smc_close_wq, &conn->abort_work))
> 			sock_put(&smc->sk);
>     ...
> }
> 
> That reference is normally dropped at the end of smc_conn_abort_work():
> 
> 	sock_put(&smc->sk); /* sock_hold done by schedulers of abort_work */
> 
> smc_conn_free() ignores the return value of the cancel:
> 
> net/smc/smc_core.c:smc_conn_free() {
>     ...
> 		if (current_work() != &conn->abort_work)
> 			cancel_work_sync(&conn->abort_work);
>     ...
> }
> 
> Suppose smc_conn_free() runs from __smc_release() or
> smc_close_passive_work() while abort_work is still pending. Then
> cancel_work_sync() returns true, and nothing drops the reference the work
> was holding.
> 
> smc_close_cancel_work() handles the same pattern for close_work:
> 
> 	if (cancel_work_sync(&smc->conn.close_work))
> 		sock_put(sk);
> 
> Should smc_conn_free() do the same for abort_work?
> 
>>  			if (smc->clcsock)
>>  				release_clcsock = true;
>> 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);
> 
> [Severity: Medium]
> Does this hide sockets that stay open from sock_diag for the rest of their
> lifetime?

Yes, you are right. The unhash at the top of smc_conn_kill() hides
sockets that stay open after the kill (SMC_ACTIVE, SMC_APPCLOSEWAIT*,
and the early-break exits in smc_close_active_abort()). It also
decrements the inuse count while they are still alive. Only the SMC-D
dmb_nocopy smcd_buf_detach() needs the socket unhashed first.

v10 moves the unhash into the smc_ism_support_dmb_nocopy() branch,
right before smcd_buf_detach(). The sites that really free the
connection (smc_close_active_abort(), smc_close_passive_work() and
__smc_release()) keep their own unhash, and smc_unhash_sk() is
idempotent, so there is no double accounting.

> 
> The unhash runs for every SMC-R and SMC-D kill. Only the SMC-D dmb_nocopy
> smcd_buf_detach() further down needs it.
> 
> smc_conn_kill() is called from __smc_lgr_terminate() for every connection
> of a terminating link group (link down, device removal, admin
> termination). It is also called from smc_conn_abort_work().
> 
> For SMC_ACTIVE, SMC_APPCLOSEWAIT1 and SMC_APPCLOSEWAIT2,
> smc_close_active_abort() does not free the conn afterwards:
> 
> net/smc/smc_close.c:smc_close_active_abort() {
>     ...
> 	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 */
> 		break;
>     ...
> }
> 
> The early "if (sk->sk_state != SMC_PEERABORTWAIT) break;" exits also leave
> the socket alive. The application keeps its fd until close().
> 
> SMC sockets are hashed only once, in smc_sk_init(), so they never go back
> into the hash table.
> 
> smc_unhash_sk() also calls sock_prot_inuse_add(..., -1). The inuse count in
> /proc/net/protocols then drops while the socket is still alive.
> 
> On SMC-R this doesn't seem to add any safety:
> - smc_lgr_unregister_conn() clears alert_token_local
> - the conn keeps its lgr and link references until smc_conn_free()
> - the free inside smc_close_active_abort() now has its own unhash
> 
> The v6 changelog says the per-site placement "avoids prematurely hiding
> live SMC_ACTIVE/APPCLOSEWAIT sockets from smcss". The commit message
> describes this hunk only as an SMC-D dmb_nocopy reorder.
> 
> Would it be enough to move the unhash into the
> smc_ism_support_dmb_nocopy() branch, right before smcd_buf_detach()?
> 
>>  	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 bf0beaa23bdb6..36a3b8f31f28d 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 &&
>> +		 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 (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED)
>> +		goto out;
>> +
> 
> [Severity: Medium]
> Do these two sk_state checks change the sock_diag uAPI output for sockets
> whose connection is still live?

Yes, they do, and the commit message did not say so. v10 documents it:

 - SMC_DIAG_CONNINFO, SMC_DIAG_LGRINFO and SMC_DIAG_DMBINFO are no
   longer reported, even if requested, while a socket is in SMC_INIT
   or SMC_CLOSED.
 - diag_mode is reported as SMC_DIAG_MODE_SMCR for a non-fallback SMC-D
   socket in SMC_INIT. This was already the case before
   smc_conn_create() had run, because conn->lgr is still NULL then and
   the code falls through to SMC_DIAG_MODE_SMCR.
 - smcss, the only consumer, does not print any of these attributes
   for SMC_INIT and SMC_CLOSED rows. It does apply its -R/-D filters to
   diag_mode, so with -a an SMC-D socket in SMC_INIT is matched as
   SMC-R. The commit message says so, instead of claiming that there
   is no visible change.

I am keeping the guard. For SMC_INIT it is what keeps a reader from
dereferencing conn->lgr while smc_conn_abort() frees it synchronously,
and the uAPI has no value to report "unknown" for diag_mode. For
SMC_CLOSED, the extension data describes a connection that no longer
transfers data, and smcss never reads it for those rows.

> 
> With the diag_mode change, every non-fallback SMC-D socket in SMC_INIT now
> reports SMC_DIAG_MODE_SMCR. In include/uapi/linux/smc_diag.h, SMCR is 0
> and SMCD has its own value, so there is no "unknown" value to fall back on.
> 
> SMC_INIT is not limited to the abort window:
> - smc_connect_ism() and smc_connect_rdma() run smc_conn_create(),
>   smc_buf_create(), smcd_buf_attach() and the whole CLC confirm exchange
>   while in SMC_INIT
> - on the server side the socket stays in SMC_INIT through
>   smc_listen_work() until smc_listen_out_connected()
> 
> During these windows the lgr, alert_token_local and the buffers are all
> valid, and the socket is hashed and visible to diag.
> 
> The new goto out also skips SMC_DIAG_CONNINFO, SMC_DIAG_LGRINFO and
> SMC_DIAG_DMBINFO even when req->diag_ext asks for them. That bypasses the
> existing alert_token_local, smc_conn_lgr_valid(), list_empty(&lgr->list)
> and rmb_desc checks.
> 
> SMC_CLOSED doesn't mean the conn was freed either:
> - smc_close_active() reached through shutdown(SHUT_RDWR) sets SMC_CLOSED in
>   the APPCLOSEWAIT/APPFINCLOSEWAIT/PROCESSABORT cases without calling
>   smc_conn_free()
> - the SMC_ACTIVE branch of smc_close_active_abort() does the same
> - smc_close_passive_work() frees the conn only when SOCK_DEAD is set or
>   sk->sk_socket is NULL:
> 
> net/smc/smc_close.c:smc_close_passive_work() {
>     ...
> 		if ((sk->sk_state == SMC_CLOSED) &&
> 		    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {
>     ...
> }
> 
> These sockets keep their conn until close(), but now report no extension
> data.
> 
> The commit message says:
> 
>     As a side effect the diag_mode field is
>     reported as SMC_DIAG_MODE_SMCR for SMC-D sockets still in SMC_INIT; smcss
>     suppresses those rows before printing the mode string so there is no rendered
>     regression.
> 
> That covers only one consumer, smcss. It also doesn't mention the dropped
> extensions for live SMC_CLOSED sockets.
> 
> Is this change in uAPI output intended for sockets that still have a live
> connection?
> 
>>  	if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
>>  	    smc->conn.alert_token_local) {
> 
> [Severity: Medium]
> This is a pre-existing issue, not one introduced by this patch. Can
> req->diag_ext be read past the end of the request payload here?

Agreed, this is pre-existing and not part of this patch. I will send a
separate fix that returns -EINVAL from smc_diag_handler_dump() when
nlmsg_len(h) < sizeof(struct smc_diag_req), as inet_diag and unix_diag
do, with Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets").

> 
> smc_diag_handler_dump() calls netlink_dump_start() without checking
> nlmsg_len(h) against sizeof(struct smc_diag_req). The only length check
> before that is in __sock_diag_cmd():
> 
> net/core/sock_diag.c:__sock_diag_cmd() {
>     ...
> 	if (nlmsg_len(nlh) < sizeof(*req))
> 		return -EINVAL;
>     ...
> }
> 
> That checks only the 2-byte struct sock_diag_req, but diag_ext is at
> payload offset 3.
> 
> An unprivileged user can send a SOCK_DIAG_BY_FAMILY/NLM_F_DUMP request with
> nlmsg_len = NLMSG_HDRLEN + 2. This read then lands in uninitialized
> tailroom of the request skb.
> 
> Whether CONNINFO, LGRINFO and DMBINFO appear in the reply would then reveal
> bits of that byte, and KMSAN would report uninit-value.
> 
> inet_diag and unix_diag both check nlmsg_len against their full request
> struct. Should smc_diag_handler_dump() return -EINVAL when
> nlmsg_len(h) < sizeof(struct smc_diag_req)?
> 
> [ ... ]
> 
>> @@ -154,8 +158,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));
> 
> [Severity: Low]
> Can this report a stale RDMA device name after the device is renamed?

Yes, but this is intended. The cached link->ibname is what the SMC
netlink link dump (smc_nl_fill_lgr_link()) and the tracepoints already
report, so diag now follows the same convention. The cached name is
needed because __smcr_link_clear() memsets the link, so
link->smcibdev can be NULL under the diag reader. Following a rename
would need an ib_client .rename callback that walks all link groups,
which is not something I would put in a fix for net. v10 now says in the
commit message that the name is the one cached at link creation. I
will leave the callback as possible follow-up work.

pw-bot: cr