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

[PATCH net v12] 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. Several 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 do not look at conn->lgr to determine diag_mode in
SMC_INIT.

This changes the sock_diag output for such sockets. SMC_DIAG_CONNINFO,
SMC_DIAG_LGRINFO and SMC_DIAG_DMBINFO are no longer reported while a
socket is in SMC_INIT or SMC_CLOSED; diag_mode is SMC_DIAG_MODE_SMCR for
a non-fallback SMC-D socket in SMC_INIT, as it already was before
smc_conn_create() had run. smcss does not use these attributes for such
sockets. 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().
Other kill paths leave the socket hashed, since it can stay open until
close().

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.

Fifth, on the smc_lgr_cleanup_early() path (e.g. server CLC decline with
SYNCERR) __smc_lgr_terminate() sets terminating=1 and unlinks the LGR
without setting freeing=1. smc_conn_free() gated
smc_lgr_unregister_conn() on !list_empty(&lgr->list), skipping
unregister on the unlinked LGR and leaving alert_token_local non-zero; a
fallback socket could then reach SMC_ACTIVE and a diag reader could
dereference sndbuf_desc/rmb_desc freed by smc_lgr_free_bufs(). Also,
smc_lgr_schedule_free_work() only checked !lgr->freeing, so a concurrent
smc_conn_free() decrementing conns_num to 0 could re-arm free_work; the
subsequent kfree(lgr) leaves the armed work pointing at freed memory.
Unregister unconditionally in smc_conn_free(), and guard
smc_lgr_schedule_free_work() with !lgr->terminating in addition to
!lgr->freeing.

Finally, in __smc_diag_dump(), copy the cached link->ibname instead of
dereferencing link->smcibdev->ibdev->name to avoid the NULL pointer
dereference / UAF if __smcr_link_clear() zeroes the link during
concurrent SMC-R link failover. 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: 2a0674fffb6b ("net/smc: improve abnormal termination of link groups")
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 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
  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  | 11 ++++++-----
 net/smc/smc_diag.c  | 12 ++++++++----
 4 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index e9f93b3ab435..41b8af13efc4 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..9d8165d1b40c 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -84,7 +84,7 @@ static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
 	 * creation. For client use a somewhat higher removal delay time,
 	 * otherwise there is a risk of out-of-sync link groups.
 	 */
-	if (!lgr->freeing) {
+	if (!lgr->freeing && !lgr->terminating) {
 		mod_delayed_work(system_percpu_wq, &lgr->free_work,
 				 (!lgr->is_smcd && lgr->role == SMC_CLNT) ?
 						SMC_LGR_FREE_DELAY_CLNT :
@@ -1279,10 +1279,9 @@ void smc_conn_free(struct smc_connection *conn)
 		if (current_work() != &conn->abort_work)
 			cancel_work_sync(&conn->abort_work);
 	}
-	if (!list_empty(&lgr->list)) {
+	if (!list_empty(&lgr->list))
 		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
-		smc_lgr_unregister_conn(conn);
-	}
+	smc_lgr_unregister_conn(conn);

 	if (!lgr->conns_num)
 		smc_lgr_schedule_free_work(lgr);
@@ -1530,8 +1529,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 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 v12] 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 v12] net/smc: fix lgr/lnk lifetime vs diag reader race

[Mahanta Jambigi]

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

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

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

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] net/smc: race in smc_lgr_schedule_free_work() can arm free_work before kfree
- [High] net/smc: missing release barrier on client SMC_INIT to SMC_CLOSED transition
--

commit ac5d5191510569377b3ad3881fe61cfb7d941a9d
Author: Mahanta Jambigi <mjambigi@linux.ibm.com>

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

