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

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

On 19/10/26 2:49 pm, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 1 · High: 0 · Medium: 0 · 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).
> 
> - [Critical] The patch makes smc_conn_free() call
>   smc_lgr_unregister_conn() unconditionally.
> 
> --
> 
> net/smc: fix lgr/lnk lifetime vs diag reader race
> 
> This patch closes races between the SMC sock_diag dump and teardown of
> conn/lgr/lnk state. It unhashes sockets before smc_conn_free() and
> smcd_buf_detach(), orders sk_state with acquire/release, skips the
> extension attributes for sockets in SMC_INIT/SMC_CLOSED, and uses the
> cached link->ibname. It also makes smc_conn_free() always call
> smc_lgr_unregister_conn().
> 
>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>> index 9974149659c2..6735f185cf48 100644
>> --- a/net/smc/smc_core.c
>> +++ b/net/smc/smc_core.c
>> @@ -1279,10 +1279,9 @@ void smc_conn_free(struct smc_connection *conn)
>>  		if (current_work() != &conn->abort_work)
>>  			cancel_work_sync(&conn->abort_work);
>>  	}
>> -	if (!list_empty(&lgr->list)) {
>> +	if (!list_empty(&lgr->list))
>>  		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
>> -		smc_lgr_unregister_conn(conn);
>> -	}
>> +	smc_lgr_unregister_conn(conn);
>>  
>>  	if (!lgr->conns_num)
>>  		smc_lgr_schedule_free_work(lgr);
> 
> [Severity: Critical]
> Now that the unregister is unconditional, can lgr->free_work get re-armed
> on a link group that is already being torn down? If so, the lgr could be
> freed while that timer is still pending.
> 
> smc_lgr_cleanup_early() unlinks the lgr but never sets lgr->freeing:
> 
> smc_lgr_cleanup_early() {
>     ...
> 	if (!list_empty(&lgr->list))
> 		list_del_init(&lgr->list);
> 	spin_unlock_bh(lgr_lock);
> 	__smc_lgr_terminate(lgr, true);
> }
> 
> smc_lgr_schedule_free_work() checks only lgr->freeing, not
> lgr->terminating:
> 
> 	if (!lgr->freeing) {
> 		mod_delayed_work(system_percpu_wq, &lgr->free_work, ...
> 
> __smc_lgr_terminate() cancels free_work once, before a loop that sleeps
> in lock_sock(). After the loop it calls smc_lgr_free(), and nothing on
> that path cancels free_work again:
> 
> __smc_lgr_terminate() {
>     ...
> 	cancel_delayed_work(&lgr->free_work);
> 	lgr->terminating = 1;
> 	...
> 		lock_sock(&smc->sk);
> 		smc_conn_kill(conn, soft);
> 	...
> 	smc_lgr_cleanup(lgr);
> 	smc_lgr_free(lgr);
> }
> 
> One possible sequence on an SMC-D server:
> 
> - Conn X creates a first-contact lgr L. smc_listen_work() releases
>   smc_server_lgr_pending for SMC-D right after sending ACCEPT.
> 
> - Conn Y joins L. smc_conn_create() has no conns_num limit for SMC-D.
> 
> - X's CONFIRM fails:
>   smc_listen_decline(X) -> smc_conn_abort() -> smc_conn_free(X), while
>   L is still listed. Then smc_lgr_cleanup_early(L) runs list_del_init()
>   and __smc_lgr_terminate(L) cancels free_work, sets terminating = 1 and
>   blocks in lock_sock(Y). Y's listen worker holds that lock while it
>   waits for its own CONFIRM.
> 
> - Y's CONFIRM also fails:
>   smc_listen_decline(Y) -> smc_conn_abort() -> smc_conn_free(Y). The
>   lgr list is now empty, but with this change smc_lgr_unregister_conn(Y)
>   still runs. conns_num drops to 0, freeing is 0, so
>   smc_lgr_schedule_free_work() arms free_work with
>   SMC_LGR_FREE_DELAY_SERV.
> 
> - X's worker resumes, the loop ends, and smc_lgr_free(L) ->
>   smc_lgr_put() -> __smc_lgr_free() -> kfree(L) runs while free_work is
>   still pending.
> 
> Would the timer then fire on freed memory? If L were still alive at that
> point, smc_lgr_free_work() would see freeing == 0 and an empty conns_all
> and call smc_lgr_free() a second time.
> 
> Before this patch, smc_conn_free(Y) skipped smc_lgr_unregister_conn() when
> the lgr list was empty, so conns_num stayed at 1 and free_work was not
> re-armed.
> 
> The peer decides whether and when each CONFIRM fails, and with
> loopback-ism the client can be a local unprivileged user.
> 
> Should smc_lgr_cleanup_early() set lgr->freeing like the other unlink
> paths do? Alternatively, smc_lgr_schedule_free_work() could also check
> lgr->terminating.
> 
> [ ... ]
> 

You are correct. v11 introduced unconditional smc_lgr_unregister_conn()
which correctly fixes the alert_token_local UAF, but left
smc_lgr_schedule_free_work() with only the !lgr->freeing check. On the
smc_lgr_cleanup_early() path, __smc_lgr_terminate() sets terminating=1
and cancels free_work without setting freeing=1, so a concurrent
smc_conn_free() decrementing conns_num to 0 would re-arm free_work. The
subsequent kfree(lgr) inside __smc_lgr_terminate() → smc_lgr_free()
leaves the re-armed work pointing at freed memory.

Fixed in v12 by adding !lgr->terminating to the guard in
smc_lgr_schedule_free_work():

-	if (!lgr->freeing) {
+	if (!lgr->freeing && !lgr->terminating) {

This is safe: on the smc_lgr_cleanup_early() path __smc_lgr_terminate()
walks the full conns_all tree, kills every connection via
smc_conn_kill(), and then calls smc_lgr_free() unconditionally — it does
not rely on free_work to complete the teardown. Blocking the re-arm when
terminating=1 just prevents the UAF; the LGR is still freed by
__smc_lgr_terminate() itself.

On all other teardown paths (smc_lgr_terminate_sched,
smc_smcd_terminate, smc_smcd_terminate_all, smc_smcr_terminate_all)
freeing=1 is set before __smc_lgr_terminate() or terminate_work is
scheduled, so !lgr->freeing already blocked smc_lgr_schedule_free_work()
there — the !lgr->terminating addition changes nothing for those paths.

pw-bot: cr