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

[Kory Maincent <kory.maincent@bootlin.com>]

On Mon, 13 Apr 2026 15:50:21 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: pse-pd: add devm_pse_poll_helper()
> 
> Extract the common event handling loop from pse_isr() into a shared
> pse_handle_events() function, and add a generic poll-based alternative
> to the IRQ path for PSE controllers that lack interrupt support.
> 
> > Work cancellation on teardown is handled via devm_add_action_or_reset()
> > to ensure the delayed work is cancelled before poll_notifs is freed
> > by devres, avoiding a use-after-free when devm_pse_poll_helper() is
> > called after devm_pse_controller_register() (devres LIFO ordering).  
> 
> Is it safe to rely on this devm registration order to prevent a
> use-after-free?
> 
> Standard Linux driver initialization typically sets up interrupts and
> pollers before device registration to prevent race conditions where an
> unready device is exposed to userspace.
> 
> If a driver follows this standard ordering by calling the helper before
> registration, devres will unregister the controller first. This will
> tear down resources in pse_controller_unregister() while the poll work
> remains active.
> 
> If the worker executes during this race window, could it access the
> already-freed pcdev->pi array or push to the destroyed ntf_fifo? Would
> it be safer to explicitly cancel asynchronous event sources within
> pse_controller_unregister() instead?
> 
> > --- a/drivers/net/pse-pd/pse_core.c
> > +++ b/drivers/net/pse-pd/pse_core.c  
> [ ... ]
> > +static void pse_handle_events(struct pse_controller_dev *pcdev,
> > +			      unsigned long *notifs,
> > +			      unsigned long notifs_mask)
> >  {
> > -	struct pse_controller_dev *pcdev;
> > -	unsigned long notifs_mask = 0;
> > -	struct pse_irq_desc *desc;
> > -	struct pse_irq *h = data;
> > -	int ret, i;
> > -
> > -	desc = &h->desc;
> > -	pcdev = h->pcdev;
> > -
> > -	/* Clear notifs mask */
> > -	memset(h->notifs, 0, pcdev->nr_lines * sizeof(*h->notifs));
> > -	mutex_lock(&pcdev->lock);
> > -	ret = desc->map_event(irq, pcdev, h->notifs, &notifs_mask);
> > -	if (ret || !notifs_mask) {
> > -		mutex_unlock(&pcdev->lock);
> > -		return IRQ_NONE;
> > -	}
> > +	int i;
> >  
> >  	for_each_set_bit(i, &notifs_mask, pcdev->nr_lines) {  
> 
> Can this lead to an out-of-bounds read on the stack if a controller
> supports more than BITS_PER_LONG ports?
> 
> The notifs_mask variable is passed by value to pse_handle_events(), but
> for_each_set_bit() takes its address. If pcdev->nr_lines is greater than
> BITS_PER_LONG (e.g., a 48-port switch on a 32-bit architecture), the
> macro will read past the function argument on the stack into uninitialized
> memory.

It's seems there is a possible out-of-bound issue in my code :/ Oops.
Carlo, could you take a look and propose a fix? Otherwise, I'll handle it.

Regards
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com