Re: [PATCH net] net/smc: serialize link group free work scheduling

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (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] The fix is incomplete.
- [Medium] The patch treats lgr_lock as the lock for lgr->freeing (the
  new comment reads 'protects lgr->freeing').

Pre-existing issues:
- [High] Separately from the pending-timer rearm this patch fixes, a
  free_work callback that has already started can race with lgr teardown…

--

net/smc: serialize link group free work scheduling

This patch takes the link group list lock in smc_lgr_schedule_free_work()
around the lgr->freeing check and the mod_delayed_work() call. The goal is
to stop a concurrent smc_lgr_free_work() from setting freeing and cancelling
the work between the check and the rearm, which would leave a timer pending
on a freed link group.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 9974149659c2f..8feb486ada562 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -80,6 +80,10 @@ static void smc_ibdev_cnt_dec(struct smc_link *lnk)
>  
>  static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
>  {
> +	spinlock_t *lgr_lock; /* protects lgr->freeing */

[Severity: Medium]
Is lgr_lock enough to protect lgr->freeing? freeing is a 1-bit bitfield,
and it shares a u8 with sync_err and terminating:

net/smc/smc_core.h:struct smc_link_group {
	...
	u8			sync_err : 1;	/* lgr no longer fits to peer */
	u8			terminating : 1;/* lgr is terminating */
	u8			freeing : 1;	/* lgr is being freed */
	...
}

Both neighbouring bits are written without lgr_lock. One example is
smc_clc_wait_msg():

	smc->conn.lgr->sync_err = 1;
	smc_lgr_terminate_sched(smc->conn.lgr);

Another is __smc_lgr_terminate():

	lgr->terminating = 1;

Each of these stores is a read-modify-write of the whole byte.
smc_smcd_terminate() and smc_lgr_free_work() set freeing = 1 under
lgr_lock. Can a concurrent sync_err or terminating store write back a stale
byte and lose that freeing = 1?

In the sync_err case, smc_lgr_terminate_sched() then returns early because
the lgr is already unlinked, so freeing is never set again.

Consider a later smc_conn_free() that passed its unlocked list_empty()
check before the unlink. Once __smc_lgr_terminate() has cancelled
free_work, that call could reach smc_lgr_schedule_free_work() with
conns_num == 0. It would read freeing == 0 under the new lock and rearm the
timer on an lgr that is about to be freed.

Would it work better to give freeing its own storage (for example a bool),
or to make every writer of that byte hold lgr_lock?

> +
> +	smc_lgr_list_head(lgr, &lgr_lock);
> +	spin_lock_bh(lgr_lock);
>  	/* client link group creation always follows the server link group
>  	 * creation. For client use a somewhat higher removal delay time,
>  	 * otherwise there is a risk of out-of-sync link groups.
> @@ -90,6 +94,7 @@ static void smc_lgr_schedule_free_work(struct smc_link_group *lgr)
>  						SMC_LGR_FREE_DELAY_CLNT :
>  						SMC_LGR_FREE_DELAY_SERV);
>  	}
> +	spin_unlock_bh(lgr_lock);
>  }

[Severity: High]
Does this fully close the rearm race? The commit message says a rearm
"either precedes the freeing transition and is caught by the subsequent
cancellation, or sees freeing set and is skipped". That only holds if every
teardown path sets lgr->freeing under lgr_lock before it cancels free_work.

smc_lgr_cleanup_early() unlinks the lgr under lgr_lock but never sets
freeing:

net/smc/smc_core.c:smc_lgr_cleanup_early() {
	...
	spin_lock_bh(lgr_lock);
	/* do not use this link group for new connections */
	if (!list_empty(&lgr->list))
		list_del_init(&lgr->list);
	spin_unlock_bh(lgr_lock);
	__smc_lgr_terminate(lgr, true);
}

__smc_lgr_terminate() then cancels only once, outside the lock. It sets
terminating, not freeing:

net/smc/smc_core.c:__smc_lgr_terminate() {
	if (lgr->terminating)
		return;	/* lgr already terminating */
	/* cancel free_work sync, will terminate when lgr->freeing is set */
	cancel_delayed_work(&lgr->free_work);
	lgr->terminating = 1;
	...
	smc_lgr_cleanup(lgr);
	smc_lgr_free(lgr);
}

On an SMC-D server, smc_listen_work() drops smc_server_lgr_pending after
it sends the ACCEPT. A second connection (conn2) from the same peer can
then join the first-contact lgr.

If the first connection then fails (no CONFIRM, or a DECLINE), teardown
goes through:

smc_listen_work()->smc_listen_decline()->smc_conn_abort()->
    smc_lgr_cleanup_early()->__smc_lgr_terminate()

Meanwhile conn2 can be freed on another CPU:

CPU0 (conn1 abort)                CPU1 (smc_conn_free(conn2))
                                  !list_empty(&lgr->list) is true
                                  smc_lgr_unregister_conn(conn2)
smc_lgr_cleanup_early()
  list_del_init(&lgr->list)
  __smc_lgr_terminate()
    cancel_delayed_work()
    lgr->terminating = 1
                                  lgr->conns_num == 0
                                  smc_lgr_schedule_free_work()
                                    spin_lock_bh(lgr_lock)
                                    lgr->freeing == 0
                                    mod_delayed_work()
    smc_lgr_free()
                                  smc_lgr_put() drops last reference

This would leave free_work's timer pending on a kfree'd lgr, which is the
same __run_timers use-after-free as in the KASAN report.

Should smc_lgr_cleanup_early() set lgr->freeing under lgr_lock? Or should
smc_lgr_schedule_free_work() also skip the rearm when the lgr is unlinked
or terminating?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can a
smc_lgr_free_work() callback that has already started running race with
lgr teardown?

smc_lgr_free_work() holds no reference on the lgr and dereferences it
straight away:

net/smc/smc_core.c:smc_lgr_free_work() {
	...
	smc_lgr_list_head(lgr, &lgr_lock);
	spin_lock_bh(lgr_lock);
	if (lgr->freeing) {
	...
}

Suppose the idle timer fires and the worker starts, but is delayed before
this point (by preemption, or on PREEMPT_RT). A teardown can then set
freeing and reach __smc_lgr_terminate(). Examples are smc_smcd_terminate()
from an ISM peer shutdown event in smcd_handle_sw_event(),
smc_lgr_terminate_sched(), and smc_smcd_terminate_all() or
smc_smcr_terminate_all(). The only cancellation there is:

	cancel_delayed_work(&lgr->free_work);

That call neither cancels nor waits for a callback that is already
running.

With no connections left, smc_lgr_free() destroys lgr->tx_wq. free_work is
on system_percpu_wq, so this does not drain it. smc_lgr_put() then calls
__smc_lgr_free(), which frees the lgr.

When the worker resumes, it takes spin_lock_bh() on a lock pointer loaded
from freed memory. If the stale freeing bit reads 0, it also calls
list_del_init() and smc_lgr_free() a second time.

Should teardown use cancel_delayed_work_sync() where the context allows,
or should the queued work hold its own lgr reference?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927074520.3694663-1-nicoyip.dev%40gmail.com