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

[Mahanta Jambigi <mjambigi@linux.ibm.com>]

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.