Linux RDMA and InfiniBand development
 help / color / mirror / Atom feed
* [PATCH net v8] net/smc: fix lgr/lnk lifetime vs diag reader race
@ 2026-10-05 13:12 Mahanta Jambigi
  2026-10-05 13:19 ` netdev-bot+sinfo
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Mahanta Jambigi @ 2026-10-05 13:12 UTC (permalink / raw)
  To: andrew+netdev, davem, edumazet, kuba, pabeni, alibuda, dust.li,
	sidraya
  Cc: hidayath, pasic, horms, tonylu, guwen, stable, netdev, linux-s390,
	linux-rdma, Mahanta Jambigi

The diag dump walks the socket hash table under a read_lock and dereferences
conn->lgr and conn->lnk. Three paths expose a window where a hashed socket has
freed or partially freed conn/lgr/lnk state.

First, smc_conn_abort() calls smc_conn_free() during early handshake aborts
while the socket is still hashed in SMC_INIT, 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; smcss already suppresses these attributes
for closed/unconnected sockets so there is no observability regression. Use
smp_load_acquire() / smp_store_release() on sk_state to prevent speculative load
reordering on weakly ordered CPUs (e.g. ARM64) during fallback.

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.

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 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 (smc_find_ism_v2_device_serv(),
  smc_find_rdma_v2_device_serv()) call smc_conn_abort() on a failed
  attempt and then smc_conn_create() on the next device; unhashing in
  smc_conn_free() would permanently hide the established connection from
  smc_diag since smc_conn_create() does not re-hash the socket

Changes in v3:
- redesigned as a single patch; dropped the lgr_lnk_lock spinlock
  approach and the 2-patch split
- fix is now at the socket hash layer: introduce smc_conn_unhash() with
  a per-connection unhashed flag; smc_conn_free() unhashes before
  dropping lgr/lnk refs, so any socket visible to the diag reader under
  the hash read_lock has valid conn->lgr and conn->lnk pointers
- __smc_release() updated to call smc_conn_unhash() for non-fallback
  sockets so the flag is honoured when smc_conn_free() already ran first
- smc_diag.c needs no changes; the hash read_lock invariant is
  sufficient without any per-connection lock in the dump path
- dropped the clcsock/mutex_trylock fix as that will be addressed
  separately

Changes in v2:
- this is v2 of the 2-patch series; the earlier submission was
  mislabelled [PATCH v3] but was in fact the first version sent to the
  list
- split into a 2-patch series; patch 1/2 adds per-connection
  lgr_lnk_lock infrastructure to smc_core, patch 2/2 fixes the diag
  dump path using it
- dropped lock_sock()/release_sock() from __smc_diag_dump(); v1 held
  the socket lock across all lgr/lnk dereferences, requiring the hash
  read_lock to be dropped and re-acquired around each socket
- dropped the restart-from-head loop in smc_diag_dump_proto(); the new
  design does not drop the hash read_lock mid-walk so the hlist
  truncation concern no longer applies
- dropped refcount_inc_not_zero() socket pinning from the dump loop for
  the same reason: the hash read_lock is now held for the full walk
- added per-connection lgr_lnk_lock spinlock to struct smc_connection;
  conn->lgr and conn->lnk are NULLed under this lock in smc_conn_free()
  before borrowed references are released, establishing the invariant: a
  non-NULL conn->lgr seen under lgr_lnk_lock guarantees the lgr is alive
- added lgr_lnk_lock to smc_switch_link_and_count() to protect the
  conn->lnk pointer swap from concurrent diag readers
- replaced mutex_lock() on clcsock_release_lock in
  smc_diag_msg_common_fill() with mutex_trylock(); mutex_lock() was
  valid in v1 because the hash spinlock had been dropped, but the new
  design holds the hash read_lock throughout so only a non-sleeping
  trylock is safe; a failed trylock leaves address fields zeroed, which
  is acceptable for a monitoring tool
- all conn->lgr and conn->lnk accesses in __smc_diag_dump() use a
  snapshot-then-use pattern: fields are copied into local stack
  variables under lgr_lnk_lock and nla_put() is called after releasing
  the lock, avoiding any sleeping operation under the spinlock

 net/smc/af_smc.c    | 8 ++++----
 net/smc/smc_close.c | 3 +++
 net/smc/smc_core.c  | 1 +
 net/smc/smc_diag.c  | 9 +++++++--
 4 files changed, 15 insertions(+), 7 deletions(-)

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

+	smc->sk.sk_prot->unhash(&smc->sk);
 	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
 		conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
 	else
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index bf0beaa23bdb..6ee51088d227 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,7 +158,6 @@ 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)

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net v8] net/smc: fix lgr/lnk lifetime vs diag reader race
  2026-10-05 13:12 [PATCH net v8] net/smc: fix lgr/lnk lifetime vs diag reader race Mahanta Jambigi
@ 2026-10-05 13:19 ` netdev-bot+sinfo
  2026-10-05 17:09   ` Mahanta Jambigi
  2026-10-05 13:23 ` sashiko-bot
  2026-10-08 16:13 ` netdev-bot+sashiko
  2 siblings, 1 reply; 5+ messages in thread
From: netdev-bot+sinfo @ 2026-10-05 13:19 UTC (permalink / raw)
  To: Mahanta Jambigi
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, alibuda, dust.li,
	sidraya, hidayath, pasic, horms, tonylu, guwen, stable, netdev,
	linux-s390, linux-rdma

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net v8] net/smc: fix lgr/lnk lifetime vs diag reader race
  2026-10-05 13:12 [PATCH net v8] net/smc: fix lgr/lnk lifetime vs diag reader race Mahanta Jambigi
  2026-10-05 13:19 ` netdev-bot+sinfo
@ 2026-10-05 13:23 ` sashiko-bot
  2026-10-08 16:13 ` netdev-bot+sashiko
  2 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-10-05 13:23 UTC (permalink / raw)
  To: Mahanta Jambigi
  Cc: Christian Borntraeger, linux-s390, Alexander Gordeev,
	Heiko Carstens, Vasily Gorbik, linux-rdma

> The diag dump walks the socket hash table under a read_lock and dereferences
> conn->lgr and conn->lnk. Three paths expose a window where a hashed socket has
> freed or partially freed conn/lgr/lnk state.
> 
> First, smc_conn_abort() calls smc_conn_free() during early handshake aborts
> while the socket is still hashed in SMC_INIT, 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; smcss already suppresses these attributes
> for closed/unconnected sockets so there is no observability regression. Use
> smp_load_acquire() / smp_store_release() on sk_state to prevent speculative load
> reordering on weakly ordered CPUs (e.g. ARM64) during fallback.
> 
> Second, in smc_conn_kill() on SMC-D with dmb_nocopy, smcd_buf_detach() NULLs and
> [ ... ]
> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net v8] net/smc: fix lgr/lnk lifetime vs diag reader race
  2026-10-05 13:19 ` netdev-bot+sinfo
@ 2026-10-05 17:09   ` Mahanta Jambigi
  0 siblings, 0 replies; 5+ messages in thread
From: Mahanta Jambigi @ 2026-10-05 17:09 UTC (permalink / raw)
  To: netdev-bot+sinfo
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, alibuda, dust.li,
	sidraya, hidayath, pasic, horms, tonylu, guwen, stable, netdev,
	linux-s390, linux-rdma



On 05/10/26 6:49 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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net v8] net/smc: fix lgr/lnk lifetime vs diag reader race
  2026-10-05 13:12 [PATCH net v8] net/smc: fix lgr/lnk lifetime vs diag reader race Mahanta Jambigi
  2026-10-05 13:19 ` netdev-bot+sinfo
  2026-10-05 13:23 ` sashiko-bot
@ 2026-10-08 16:13 ` netdev-bot+sashiko
  2 siblings, 0 replies; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-10-08 16:13 UTC (permalink / raw)
  To: mjambigi
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, alibuda, dust.li,
	sidraya, hidayath, pasic, horms, tonylu, guwen, stable, netdev,
	linux-s390, linux-rdma

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says the conn/lgr/lnk blocks of
  __smc_diag_dump() are guarded for SMC_INIT and SMC_CLOSED sockets.
- [Low] In __smc_diag_dump() (net/smc/smc_diag.c:93-97) the new
  `r->diag_state != SMC_INIT &&` check in the diag_mode selection sends…

--

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

The patch closes races between the SMC diag dump and connection teardown.
It skips the conn/lgr/lnk attribute blocks for SMC_INIT and SMC_CLOSED
sockets, and pairs smp_load_acquire()/smp_store_release() on sk_state. It
also unhashes the socket before smc_conn_free() or smcd_buf_detach() in
several teardown paths, and copies the cached link->ibname.

> 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: Low]
Is this change to diag_mode intended? With the new r->diag_state !=
SMC_INIT check, every non-fallback SMC_INIT socket falls into the else
branch and is reported as SMC_DIAG_MODE_SMCR.

Before this patch, an SMC_INIT socket with a registered SMC-D link group
was reported as SMC_DIAG_MODE_SMCD.

On the client side, this window runs from smc_conn_create() in
smc_connect_ism() until the smp_store_release() to SMC_ACTIVE. On the
server side it lasts until smc_listen_out_connected(), which includes
waiting for the peer's CLC CONFIRM. During that time, sock_diag users see
an SMC-D connection labelled as SMC-R. The "Mode of a connection" enum in
include/uapi/linux/smc_diag.h has no "unknown" value for this case.

The commit message says:

  "Guard the conn/lgr/lnk blocks in __smc_diag_dump() for SMC_INIT and
   SMC_CLOSED sockets; smcss already suppresses these attributes for
   closed/unconnected sockets so there is no observability regression."

It doesn't mention the diag_mode change, which only appears in the v6
changelog below the --- line.

Could the transport type be recorded in a field of struct smc_sock when
the lgr is assigned? That would avoid both the lgr dereference and the
wrong mode.

[ ... ]

> @@ -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]
The commit message says the conn/lgr/lnk blocks are guarded for both
SMC_INIT and SMC_CLOSED. The diag_mode selection above excludes only
SMC_INIT, and it runs before this early exit.

For an SMC_CLOSED socket, can __smc_diag_dump() still call
smc_conn_lgr_valid() and then dereference smc->conn.lgr->is_smcd?

In the server handshake error path, a CLOSED socket stays hashed after
its conn has been freed:

smc_listen_decline()
  smc_conn_abort()
    smc_conn_free()
      smc_lgr_unregister_conn()
        write_lock_bh(&lgr->conns_lock);
        __smc_lgr_unregister_conn()
          conn->alert_token_local = 0;
        write_unlock_bh(&lgr->conns_lock);
      smcr_link_put(conn->lnk);
      smc_lgr_put(lgr);          <-- conn->lgr is left set
  smc_listen_out_err()
    newsmcsk->sk_state = SMC_CLOSED;

The SMC_CLOSED store in smc_listen_out_err() is a plain store, not the
smp_store_release() this patch adds for SMC_ACTIVE.

Between the alert_token_local reset and the CLOSED store, the only
operations are release-ordered ones: write_unlock_bh(),
refcount_dec_and_test() via smcr_link_put()/smc_lgr_put(), and
sock_put(). None of them orders the later CLOSED store.

On a weakly ordered CPU such as arm64, could the reader's
smp_load_acquire() see SMC_CLOSED while alert_token_local still reads as
non-zero? The reader would then dereference an lgr that the connection no
longer holds a reference to. If link-group freeing runs concurrently,
that read is a use-after-free.

This needs a non-local-first lgr with a negative reason code, so that
smc_lgr_cleanup_early() is not called. The window is narrow.

Would it help to exclude SMC_CLOSED from the diag_mode condition as well,
or to move this early exit ahead of the diag_mode selection? Publishing
SMC_CLOSED in smc_listen_out_err() with smp_store_release() would also
close the gap.

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

[ ... ]

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-10-08 16:13 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-05 13:12 [PATCH net v8] net/smc: fix lgr/lnk lifetime vs diag reader race Mahanta Jambigi
2026-10-05 13:19 ` netdev-bot+sinfo
2026-10-05 17:09   ` Mahanta Jambigi
2026-10-05 13:23 ` sashiko-bot
2026-10-08 16:13 ` netdev-bot+sashiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox