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

[netdev-bot+sashiko@kernel.org]

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