[PATCH v2 0/9] of: fix bugs about refcount

[PATCH v2 0/9] of: fix bugs about refcount

[Zijun Hu]

This patch series is to fix of bugs about refcount.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Changes in v2:
- Add 2 unittest patches + 1 refcount bug fix + 1 refcount comments patch
- Correct titles and commit messages
- Link to v1: https://lore.kernel.org/r/20241209-of_irq_fix-v1-0-782f1419c8a1@quicinc.com

---
Zijun Hu (9):
      of: unittest: Add a case to test if API of_irq_parse_one() leaks refcount
      of/irq: Fix device node refcount leakage in API of_irq_parse_one()
      of: unittest: Add a case to test if API of_irq_parse_raw() leaks refcount
      of/irq: Fix device node refcount leakage in API of_irq_parse_raw()
      of/irq: Fix device node refcount leakages in of_irq_count()
      of/irq: Fix device node refcount leakage in API irq_of_parse_and_map()
      of/irq: Fix device node refcount leakages in of_irq_init()
      of/irq: Add comments about refcount for API of_irq_find_parent()
      of: resolver: Fix device node refcount leakage in of_resolve_phandles()

 drivers/of/irq.c                               | 34 ++++++++++---
 drivers/of/resolver.c                          |  2 +
 drivers/of/unittest-data/tests-interrupts.dtsi | 13 +++++
 drivers/of/unittest.c                          | 67 ++++++++++++++++++++++++++
 4 files changed, 110 insertions(+), 6 deletions(-)
---
base-commit: 40fc0083a9dbcf2e81b1506274cb541f84d022ed
change-id: 20241208-of_irq_fix-659514bc9aa3

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

[PATCH v2 2/9] of/irq: Fix device node refcount leakage in API of_irq_parse_one()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

of_irq_parse_one(@int_gen_dev, i, ...) will leak refcount of @i_th_phandle

int_gen_dev {
    ...
    interrupts-extended = ..., <&i_th_phandle ...>, ...;
    ...
};

Refcount of @i_th_phandle is increased by of_parse_phandle_with_args()
but is not decreased by API of_irq_parse_one() before return, so causes
refcount leakage.

Fix by putting the node's refcount before return as the other branch does.
Also add comments about refcount of node @out_irq->np got by the API.

Fixes: 79d9701559a9 ("of/irq: create interrupts-extended property")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 6c843d54ebb116132144e6300d6d7ce94e763cf8..d88719c316a3931502c34a65b2db8921f7528d99 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -339,6 +339,8 @@ EXPORT_SYMBOL_GPL(of_irq_parse_raw);
  * This function resolves an interrupt for a node by walking the interrupt tree,
  * finding which interrupt controller node it is attached to, and returning the
  * interrupt specifier that can be used to retrieve a Linux IRQ number.
