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

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

[Mahanta Jambigi]

The SMC diag dump path reads conn->lgr and conn->lnk while iterating the socket
hash table under a read_lock.  Concurrently, RDMA link failure teardown
(__smc_lgr_terminate -> smc_conn_kill -> smc_close_active_abort ->
smc_conn_free) and the passive close workqueue path (smc_close_passive_work ->
smc_conn_free) can drop the connection-owned references to lgr and lnk while the
socket is still visible in the hash, allowing the diag reader to dereference a
freed lgr or lnk.

Fix this by ensuring that for all non-fallback paths the socket is removed from
the hash table before the lgr/lnk references are dropped in smc_conn_free().

Introduce smc_conn_unhash() with a per-connection 'unhashed' flag so that the
unhash executes exactly once regardless of which path reaches smc_conn_free()
first.  smc_conn_free() calls smc_conn_unhash() before the lgr_put/link_put
sequence, establishing the invariant: any socket still visible to the diag
reader under the hash read_lock has valid conn->lgr and conn->lnk pointers.

__smc_release() is updated to use smc_conn_unhash() for non-fallback sockets (so
the flag is honoured when smc_conn_free() already ran, e.g.  via smc_conn_kill()
ahead of the user-space close()), and keeps the direct sk->sk_prot->unhash()
call only for fallback sockets, which never call smc_conn_free().

The diag path therefore reduces to:
  hold hash read_lock -> read conn->lgr -> if non-NULL, dereference -> done
with no new lock, no extra reference count, and no trylock.

Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets")
Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections")
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
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   | 10 +++++++++-
 net/smc/smc.h      |  1 +
 net/smc/smc_core.c | 20 ++++++++++++++++++++
 net/smc/smc_core.h |  1 +
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index e9f93b3ab435..8c781a4a4485 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -310,7 +310,15 @@ static int __smc_release(struct smc_sock *smc)
 		smc_restore_fallback_changes(smc);
 	}

-	sk->sk_prot->unhash(sk);
+	/* Fallback sockets never call smc_conn_free(), so unhash directly.
+	 * Non-fallback sockets use smc_conn_unhash() so that the conn->unhashed
+	 * flag keeps the unhash exactly once even when smc_conn_free() already ran
+	 * first (e.g. via smc_conn_kill()).
+	 */
+	if (smc->use_fallback)
+		sk->sk_prot->unhash(sk);
+	else
+		smc_conn_unhash(&smc->conn);

 	if (sk->sk_state == SMC_CLOSED) {
 		if (smc->clcsock) {
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 427b6d63b993..075312278835 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -279,6 +279,7 @@ struct smc_connection {
 	u64			peer_token;	/* SMC-D token of peer */
 	u8			killed;		/* abnormal termination */
 	u8			freed;		/* normal termination */
+	u8			unhashed;	/* removed from sock hash */
 	u8			out_of_sync;	/* out of sync with peer */
 };

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 04aedd957543..e302221c35e3 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1251,6 +1251,20 @@ static void smc_buf_unuse(struct smc_connection *conn,
 	}
 }

+/* unhash the socket once; owns the single unhash for all non-fallback paths.
+ * Every caller holds lock_sock for this socket, so conn->unhashed is protected
+ * by that lock and no separate synchronisation is needed.
+ */
+void smc_conn_unhash(struct smc_connection *conn)
+{
+	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);
+
+	if (!conn->unhashed) {
+		conn->unhashed = 1;
+		smc->sk.sk_prot->unhash(&smc->sk);
+	}
+}
+
 /* remove a finished connection from its link group */
 void smc_conn_free(struct smc_connection *conn)
 {
@@ -1263,6 +1277,11 @@ void smc_conn_free(struct smc_connection *conn)
 		return;

 	conn->freed = 1;
+	/* Unhash before dropping lgr/lnk refs so the diag reader, which
+	 * iterates under the socket hash read_lock, cannot see a connection whose
+	 * lgr or lnk is being freed concurrently.
+	 */
+	smc_conn_unhash(conn);
 	if (!smc_conn_lgr_valid(conn))
 		/* Connection has already unregistered from
 		 * link group.
@@ -2053,6 +2072,7 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
 	if (!conn->lgr->is_smcd)
 		smcr_link_hold(conn->lnk); /* link_put in smc_conn_free() */
 	conn->freed = 0;
+	conn->unhashed = 0;
 	conn->local_tx_ctrl.common.type = SMC_CDC_MSG_TYPE;
 	conn->local_tx_ctrl.len = SMC_WR_TX_SIZE;
 	conn->urg_state = SMC_URG_READ;
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index 5c18f08a4c8a..f23d60aef0b7 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -595,6 +595,7 @@ void smc_sndbuf_sync_sg_for_device(struct smc_connection *conn);
 void smc_rmb_sync_sg_for_cpu(struct smc_connection *conn);
 int smc_vlan_by_tcpsk(struct socket *clcsock, struct smc_init_info *ini);

+void smc_conn_unhash(struct smc_connection *conn);
 void smc_conn_free(struct smc_connection *conn);
 int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini);
 int smc_core_init(void);
