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

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

On 27/10/26 8:45 pm, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] Use-after-free read of the link group in __smc_diag_dump()
>   (net/smc/smc_diag.c, lines 93-94) for hashed SMC_CLOSED sockets that…
> - [Medium] The new state-based guard in __smc_diag_dump() (smc_diag.c:93
>   and 107-109) can be bypassed by a stale-registration socket that…
> 
> --
> 
> net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This patch tries to close several windows where the SMC sock_diag dump
> can dereference a freed or partially freed conn/lgr/lnk on a socket that
> is still hashed. It adds an SMC_INIT/SMC_CLOSED/fallback guard in
> __smc_diag_dump(), pairs it with acquire/release on sk_state, and unhashes
> sockets before smc_conn_free() and smcd_buf_detach() in the teardown
> paths. It also uses the cached link->ibname for the LGRINFO attribute.
> 
> [ ... ]
> 
>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
>> index bf0beaa23bdb6..84c45aabc196c 100644
>> --- a/net/smc/smc_diag.c
>> +++ b/net/smc/smc_diag.c
>> @@ -87,10 +87,11 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>>  
>>  	r = nlmsg_data(nlh);
>>  	smc_diag_msg_common_fill(r, sk);
>> -	r->diag_state = sk->sk_state;
>> +	r->diag_state = smp_load_acquire(&sk->sk_state);
>>  	if (smc->use_fallback)
>>  		r->diag_mode = SMC_DIAG_MODE_FALLBACK_TCP;
>> -	else if (smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd)
>> +	else if (r->diag_state != SMC_INIT &&
>> +		 smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd)
> 
> [Severity: High]
> Can this still dereference a freed link group for a hashed socket in
> SMC_CLOSED?
> 
> The diag_mode check here only excludes SMC_INIT. The new SMC_INIT /
> SMC_CLOSED / use_fallback guard comes later, after
> smc->conn.lgr->is_smcd has already been loaded.
> 
> The v13 changelog describes how a stale alert_token_local can survive.
> Once smc_lgr_terminate_sched() has emptied lgr->list, smc_conn_free()
> skips the unregister but still drops the references:
> 
> net/smc/smc_core.c:smc_conn_free() {
>     ...
> 	if (!list_empty(&lgr->list)) {
> 		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
> 		smc_lgr_unregister_conn(conn);
> 	}
>     ...
> lgr_put:
> 	if (!lgr->is_smcd)
> 		smcr_link_put(conn->lnk); /* link_hold in smc_conn_create() */
> 	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
> }
> 
> On the server side this looks reachable as:
> 
> smc_listen_work()
>   smc_conn_create()          <- joins existing SMC-R lgr L
>                              <- smc_lgr_terminate_sched(L) empties L->list
>   smc_clc_wait_msg()         <- peer resets, rc < 0 (-ECONNRESET/-EPROTO)
>   smc_listen_decline()
>     smc_conn_abort()
>       smc_conn_free()        <- unregister skipped, token stays set
>     smc_listen_out_err()     <- SMC_CLOSED, use_fallback false, hashed
> 
> Since rc < 0 there is no fallback. The socket stays hashed in the accept
> queue or until smc_close_non_accepted() takes the sock lock. Later,
> terminate_work runs:
> 
> __smc_lgr_terminate()
>   smc_conn_kill()            <- clears token under conns_lock
>     smc_close_active_abort() <- SMC_CLOSED case just breaks, no unhash
>   smc_lgr_cleanup()
>   smc_lgr_free()
>     smc_lgr_put()
>       __smc_lgr_free()       <- kfree(lgr)
> 
> Nothing in that sequence synchronizes with the hash read_lock that the
> diag reader holds. A reader could see the non-zero token in
> smc_conn_lgr_valid() and then load smc->conn.lgr->is_smcd after the
> kfree. That would read freed memory.
> 
> The "Fourth" paragraph of the commit message says this case must not
> happen: observing SMC_CLOSED and dereferencing an lgr whose reference was
> already dropped. The smp_store_release() added in smc_listen_out_err()
> only helps when the token was actually cleared, and here it was not.
> 
> The client close() path (SMC_INIT -> SMC_CLOSED in smc_close_active())
> doesn't seem to be exploitable this way. The reader's read_lock blocks
> the unhash in __smc_release() while the sock lock is held. The server
> path above has no such protection.
> 
> Could the SMC_INIT / SMC_CLOSED / use_fallback check move ahead of the
> diag_mode computation? Adding r->diag_state != SMC_CLOSED to this
> condition would also work.

Valid finding, fixed in v14.