+ *
+ * Note: refcount of node @out_irq->np is increased by 1 on success.
  */
 int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_args *out_irq)
 {
@@ -367,8 +369,11 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
 	/* Try the new-style interrupts-extended first */
 	res = of_parse_phandle_with_args(device, "interrupts-extended",
 					"#interrupt-cells", index, out_irq);
-	if (!res)
-		return of_irq_parse_raw(addr_buf, out_irq);
+	if (!res) {
+		p = out_irq->np;
+		res = of_irq_parse_raw(addr_buf, out_irq);
+		goto out;
+	}
 
 	/* Look for the interrupt parent. */
 	p = of_irq_find_parent(device);

-- 
2.34.1

[PATCH v2 4/9] of/irq: Fix device node refcount leakage in API of_irq_parse_raw()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

if the node @out_irq->np got by of_irq_parse_raw() is a combo node which
consists of both controller and nexus, namely, of_irq_parse_raw() returns
due to condition (@ipar == @newpar), then the node's refcount was increased
twice, hence causes refcount leakage.

Fix by putting @out_irq->np refcount before returning due to the condition.
Also add comments about refcount of node @out_irq->np got by the API.

Fixes: 041284181226 ("of/irq: Allow matching of an interrupt-map local to an interrupt controller")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index d88719c316a3931502c34a65b2db8921f7528d99..c41b2533d86d8eceabe8f2e43842af33f22febff 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -165,6 +165,8 @@ const __be32 *of_irq_parse_imap_parent(const __be32 *imap, int len, struct of_ph
  * the specifier for each map, and then returns the translated map.
  *
  * Return: 0 on success and a negative number on error
+ *
+ * Note: refcount of node @out_irq->np is increased by 1 on success.
  */
 int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 {
@@ -310,6 +312,12 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 		addrsize = (imap - match_array) - intsize;
 
 		if (ipar == newpar) {
+			/*
+			 * Has got @ipar's refcount, but the refcount was
+			 * got again by of_irq_parse_imap_parent() via its
+			 * alias @newpair.
+			 */
+			of_node_put(ipar);
 			pr_debug("%pOF interrupt-map entry to self\n", ipar);
 			return 0;
 		}

-- 
2.34.1

[PATCH v2 5/9] of/irq: Fix device node refcount leakages in of_irq_count()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

of_irq_count() invokes of_irq_parse_one() to count IRQs, and successful
invocation of the later will get device node @irq.np refcount, but the
former does not put the refcount before next iteration invocation, hence
causes device node refcount leakages.

Fix by putting @irq.np refcount before the next iteration invocation.

Fixes: 3da5278727a8 ("of/irq: Rework of_irq_count()")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index c41b2533d86d8eceabe8f2e43842af33f22febff..95a482a584740c3a0f55b791157072166dffffe0 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -518,8 +518,10 @@ int of_irq_count(struct device_node *dev)
 	struct of_phandle_args irq;
 	int nr = 0;
 
-	while (of_irq_parse_one(dev, nr, &irq) == 0)
+	while (of_irq_parse_one(dev, nr, &irq) == 0) {
+		of_node_put(irq.np);
 		nr++;
+	}
 
 	return nr;
 }

-- 
2.34.1

[PATCH v2 6/9] of/irq: Fix device node refcount leakage in API irq_of_parse_and_map()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

In irq_of_parse_and_map(), refcount of device node @oirq.np was got
by successful of_irq_parse_one() invocation, but it does not put the
refcount before return, so causes @oirq.np refcount leakage.

Fix by putting @oirq.np refcount before return.

Fixes: e3873444990d ("of/irq: Move irq_of_parse_and_map() to common code")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 95a482a584740c3a0f55b791157072166dffffe0..064db004eea5129efb7d267abf7c1133c9a76e26 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -38,11 +38,15 @@
 unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
 {
 	struct of_phandle_args oirq;
+	unsigned int ret;
 
 	if (of_irq_parse_one(dev, index, &oirq))
 		return 0;
 
-	return irq_create_of_mapping(&oirq);
+	ret = irq_create_of_mapping(&oirq);
+	of_node_put(oirq.np);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
 

-- 
2.34.1

[PATCH v2 7/9] of/irq: Fix device node refcount leakages in of_irq_init()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

of_irq_init() will leak interrupt controller device node refcounts
in two places as explained below:

1) Leak refcounts of both @desc->dev and @desc->interrupt_parent when
   suffers @desc->irq_init_cb() failure.
2) Leak refcount of @desc->interrupt_parent when cleans up list
   @intc_desc_list in the end.

Refcounts of both @desc->dev and @desc->interrupt_parent were got in
the first loop, but of_irq_init() does not put them before kfree(@desc)
in places mentioned above, so causes refcount leakages.

Fix by putting refcounts involved before kfree(@desc).

Fixes: 8363ccb917c6 ("of/irq: add missing of_node_put")
Fixes: c71a54b08201 ("of/irq: introduce of_irq_init")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/irq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 064db004eea5129efb7d267abf7c1133c9a76e26..ded2a18776671bb30b3c75367e0857494a5c8570 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -642,6 +642,8 @@ void __init of_irq_init(const struct of_device_id *matches)
 				       __func__, desc->dev, desc->dev,
 				       desc->interrupt_parent);
 				of_node_clear_flag(desc->dev, OF_POPULATED);
+				of_node_put(desc->interrupt_parent);
+				of_node_put(desc->dev);
 				kfree(desc);
 				continue;
 			}
@@ -672,6 +674,7 @@ void __init of_irq_init(const struct of_device_id *matches)
 err:
 	list_for_each_entry_safe(desc, temp_desc, &intc_desc_list, list) {
 		list_del(&desc->list);
+		of_node_put(desc->interrupt_parent);
 		of_node_put(desc->dev);
 		kfree(desc);
 	}

-- 
2.34.1

[PATCH v2 9/9] of: resolver: Fix device node refcount leakage in of_resolve_phandles()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

