* [PATCH net v2 0/2] net/smc: fix diag dump lifetime races
@ 2026-08-28 6:54 Mahanta Jambigi
2026-08-28 6:54 ` [PATCH net v2 1/2] net/smc: add connection lifetime infrastructure for diag Mahanta Jambigi
2026-08-28 6:54 ` [PATCH net v2 2/2] net/smc: fix races in smc_diag dump path Mahanta Jambigi
0 siblings, 2 replies; 5+ messages in thread
From: Mahanta Jambigi @ 2026-08-28 6:54 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, alibuda, dust.li,
sidraya, hidayath
Cc: pasic, horms, tonylu, guwen, stable, netdev, linux-s390,
linux-rdma, Mahanta Jambigi
This series fixes multiple lifetime races in the SMC diag dump path.
The first patch adds the basic infrastructure needed to synchronize diag readers
against connection-owned conn->lgr/conn->lnk updates. It introduces a
per-connection spinlock and uses it in the link switch and connection free
handoff paths. conn->lgr and conn->lnk are NULLed under the lock before the
borrowed references are released, so a non-NULL conn->lgr seen under the lock
guarantees the lgr object is alive. The diag reader can rely on this invariant
without borrowing any extra reference.
The second patch fixes two races in smc_diag itself:
- serialize clcsock field access against smc_clcsock_release() with
mutex_trylock()
- take conn->lgr_lnk_lock when reading conn->lgr and conn->lnk; use
smc_conn_lgr_valid() inside the lock to check that the connection is
still registered, then snapshot all required fields and call nla_put()
after releasing the lock
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
Changes in v2:
- this is v2 of the 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net v2 1/2] net/smc: add connection lifetime infrastructure for diag
2026-08-28 6:54 [PATCH net v2 0/2] net/smc: fix diag dump lifetime races Mahanta Jambigi
@ 2026-08-28 6:54 ` Mahanta Jambigi
2026-08-29 6:55 ` sashiko-bot
2026-08-28 6:54 ` [PATCH net v2 2/2] net/smc: fix races in smc_diag dump path Mahanta Jambigi
1 sibling, 1 reply; 5+ messages in thread
From: Mahanta Jambigi @ 2026-08-28 6:54 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, alibuda, dust.li,
sidraya, hidayath
Cc: pasic, horms, tonylu, guwen, stable, netdev, linux-s390,
linux-rdma, Mahanta Jambigi
Add the basic lifetime synchronization needed by the diag dump path. Introduce
a per-connection lock that serializes connection-owned conn->lgr/conn->lnk
updates against readers and use the new lock in the switch/free handoff paths.
In smc_conn_free(), NULL out conn->lgr and conn->lnk under lgr_lnk_lock before
releasing the borrowed references. This closes the window where a concurrent
diag reader could observe a non-NULL conn->lgr that points at already-freed
memory.
A non-NULL conn->lgr seen under lgr_lnk_lock guarantees the connection-owned lgr
reference is still held and the object is alive; conn->lnk is likewise valid
when non-NULL under the lock. The follow-on diag fix relies on this invariant to
safely read lgr and link fields without borrowing an extra reference.
Reviewed-by: Hidayath Khan <hidayath@linux.ibm.com>
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
net/smc/af_smc.c | 1 +
net/smc/smc.h | 1 +
net/smc/smc_core.c | 10 +++++++++-
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index e9f93b3ab435..8eece96a9edf 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -408,6 +408,7 @@ void smc_sk_init(struct net *net, struct sock *sk, int protocol)
sock_lock_init_class_and_name(sk, "slock-AF_SMC", &smc_slock_key,
"sk_lock-AF_SMC", &smc_key);
spin_lock_init(&smc->accept_q_lock);
+ spin_lock_init(&smc->conn.lgr_lnk_lock);
spin_lock_init(&smc->conn.send_lock);
mutex_init(&smc->clcsock_release_lock);
smc_init_saved_callbacks(smc);
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 52145df83f6e..d5fb5d92809b 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -232,6 +232,7 @@ struct smc_connection {
atomic_t sndbuf_space; /* remaining space in sndbuf */
u16 tx_cdc_seq; /* sequence # for CDC send */
u16 tx_cdc_seq_fin; /* sequence # - tx completed */
+ spinlock_t lgr_lnk_lock; /* protect conn owned lgr/lnk refs */
spinlock_t send_lock; /* protect wr_sends */
atomic_t cdc_pend_tx_wr; /* number of pending tx CDC wqe
* - inc when post wqe,
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 181647982490..cc69b86b2ee5 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1083,6 +1083,7 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
void smc_switch_link_and_count(struct smc_connection *conn,
struct smc_link *to_lnk)
{
+ spin_lock_bh(&conn->lgr_lnk_lock);
atomic_dec(&conn->lnk->conn_cnt);
/* link_hold in smc_conn_create() */
smcr_link_put(conn->lnk);
@@ -1090,6 +1091,7 @@ void smc_switch_link_and_count(struct smc_connection *conn,
atomic_inc(&conn->lnk->conn_cnt);
/* link_put in smc_conn_free() */
smcr_link_hold(conn->lnk);
+ spin_unlock_bh(&conn->lgr_lnk_lock);
}
struct smc_link *smc_switch_conns(struct smc_link_group *lgr,
@@ -1255,6 +1257,7 @@ static void smc_buf_unuse(struct smc_connection *conn,
void smc_conn_free(struct smc_connection *conn)
{
struct smc_link_group *lgr = conn->lgr;
+ struct smc_link *lnk;
if (!lgr || conn->freed)
/* Connection has never been registered in a
@@ -1287,8 +1290,13 @@ void smc_conn_free(struct smc_connection *conn)
if (!lgr->conns_num)
smc_lgr_schedule_free_work(lgr);
lgr_put:
+ spin_lock_bh(&conn->lgr_lnk_lock);
+ lnk = conn->lnk;
+ conn->lnk = NULL;
+ conn->lgr = NULL;
+ spin_unlock_bh(&conn->lgr_lnk_lock);
if (!lgr->is_smcd)
- smcr_link_put(conn->lnk); /* link_hold in smc_conn_create() */
+ smcr_link_put(lnk); /* link_hold in smc_conn_create() */
smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
}
--
2.50.1 (Apple Git-155)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH net v2 2/2] net/smc: fix races in smc_diag dump path
2026-08-28 6:54 [PATCH net v2 0/2] net/smc: fix diag dump lifetime races Mahanta Jambigi
2026-08-28 6:54 ` [PATCH net v2 1/2] net/smc: add connection lifetime infrastructure for diag Mahanta Jambigi
@ 2026-08-28 6:54 ` Mahanta Jambigi
2026-08-29 6:55 ` sashiko-bot
1 sibling, 1 reply; 5+ messages in thread
From: Mahanta Jambigi @ 2026-08-28 6:54 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, alibuda, dust.li,
sidraya, hidayath
Cc: pasic, horms, tonylu, guwen, stable, netdev, linux-s390,
linux-rdma, Mahanta Jambigi
Two races exist in the SMC diag dump path.
Race 1: smc->clcsock can be set to NULL by smc_clcsock_release() after a bare
NULL check in smc_diag_msg_common_fill() and before the subsequent field reads.
Use mutex_trylock() on clcsock_release_lock to serialize the check and the reads
with that writer. If the lock cannot be taken, teardown or fallback is in
progress; leaving address fields zeroed is a safe transient gap for a monitoring
tool.
Race 2: conn->lgr and conn->lnk can be torn down concurrently by
smc_conn_free(). Take conn->lgr_lnk_lock when accessing these fields in the dump
path. This covers three sites: the diag_mode classification
(conn->lgr->is_smcd), the SMC_DIAG_LGRINFO block, and the SMC_DIAG_DMBINFO
block. In each case, fields are snapshotted into local variables under the lock
and used after releasing it. With patch 1/2, a non-NULL conn->lgr seen under
that lock guarantees the lgr is alive. conn->lnk is likewise valid when non-NULL
under the lock. nla_put() is called after releasing the lock.
This design avoids borrowing an extra lgr reference, which would require
smc_lgr_put() to be called inside __smc_diag_dump() -- a problem because
smc_diag_dump_proto() holds an atomic read_lock for the duration of that call.
Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
Reviewed-by: Hidayath Khan <hidayath@linux.ibm.com>
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index bf0beaa23bdb..06db3d769929 100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -39,8 +39,20 @@ static void smc_diag_msg_common_fill(struct smc_diag_msg *r, struct sock *sk)
memset(r, 0, sizeof(*r));
r->diag_family = sk->sk_family;
sock_diag_save_cookie(sk, r->id.idiag_cookie);
- if (!smc->clcsock)
+ /* smc_clcsock_release() sets smc->clcsock = NULL under
+ * clcsock_release_lock. Use mutex_trylock() to make the NULL check and all
+ * field reads atomic with that writer. mutex_trylock() is safe under the
+ * hash read_lock held by smc_diag_dump_proto() because it never sleeps. If
+ * it fails, the socket's clcsock is being modified or released (teardown
+ * or fallback transition); leaving address fields zeroed is a safe
+ * transient gap for a monitoring tool.
+ */
+ if (!mutex_trylock(&smc->clcsock_release_lock))
return;
+ if (!smc->clcsock) {
+ mutex_unlock(&smc->clcsock_release_lock);
+ return;
+ }
r->id.idiag_sport = htons(smc->clcsock->sk->sk_num);
r->id.idiag_dport = smc->clcsock->sk->sk_dport;
r->id.idiag_if = smc->clcsock->sk->sk_bound_dev_if;
@@ -55,6 +67,7 @@ static void smc_diag_msg_common_fill(struct smc_diag_msg *r, struct sock *sk)
sizeof(smc->clcsock->sk->sk_v6_daddr));
#endif
}
+ mutex_unlock(&smc->clcsock_release_lock);
}
static int smc_diag_msg_attrs_fill(struct sock *sk, struct sk_buff *skb,
@@ -88,12 +101,18 @@ 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;
- if (smc->use_fallback)
+ 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)
- r->diag_mode = SMC_DIAG_MODE_SMCD;
- else
- r->diag_mode = SMC_DIAG_MODE_SMCR;
+ } else {
+ bool is_smcd = false;
+
+ spin_lock_bh(&smc->conn.lgr_lnk_lock);
+ if (smc_conn_lgr_valid(&smc->conn))
+ is_smcd = smc->conn.lgr->is_smcd;
+ spin_unlock_bh(&smc->conn.lgr_lnk_lock);
+ r->diag_mode = is_smcd ? SMC_DIAG_MODE_SMCD
+ : SMC_DIAG_MODE_SMCR;
+ }
user_ns = sk_user_ns(NETLINK_CB(cb->skb).sk);
if (smc_diag_msg_attrs_fill(sk, skb, r, user_ns))
goto errout;
@@ -143,45 +162,57 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
goto errout;
}
- if (smc_conn_lgr_valid(&smc->conn) && !smc->conn.lgr->is_smcd &&
- (req->diag_ext & (1 << (SMC_DIAG_LGRINFO - 1))) &&
- !list_empty(&smc->conn.lgr->list)) {
- struct smc_link *link = smc->conn.lnk;
-
- struct smc_diag_lgrinfo linfo = {
- .role = smc->conn.lgr->role,
- .lnk[0].ibport = link->ibport,
- .lnk[0].link_id = link->link_id,
- };
-
- memcpy(linfo.lnk[0].ibname, link->smcibdev->ibdev->name,
- sizeof(link->smcibdev->ibdev->name));
- smc_gid_be16_convert(linfo.lnk[0].gid, link->gid);
- smc_gid_be16_convert(linfo.lnk[0].peer_gid, link->peer_gid);
-
- if (nla_put(skb, SMC_DIAG_LGRINFO, sizeof(linfo), &linfo) < 0)
+ if (req->diag_ext & (1 << (SMC_DIAG_LGRINFO - 1))) {
+ struct smc_connection *conn = &smc->conn;
+ struct smc_diag_lgrinfo linfo;
+ bool lgr_valid = false;
+
+ memset(&linfo, 0, sizeof(linfo));
+ spin_lock_bh(&conn->lgr_lnk_lock);
+ if (smc_conn_lgr_valid(conn) && !conn->lgr->is_smcd &&
+ conn->lnk && !list_empty(&conn->lgr->list)) {
+ linfo.role = conn->lgr->role;
+ linfo.lnk[0].ibport = conn->lnk->ibport;
+ linfo.lnk[0].link_id = conn->lnk->link_id;
+ memcpy(linfo.lnk[0].ibname,
+ conn->lnk->smcibdev->ibdev->name,
+ sizeof(conn->lnk->smcibdev->ibdev->name));
+ smc_gid_be16_convert(linfo.lnk[0].gid,
+ conn->lnk->gid);
+ smc_gid_be16_convert(linfo.lnk[0].peer_gid,
+ conn->lnk->peer_gid);
+ lgr_valid = true;
+ }
+ spin_unlock_bh(&conn->lgr_lnk_lock);
+ if (lgr_valid &&
+ nla_put(skb, SMC_DIAG_LGRINFO, sizeof(linfo), &linfo) < 0)
goto errout;
}
- if (smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd &&
- (req->diag_ext & (1 << (SMC_DIAG_DMBINFO - 1))) &&
- !list_empty(&smc->conn.lgr->list) && smc->conn.rmb_desc) {
+ if (req->diag_ext & (1 << (SMC_DIAG_DMBINFO - 1))) {
struct smc_connection *conn = &smc->conn;
struct smcd_diag_dmbinfo dinfo;
- struct smcd_dev *smcd = conn->lgr->smcd;
- struct smcd_gid smcd_gid;
+ bool lgr_valid = false;
memset(&dinfo, 0, sizeof(dinfo));
-
- dinfo.linkid = *((u32 *)conn->lgr->id);
- dinfo.peer_gid = conn->lgr->peer_gid.gid;
- dinfo.peer_gid_ext = conn->lgr->peer_gid.gid_ext;
- copy_to_smcdgid(&smcd_gid, &smcd->dibs->gid);
- dinfo.my_gid = smcd_gid.gid;
- dinfo.my_gid_ext = smcd_gid.gid_ext;
- dinfo.token = conn->rmb_desc->token;
- dinfo.peer_token = conn->peer_token;
-
- if (nla_put(skb, SMC_DIAG_DMBINFO, sizeof(dinfo), &dinfo) < 0)
+ spin_lock_bh(&conn->lgr_lnk_lock);
+ if (smc_conn_lgr_valid(conn) && conn->lgr->is_smcd &&
+ conn->rmb_desc && !list_empty(&conn->lgr->list)) {
+ struct smcd_dev *smcd = conn->lgr->smcd;
+ struct smcd_gid smcd_gid;
+
+ dinfo.linkid = *((u32 *)conn->lgr->id);
+ dinfo.peer_gid = conn->lgr->peer_gid.gid;
+ dinfo.peer_gid_ext = conn->lgr->peer_gid.gid_ext;
+ copy_to_smcdgid(&smcd_gid, &smcd->dibs->gid);
+ dinfo.my_gid = smcd_gid.gid;
+ dinfo.my_gid_ext = smcd_gid.gid_ext;
+ dinfo.token = conn->rmb_desc->token;
+ dinfo.peer_token = conn->peer_token;
+ lgr_valid = true;
+ }
+ spin_unlock_bh(&conn->lgr_lnk_lock);
+ if (lgr_valid &&
+ nla_put(skb, SMC_DIAG_DMBINFO, sizeof(dinfo), &dinfo) < 0)
goto errout;
}
--
2.50.1 (Apple Git-155)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net v2 2/2] net/smc: fix races in smc_diag dump path
2026-08-28 6:54 ` [PATCH net v2 2/2] net/smc: fix races in smc_diag dump path Mahanta Jambigi
@ 2026-08-29 6:55 ` sashiko-bot
0 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-08-29 6:55 UTC (permalink / raw)
To: Mahanta Jambigi
Cc: linux-s390, Heiko Carstens, Alexander Gordeev,
Christian Borntraeger, Vasily Gorbik
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Incomplete fix: Missing protection against concurrent connection teardown for SMC_DIAG_CONNINFO, leading to Use-After-Free on conn->sndbuf_desc and conn->rmb_desc.
--
commit 7eff481cca86fa2c9b695508a4e20ae6a02805ad
Author: Mahanta Jambigi <mjambigi@linux.ibm.com>
net/smc: fix races in smc_diag dump path
This commit addresses concurrent teardown races in the SMC diag dump path
by adding mutex protection to clcsock reads and spinlock protection to
the LGRINFO and DMBINFO block accesses.
> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index bf0beaa23bdb6..06db3d769929a 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
[ ... ]
> @@ -143,45 +162,57 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
> goto errout;
> }
>
[Severity: High]
This is a pre-existing issue, but does this incomplete fix leave a
use-after-free regression open in the SMC_DIAG_CONNINFO block just above?
The patch successfully adds lgr_lnk_lock protection to the LGRINFO and
DMBINFO blocks below to prevent concurrent teardown races, but completely
omits this protection for the CONNINFO block.
In __smc_diag_dump(), the CONNINFO block locklessly checks
smc->conn.alert_token_local:
if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) &&
smc->conn.alert_token_local) {
struct smc_connection *conn = &smc->conn;
struct smc_diag_conninfo cinfo = {
.token = conn->alert_token_local,
.sndbuf_size = conn->sndbuf_desc ?
conn->sndbuf_desc->len : 0,
If smc_conn_free() executes concurrently on another CPU immediately after
the token check, it will free conn->sndbuf_desc and conn->rmb_desc via
smcd_buf_detach().
The dump thread then locklessly dereferences these pointers when
accessing ->len, leading to a use-after-free that can be triggered by
an unprivileged user running a socket dump during a network abort.
Should the CONNINFO block also be wrapped in the same lgr_lnk_lock
protection that was applied to the other diagnostic blocks?
> - if (smc_conn_lgr_valid(&smc->conn) && !smc->conn.lgr->is_smcd &&
> - (req->diag_ext & (1 << (SMC_DIAG_LGRINFO - 1))) &&
> - !list_empty(&smc->conn.lgr->list)) {
> - struct smc_link *link = smc->conn.lnk;
> -
> - struct smc_diag_lgrinfo linfo = {
> - .role = smc->conn.lgr->role,
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260828065439.3582783-1-mjambigi@linux.ibm.com?part=2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v2 1/2] net/smc: add connection lifetime infrastructure for diag
2026-08-28 6:54 ` [PATCH net v2 1/2] net/smc: add connection lifetime infrastructure for diag Mahanta Jambigi
@ 2026-08-29 6:55 ` sashiko-bot
0 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-08-29 6:55 UTC (permalink / raw)
To: Mahanta Jambigi
Cc: Alexander Gordeev, Christian Borntraeger, linux-s390,
Heiko Carstens, Vasily Gorbik
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Setting `conn->lnk = NULL` and `conn->lgr = NULL` in `smc_conn_free()` introduces fatal NULL pointer dereferences in concurrent and deferred readers.
- [Medium] `conn->lnk` is modified in `smc_cdc_msg_validate()` without holding the newly introduced `lgr_lnk_lock`, violating the stated synchronization invariants.
--
commit 340542b4422ee1313d9e6622b416469a93adf1be
Author: Mahanta Jambigi <mjambigi@linux.ibm.com>
net/smc: add connection lifetime infrastructure for diag
Add the basic lifetime synchronization needed by the diag dump path.
Introduce a per-connection lock that serializes connection-owned
conn->lgr/conn->lnk updates against readers. In smc_conn_free(), NULL out
conn->lgr and conn->lnk under lgr_lnk_lock before releasing references to
close the window where a concurrent diag reader could observe freed memory.
> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 427b6d63b9931..f5ed51d8eb939 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -232,6 +232,7 @@ struct smc_connection {
> atomic_t sndbuf_space; /* remaining space in sndbuf */
> u16 tx_cdc_seq; /* sequence # for CDC send */
> u16 tx_cdc_seq_fin; /* sequence # - tx completed */
> + spinlock_t lgr_lnk_lock; /* protect conn owned lgr/lnk refs */
[Severity: Medium]
If this lock is required for all updates to conn->lnk as stated in the
commit message, should we also take it in
net/smc/smc_cdc.c:smc_cdc_msg_validate()?
net/smc/smc_cdc.c:smc_cdc_msg_validate() {
...
conn->out_of_sync = 1;
spin_lock_bh(&conn->send_lock);
conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
conn->lnk = link;
spin_unlock_bh(&conn->send_lock);
...
}
Without lgr_lnk_lock here, concurrent diag readers could observe an
inconsistent link state.
> spinlock_t send_lock; /* protect wr_sends */
> atomic_t cdc_pend_tx_wr; /* number of pending tx CDC wqe
> * - inc when post wqe,
[ ... ]
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 04aedd957543a..5af55abece577 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
[ ... ]
> @@ -1287,8 +1290,13 @@ void smc_conn_free(struct smc_connection *conn)
> if (!lgr->conns_num)
> smc_lgr_schedule_free_work(lgr);
> lgr_put:
> + spin_lock_bh(&conn->lgr_lnk_lock);
> + lnk = conn->lnk;
> + conn->lnk = NULL;
> + conn->lgr = NULL;
[Severity: Critical]
Does setting these to NULL here introduce NULL pointer dereferences in
existing paths that don't take lgr_lnk_lock?
For example, in net/smc/smc_core.c:smc_conn_kill(), if smc_conn_free()
sets conn->lgr to NULL, and a concurrent smc_lgr_terminate_work() is
waiting for the socket lock:
smc_conn_free()
conn->lgr = NULL;
smc_lgr_terminate_work()
smc_conn_kill()
if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
Similarly, a deferred release callback in net/smc/smc_tx.c can race:
smc_release_cb()
smc_tx_sndbuf_nonempty()
if (conn->lgr->is_smcd)
Also, concurrent threads calling smc_switch_conns() in
net/smc/smc_core.c can race and dereference the newly NULL conn->lnk:
smc_switch_conns()
smc_switch_link_and_count()
atomic_dec(&conn->lnk->conn_cnt);
> + spin_unlock_bh(&conn->lgr_lnk_lock);
> if (!lgr->is_smcd)
> - smcr_link_put(conn->lnk); /* link_hold in smc_conn_create() */
> + smcr_link_put(lnk); /* link_hold in smc_conn_create() */
> smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260828065439.3582783-1-mjambigi@linux.ibm.com?part=1
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-08-29 6:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-28 6:54 [PATCH net v2 0/2] net/smc: fix diag dump lifetime races Mahanta Jambigi
2026-08-28 6:54 ` [PATCH net v2 1/2] net/smc: add connection lifetime infrastructure for diag Mahanta Jambigi
2026-08-29 6:55 ` sashiko-bot
2026-08-28 6:54 ` [PATCH net v2 2/2] net/smc: fix races in smc_diag dump path Mahanta Jambigi
2026-08-29 6:55 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox