[PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[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

[PATCH net v2 1/2] net/smc: add connection lifetime infrastructure for diag

[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)

[PATCH net v2 2/2] net/smc: fix races in smc_diag dump path

[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)

Re: [PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[Dust Li]

On 2026-08-28 08:54:37, Mahanta Jambigi wrote:
>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

Hi Mahanta,

As discussed in the other thread, I think we should defer the release of
smc->clcsock and remove clcsock_release_lock.

In that case, we should no longer need these two patches. Also,
introducing more locks in SMC is the last thing I want to do :)

Best regards,
Dust

Re: [PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[Mahanta Jambigi]

On 31/08/26 7:12 pm, Dust Li wrote:
> On 2026-08-28 08:54:37, Mahanta Jambigi wrote:
>> 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
> 
> Hi Mahanta,
> 
> As discussed in the other thread, I think we should defer the release of
> smc->clcsock and remove clcsock_release_lock.
> 
> In that case, we should no longer need these two patches. Also,
> introducing more locks in SMC is the last thing I want to do :)

Does your "[RFC net-next 0/7] net/smc: tie clcsock lifetime to the smc
socket and remove clcsock_release_lock" resolve conn.lgr/conn.lnk races
in __smc_diag_dump() against the concurrent tear-down paths?

Re: [PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[Mahanta Jambigi]

On 31/08/26 7:12 pm, Dust Li wrote:
> On 2026-08-28 08:54:37, Mahanta Jambigi wrote:
>> 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
> 
> Hi Mahanta,
> 
> As discussed in the other thread, I think we should defer the release of
> smc->clcsock and remove clcsock_release_lock.
> 
> In that case, we should no longer need these two patches. Also,
> introducing more locks in SMC is the last thing I want to do :)

Thanks for the new series "[RFC net-next 0/7] net/smc: tie clcsock
lifetime to the smc socket and remove clcsock_release_lock" — once it
lands, we can drop the mutex_trylock() fix for Race 1 (clcsock).

However, Race 2 remains open. Your series does not touch smc_core.c or
smc_cdc.c, so smc_conn_free(), smc_switch_link_and_count(), and
smc_cdc_msg_validate() still write conn->lgr/conn->lnk with no
synchronization against the diag reader.

On the lock concern — lgr_lnk_lock is a per-connection spinlock. It is
taken in smc_conn_free() (teardown), smc_switch_link_and_count() (link
failover), smc_cdc_msg_validate() (failover validation branch only, not
the normal CDC data path), and the diag reader. None of these are on the
per-message send/receive hot path. Could you explain what specifically
concerns you — lock ordering, memory footprint, or something else? That
would help us understand whether you have a different mechanism in mind.

Re: [PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[Dust Li]

On 2026-08-31 20:27:38, Mahanta Jambigi wrote:
>
>
>On 31/08/26 7:12 pm, Dust Li wrote:
>> On 2026-08-28 08:54:37, Mahanta Jambigi wrote:
>>> 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
>> 
>> Hi Mahanta,
>> 
>> As discussed in the other thread, I think we should defer the release of
>> smc->clcsock and remove clcsock_release_lock.
>> 
>> In that case, we should no longer need these two patches. Also,
>> introducing more locks in SMC is the last thing I want to do :)
>
>Thanks for the new series "[RFC net-next 0/7] net/smc: tie clcsock
>lifetime to the smc socket and remove clcsock_release_lock" — once it
>lands, we can drop the mutex_trylock() fix for Race 1 (clcsock).
>
>However, Race 2 remains open. Your series does not touch smc_core.c or
>smc_cdc.c, so smc_conn_free(), smc_switch_link_and_count(), and
>smc_cdc_msg_validate() still write conn->lgr/conn->lnk with no
>synchronization against the diag reader.

Hi Mahanta,

Thanks for the detailed explanation. You are right that a per-connection
spinlock can work here, and I agree none of the lock sites are on the
per-message hot path. But I think we can also do the same thing we did with
clcsock_release_lock: instead of adding a lock, tie the lifetime of
conn->lgr/conn->lnk to the point where the connection stops being observable,
and remove the need for synchronization altogether.

For lgr/lnk, that point is the hash table. Once the connection is unhashed, the
diag dump (which iterates under the hash read_lock) can no longer reach it. So
if we make sure the connection-owned references are only dropped after unhash,
the invariant becomes: holding the hash read_lock and seeing a non-NULL
conn->lgr implies it is safe to dereference. The diag path then reduces to
`hold hash read_lock -> read conn->lgr -> if non-NULL, use it -> done` with no
new lock, no extra reference, and no trylock. The invariant is carried by
object lifetime rather than by a lock, which I find easier to keep correct over
time.

Best regards,
Dust

Re: [PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[Mahanta Jambigi]

On 01/09/26 6:32 pm, Dust Li wrote:
> On 2026-08-31 20:27:38, Mahanta Jambigi wrote:
>>
>>
>> On 31/08/26 7:12 pm, Dust Li wrote:
>>> On 2026-08-28 08:54:37, Mahanta Jambigi wrote:
>>>> 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
>>>
>>> Hi Mahanta,
>>>
>>> As discussed in the other thread, I think we should defer the release of
>>> smc->clcsock and remove clcsock_release_lock.
>>>
>>> In that case, we should no longer need these two patches. Also,
>>> introducing more locks in SMC is the last thing I want to do :)
>>
>> Thanks for the new series "[RFC net-next 0/7] net/smc: tie clcsock
>> lifetime to the smc socket and remove clcsock_release_lock" — once it
>> lands, we can drop the mutex_trylock() fix for Race 1 (clcsock).
>>
>> However, Race 2 remains open. Your series does not touch smc_core.c or
>> smc_cdc.c, so smc_conn_free(), smc_switch_link_and_count(), and
>> smc_cdc_msg_validate() still write conn->lgr/conn->lnk with no
>> synchronization against the diag reader.
> 
> Hi Mahanta,
> 
> Thanks for the detailed explanation. You are right that a per-connection
> spinlock can work here, and I agree none of the lock sites are on the
> per-message hot path. But I think we can also do the same thing we did with
> clcsock_release_lock: instead of adding a lock, tie the lifetime of
> conn->lgr/conn->lnk to the point where the connection stops being observable,
> and remove the need for synchronization altogether.
> 
> For lgr/lnk, that point is the hash table. Once the connection is unhashed, the
> diag dump (which iterates under the hash read_lock) can no longer reach it. So
> if we make sure the connection-owned references are only dropped after unhash,
> the invariant becomes: holding the hash read_lock and seeing a non-NULL
> conn->lgr implies it is safe to dereference. The diag path then reduces to
> `hold hash read_lock -> read conn->lgr -> if non-NULL, use it -> done` with no
> new lock, no extra reference, and no trylock. The invariant is carried by
> object lifetime rather than by a lock, which I find easier to keep correct over
> time.

I looked carefully at the new design and found one remaining gap.

The *unhash* invariant — "any socket in the hash has its
connection-owned lgr/lnk refs held" — protects against smc_conn_free()
dropping refs while the socket is still hashed. However it does not
protect the conn->lnk->smcibdev->ibdev->name access in the
SMC_DIAG_LGRINFO block in smc_diag.c file.

The gap is in *smc_switch_link_and_count*(). It is called under
send_lock (not under any hash-related lock) and calls
smcr_link_put(conn->lnk) on the old link before reassigning conn->lnk.
If that put drops the last reference, __smcr_link_clear() runs
immediately, doing memset(lnk, 0, sizeof(struct smc_link)) which zeroes
lnk->smcibdev. The socket remains hashed throughout — so the *unhash*
invariant is not violated — but the diag reader can hold a stale pointer
to the old link and race this memset. The timeline:

diag reader [hash read_lock held]:
    conn->lnk → old_lnk (non-NULL, socket hashed ✓)
    [about to read old_lnk->smcibdev->ibdev->name]

smc_switch_link_and_count() [send_lock held]:
    smcr_link_put(old_lnk) → last ref → __smcr_link_clear()
        memset(old_lnk, 0, ...) ← smcibdev = NULL

diag reader:
    old_lnk->smcibdev->ibdev->name ← NULL deref

The *unhash* invariant says nothing about the link a connection used to
point at before *smc_switch_link_and_count*() swapped it. Hash
membership of the socket provides no protection here because the socket
is still hashed — the link pointer simply changed underneath the diag
reader.

Any ideas on this?

Re: [PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[Dust Li]

On 2026-09-01 20:43:11, Mahanta Jambigi wrote:
>
>
>On 01/09/26 6:32 pm, Dust Li wrote:
>> On 2026-08-31 20:27:38, Mahanta Jambigi wrote:
>>>
>>>
>>> On 31/08/26 7:12 pm, Dust Li wrote:
>>>> On 2026-08-28 08:54:37, Mahanta Jambigi wrote:
>>>>> 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
>>>>
>>>> Hi Mahanta,
>>>>
>>>> As discussed in the other thread, I think we should defer the release of
>>>> smc->clcsock and remove clcsock_release_lock.
>>>>
>>>> In that case, we should no longer need these two patches. Also,
>>>> introducing more locks in SMC is the last thing I want to do :)
>>>
>>> Thanks for the new series "[RFC net-next 0/7] net/smc: tie clcsock
>>> lifetime to the smc socket and remove clcsock_release_lock" — once it
>>> lands, we can drop the mutex_trylock() fix for Race 1 (clcsock).
>>>
>>> However, Race 2 remains open. Your series does not touch smc_core.c or
>>> smc_cdc.c, so smc_conn_free(), smc_switch_link_and_count(), and
>>> smc_cdc_msg_validate() still write conn->lgr/conn->lnk with no
>>> synchronization against the diag reader.
>> 
>> Hi Mahanta,
>> 
>> Thanks for the detailed explanation. You are right that a per-connection
>> spinlock can work here, and I agree none of the lock sites are on the
>> per-message hot path. But I think we can also do the same thing we did with
>> clcsock_release_lock: instead of adding a lock, tie the lifetime of
>> conn->lgr/conn->lnk to the point where the connection stops being observable,
>> and remove the need for synchronization altogether.
>> 
>> For lgr/lnk, that point is the hash table. Once the connection is unhashed, the
>> diag dump (which iterates under the hash read_lock) can no longer reach it. So
>> if we make sure the connection-owned references are only dropped after unhash,
>> the invariant becomes: holding the hash read_lock and seeing a non-NULL
>> conn->lgr implies it is safe to dereference. The diag path then reduces to
>> `hold hash read_lock -> read conn->lgr -> if non-NULL, use it -> done` with no
>> new lock, no extra reference, and no trylock. The invariant is carried by
>> object lifetime rather than by a lock, which I find easier to keep correct over
>> time.
>
>I looked carefully at the new design and found one remaining gap.
>
>The *unhash* invariant — "any socket in the hash has its
>connection-owned lgr/lnk refs held" — protects against smc_conn_free()
>dropping refs while the socket is still hashed. However it does not
>protect the conn->lnk->smcibdev->ibdev->name access in the
>SMC_DIAG_LGRINFO block in smc_diag.c file.
>
>The gap is in *smc_switch_link_and_count*(). It is called under
>send_lock (not under any hash-related lock) and calls
>smcr_link_put(conn->lnk) on the old link before reassigning conn->lnk.
>If that put drops the last reference, __smcr_link_clear() runs
>immediately, doing memset(lnk, 0, sizeof(struct smc_link)) which zeroes
>lnk->smcibdev. The socket remains hashed throughout — so the *unhash*
>invariant is not violated — but the diag reader can hold a stale pointer
>to the old link and race this memset. The timeline:
>
>diag reader [hash read_lock held]:
>    conn->lnk → old_lnk (non-NULL, socket hashed ✓)
>    [about to read old_lnk->smcibdev->ibdev->name]
>
>smc_switch_link_and_count() [send_lock held]:
>    smcr_link_put(old_lnk) → last ref → __smcr_link_clear()
>        memset(old_lnk, 0, ...) ← smcibdev = NULL
>
>diag reader:
>    old_lnk->smcibdev->ibdev->name ← NULL deref
>
>The *unhash* invariant says nothing about the link a connection used to
>point at before *smc_switch_link_and_count*() swapped it. Hash
>membership of the socket provides no protection here because the socket
>is still hashed — the link pointer simply changed underneath the diag
>reader.
>
>Any ideas on this?

Good catch ! You are right that the "unhash invariant" as stated
does not cover the stale-conn->lnk race through a link switch + clear.

I think we can close this window by holding the SMC hash table lock in
smcr_link_clear()? As the diag walker captures and dereferences the stale
pointer within a single read_lock hold on the SMC hash table. Since
smc_link_clear() is a cold path, and the region where we hold the SMC hash
table lock is small, the overhead should be negligible.


Something like this:

```diff
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 5af55abece57..ca3030f02e70 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1361,6 +1361,18 @@ static void __smcr_link_clear(struct smc_link *lnk)
        struct smc_ib_device *smcibdev;
 
        smc_wr_free_link_mem(lnk);
 
+       write_lock_bh(&smc_v4_hashinfo.lock);
+       write_lock_bh(&smc_v6_hashinfo.lock);
        smc_ibdev_cnt_dec(lnk);
        put_device(&lnk->smcibdev->ibdev->dev);
        smcibdev = lnk->smcibdev;
@@ -1368,6 +1380,9 @@ static void __smcr_link_clear(struct smc_link *lnk)
        lnk->state = SMC_LNK_UNUSED;
        if (!atomic_dec_return(&smcibdev->lnk_cnt))
                wake_up(&smcibdev->lnks_deleted);
+       write_unlock_bh(&smc_v6_hashinfo.lock);
+       write_unlock_bh(&smc_v4_hashinfo.lock);
```

What do you think ?

Best regards,
Dust

Re: [PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[Mahanta Jambigi]

On 02/09/26 5:46 pm, Dust Li wrote:
> On 2026-09-01 20:43:11, Mahanta Jambigi wrote:
>>
>>
>> On 01/09/26 6:32 pm, Dust Li wrote:
>>> On 2026-08-31 20:27:38, Mahanta Jambigi wrote:
>>>>
>>>>
>>>> On 31/08/26 7:12 pm, Dust Li wrote:
>>>>> On 2026-08-28 08:54:37, Mahanta Jambigi wrote:
>>>>>> 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
>>>>>
>>>>> Hi Mahanta,
>>>>>
>>>>> As discussed in the other thread, I think we should defer the release of
>>>>> smc->clcsock and remove clcsock_release_lock.
>>>>>
>>>>> In that case, we should no longer need these two patches. Also,
>>>>> introducing more locks in SMC is the last thing I want to do :)
>>>>
>>>> Thanks for the new series "[RFC net-next 0/7] net/smc: tie clcsock
>>>> lifetime to the smc socket and remove clcsock_release_lock" — once it
>>>> lands, we can drop the mutex_trylock() fix for Race 1 (clcsock).
>>>>
>>>> However, Race 2 remains open. Your series does not touch smc_core.c or
>>>> smc_cdc.c, so smc_conn_free(), smc_switch_link_and_count(), and
>>>> smc_cdc_msg_validate() still write conn->lgr/conn->lnk with no
>>>> synchronization against the diag reader.
>>>
>>> Hi Mahanta,
>>>
>>> Thanks for the detailed explanation. You are right that a per-connection
>>> spinlock can work here, and I agree none of the lock sites are on the
>>> per-message hot path. But I think we can also do the same thing we did with
>>> clcsock_release_lock: instead of adding a lock, tie the lifetime of
>>> conn->lgr/conn->lnk to the point where the connection stops being observable,
>>> and remove the need for synchronization altogether.
>>>
>>> For lgr/lnk, that point is the hash table. Once the connection is unhashed, the
>>> diag dump (which iterates under the hash read_lock) can no longer reach it. So
>>> if we make sure the connection-owned references are only dropped after unhash,
>>> the invariant becomes: holding the hash read_lock and seeing a non-NULL
>>> conn->lgr implies it is safe to dereference. The diag path then reduces to
>>> `hold hash read_lock -> read conn->lgr -> if non-NULL, use it -> done` with no
>>> new lock, no extra reference, and no trylock. The invariant is carried by
>>> object lifetime rather than by a lock, which I find easier to keep correct over
>>> time.
>>
>> I looked carefully at the new design and found one remaining gap.
>>
>> The *unhash* invariant — "any socket in the hash has its
>> connection-owned lgr/lnk refs held" — protects against smc_conn_free()
>> dropping refs while the socket is still hashed. However it does not
>> protect the conn->lnk->smcibdev->ibdev->name access in the
>> SMC_DIAG_LGRINFO block in smc_diag.c file.
>>
>> The gap is in *smc_switch_link_and_count*(). It is called under
>> send_lock (not under any hash-related lock) and calls
>> smcr_link_put(conn->lnk) on the old link before reassigning conn->lnk.
>> If that put drops the last reference, __smcr_link_clear() runs
>> immediately, doing memset(lnk, 0, sizeof(struct smc_link)) which zeroes
>> lnk->smcibdev. The socket remains hashed throughout — so the *unhash*
>> invariant is not violated — but the diag reader can hold a stale pointer
>> to the old link and race this memset. The timeline:
>>
>> diag reader [hash read_lock held]:
>>    conn->lnk → old_lnk (non-NULL, socket hashed ✓)
>>    [about to read old_lnk->smcibdev->ibdev->name]
>>
>> smc_switch_link_and_count() [send_lock held]:
>>    smcr_link_put(old_lnk) → last ref → __smcr_link_clear()
>>        memset(old_lnk, 0, ...) ← smcibdev = NULL
>>
>> diag reader:
>>    old_lnk->smcibdev->ibdev->name ← NULL deref
>>
>> The *unhash* invariant says nothing about the link a connection used to
>> point at before *smc_switch_link_and_count*() swapped it. Hash
>> membership of the socket provides no protection here because the socket
>> is still hashed — the link pointer simply changed underneath the diag
>> reader.
>>
>> Any ideas on this?
> 
> Good catch ! You are right that the "unhash invariant" as stated
> does not cover the stale-conn->lnk race through a link switch + clear.
> 
> I think we can close this window by holding the SMC hash table lock in
> smcr_link_clear()? As the diag walker captures and dereferences the stale
> pointer within a single read_lock hold on the SMC hash table. Since
> smc_link_clear() is a cold path, and the region where we hold the SMC hash
> table lock is small, the overhead should be negligible.
> 
> 
> Something like this:
> 
> ```diff
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 5af55abece57..ca3030f02e70 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1361,6 +1361,18 @@ static void __smcr_link_clear(struct smc_link *lnk)
>         struct smc_ib_device *smcibdev;
>  
>         smc_wr_free_link_mem(lnk);
>  
> +       write_lock_bh(&smc_v4_hashinfo.lock);
> +       write_lock_bh(&smc_v6_hashinfo.lock);
>         smc_ibdev_cnt_dec(lnk);
>         put_device(&lnk->smcibdev->ibdev->dev);
>         smcibdev = lnk->smcibdev;
> @@ -1368,6 +1380,9 @@ static void __smcr_link_clear(struct smc_link *lnk)
>         lnk->state = SMC_LNK_UNUSED;
>         if (!atomic_dec_return(&smcibdev->lnk_cnt))
>                 wake_up(&smcibdev->lnks_deleted);
> +       write_unlock_bh(&smc_v6_hashinfo.lock);
> +       write_unlock_bh(&smc_v4_hashinfo.lock);
> ```
> 
> What do you think ?
Hi Dust,

After debugging further, I believe that __smcr_link_clear() cannot be
called from smc_switch_link_and_count() because
refcount_dec_and_test(&lnk->refcnt) is always false on that path — the
initial ref (set in smcr_link_init(), released only in
smcr_link_clear()) is always held while the switch executes. So there is
no UAF from that path. The only consequence is that the diag reader may
observe a stale conn->lnk pointer value — pointing to the old link which
is still fully live — and read the old link's ibport/link_id/ibname from it.

The real UAF is via smc_conn_free(): it drops both the connection-owned
conn->lnk reference via smcr_link_put() and the connection-owned
conn->lgr reference via smc_lgr_put(), while the socket is still visible
in the hash table. For conn->lgr this means kfree(lgr) can race with the
diag reader dereferencing lgr->is_smcd, lgr->role, lgr->list etc. For
conn->lnk this means memset(lnk, 0, ...) in __smcr_link_clear() can race
with the diag reader dereferencing link->smcibdev->ibdev->name. The
current patch tries to fix this using lgr_lnk_lock. Since you suggested
we fix it with *unhash-before-free*, I thought about it and here is my
proposal.

Proposed fix (high level):

Introduce a smc_conn_unhash() helper with a per-connection unhashed flag
that ensures the socket is removed from the hash table exactly once.
Call it at the top of smc_conn_free(), before any lgr/lnk references are
dropped.

This establishes the invariant: any socket still visible to the diag
reader under read_lock(hash->lock) has valid conn->lgr and conn->lnk
pointers. The mutual exclusion between write_lock_bh(hash->lock) inside
smc_conn_unhash() and the diag reader's read_lock(hash->lock) ensures
that by the time smcr_link_put() runs in smc_conn_free(), the socket is
already gone from the hash — the diag reader either completes before the
unhash or never sees the socket at all.

The unhashed flag is needed because smc_conn_free() can be called from
multiple paths (e.g. smc_conn_kill() ahead of __smc_release()), so we
need to guarantee the unhash happens exactly once.

Does this design look reasonable to you?

There is a separate minor issue — smc_switch_link_and_count() updates
conn->lnk under send_lock while the diag reader loads it without any
lock, which can cause stale ibport/link_id/ibname in *smcss -R* during a
failover event. This is a data race but not a safety issue since the
link struct is always live at that point.

This race is very rare in practice — it requires a concurrent smcss -R
dump to hit the exact CPU-cycle window during a conn->lnk pointer write,
which itself only happens during an exceptional link failover event.
Even when it occurs, the effect is transient: one dump may show the old
link's ibport/link_id/ibname, and the next dump will show the correct
values.

Re: [PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[Dust Li]

On 2026-09-03 13:26:30, Mahanta Jambigi wrote:
> On 02/09/26 5:46 pm, Dust Li wrote:
> > I think we can close this window by holding the SMC hash table lock in
> > smcr_link_clear()? [...]
>
> Hi Dust,
>
> After debugging further, I believe that __smcr_link_clear() cannot be
> called from smc_switch_link_and_count() because
> refcount_dec_and_test(&lnk->refcnt) is always false on that path — the
> initial ref (set in smcr_link_init(), released only in
> smcr_link_clear()) is always held while the switch executes. So there is
> no UAF from that path. The only consequence is that the diag reader may
> observe a stale conn->lnk pointer value — pointing to the old link which
> is still fully live — and read the old link's ibport/link_id/ibname from it.

Hi Mahanta,

Sorry for the late reply. I've done some more thinking on this topic.

That's true, but strictly I think there is still a small gap. The old link is
memset later, in smcr_link_clear() after all connections have migrated, so a
reader that loaded conn->lnk just before its own connection was switched has a
few-instruction window before it dereferences. In practice this window is
really narrow: the clear path runs the whole switch + LLC + QP teardown, orders
of magnitude longer than the reader's load+deref.

> The real UAF is via smc_conn_free(): it drops both the connection-owned
> conn->lnk reference via smcr_link_put() and the connection-owned
> conn->lgr reference via smc_lgr_put(), while the socket is still visible
> in the hash table. [...] The current patch tries to fix this using
> lgr_lnk_lock. Since you suggested we fix it with *unhash-before-free*, I
> thought about it and here is my proposal.
>
> Proposed fix (high level):
>
> Introduce a smc_conn_unhash() helper with a per-connection unhashed flag
> that ensures the socket is removed from the hash table exactly once.
> Call it at the top of smc_conn_free(), before any lgr/lnk references are
> dropped.
>
> This establishes the invariant: any socket still visible to the diag
> reader under read_lock(hash->lock) has valid conn->lgr and conn->lnk
> pointers. [...]
>
> Does this design look reasonable to you?

Yes, I think it's the right fix and we can go ahead and fix it this way first.

> There is a separate minor issue — smc_switch_link_and_count() updates
> conn->lnk under send_lock while the diag reader loads it without any
> lock, which can cause stale ibport/link_id/ibname in *smcss -R* during a
> failover event. This is a data race but not a safety issue since the
> link struct is always live at that point.
>
> This race is very rare in practice [...] Even when it occurs, the effect
> is transient: one dump may show the old link's ibport/link_id/ibname, and
> the next dump will show the correct values.

I think this is acceptable.

---

I've been re-thinking this a bit more. We've been plagued by SMC's tangled
locks and ad-hoc lifetime handling for years — every fix adds another lock or
another ordering rule. I think it's time to step back and refactor this area as
a whole, and set up some rules for how we use locks/refcounts in SMC, instead of
keeping patching individual races. Below are some rough thoughts.

1. Object layering

SMC really has three lifetime tiers, and the top one has to be split the way
TCP splits struct socket from struct sock:

- the file (struct socket) — lifetime tied to fput;
- the connection sock (smc_sock, with conn embedded) — lifetime tied to
  sk_refcnt; like a TCP sock it can be orphaned and outlive the file. On an
  active close, close(fd) orphans it first, but it stays alive — still bound to
  the transport — to finish the close handshake;
- the shared transport (lgr / link / device) — lifetime by refcount, multiplexed
  across connections.

It's the socket/sock split, plus one more tier because our transport is shared
(TCP's sock is 1:1, our lgr/link is not). Today smc_conn_free() mixes all three
and is called from the transport layer while the socket is still hashed —
that's exactly where these races come from.

With the tiers separated, teardown becomes two independent, idempotent steps
rather than one:

- orphan (file <-> sock): at fput, via sock_orphan();
- transport-detach (sock <-> transport): the connection drops its lgr/link
  refs. On an active close this happens later, after the handshake completes;
  on a transport fault it happens immediately.

The sock is freed only once it is orphaned, transport-detached, and the last
sock_put lands; the order of the two steps just depends on who initiates
teardown (app close vs. transport fault).

2. Lifetime & boundary contract

Give each tier the right tool, and a clear boundary between them:

- ownership by kref; traversal by RCU (conn->lgr / conn->lnk become RCU
  pointers, the lgr is freed with kfree_rcu); fd-visible objects (clcsock)
  released only after the last fput; and no in-place mutation of a published
  object (stop memset()ing a link — mark it dead and reclaim it with the lgr
  after a grace period);

- boundary rule: on a transport fault the lower layer only signals and
  transport-detaches; it never orphans, unhashes, or frees the sock.
  - signal = wake the app with an error (sk_err + wakeup);
  - transport-detach = publish rcu_assign_pointer(conn->lgr/lnk, NULL), then
    drop the usage references (smc_lgr_put / smcr_link_put). The transport
    object itself is reclaimed later by its own refcount + kfree_rcu; the sock
    is left untouched.
  Handle-side teardown (orphan, unhash, clcsock release, final sock_put)
  belongs to the connection/file side and is driven by close — never by the
  transport layer.

With that, a diag reader under rcu_read_lock is guaranteed the link/lgr
outlives its critical section, so the per-connection lgr_lnk_lock is no longer
needed and smc_conn_unhash() can be retired too. (clcsock_release_lock goes
away separately, via the clcsock lifetime series.) It's a larger, mostly
mechanical change and would take a lot of careful rework, so I'd do it as a
follow-up. Your smc_conn_unhash() is the right fix to take now (and for
stable); the rework, if we agree on this direction, would retire it afterwards.

Best regards,
Dust

Re: [PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[Hidayath Khan]

On 04/09/26 8:51 pm, Dust Li wrote:
> On 2026-09-03 13:26:30, Mahanta Jambigi wrote:
>> On 02/09/26 5:46 pm, Dust Li wrote:
>>> I think we can close this window by holding the SMC hash table lock in
>>> smcr_link_clear()? [...]
>> Hi Dust,
>>
>> After debugging further, I believe that __smcr_link_clear() cannot be
>> called from smc_switch_link_and_count() because
>> refcount_dec_and_test(&lnk->refcnt) is always false on that path — the
>> initial ref (set in smcr_link_init(), released only in
>> smcr_link_clear()) is always held while the switch executes. So there is
>> no UAF from that path. The only consequence is that the diag reader may
>> observe a stale conn->lnk pointer value — pointing to the old link which
>> is still fully live — and read the old link's ibport/link_id/ibname from it.
> Hi Mahanta,
>
> Sorry for the late reply. I've done some more thinking on this topic.
>
> That's true, but strictly I think there is still a small gap. The old link is
> memset later, in smcr_link_clear() after all connections have migrated, so a
> reader that loaded conn->lnk just before its own connection was switched has a
> few-instruction window before it dereferences. In practice this window is
> really narrow: the clear path runs the whole switch + LLC + QP teardown, orders
> of magnitude longer than the reader's load+deref.
>
>> The real UAF is via smc_conn_free(): it drops both the connection-owned
>> conn->lnk reference via smcr_link_put() and the connection-owned
>> conn->lgr reference via smc_lgr_put(), while the socket is still visible
>> in the hash table. [...] The current patch tries to fix this using
>> lgr_lnk_lock. Since you suggested we fix it with *unhash-before-free*, I
>> thought about it and here is my proposal.
>>
>> Proposed fix (high level):
>>
>> Introduce a smc_conn_unhash() helper with a per-connection unhashed flag
>> that ensures the socket is removed from the hash table exactly once.
>> Call it at the top of smc_conn_free(), before any lgr/lnk references are
>> dropped.
>>
>> This establishes the invariant: any socket still visible to the diag
>> reader under read_lock(hash->lock) has valid conn->lgr and conn->lnk
>> pointers. [...]
>>
>> Does this design look reasonable to you?
> Yes, I think it's the right fix and we can go ahead and fix it this way first.
>
>> There is a separate minor issue — smc_switch_link_and_count() updates
>> conn->lnk under send_lock while the diag reader loads it without any
>> lock, which can cause stale ibport/link_id/ibname in *smcss -R* during a
>> failover event. This is a data race but not a safety issue since the
>> link struct is always live at that point.
>>
>> This race is very rare in practice [...] Even when it occurs, the effect
>> is transient: one dump may show the old link's ibport/link_id/ibname, and
>> the next dump will show the correct values.
> I think this is acceptable.
>
> ---
>
> I've been re-thinking this a bit more. We've been plagued by SMC's tangled
> locks and ad-hoc lifetime handling for years — every fix adds another lock or
> another ordering rule. I think it's time to step back and refactor this area as
> a whole, and set up some rules for how we use locks/refcounts in SMC, instead of
> keeping patching individual races. Below are some rough thoughts.
>
> 1. Object layering
>
> SMC really has three lifetime tiers, and the top one has to be split the way
> TCP splits struct socket from struct sock:
>
> - the file (struct socket) — lifetime tied to fput;
> - the connection sock (smc_sock, with conn embedded) — lifetime tied to
>    sk_refcnt; like a TCP sock it can be orphaned and outlive the file. On an
>    active close, close(fd) orphans it first, but it stays alive — still bound to
>    the transport — to finish the close handshake;
> - the shared transport (lgr / link / device) — lifetime by refcount, multiplexed
>    across connections.
>
> It's the socket/sock split, plus one more tier because our transport is shared
> (TCP's sock is 1:1, our lgr/link is not). Today smc_conn_free() mixes all three
> and is called from the transport layer while the socket is still hashed —
> that's exactly where these races come from.
>
> With the tiers separated, teardown becomes two independent, idempotent steps
> rather than one:
>
> - orphan (file <-> sock): at fput, via sock_orphan();
> - transport-detach (sock <-> transport): the connection drops its lgr/link
>    refs. On an active close this happens later, after the handshake completes;
>    on a transport fault it happens immediately.
>
> The sock is freed only once it is orphaned, transport-detached, and the last
> sock_put lands; the order of the two steps just depends on who initiates
> teardown (app close vs. transport fault).
>
> 2. Lifetime & boundary contract
>
> Give each tier the right tool, and a clear boundary between them:
>
> - ownership by kref; traversal by RCU (conn->lgr / conn->lnk become RCU
>    pointers, the lgr is freed with kfree_rcu); fd-visible objects (clcsock)
>    released only after the last fput; and no in-place mutation of a published
>    object (stop memset()ing a link — mark it dead and reclaim it with the lgr
>    after a grace period);
>
> - boundary rule: on a transport fault the lower layer only signals and
>    transport-detaches; it never orphans, unhashes, or frees the sock.
>    - signal = wake the app with an error (sk_err + wakeup);
>    - transport-detach = publish rcu_assign_pointer(conn->lgr/lnk, NULL), then
>      drop the usage references (smc_lgr_put / smcr_link_put). The transport
>      object itself is reclaimed later by its own refcount + kfree_rcu; the sock
>      is left untouched.
>    Handle-side teardown (orphan, unhash, clcsock release, final sock_put)
>    belongs to the connection/file side and is driven by close — never by the
>    transport layer.
>
> With that, a diag reader under rcu_read_lock is guaranteed the link/lgr
> outlives its critical section, so the per-connection lgr_lnk_lock is no longer
> needed and smc_conn_unhash() can be retired too. (clcsock_release_lock goes
> away separately, via the clcsock lifetime series.) It's a larger, mostly
> mechanical change and would take a lot of careful rework, so I'd do it as a
> follow-up. Your smc_conn_unhash() is the right fix to take now (and for
> stable); the rework, if we agree on this direction, would retire it afterwards.
Hi Dust,

Thanks for writing this up and for the pointer from my abort_work patch.

You describe three tiers, with lgr, link and device as the transport one.
I was not sure where the buffer descriptor fits. It does not seem to
follow the lgr: smc_buf_unuse() returns conn->rmb_desc to lgr->rmbs, and
smc_buf_get_slot() can then hand it to another connection while the lgr is
still alive.

Two places look like they can outlive it:

- a splice reader still holding pipe pages, since smc_buf_unuse() does not
   consult conn->splice_pending;

- smc_cdc_msg_recv_action() and smc_cdc_handle_urg_data_arrival(), which
   read conn->rmb_desc after smc_cdc_rx_handler() has already left
   conns_lock. On the is_reg_err path smcr_buf_unuse() frees the
   descriptor rather than recycling it.

Have you already considered the descriptor in this model? Would it need
its own kref, or is it meant to sit inside the transport tier? I may be
missing something here.

One smaller question, on the boundary rule. The SMC-D side already looks
close to what you describe: smcd_handle_irq() holds smcd->lock across both
the lookup and the tasklet_schedule(), and smc_ism_unset_conn() clears the
slot under the same lock. The SMC-R receive path drops conns_lock between
the lookup and the action. Is that difference something the refactor
would make uniform, or is there a reason the receive path cannot hold the
lock that long?

If it would be useful, I can write up the lifetime assumptions I have run
into while working through the teardown paths. Please tell me if that
helps, or if it would only repeat what you already have.

Thanks,
Hidayath
>
> Best regards,
> Dust

Re: [PATCH net v2 0/2] net/smc: fix diag dump lifetime races

[Dust Li]

On 2026-09-08 21:53:14, Hidayath Khan wrote:
>
>On 04/09/26 8:51 pm, Dust Li wrote:
>> On 2026-09-03 13:26:30, Mahanta Jambigi wrote:
>> > On 02/09/26 5:46 pm, Dust Li wrote:
>> > > I think we can close this window by holding the SMC hash table lock in
>> > > smcr_link_clear()? [...]
>> > Hi Dust,
>> > 
>> > After debugging further, I believe that __smcr_link_clear() cannot be
>> > called from smc_switch_link_and_count() because
>> > refcount_dec_and_test(&lnk->refcnt) is always false on that path — the
>> > initial ref (set in smcr_link_init(), released only in
>> > smcr_link_clear()) is always held while the switch executes. So there is
>> > no UAF from that path. The only consequence is that the diag reader may
>> > observe a stale conn->lnk pointer value — pointing to the old link which
>> > is still fully live — and read the old link's ibport/link_id/ibname from it.
>> Hi Mahanta,
>> 
>> Sorry for the late reply. I've done some more thinking on this topic.
>> 
>> That's true, but strictly I think there is still a small gap. The old link is
>> memset later, in smcr_link_clear() after all connections have migrated, so a
>> reader that loaded conn->lnk just before its own connection was switched has a
>> few-instruction window before it dereferences. In practice this window is
>> really narrow: the clear path runs the whole switch + LLC + QP teardown, orders
>> of magnitude longer than the reader's load+deref.
>> 
>> > The real UAF is via smc_conn_free(): it drops both the connection-owned
>> > conn->lnk reference via smcr_link_put() and the connection-owned
>> > conn->lgr reference via smc_lgr_put(), while the socket is still visible
>> > in the hash table. [...] The current patch tries to fix this using
>> > lgr_lnk_lock. Since you suggested we fix it with *unhash-before-free*, I
>> > thought about it and here is my proposal.
>> > 
>> > Proposed fix (high level):
>> > 
>> > Introduce a smc_conn_unhash() helper with a per-connection unhashed flag
>> > that ensures the socket is removed from the hash table exactly once.
>> > Call it at the top of smc_conn_free(), before any lgr/lnk references are
>> > dropped.
>> > 
>> > This establishes the invariant: any socket still visible to the diag
>> > reader under read_lock(hash->lock) has valid conn->lgr and conn->lnk
>> > pointers. [...]
>> > 
>> > Does this design look reasonable to you?
>> Yes, I think it's the right fix and we can go ahead and fix it this way first.
>> 
>> > There is a separate minor issue — smc_switch_link_and_count() updates
>> > conn->lnk under send_lock while the diag reader loads it without any
>> > lock, which can cause stale ibport/link_id/ibname in *smcss -R* during a
>> > failover event. This is a data race but not a safety issue since the
>> > link struct is always live at that point.
>> > 
>> > This race is very rare in practice [...] Even when it occurs, the effect
>> > is transient: one dump may show the old link's ibport/link_id/ibname, and
>> > the next dump will show the correct values.
>> I think this is acceptable.
>> 
>> ---
>> 
>> I've been re-thinking this a bit more. We've been plagued by SMC's tangled
>> locks and ad-hoc lifetime handling for years — every fix adds another lock or
>> another ordering rule. I think it's time to step back and refactor this area as
>> a whole, and set up some rules for how we use locks/refcounts in SMC, instead of
>> keeping patching individual races. Below are some rough thoughts.
>> 
>> 1. Object layering
>> 
>> SMC really has three lifetime tiers, and the top one has to be split the way
>> TCP splits struct socket from struct sock:
>> 
>> - the file (struct socket) — lifetime tied to fput;
>> - the connection sock (smc_sock, with conn embedded) — lifetime tied to
>>    sk_refcnt; like a TCP sock it can be orphaned and outlive the file. On an
>>    active close, close(fd) orphans it first, but it stays alive — still bound to
>>    the transport — to finish the close handshake;
>> - the shared transport (lgr / link / device) — lifetime by refcount, multiplexed
>>    across connections.
>> 
>> It's the socket/sock split, plus one more tier because our transport is shared
>> (TCP's sock is 1:1, our lgr/link is not). Today smc_conn_free() mixes all three
>> and is called from the transport layer while the socket is still hashed —
>> that's exactly where these races come from.
>> 
>> With the tiers separated, teardown becomes two independent, idempotent steps
>> rather than one:
>> 
>> - orphan (file <-> sock): at fput, via sock_orphan();
>> - transport-detach (sock <-> transport): the connection drops its lgr/link
>>    refs. On an active close this happens later, after the handshake completes;
>>    on a transport fault it happens immediately.
>> 
>> The sock is freed only once it is orphaned, transport-detached, and the last
>> sock_put lands; the order of the two steps just depends on who initiates
>> teardown (app close vs. transport fault).
>> 
>> 2. Lifetime & boundary contract
>> 
>> Give each tier the right tool, and a clear boundary between them:
>> 
>> - ownership by kref; traversal by RCU (conn->lgr / conn->lnk become RCU
>>    pointers, the lgr is freed with kfree_rcu); fd-visible objects (clcsock)
>>    released only after the last fput; and no in-place mutation of a published
>>    object (stop memset()ing a link — mark it dead and reclaim it with the lgr
>>    after a grace period);
>> 
>> - boundary rule: on a transport fault the lower layer only signals and
>>    transport-detaches; it never orphans, unhashes, or frees the sock.
>>    - signal = wake the app with an error (sk_err + wakeup);
>>    - transport-detach = publish rcu_assign_pointer(conn->lgr/lnk, NULL), then
>>      drop the usage references (smc_lgr_put / smcr_link_put). The transport
>>      object itself is reclaimed later by its own refcount + kfree_rcu; the sock
>>      is left untouched.
>>    Handle-side teardown (orphan, unhash, clcsock release, final sock_put)
>>    belongs to the connection/file side and is driven by close — never by the
>>    transport layer.
>> 
>> With that, a diag reader under rcu_read_lock is guaranteed the link/lgr
>> outlives its critical section, so the per-connection lgr_lnk_lock is no longer
>> needed and smc_conn_unhash() can be retired too. (clcsock_release_lock goes
>> away separately, via the clcsock lifetime series.) It's a larger, mostly
>> mechanical change and would take a lot of careful rework, so I'd do it as a
>> follow-up. Your smc_conn_unhash() is the right fix to take now (and for
>> stable); the rework, if we agree on this direction, would retire it afterwards.
>Hi Dust,
>
>Thanks for writing this up and for the pointer from my abort_work patch.
>
>You describe three tiers, with lgr, link and device as the transport one.
>I was not sure where the buffer descriptor fits. It does not seem to
>follow the lgr: smc_buf_unuse() returns conn->rmb_desc to lgr->rmbs, and
>smc_buf_get_slot() can then hand it to another connection while the lgr is
>still alive.
>
>Two places look like they can outlive it:
>
>- a splice reader still holding pipe pages, since smc_buf_unuse() does not
>  consult conn->splice_pending;
>
>- smc_cdc_msg_recv_action() and smc_cdc_handle_urg_data_arrival(), which
>  read conn->rmb_desc after smc_cdc_rx_handler() has already left
>  conns_lock. On the is_reg_err path smcr_buf_unuse() frees the
>  descriptor rather than recycling it.
>
>Have you already considered the descriptor in this model? Would it need
>its own kref, or is it meant to sit inside the transport tier? I may be
>missing something here.

Hi Hidayath,

I didn't think much about the descriptor before and thanks for point this out.
In this model, it belongs to the transport tier (it lives in the lgr buffer
pool, multiplexed across connections), and the conn->rmb_desc / sndbuf_desc
binding is a usage reference just like conn->lnk.

So buf_desc should get a kref: the pool holds one reference for existence,
and every user holds one -- the bound connection, each outstanding splice
pipe buffer, and an in-flight rx reader. A descriptor may return to the
pool (->used cleared, eligible to be re-handed by smc_buf_get_slot()) only
once *all* of them are dropped, not just when the owning conn detaches --
otherwise a lingering reader sees zeroed or another connection's data,
since the same memory gets recycled.

That should cover your two cases:

- splice: get_page() pins the physical page, but nothing stops
  smc_buf_unuse() from zeroing the buffer and returning the slot to the
  pool while a pipe still references it (and it never consults
  splice_pending). With the kref, each splice pipe buffer holds a
  reference, so the slot is recycled only after the last page is
  released.

- rx after conns_lock: the receive action reads conn->rmb_desc holding
  only a sock reference, racing smc_conn_free() -> smc_buf_unuse() (and
  on the is_reg_err path smcr_buf_unuse() frees the descriptor outright).
  Making conn->rmb_desc an RCU pointer, read under rcu_read_lock in the
  receive path, closes this: a late reader either sees NULL (detached) or
  a descriptor that outlives its read-side section.

>
>One smaller question, on the boundary rule. The SMC-D side already looks
>close to what you describe: smcd_handle_irq() holds smcd->lock across both
>the lookup and the tasklet_schedule(), and smc_ism_unset_conn() clears the
>slot under the same lock. The SMC-R receive path drops conns_lock between
>the lookup and the action. Is that difference something the refactor
>would make uniform, or is there a reason the receive path cannot hold the
>lock that long?

There is a real reason they differ today: SMC-R does the receive inline in
the completion softirq, and holding conns_lock (a bh rwlock) across that
would invert against the teardown/migration paths that take it for write
under lock_sock. SMC-D only schedules a tasklet under smcd->lock, which is
cheap, and defers the work.

But I'd rather not unify them by holding a lock longer -- that is the same
"use a lock for lifetime" pattern I'd like to move away from. The refactor
unifies the mechanism instead: both sides bridge the lookup -> action gap
with a reference + RCU, not with a lock or a drain.

For SMC-D the tasklet is the subtle part. RCU does not span the tasklet
boundary: smcd_handle_irq()'s read-side section ends when the IRQ returns,
and the tasklet runs later, possibly after a grace period. So a reference,
not RCU, has to carry the connection across that gap:

- smcd_handle_irq() takes a sock reference under smcd->lock before
  tasklet_schedule(), and the tasklet drops it at the end -- exactly
  what the SMC-R rx handler already does;
- the tasklet reads conn->rmb_desc under its own rcu_read_lock();
- smcd->lock stays as the state lock for the dmbno -> conn slot table,
  and smc_ism_unset_conn() keeps clearing the slot, but its job becomes
  "stop scheduling new work" rather than "keep the tasklet off freed
  memory";

So yes, the refactor makes the two uniform -- through references + RCU,
not by extending conns_lock.

More generally, the discipline I'd like us to adopt: don't use locks (or
drains, or ad-hoc flags) to manage object lifetime -- use a kref for ownership
plus RCU for traversal. A lock only covers the paths that remember to take it,
so it always leaves one uncovered; a reference + RCU makes "can't be reclaimed
while observed" structural.


>
>If it would be useful, I can write up the lifetime assumptions I have run
>into while working through the teardown paths. Please tell me if that
>helps, or if it would only repeat what you already have.

Please do -- I'd frame it as stress-testing the model rather than just
documenting it. buf_desc is exactly the kind of gap I want to shake out
before we commit to this direction: an object my write-up missed, which
you found by walking the teardown paths. So it wouldn't be a repeat of
what I have.

Best regards,
Dust