* [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: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: 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: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