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

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

[Radu Rendec]

When a driver initialization function is successful, of_irq_init() keeps
a refcount on the device node it has passed, which makes it safe for the
driver to store a pointer to the device node and use it after the
initialization function returns.

This behavior is currently not documented anywhere, and only assumed.
Add a paragraph to the comment block preceding of_irq_init() to document
the behavior and turn it into a contract.

It is worth noting that of_irq_init() did leak device node refcounts in
the past, and this was addressed in multiple commits; most recently in
commit 708124d9e6e7 ("of/irq: Fix device node refcount leakages in
of_irq_init()"), which fixed the refcount leaks for the initialization
failure path only. That is an indirect confirmation that keeping the
refcount on the successful path is intentional.

Signed-off-by: Radu Rendec <radu@rendec.net>
---
 drivers/of/irq.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index ef1ed9743907..bf277781cdb6 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)
 {
-- 
2.55.0

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

[sashiko-bot]

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