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

[sashiko-bot@kernel.org]

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@rendec.net>

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.

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009011324.1503697-1-radu@rendec.net?part=1