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

[netdev-bot+sashiko@kernel.org]

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.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015081349.1252391-1-mjambigi%40linux.ibm.com