Re: [PATCH] of/irq: Document of_irq_init() device node refcount contract

[Radu Rendec]

On Fri, 2026-10-09 at 01:19 +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] of/irq: false API contract documentation encourages use-after-free
> --
> 
> commit 553b68f6aa4fe4b4ead9f98bb63dfba3cbd9c9a9
> Author: Radu Rendec <radu@xxxxxxxxxx>
> 
> of/irq: Document of_irq_init() device node refcount contract
> 
> This commit adds documentation to of_irq_init() claiming that the device
> node refcount is intentionally never dropped upon successful initialization.
> 
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index ef1ed9743907b..bf277781cdb6c 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -647,6 +647,15 @@ struct of_intc_desc {
> >   *
> >   * This function scans the device tree for matching interrupt controller nodes,
> >   * and calls their initialization functions in order with parents first.
> > + *
> > + * The initialization functions are called while holding a refcount on the
> > + * device node corresponding to the device that is being initialized (passed
> > + * as the first parameter). If an initialization function is successful, the
> > + * device node refcount is *not* dropped (ever); this is intentional and
> > + * guarantees that the pointer passed to the initialization function is valid
> > + * not only while the function runs, but also for the rest of the kernel
> > + * lifetime (i.e. it is safe for a driver to store the device node pointer
> > + * and use it *after* the initialization function returns).
> >   */
> >  void __init of_irq_init(const struct of_device_id *matches)
> >  {
> 
> [Severity: Medium]
> Does this documentation accurately reflect the code's behavior?
> 
> Looking at the end of of_irq_init() in drivers/of/irq.c, there is a cleanup
> loop that explicitly drops the device node refcounts for successfully
> initialized controllers remaining in the parent list:
> 
> 	list_for_each_entry_safe(desc, temp_desc, &intc_parent_list, list) {
> 		list_del(&desc->list);
> 		of_node_put(desc->interrupt_parent);
> 		of_node_put(desc->dev);
> 		kfree(desc);
> 	}
> 
> Any successfully initialized leaf interrupt controller, and any sibling
> controllers remaining in intc_parent_list when intc_desc_list becomes empty,
> will have their references dropped by this final drain loop.

This is correct. At the time when I sent the patch, commit 30724547b221
("of/irq: Fix remaining refcount leaks in of_irq_init()") hadn't been
merged yet (or it had very recently and I hadn't picked it up yet).

Sorry for the noise!

By the way, I manually re-added the email addresses I had used on the
initial email. It looks like sashiko replies to a fixed list of
addresses.

> The documentation appears to mistake a pre-existing memory leak (where parent
> controllers popped during the while loop leak their references) for an
> intentional, universal API contract, while ignoring this final drain loop that
> cleans up the remaining nodes.
> 
> If driver authors rely on this newly documented contract, they might assume
> they do not need to take their own reference on the device node. Could this
> lead to a use-after-free when the final loop drops the reference?