This commit addresses several races between SMC socket/link group teardown
and concurrent sock_diag readers by adjusting sk_state ordering, unhashing
sockets earlier in teardown paths, skipping extended diag attributes in
SMC_INIT/SMC_CLOSED, and unconditionally unregistering connections in
smc_conn_free().

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 9974149659c2f..9d8165d1b40c5 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -84,7 +84,7 @@ static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
>  	 * creation. For client use a somewhat higher removal delay time,
>  	 * otherwise there is a risk of out-of-sync link groups.
>  	 */
> -	if (!lgr->freeing) {
> +	if (!lgr->freeing && !lgr->terminating) {
>  		mod_delayed_work(system_percpu_wq, &lgr->free_work,
>  				 (!lgr->is_smcd && lgr->role == SMC_CLNT) ?
>  						SMC_LGR_FREE_DELAY_CLNT :
> @@ -1279,10 +1279,9 @@ void smc_conn_free(struct smc_connection *conn)
>  		if (current_work() != &conn->abort_work)
>  			cancel_work_sync(&conn->abort_work);
>  	}
> -	if (!list_empty(&lgr->list)) {
> +	if (!list_empty(&lgr->list))
>  		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
> -		smc_lgr_unregister_conn(conn);
> -	}
> +	smc_lgr_unregister_conn(conn);
>  
>  	if (!lgr->conns_num)
>  		smc_lgr_schedule_free_work(lgr);

[Severity: Critical]
Can smc_lgr_schedule_free_work() still race with __smc_lgr_terminate() and
re-arm lgr->free_work right before lgr is freed?

In smc_lgr_cleanup_early(), the link group is unlinked from lgr->list while
both lgr->freeing and lgr->terminating are still 0, and then
__smc_lgr_terminate() cancels free_work before setting lgr->terminating:

net/smc/smc_core.c:__smc_lgr_terminate() {
	if (lgr->terminating)
		return;	/* lgr already terminating */
	/* cancel free_work sync, will terminate when lgr->freeing is set */
	cancel_delayed_work(&lgr->free_work);
	lgr->terminating = 1;
	...
}

Because smc_conn_free() now calls smc_lgr_unregister_conn(conn)
unconditionally and invokes smc_lgr_schedule_free_work(lgr) outside
lgr->conns_lock, can the following interleaving occur when another
connection on the same link group is concurrently freed?

CPU0 (smc_conn_free)                   CPU1 (smc_lgr_cleanup_early)
--------------------                   ----------------------------
smc_lgr_unregister_conn(conn)
  lgr->conns_num becomes 0
smc_lgr_schedule_free_work(lgr)
  sees !lgr->freeing &&
       !lgr->terminating
                                       __smc_lgr_terminate(lgr, true)
                                         cancel_delayed_work(&lgr->free_work)
                                         lgr->terminating = 1
                                         ...
                                         smc_lgr_free(lgr)
                                           smc_lgr_put(lgr)
  mod_delayed_work(..., &lgr->free_work, ...)
smc_lgr_put(lgr)
  __smc_lgr_free(lgr)
    kfree(lgr)

If that happens, wouldn't lgr be freed via kfree(lgr) while lgr->free_work
remains queued in system_percpu_wq?

[ ... ]

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

[Severity: High]
Should this condition also exclude r->diag_state == SMC_CLOSED, or should
smc_close_active() use smp_store_release() when transitioning from SMC_INIT
to SMC_CLOSED?

On the client side, if smc_connect_rdma() or smc_connect_ism() fails after
smc_conn_create() and jumps to connect_abort, smc_conn_abort() calls
smc_conn_free() and smc_lgr_cleanup_early(), which clears
conn->alert_token_local and frees conn->lgr while leaving conn->lgr non-NULL.

If fallback is not possible in smc_connect_decline_fallback() (or fails in
smc_connect_fallback()), smc->use_fallback remains false and sk->sk_state
remains SMC_INIT.

When the socket is then closed via smc_release() -> __smc_release() ->
smc_close_active(), sk->sk_state is set to SMC_CLOSED with a plain store
while the socket is still hashed:

net/smc/smc_close.c:smc_close_active() {
	...
	switch (sk->sk_state) {
	case SMC_INIT:
		sk->sk_state = SMC_CLOSED;
		break;
	...
}

On weakly ordered CPUs, if a concurrent reader in __smc_diag_dump() observes
r->diag_state == SMC_CLOSED from that plain store, there is no release
barrier ordering the earlier conn->alert_token_local = 0 store before the
sk_state transition, and the diag_mode check here only filters out SMC_INIT
(unlike the SMC_INIT || SMC_CLOSED check further down). Could
__smc_diag_dump() then see a stale non-zero alert_token_local and
dereference the freed smc->conn.lgr->is_smcd?

[ ... ]

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

[ ... ]

>
> base-commit: 07bd391eb9572e74f073c49acef327611234f9bb

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019111008.3367567-1-mjambigi@linux.ibm.com?part=1

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

[Mahanta Jambigi]

On 20/10/26 4:40 pm, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] net/smc: race in smc_lgr_schedule_free_work() can arm free_work before kfree
> - [High] net/smc: missing release barrier on client SMC_INIT to SMC_CLOSED transition
> --
> 
> commit ac5d5191510569377b3ad3881fe61cfb7d941a9d
> Author: Mahanta Jambigi <mjambigi@linux.ibm.com>
> 
> net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This commit addresses several races between SMC socket/link group teardown
> and concurrent sock_diag readers by adjusting sk_state ordering, unhashing
> sockets earlier in teardown paths, skipping extended diag attributes in
> SMC_INIT/SMC_CLOSED, and unconditionally unregistering connections in
> smc_conn_free().
> 
>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>> index 9974149659c2f..9d8165d1b40c5 100644
>> --- a/net/smc/smc_core.c
>> +++ b/net/smc/smc_core.c
>> @@ -84,7 +84,7 @@ static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
>>  	 * creation. For client use a somewhat higher removal delay time,
>>  	 * otherwise there is a risk of out-of-sync link groups.
>>  	 */
>> -	if (!lgr->freeing) {
>> +	if (!lgr->freeing && !lgr->terminating) {
>>  		mod_delayed_work(system_percpu_wq, &lgr->free_work,
>>  				 (!lgr->is_smcd && lgr->role == SMC_CLNT) ?
>>  						SMC_LGR_FREE_DELAY_CLNT :
>> @@ -1279,10 +1279,9 @@ void smc_conn_free(struct smc_connection *conn)
>>  		if (current_work() != &conn->abort_work)
>>  			cancel_work_sync(&conn->abort_work);
>>  	}
>> -	if (!list_empty(&lgr->list)) {
>> +	if (!list_empty(&lgr->list))
>>  		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
>> -		smc_lgr_unregister_conn(conn);
>> -	}
>> +	smc_lgr_unregister_conn(conn);
>>  
>>  	if (!lgr->conns_num)
>>  		smc_lgr_schedule_free_work(lgr);
> 
> [Severity: Critical]
> Can smc_lgr_schedule_free_work() still race with __smc_lgr_terminate() and
> re-arm lgr->free_work right before lgr is freed?
> 
> In smc_lgr_cleanup_early(), the link group is unlinked from lgr->list while
> both lgr->freeing and lgr->terminating are still 0, and then
> __smc_lgr_terminate() cancels free_work before setting lgr->terminating:
> 
> net/smc/smc_core.c:__smc_lgr_terminate() {
> 	if (lgr->terminating)
> 		return;	/* lgr already terminating */
> 	/* cancel free_work sync, will terminate when lgr->freeing is set */
> 	cancel_delayed_work(&lgr->free_work);
> 	lgr->terminating = 1;
> 	...
> }
> 
> Because smc_conn_free() now calls smc_lgr_unregister_conn(conn)
> unconditionally and invokes smc_lgr_schedule_free_work(lgr) outside
> lgr->conns_lock, can the following interleaving occur when another
> connection on the same link group is concurrently freed?
> 
> CPU0 (smc_conn_free)                   CPU1 (smc_lgr_cleanup_early)
> --------------------                   ----------------------------
> smc_lgr_unregister_conn(conn)
>   lgr->conns_num becomes 0
> smc_lgr_schedule_free_work(lgr)
>   sees !lgr->freeing &&
>        !lgr->terminating
>                                        __smc_lgr_terminate(lgr, true)
>                                          cancel_delayed_work(&lgr->free_work)
>                                          lgr->terminating = 1
>                                          ...
>                                          smc_lgr_free(lgr)
>                                            smc_lgr_put(lgr)
>   mod_delayed_work(..., &lgr->free_work, ...)
> smc_lgr_put(lgr)
>   __smc_lgr_free(lgr)
>     kfree(lgr)
> 
> If that happens, wouldn't lgr be freed via kfree(lgr) while lgr->free_work
> remains queued in system_percpu_wq?

The interleaving is theoretically possible but *pre-existing* and not
widened by this patch. It requires a first-contact connection A to fail
and take the smc_lgr_cleanup_early() path, a second connection B to have
joined the same lgr after smc_server_lgr_pending was dropped, and B's
smc_conn_free() to bring conns_num to 0 within a few instructions of A's
__smc_lgr_terminate() cancelling free_work, with A's teardown completing
before B's final smc_lgr_put().

The unlocked "!lgr->freeing" check followed by mod_delayed_work() in
smc_lgr_schedule_free_work() has the same non-atomic window against
smc_lgr_terminate_sched() and __smc_lgr_terminate() in the baseline. The
"!lgr->terminating" check added here only avoids re-arming once
terminating is already visible, so it keeps the window no larger than
before. Closing it fully requires reworking how free_work scheduling is
serialized against lgr termination (check and arm under lgr_lock, or
cancelling free_work in __smc_lgr_free()). That is a separate lgr
lifetime fix, out of scope for this diag reader race patch.

> 
> [ ... ]
> 
>> 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;
> 
> [Severity: High]
> Should this condition also exclude r->diag_state == SMC_CLOSED, or should
> smc_close_active() use smp_store_release() when transitioning from SMC_INIT
> to SMC_CLOSED?
> 
> On the client side, if smc_connect_rdma() or smc_connect_ism() fails after
> smc_conn_create() and jumps to connect_abort, smc_conn_abort() calls
> smc_conn_free() and smc_lgr_cleanup_early(), which clears
> conn->alert_token_local and frees conn->lgr while leaving conn->lgr non-NULL.
> 
> If fallback is not possible in smc_connect_decline_fallback() (or fails in
> smc_connect_fallback()), smc->use_fallback remains false and sk->sk_state
> remains SMC_INIT.
> 
> When the socket is then closed via smc_release() -> __smc_release() ->
> smc_close_active(), sk->sk_state is set to SMC_CLOSED with a plain store
> while the socket is still hashed:
> 
> net/smc/smc_close.c:smc_close_active() {
> 	...
> 	switch (sk->sk_state) {
> 	case SMC_INIT:
> 		sk->sk_state = SMC_CLOSED;
> 		break;
> 	...
> }
> 
> On weakly ordered CPUs, if a concurrent reader in __smc_diag_dump() observes
> r->diag_state == SMC_CLOSED from that plain store, there is no release
> barrier ordering the earlier conn->alert_token_local = 0 store before the
> sk_state transition, and the diag_mode check here only filters out SMC_INIT
> (unlike the SMC_INIT || SMC_CLOSED check further down). Could
> __smc_diag_dump() then see a stale non-zero alert_token_local and
> dereference the freed smc->conn.lgr->is_smcd?


On this path alert_token_local is cleared in the connect syscall, under
conns_lock with the socket lock held. The SMC_INIT -> SMC_CLOSED store
only happens later in close() -> smc_close_active() under lock_sock().
The lock_sock()/release_sock() handoff and the conns_lock write_unlock
between the two order the clear before the sk_state store, so a reader
that sees SMC_CLOSED *cannot* see a stale non-zero alert_token_local.