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

[Kory Maincent]

On Tue, 14 Apr 2026 16:05:06 +0200
Kory Maincent <kory.maincent@xxxxxxxxxxx> wrote:

> On Mon, 13 Apr 2026 15:50:21 -0700
> Jakub Kicinski <kuba@xxxxxxxxxx> 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.

But currently it can't be reached as the only driver that support interrupt is
the TPS23881 with 8 ports.

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