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

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

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.