In of_resolve_phandles(), refcount of device node @local_fixups will be
increased if the for_each_child_of_node() exits early, but nowhere to
decrease the refcount, so cause refcount leakage for the node.

Fix by adding of_node_put(@local_fixups) before return.

Fixes: da56d04c806a ("of/resolver: Switch to new local fixups format.")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/of/resolver.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 779db058c42f5b8198ee3417dfaab80c81b43e4c..b589e59667fd3ea2c2bd5240414803cb17707ec9 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -256,6 +256,7 @@ int of_resolve_phandles(struct device_node *overlay)
 	phandle phandle, phandle_delta;
 	int err;
 
+	local_fixups = NULL;
 	tree_symbols = NULL;
 
 	if (!overlay) {
@@ -332,6 +333,7 @@ int of_resolve_phandles(struct device_node *overlay)
 	if (err)
 		pr_err("overlay phandle fixup failed: %d\n", err);
 	of_node_put(tree_symbols);
+	of_node_put(local_fixups);
 
 	return err;
 }

-- 
2.34.1

Re: [PATCH v2 0/9] of: fix bugs about refcount

[Rob Herring]

On Sun, Feb 09, 2025 at 08:58:53PM +0800, Zijun Hu wrote:
> This patch series is to fix of bugs about refcount.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> Changes in v2:
> - Add 2 unittest patches + 1 refcount bug fix + 1 refcount comments patch
> - Correct titles and commit messages
> - Link to v1: https://lore.kernel.org/r/20241209-of_irq_fix-v1-0-782f1419c8a1@quicinc.com
> 
> ---
> Zijun Hu (9):
>       of: unittest: Add a case to test if API of_irq_parse_one() leaks refcount
>       of/irq: Fix device node refcount leakage in API of_irq_parse_one()
>       of: unittest: Add a case to test if API of_irq_parse_raw() leaks refcount
>       of/irq: Fix device node refcount leakage in API of_irq_parse_raw()
>       of/irq: Fix device node refcount leakages in of_irq_count()
>       of/irq: Fix device node refcount leakage in API irq_of_parse_and_map()
>       of/irq: Fix device node refcount leakages in of_irq_init()
>       of/irq: Add comments about refcount for API of_irq_find_parent()
>       of: resolver: Fix device node refcount leakage in of_resolve_phandles()
> 
>  drivers/of/irq.c                               | 34 ++++++++++---
>  drivers/of/resolver.c                          |  2 +
>  drivers/of/unittest-data/tests-interrupts.dtsi | 13 +++++
>  drivers/of/unittest.c                          | 67 ++++++++++++++++++++++++++
>  4 files changed, 110 insertions(+), 6 deletions(-)

I've applied the series. I made a few adjustments to use __free() 
cleanup and simplify things.

Rob

Re: [PATCH v2 0/9] of: fix bugs about refcount

[Zijun Hu]

On 2025/2/25 07:26, Rob Herring wrote:
>> ---
>> Zijun Hu (9):
>>       of: unittest: Add a case to test if API of_irq_parse_one() leaks refcount
>>       of/irq: Fix device node refcount leakage in API of_irq_parse_one()
>>       of: unittest: Add a case to test if API of_irq_parse_raw() leaks refcount
>>       of/irq: Fix device node refcount leakage in API of_irq_parse_raw()
>>       of/irq: Fix device node refcount leakages in of_irq_count()
>>       of/irq: Fix device node refcount leakage in API irq_of_parse_and_map()
>>       of/irq: Fix device node refcount leakages in of_irq_init()
>>       of/irq: Add comments about refcount for API of_irq_find_parent()
>>       of: resolver: Fix device node refcount leakage in of_resolve_phandles()
>>
>>  drivers/of/irq.c                               | 34 ++++++++++---
>>  drivers/of/resolver.c                          |  2 +
>>  drivers/of/unittest-data/tests-interrupts.dtsi | 13 +++++
>>  drivers/of/unittest.c                          | 67 ++++++++++++++++++++++++++
>>  4 files changed, 110 insertions(+), 6 deletions(-)
> I've applied the series. I made a few adjustments to use __free() 
> cleanup and simplify things.

thank you, LGTM for all adjustments but perhaps a mistake fixed by
https://lore.kernel.org/all/20250225-fix_auto-v1-1-cf8b91a311dd@quicinc.com/