The v13 diag_mode check only excluded SMC_INIT, so a hashed SMC_CLOSED
socket still loaded smc->conn.lgr->is_smcd. The scenario is reachable.
If smc_lgr_terminate_sched() empties lgr->list after smc_conn_create(),
then smc_conn_free() skips the unregister and leaves alert_token_local
set. A negative rc from smc_clc_wait_msg() then reaches
smc_listen_out_err() with no fallback. The socket stays hashed in
SMC_CLOSED, and terminate_work can free the lgr.

You are also right that smp_store_release() doesn't help here, since the
token was never cleared. v14 excludes SMC_CLOSED before conn->lgr is
read in __smc_diag_dump(), and the commit message now says what the
release store does and doesn't order.

The stale token itself is not new. It comes from smc_conn_free()
skipping the unregister when lgr->list is empty (2a0674fffb6b).

> 
>>  		r->diag_mode = SMC_DIAG_MODE_SMCD;
>>  	else
>>  		r->diag_mode = SMC_DIAG_MODE_SMCR;
>> @@ -103,6 +104,10 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
>>  	if (nla_put(skb, SMC_DIAG_FALLBACK, sizeof(fallback), &fallback) < 0)
>>  		goto errout;
>>  
>> +	if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED ||
>> +	    smc->use_fallback)
>> +		goto out;
>> +
> 
> [Severity: Medium]
> Can a stale-registration socket get past this guard if it leaves
> SMC_INIT through passive close instead of fallback?
> 
> Here is a scenario on an SMC-R client:
> 
> smc_connect_rdma()
>   smc_conn_create()             <- registers conn in existing lgr L
>                                 <- smc_lgr_terminate_sched(L) empties
>                                    L->list
>   smc_clc_send_confirm() fails  <- rc < 0
>   smc_conn_abort()
>     smc_conn_free()             <- unregister skipped, lgr/lnk refs
>                                    dropped
> smc_connect_decline_fallback()  <- rc < 0, returns without fallback
> 
> A positive decline code followed by a failing smc_clc_send_decline()
> ends up in the same place.
> 
> The socket stays hashed in SMC_INIT with use_fallback false. The conn is
> still in L->conns_all with its alert_token_local set, so
> smc_cdc_rx_handler() -> smc_lgr_find_conn() still finds it. The peer has
> the token from the confirm, or can guess it from the sequential counter.
> 
> A CDC with close or abort flags then queues close_work, which moves the
> socket out of SMC_INIT:
> 
> smc_close_passive_work():
> 	case SMC_INIT:
> 		sk->sk_state = SMC_APPCLOSEWAIT1;
> 
> smc_close_passive_abort_received():
> 	case SMC_INIT:
> 	case SMC_ACTIVE:
> 	case SMC_APPCLOSEWAIT1:
> 		sk->sk_state = SMC_PROCESSABORT;
> 
> At that point the state is neither SMC_INIT nor SMC_CLOSED, use_fallback
> is false, and smc_conn_lgr_valid() is true because of the stale token.
> The reader then dereferences:
> 
> - smc->conn.lgr->is_smcd
> - conn->sndbuf_desc->len and conn->rmb_desc->len (still on L's lists,
>   since smc_buf_unuse() was skipped)
> - lgr->list and lgr->role
> - the conn->lnk fields
> 
> Meanwhile __smc_lgr_terminate() -> smc_conn_kill() clears the token and
> calls smc_close_active_abort():
> 
> - The ACTIVE/APPCLOSEWAIT branch never unhashes.
> - The PROCESSABORT branch now unhashes, but smc_conn_free() returns early
>   because conn->freed is already set.
> 
> smc_lgr_free() -> __smc_lgr_free() then frees the buffers and kfree()s L.
> 
> What makes that free wait for a diag reader that has already passed the
> token check? The v13 changelog names this stale-registration case, but it
> seems to handle only the variant where the socket falls back.
Its a pre-existing issue & I'm not going to address this in this patch.

It is *not* a regression. The stale token comes from the same old
smc_conn_free() behaviour (2a0674fffb6b), and before this patch the
reader was exposed in SMC_INIT with no further action needed. The
SMC_INIT guard makes this path harder to hit.

It also needs a misbehaving peer. The socket only leaves SMC_INIT if the
peer sends, or guesses, a CDC with a close or abort flag for the stale
token. A well-behaved peer never does that for a connection it saw fail.
On top of that, a diag reader holding the hash read_lock would have to
stall through smc_conn_kill(), smc_lgr_cleanup() and smc_lgr_free()
after passing the token check. The odds of hitting that are very low. A
peer that can forge CDCs on an established SMC-R link can already abort
connections on that lgr, so it has far simpler options than this race.

The proper fix is to clear the token in smc_conn_free() when lgr->list
is empty. That changes lgr lifetime handling, which is where v11 and v12
got stuck, so it belongs in a separate patch. I'm keeping this one
limited to the diag reader.

pw-bot: cr