Re: [PATCH] iio: adc: xilinx-xadc: free IRQ before cancelling the unmask worker on unbind

On Sun, 2 Aug 2026 10:39:58 -0500
David Lechner <[email protected]> wrote:

> On 8/2/26 3:28 AM, Fan Wu wrote:
> > The ZYNQ XADC interrupt handler xadc_zynq_interrupt_handler() arms the
> > zynq_unmask_work delayed work via schedule_delayed_work() every time an
> > alarm condition is observed, and that worker re-arms itself for as long
> > as the alarm stays asserted.
> > 
> > In xadc_probe() the IRQ is requested with devm_request_irq() before the
> > devm_add_action_or_reset() that registers xadc_cancel_delayed_work().
> > Because devres release runs in LIFO order, on unbind the delayed work is
> > cancelled before the IRQ is freed. The IRQ is still live at that point,
> > so a pending alarm can make the handler run once more and re-arm
> > zynq_unmask_work after it has been cancelled; that instance then runs
> > after the xadc structure that embeds zynq_unmask_work has been freed, a
> > use-after-free in xadc_zynq_unmask_worker().
> > 
> > Register the cancel-work devm action before requesting the IRQ so devres
> > LIFO teardown frees (and synchronizes) the IRQ first, then cancels the
> > delayed work. After free_irq() the handler can no longer re-arm the
> > work, and the subsequent cancel_delayed_work_sync() drains any instance
> > armed just before the IRQ was torn down.
> > 

This seems a bit too verbose to me.  A simpler explanation might get to the
key detail faster. Something like:

The driver clears outstanding work using a devm callback. That work is
queued up by schedule_delayed_work() in the alarm irq handler.
Reorder the work clear to occur after the devm cleanup for the irq
ensuring no additional work can be queued after the workqueue is
cleared. In practice this means registering the devm callback earlier
than the call to devm_irq_request_threaded().

If this was AI generated description (which I do as well sometimes)
it is useful to tell the AI to be brief.

> > This issue was found by an in-house static analysis tool.
> > 
> > Fixes: 2a9685d1a3b7 ("iio: adc: xilinx: use more devres helpers and remove remove()")
> > Cc: [email protected]
> > Assisted-by: Codex:gpt-5.6
> > Signed-off-by: Fan Wu <[email protected]>
> > ---
> > 
> >  drivers/iio/adc/xilinx-xadc-core.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> > --- a/drivers/iio/adc/xilinx-xadc-core.c
> > +++ b/drivers/iio/adc/xilinx-xadc-core.c
> > @@ -1395,13 +1395,16 @@ static int xadc_probe(struct platform_device *pdev)
> >  	}
> > 
> >  	if (irq > 0) {
> > -		ret = devm_request_irq(dev, irq, xadc->ops->interrupt_handler,
> > -				       0, dev_name(dev), indio_dev);
> > +		/* devm LIFO: register the cancel-work action before the IRQ,
> > +		 * so unbind frees the IRQ first, then drains the work.
> > +		 */

Wrong comment style.  Otherwise this seems correct to me but I'd like the
AMD / Xilinx folk to take a look.  Added a couple more +CC.

Not sure we need the comment, or perhaps the comment should just be
/* Clear out an work queued by the interrupt */

> > +		ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
> > +					       &xadc->zynq_unmask_work);
> >  		if (ret)
> >  			return ret;
> > 
> > -		ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
> > -					       &xadc->zynq_unmask_work);
> > +		ret = devm_request_irq(dev, irq, xadc->ops->interrupt_handler,
> > +				       0, dev_name(dev), indio_dev);
> >  		if (ret)
> >  			return ret;
> >  	}
> > --
> > 2.43.0
> >   
> 
> Makes sense.
> 
> Reviewed-by: David Lechner <[email protected]>
>