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

[sashiko-bot@kernel.org]

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