-- 
2.50.1 (Apple Git-155)

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

[Mahanta Jambigi]

On 09/09/26 2:53 pm, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Premature unhashing of SMC sockets breaks netlink diagnostics for fallback and aborted connections.
> 
> Pre-existing issues:
> - [High] Concurrent link failover clears link memory while diagnostic reader accesses it, causing a NULL pointer dereference.
> --
> 
> commit c0cbdd828a851e80ad0c79e6d2ccf5b0b5d7e16e
> Author: Mahanta Jambigi <mjambigi@linux.ibm.com>
> 
>     net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This commit aims to resolve a race condition between the SMC diagnostic dump
> reader and connection teardown. It introduces a mechanism to ensure sockets
> are unhashed before their link group and link references are released in
> smc_conn_free(), preventing the diagnostic reader from accessing freed
> structures.
> 
>>     The diag path therefore reduces to:
>>       hold hash read_lock -> read conn->lgr -> if non-NULL, dereference -> done
>>     with no new lock, no extra reference count, and no trylock.
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this approach still leave
> the link memory vulnerable to concurrent clearance during a link failover?
> 
> When __smc_diag_dump() in net/smc/smc_diag.c iterates over the hash table, it
> extracts link = smc->conn.lnk. Concurrently, a link failure could trigger
> smcr_link_down() -> smc_switch_conns() -> smc_switch_link_and_count().
> 
> The link is swapped and the old link's reference is dropped locklessly with
> respect to the dump:
> 
> net/smc/smc_core.c:smc_switch_link_and_count() {
>     ...
>     smcr_link_put(conn->lnk);
>     conn->lnk = to_lnk;
>     ...
> }
> 
> Next, smcr_link_down() calls smcr_link_clear(). When the reference count hits
> zero, __smcr_link_clear() zeroes out the link memory:
> 
> net/smc/smc_core.c:__smcr_link_clear() {
>     ...
>     memset(lnk, 0, sizeof(struct smc_link));
>     ...
> }
> 
> The diagnostic reader then evaluates link->smcibdev->ibdev->name. Since the
> link was zeroed, link->smcibdev is NULL, leading to a fatal dereference.
> Can the diagnostic reader safely dereference link->smcibdev without holding
> any connection-level lock that protects the link from concurrent clearance?

This is a false positive. The key is the link refcount accounting.

A link starts with refcnt = 1 (set in smcr_link_init()). Each
connection on that link holds an additional ref taken in
smc_conn_create() via smcr_link_hold(), paired with the
smcr_link_put() in smc_conn_free(). So for a link with N active
connections, refcnt = 1 + N.

smc_switch_link_and_count() drops only the *per-connection* ref
for the migrated connection:

    smcr_link_put(conn->lnk);   /* refcnt: (1+N) -> (1+N-1) */

For refcount_dec_and_test() to return true and trigger
__smcr_link_clear() -> memset(), the refcount would need to reach
zero. But the base ref of 1 (set at link init) is still live, so the
count never hits zero from this put.

The base ref is dropped only inside smcr_link_clear() at line 1381:

    smcr_link_put(lnk); /* theoretically last link_put */

smcr_link_clear() is called from smcr_link_down() *after*
smc_switch_conns() has already migrated all connections away. By
the time the base ref is dropped and __smcr_link_clear() fires,
no connection points to the old link anymore -- so the diag reader
cannot reach a being-zeroed link through conn->lnk.

The ordering in smcr_link_down() is:
  1. smc_switch_conns()   <- all conn->lnk updated, per-conn puts done
  2. smcr_link_clear()    <- base ref dropped, memset runs

Step 2 cannot interleave with step 1; memset is only reachable after
all connection refs have been dropped. The diag reader's conn->lnk
dereference is therefore safe.

> 
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index e9f93b3ab435b..8c781a4a4485b 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -310,7 +310,15 @@ static int __smc_release(struct smc_sock *smc)
> [ ... ]
>> -	sk->sk_prot->unhash(sk);
>> +	/* Fallback sockets never call smc_conn_free(), so unhash directly.
>> +	 * Non-fallback sockets use smc_conn_unhash() so that the conn->unhashed
>> +	 * flag keeps the unhash exactly once even when smc_conn_free() already ran
>> +	 * first (e.g. via smc_conn_kill()).
>> +	 */
> 
> [Severity: Medium]
> Is the assumption that fallback sockets never call smc_conn_free() accurate?
> 
> If a late fallback occurs, it invokes smc_conn_abort(), which calls
> smc_conn_free() before switching to TCP fallback:
> 
> net/smc/af_smc.c:smc_listen_decline() {
>     ...
>     smc_conn_abort(new_smc, local_first);
>     if (reason_code < 0 ||
>         smc_switch_to_fallback(new_smc, reason_code)) {
>     ...
> }
> 
> Does this mean fallback sockets can traverse the smc_conn_free() path?
> 
>> +	if (smc->use_fallback)
>> +		sk->sk_prot->unhash(sk);
>> +	else
>> +		smc_conn_unhash(&smc->conn);
>>  
> [ ... ]
>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>> index 04aedd957543a..e302221c35e3f 100644
>> --- a/net/smc/smc_core.c
>> +++ b/net/smc/smc_core.c
>> @@ -1263,6 +1277,11 @@ void smc_conn_free(struct smc_connection *conn)
>>  		return;
>>  
>>  	conn->freed = 1;
>> +	/* Unhash before dropping lgr/lnk refs so the diag reader, which
>> +	 * iterates under the socket hash read_lock, cannot see a connection whose
>> +	 * lgr or lnk is being freed concurrently.
>> +	 */
>> +	smc_conn_unhash(conn);
> 
> [Severity: Medium]
> Will this prematurely unhash SMC sockets during connection aborts and late
> fallback transitions?
> 
> Since smc_conn_free() is called by smc_conn_abort(), and late fallback calls
> smc_conn_abort() (e.g., smc_listen_decline() -> smc_conn_abort() ->
> smc_conn_free()), the socket is removed from the SMC hash table despite
> remaining an active, functioning fallback socket.
> 
> Could this hide fallback and aborted sockets from diagnostic monitoring tools
> that rely on the hash table (using SMC_DIAG_MODE_FALLBACK_TCP), breaking
> observability for these sockets?

You are right. smc_conn_abort() calls smc_conn_free() while
use_fallback is still false, so placing smc_conn_unhash() inside
smc_conn_free() prematurely removes the socket from the hash before
smc_switch_to_fallback() runs — making live fallback sockets
invisible to smcss. I'll fix it in the next version.

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (1) · Medium: 0 · Low: 0

- [High] The commit message asserts the invariant "any socket still
  visible to the diag reader under the hash read_lock has valid…
- [High] The unconditional smc_conn_unhash() added at the head of
  smc_conn_free() (net/smc/smc_core.c:1284) removes the socket from the…

Pre-existing issues:
- [High] smc_cdc_msg_validate() (net/smc/smc_cdc.c:308-327) retargets
  conn->lnk to the link a stray failover-validation CDC message arrived…

--

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

This patch adds smc_conn_unhash() plus a per-connection unhashed flag and
calls it at the top of smc_conn_free(), so the socket leaves the SMC socket
hash before the connection-owned lgr and lnk references are dropped.
__smc_release() is changed to use the new helper for non-fallback sockets
and keeps the direct sk->sk_prot->unhash() call for fallback sockets.

A few questions below about the paths that reach smc_conn_free() and about
the invariant described in the changelog.

> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index e9f93b3ab435b..8c781a4a4485b 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -310,7 +310,15 @@ static int __smc_release(struct smc_sock *smc)
>  		smc_restore_fallback_changes(smc);
>  	}
>  
> -	sk->sk_prot->unhash(sk);
> +	/* Fallback sockets never call smc_conn_free(), so unhash directly.
> +	 * Non-fallback sockets use smc_conn_unhash() so that the conn->unhashed
> +	 * flag keeps the unhash exactly once even when smc_conn_free() already ran
> +	 * first (e.g. via smc_conn_kill()).
> +	 */
> +	if (smc->use_fallback)
> +		sk->sk_prot->unhash(sk);
> +	else
> +		smc_conn_unhash(&smc->conn);

