[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

[sashiko-bot]

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?

> 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?

>  	if (!smc_conn_lgr_valid(conn))
>  		/* Connection has already unregistered from
>  		 * link group.

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

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.