Re: [PATCH net-next v5 1/2] net: pse-pd: add devm_pse_poll_helper()

[Carlo Szelinsky <github@szelinsky.de>]

Hi Jakub,

Thanks for sending the AI review. I checked the code again... both
points are real, but one of them looks to me like more theory than practice.
Out of curiosity, what prompt do you use for these reviews, so I can use it 
before sending the next version? Replying inline:

On Mon, 4 May 2026 18:57:57 -0700 Jakub Kicinski wrote:
> > @@ -1118,6 +1125,8 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)
> >  	pse_release_pis(pcdev);
> >  	if (pcdev->irq)
> >  		disable_irq(pcdev->irq);
> > +	if (pcdev->polling)
> > +		cancel_delayed_work_sync(&pcdev->poll_work);
>
> Because pse_release_pis() executes kfree(pcdev->pi) before the delayed work
> is cancelled, can pse_poll_worker() run concurrently and access the freed
> pcdev->pi array inside pse_handle_events() before the cancellation completes?

Yes, seems like a real bug. The IRQ thread and the poll worker can still run
after pse_release_pis() frees pcdev->pi, and both end up in
pse_handle_events() touching pcdev->pi[i]. We need to stop them
first. v6 will reorder pse_controller_unregister() to:

	if (pcdev->irq)
		disable_irq(pcdev->irq);
	if (pcdev->polling)
		cancel_delayed_work_sync(&pcdev->poll_work);
	cancel_work_sync(&pcdev->ntf_work);
	pse_release_pis(pcdev);

The IRQ side seem to be already broken before this patch. I went
through the git log:

* fc0e6db30941 ("net: pse-pd: Add support for reporting events")
  added pse_isr but never called disable_irq() in unregister at all.
* ffef61d6d273 ("net: pse-pd: Add support for budget evaluation
  strategies") added the disable_irq() call, but put it in the
  wrong place (after pse_release_pis()).

What do you suggest to do, I could imagine: ... 

1) Send the fix as a standalone "Fixes:" patch to net first,
   then rebase v6 on top once it lands? Or fold it into v6 as
   patch 1/3? My vote is standalone since the fix stands on its
   own, but up to you.

2) Which commit for the Fixes: tag? I'd pick ffef61d6d273
   (that's where the broken order came from), but fc0e6db30941
   also works (no protection at all back then). wdyt?

> > +int devm_pse_poll_helper(struct pse_controller_dev *pcdev,
> > +			 const struct pse_irq_desc *d)
> > +{
> [...]
> > +	queue_delayed_work(system_freezable_wq, &pcdev->poll_work,
> > +			   msecs_to_jiffies(pcdev->poll_interval_ms));
>
> Since memory is allocated with devm_ helpers but no devm cleanup action is
> registered to cancel the delayed work, what happens if a driver's probe
> function encounters an error after calling this helper? [...]
>
> The commit message mentions that drivers typically set up the helper before
> registering the controller. If queue_delayed_work() schedules the worker
> to run before devm_pse_controller_register() completes, will
> pse_poll_worker() attempt to acquire pcdev->lock and push events into
> pcdev->ntf_fifo before they are initialized?

The probe-failure case is a real bug. If probe fails between this
helper and devm_pse_controller_register(), the work stays queued
and fires later on a pcdev whose devres memory is gone. UAF.

The "worker fires before mutex/kfifo are ready" case is real in
theory, but I don't think you can actually hit it?! Default
poll_interval_ms is 1000 ms, so pse_controller_register() would
need to take over a second to lose the race. Worth fixing to keep
the code clean, but not urgent or?

Same fix for both: move queue_delayed_work() out of the helper.
The helper just allocates the state and does INIT_DELAYED_WORK.
pse_controller_register() then arms the work as the very last
step on success, gated on pcdev->polling. So the work only ever
runs when everything is set up AND register has succeeded.

Should I fix both in v6, or just the probe-failure leak? One
change covers both, so I would suggest to do both. wdyt?

So the steps for v6 [1/2] could be:
* standalone "Fixes:" patch reordering pse_controller_unregister()
  (sent to net first, unless you suggest to fold it in)
* move queue_delayed_work() into pse_controller_register()

Do you have any other feedback, do I miss anything ?

Cheers,
Carlo