[Severity: High]
Is the premise "Fallback sockets never call smc_conn_free()" accurate?
smc_listen_decline() calls smc_conn_abort() first and only afterwards turns
the socket into a fallback socket:

net/smc/af_smc.c:smc_listen_decline() {
	/* RDMA setup failed, switch back to TCP */
	smc_conn_abort(new_smc, local_first);
	if (reason_code < 0 ||
	    smc_switch_to_fallback(new_smc, reason_code)) {
	...
	smc_listen_out_connected(new_smc);
}

net/smc/af_smc.c:smc_conn_abort() {
	...
	smc_conn_free(conn);
	...
}

The client side does the same via smc_connect_rdma()'s connect_abort label
followed by smc_connect_decline_fallback().

Beyond the comment, smc_conn_free() is also reached for connection
*attempts* on sockets that stay alive afterwards, and there is no path that
re-inserts the socket into the hash.  Can this make live sockets invisible
to smc_diag for the rest of their lifetime?

For the server ISM retry loop:

net/smc/af_smc.c:smc_listen_ism_init() {
	rc = smc_buf_create(new_smc, true);
	if (rc) {
		smc_conn_abort(new_smc, ini->first_contact_local);
		...
}

net/smc/af_smc.c:smc_find_ism_v2_device_serv() {
	for (i = 0; i < matches; i++) {
		...
		rc = smc_listen_ism_init(new_smc, ini);
		if (rc) {
			smc_init_info_store_rc(rc, ini);
			/* try next active ISM device */
			continue;
		}
		return; /* matching and usable V2 ISM device found */
	}
}

The failed attempt unhashes the socket through smc_conn_free(), the next
ISM device then succeeds, and smc_conn_create() only does
"conn->unhashed = 0" without calling sk->sk_prot->hash() again.  The same
shape exists for the v2 -> v1 RDMA fallthrough in
smc_find_rdma_v2_device_serv() -> smc_listen_find_device().

smc_diag_dump_proto() walks only that hash:

net/smc/smc_diag.c:smc_diag_dump_proto() {
	read_lock(&prot->h.smc_hash->lock);
	head = &prot->h.smc_hash->ht;
	...
	sk_for_each(sk, head) {
	...
}

so an established SMC-D/SMC-R connection or an active TCP-fallback
connection produced by these paths would no longer show up in ss --smc or
smcss, including the SMC_DIAG_MODE_FALLBACK_TCP reporting.  The same early
unhash also hides sockets that user space still has open after
__smc_lgr_terminate() -> smc_conn_kill() -> smc_close_active_abort() ->
smc_conn_free().  Should the unhash be restricted to terminal teardown, or
should the abort paths re-hash the socket?

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 04aedd957543a..e302221c35e3f 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1251,6 +1251,20 @@ static void smc_buf_unuse(struct smc_connection *conn,
>  	}
>  }
>  
> +/* unhash the socket once; owns the single unhash for all non-fallback paths.
> + * Every caller holds lock_sock for this socket, so conn->unhashed is protected
> + * by that lock and no separate synchronisation is needed.
> + */
> +void smc_conn_unhash(struct smc_connection *conn)
> +{
> +	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);
> +
> +	if (!conn->unhashed) {
> +		conn->unhashed = 1;
> +		smc->sk.sk_prot->unhash(&smc->sk);
> +	}
> +}
> +
>  /* remove a finished connection from its link group */
>  void smc_conn_free(struct smc_connection *conn)
>  {
> @@ -1263,6 +1277,11 @@ void smc_conn_free(struct smc_connection *conn)
>  		return;
>  
>  	conn->freed = 1;
> +	/* Unhash before dropping lgr/lnk refs so the diag reader, which
> +	 * iterates under the socket hash read_lock, cannot see a connection whose
> +	 * lgr or lnk is being freed concurrently.
> +	 */
> +	smc_conn_unhash(conn);

[Severity: High]
The changelog states the invariant as "any socket still visible to the diag
reader under the hash read_lock has valid conn->lgr and conn->lnk
pointers".  Does the SMC-R link failover path still break that for
conn->lnk?

smc_switch_link_and_count() drops the connection-owned link reference and
republishes conn->lnk while the socket stays hashed:

net/smc/smc_core.c:smc_switch_link_and_count() {
	atomic_dec(&conn->lnk->conn_cnt);
	/* link_hold in smc_conn_create() */
	smcr_link_put(conn->lnk);
	conn->lnk = to_lnk;
	...
}

It runs under conn->send_lock and lgr->conns_lock only, never under the
socket hash lock and without unhashing, so a reader that already
snapshotted the old link keeps using it:

net/smc/smc_diag.c:__smc_diag_dump() {
	struct smc_link *link = smc->conn.lnk;
	...
	memcpy(linfo.lnk[0].ibname, link->smcibdev->ibdev->name,
	       sizeof(link->smcibdev->ibdev->name));
	smc_gid_be16_convert(linfo.lnk[0].gid, link->gid);
	...
}

Once the last connection has been migrated off the old link,
smcr_link_clear() -> smcr_link_put() -> __smcr_link_clear() zeroes the
whole structure:

net/smc/smc_core.c:__smcr_link_clear() {
	put_device(&lnk->smcibdev->ibdev->dev);
	smcibdev = lnk->smcibdev;
	memset(lnk, 0, sizeof(struct smc_link));
	lnk->state = SMC_LNK_UNUSED;
	...
}

so link->smcibdev becomes NULL and the dereference above is a NULL
dereference, with the device reference already dropped.  This looks like
the same class of access the cited "Fixes: 9dbe086c69b8 (net/smc: fix
invalid link access in dumping SMC-R connections)" refers to, and the
reader/writer lock sets (hash read_lock versus send_lock/conns_lock/
llc_conf_mutex) do not intersect.  The v2 posting of this series protected
smc_switch_link_and_count() with lgr_lnk_lock; is anything replacing that
protection in v3, or should the invariant in the changelog be narrowed to
conn->lgr?

[Severity: High]
This is a pre-existing issue and not introduced by this patch, but it also
bears on the conn->lnk invariant being asserted: the reference that
smc_conn_free() releases is not always the one the connection holds.

smc_cdc_msg_validate() retargets conn->lnk with no reference transfer,
reached from smc_cdc_msg_recv() when the peer sets
prod_flags.failover_validation and the seqno is stale:

net/smc/smc_cdc.c:smc_cdc_msg_validate() {
	diff = conn->local_rx_ctrl.seqno - recv_seq;
	if (diff < 0) { /* diff larger than 0x7fff */
		...
		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);
	...
}

There is no smcr_link_hold() on the new link and no smcr_link_put() on the
old one, while the counted reference was taken in smc_conn_create():

	smcr_link_hold(conn->lnk); /* link_put in smc_conn_free() */

and smc_conn_free() later releases whatever conn->lnk points at:

	if (!lgr->is_smcd)
		smcr_link_put(conn->lnk); /* link_hold in smc_conn_create() */

In a multi-link link group, does this over-put the new link and leak a
reference on the old one?  The over-put can take the new link's refcount to
zero while other connections still cache it in conn->lnk, running
__smcr_link_clear() (put_device() plus memset()) before its QP is
destroyed, after which smcr_link_clear() returns early on the
SMC_LNK_UNUSED state.  The leaked reference on the old link would keep its
__smcr_link_clear() and the nested smc_lgr_put() from ever running.

>  	if (!smc_conn_lgr_valid(conn))
>  		/* Connection has already unregistered from
>  		 * link group.
> @@ -2053,6 +2072,7 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
>  	if (!conn->lgr->is_smcd)
>  		smcr_link_hold(conn->lnk); /* link_put in smc_conn_free() */
>  	conn->freed = 0;
> +	conn->unhashed = 0;

[ ... ]

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

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

[Mahanta Jambigi]

On 11/09/26 5:54 am, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 2 (1) · Medium: 0 · Low: 0
> 
> - [High] The commit message asserts the invariant "any socket still
>   visible to the diag reader under the hash read_lock has valid…
> - [High] The unconditional smc_conn_unhash() added at the head of
>   smc_conn_free() (net/smc/smc_core.c:1284) removes the socket from the…
> 
> Pre-existing issues:
> - [High] smc_cdc_msg_validate() (net/smc/smc_cdc.c:308-327) retargets
>   conn->lnk to the link a stray failover-validation CDC message arrived…
> 
> --
> 
> net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This patch adds smc_conn_unhash() plus a per-connection unhashed flag and
> calls it at the top of smc_conn_free(), so the socket leaves the SMC socket
> hash before the connection-owned lgr and lnk references are dropped.
> __smc_release() is changed to use the new helper for non-fallback sockets
> and keeps the direct sk->sk_prot->unhash() call for fallback sockets.
> 
> A few questions below about the paths that reach smc_conn_free() and about
> the invariant described in the changelog.
> 
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index e9f93b3ab435b..8c781a4a4485b 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -310,7 +310,15 @@ static int __smc_release(struct smc_sock *smc)
>>  		smc_restore_fallback_changes(smc);
>>  	}
>>  
>> -	sk->sk_prot->unhash(sk);
>> +	/* Fallback sockets never call smc_conn_free(), so unhash directly.
>> +	 * Non-fallback sockets use smc_conn_unhash() so that the conn->unhashed
>> +	 * flag keeps the unhash exactly once even when smc_conn_free() already ran
>> +	 * first (e.g. via smc_conn_kill()).
>> +	 */
>> +	if (smc->use_fallback)
>> +		sk->sk_prot->unhash(sk);
>> +	else
>> +		smc_conn_unhash(&smc->conn);
> 
> [Severity: High]
> Is the premise "Fallback sockets never call smc_conn_free()" accurate?
> smc_listen_decline() calls smc_conn_abort() first and only afterwards turns
> the socket into a fallback socket:
> 
> net/smc/af_smc.c:smc_listen_decline() {
> 	/* RDMA setup failed, switch back to TCP */
> 	smc_conn_abort(new_smc, local_first);
> 	if (reason_code < 0 ||
> 	    smc_switch_to_fallback(new_smc, reason_code)) {
> 	...
> 	smc_listen_out_connected(new_smc);
> }
> 
> net/smc/af_smc.c:smc_conn_abort() {
> 	...
> 	smc_conn_free(conn);
> 	...
> }
> 
> The client side does the same via smc_connect_rdma()'s connect_abort label
> followed by smc_connect_decline_fallback().
> 
> Beyond the comment, smc_conn_free() is also reached for connection
> *attempts* on sockets that stay alive afterwards, and there is no path that
> re-inserts the socket into the hash.  Can this make live sockets invisible
> to smc_diag for the rest of their lifetime?
> 
> For the server ISM retry loop:
> 
> net/smc/af_smc.c:smc_listen_ism_init() {
> 	rc = smc_buf_create(new_smc, true);
> 	if (rc) {
> 		smc_conn_abort(new_smc, ini->first_contact_local);
> 		...
> }
> 
> net/smc/af_smc.c:smc_find_ism_v2_device_serv() {
> 	for (i = 0; i < matches; i++) {
> 		...
> 		rc = smc_listen_ism_init(new_smc, ini);
> 		if (rc) {
> 			smc_init_info_store_rc(rc, ini);
> 			/* try next active ISM device */
> 			continue;
> 		}
> 		return; /* matching and usable V2 ISM device found */
> 	}
> }
> 
> The failed attempt unhashes the socket through smc_conn_free(), the next
> ISM device then succeeds, and smc_conn_create() only does
> "conn->unhashed = 0" without calling sk->sk_prot->hash() again.  The same
> shape exists for the v2 -> v1 RDMA fallthrough in
> smc_find_rdma_v2_device_serv() -> smc_listen_find_device().
> 
> smc_diag_dump_proto() walks only that hash:
> 
> net/smc/smc_diag.c:smc_diag_dump_proto() {
> 	read_lock(&prot->h.smc_hash->lock);
> 	head = &prot->h.smc_hash->ht;
> 	...
> 	sk_for_each(sk, head) {
> 	...
> }
> 
> so an established SMC-D/SMC-R connection or an active TCP-fallback
> connection produced by these paths would no longer show up in ss --smc or
> smcss, including the SMC_DIAG_MODE_FALLBACK_TCP reporting.  The same early
> unhash also hides sockets that user space still has open after
> __smc_lgr_terminate() -> smc_conn_kill() -> smc_close_active_abort() ->
> smc_conn_free().  Should the unhash be restricted to terminal teardown, or
> should the abort paths re-hash the socket?

Good catch. The v3 approach was wrong to put the unhash inside
smc_conn_free() — that path is also reached by the ISM/RDMA retry loop
and the fallback abort paths, which must leave the socket hashed.

Fixed in the next version(v4) by restricting the unhash to the two
terminal teardown sites directly: smc_close_active_abort() (covering the
smc_conn_kill() path) and smc_close_passive_work() (covering the passive
close path). smc_conn_free() is left untouched, so retry aborts and
fallback transitions no longer affect the socket's hash membership.

> 
>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>> index 04aedd957543a..e302221c35e3f 100644
>> --- a/net/smc/smc_core.c
>> +++ b/net/smc/smc_core.c
>> @@ -1251,6 +1251,20 @@ static void smc_buf_unuse(struct smc_connection *conn,
>>  	}
>>  }
>>  
>> +/* unhash the socket once; owns the single unhash for all non-fallback paths.
>> + * Every caller holds lock_sock for this socket, so conn->unhashed is protected
>> + * by that lock and no separate synchronisation is needed.
>> + */
>> +void smc_conn_unhash(struct smc_connection *conn)
>> +{
>> +	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);
>> +
>> +	if (!conn->unhashed) {
>> +		conn->unhashed = 1;
>> +		smc->sk.sk_prot->unhash(&smc->sk);
>> +	}
>> +}
>> +
>>  /* remove a finished connection from its link group */
>>  void smc_conn_free(struct smc_connection *conn)
>>  {
>> @@ -1263,6 +1277,11 @@ void smc_conn_free(struct smc_connection *conn)
>>  		return;
>>  
>>  	conn->freed = 1;
>> +	/* Unhash before dropping lgr/lnk refs so the diag reader, which
>> +	 * iterates under the socket hash read_lock, cannot see a connection whose
>> +	 * lgr or lnk is being freed concurrently.
>> +	 */
>> +	smc_conn_unhash(conn);
> 
> [Severity: High]
> The changelog states the invariant as "any socket still visible to the diag
> reader under the hash read_lock has valid conn->lgr and conn->lnk
> pointers".  Does the SMC-R link failover path still break that for
> conn->lnk?
> 
> smc_switch_link_and_count() drops the connection-owned link reference and
> republishes conn->lnk while the socket stays hashed:
> 
> net/smc/smc_core.c:smc_switch_link_and_count() {
> 	atomic_dec(&conn->lnk->conn_cnt);
> 	/* link_hold in smc_conn_create() */
> 	smcr_link_put(conn->lnk);
> 	conn->lnk = to_lnk;
> 	...
> }
> 
> It runs under conn->send_lock and lgr->conns_lock only, never under the
> socket hash lock and without unhashing, so a reader that already
> snapshotted the old link keeps using it:
> 
> net/smc/smc_diag.c:__smc_diag_dump() {
> 	struct smc_link *link = smc->conn.lnk;
> 	...
> 	memcpy(linfo.lnk[0].ibname, link->smcibdev->ibdev->name,
> 	       sizeof(link->smcibdev->ibdev->name));
> 	smc_gid_be16_convert(linfo.lnk[0].gid, link->gid);
> 	...
> }
> 
> Once the last connection has been migrated off the old link,
> smcr_link_clear() -> smcr_link_put() -> __smcr_link_clear() zeroes the
> whole structure:

Actually it doesn't clear the structure because it *never calls*
__smcr_link_clear().

smc_switch_link_and_count() drops one per-connection hold on from_lnk,
but the structural reference set in smcr_link_init()
(refcount_set(&lnk->refcnt, 1)) is still held — smcr_link_clear() has
not run yet at that point. So refcount_dec_and_test() cannot return
true, __smcr_link_clear() is never reached from this path, and the
memset/NULL-deref scenario does not apply here.

The remaining concern is a data race on the conn->lnk pointer itself:
smc_switch_link_and_count() writes conn->lnk = to_lnk under
conn->send_lock, while the diag reader reads it under the hash read_lock
— two independent locks with no intersection. However, because the old
link's structural refcount is still live, reading the stale old pointer
is safe; the worst outcome is a diag dump reporting stale link info
during an active failover, not a crash or use-after-free. The invariant
in the changelog holds for the no-crash case; it just doesn't guarantee
freshness of conn->lnk during failover.
> 
> net/smc/smc_core.c:__smcr_link_clear() {
> 	put_device(&lnk->smcibdev->ibdev->dev);
> 	smcibdev = lnk->smcibdev;
> 	memset(lnk, 0, sizeof(struct smc_link));
> 	lnk->state = SMC_LNK_UNUSED;
> 	...
> }
> 
> so link->smcibdev becomes NULL and the dereference above is a NULL
> dereference, with the device reference already dropped.  This looks like
> the same class of access the cited "Fixes: 9dbe086c69b8 (net/smc: fix
> invalid link access in dumping SMC-R connections)" refers to, and the
> reader/writer lock sets (hash read_lock versus send_lock/conns_lock/
> llc_conf_mutex) do not intersect.  The v2 posting of this series protected
> smc_switch_link_and_count() with lgr_lnk_lock; is anything replacing that
> protection in v3, or should the invariant in the changelog be narrowed to
> conn->lgr?
> 
> [Severity: High]
> This is a pre-existing issue and not introduced by this patch, but it also
> bears on the conn->lnk invariant being asserted: the reference that
> smc_conn_free() releases is not always the one the connection holds.

Agreed, this is pre-existing and orthogonal to the diag race fix. I'll
address it in a separate patch.

The root cause is that smc_cdc_msg_validate() reassigns conn->lnk
without a reference transfer — no smcr_link_hold() on the incoming link
and no smcr_link_put() on the old one — breaking the invariant
established by smc_conn_create(). The fix is to add the missing hold/put
pair around the reassignment, mirroring what smc_switch_link_and_count()
already does correctly.