Re: [PATCH net v2 08/14] net: pse-pd: stop async event sources before freeing PI data in unregister

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · 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).

- [High] This patch's reorder lets ntf_work be queued again after
  cancel_work_sync().

--

net: pse-pd: stop async event sources before freeing PI data in unregister

This commit reorders pse_controller_unregister() so the IRQ is disabled
and the notification worker is cancelled before the power domains are
flushed and the PI array is released. The aim is to stop pse_isr() and
the worker from using freed PI data.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 13d6bd29525f3..21280479312e6 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -1137,11 +1137,15 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
>   */
>  void pse_controller_unregister(struct pse_controller_dev *pcdev)
>  {
> -	pse_flush_pw_ds(pcdev);
> -	pse_release_pis(pcdev);
> +	/* Stop the IRQ and the notification worker first: pse_isr() uses
> +	 * pcdev->pi and the power domains that pse_flush_pw_ds() drops,
> +	 * and the worker reads the kfifo that is freed below.
> +	 */
>  	if (pcdev->irq)
>  		disable_irq(pcdev->irq);
>  	cancel_work_sync(&pcdev->ntf_work);
> +	pse_flush_pw_ds(pcdev);
> +	pse_release_pis(pcdev);
>  	kfifo_free(&pcdev->ntf_fifo);

[Severity: High]
Can ntf_work be queued again after this cancel_work_sync()?

pse_isr() is not the only producer. pse_disable_pi_pol() also queues the
worker, and the IRQ has nothing to do with it:

drivers/net/pse-pd/pse_core.c:pse_disable_pi_pol() {
    ...
	kfifo_in_spinlocked(&pcdev->ntf_fifo, &ntf, 1, &pcdev->ntf_fifo_lock);
	schedule_work(&pcdev->ntf_work);
    ...
}

It is reached through this chain:

_pse_pi_delivery_power_sw_pw_ctrl()
  pse_pi_allocate_pw_budget()
    pse_pi_allocate_pw_budget_static_prio()
      pse_disable_pi_prio()
        pse_disable_pi_pol()

That chain can start from three places:

- pse_pi_enable(). The PI regulators are devres-managed, so they are
  still registered when unregister runs.
- _pse_pi_disable()->pse_pw_d_retry_power_delivery().
- pse_ethtool_set_prio().

On these paths the only gate is pse_pw_d_is_sw_pw_control(). For a STATIC
budget domain it returns true until pse_flush_pw_ds() sets pw_d to NULL:

	if (pw_d->budget_eval_strategy == PSE_BUDGET_EVAL_STRAT_STATIC)
		return true;

The old order cleared pw_d before the final cancel_work_sync(). Take
tps23881 as an example: it uses the STATIC strategy and allocates priv
with devm_kzalloc(). With the new order, this sequence looks possible:

CPU0                                  CPU1
pse_controller_unregister()
  disable_irq()
  cancel_work_sync(&pcdev->ntf_work)
                                      pse_ethtool_set_prio()
                                        mutex_lock(&pcdev->lock)
                                        ... goes over budget ...
                                        pse_disable_pi_pol()
                                          schedule_work(&pcdev->ntf_work)
  pse_flush_pw_ds()
  pse_release_pis()
  kfifo_free(&pcdev->ntf_fifo)
devres frees priv (embeds pcdev)
                                      pse_send_ntf_worker()
                                        kfifo_out(&pcdev->ntf_fifo, ...)

Would pse_send_ntf_worker() then use the freed pcdev and the freed kfifo?

The new comment says the worker is stopped first. Nothing seems to stop
it from being queued again between cancel_work_sync() and
pse_flush_pw_ds().

A later patch in this series, "net: pse-pd: keep the PI data until the
PI regulators are gone", appears to close this window. It sets
pcdev->unregistering under pcdev->lock before cancel_work_sync(). While
that flag is set, _pse_pi_delivery_power_sw_pw_ctrl() and
pse_pi_enable() return -ENODEV.

Could that check be moved into this patch, or the patches reordered, so
the intermediate state does not have this window?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020171249.853085-1-github%40szelinsky.de