[PATCH 0/3] of: irq: Fixes refcount issues with of_irq_parse_one()/of_irq_parse_raw()

[PATCH 0/3] of: irq: Fixes refcount issues with of_irq_parse_one()/of_irq_parse_raw()

[Jean-Jacques Hiblot]

This series attempts to fix refcounting issues related to of_irq_parse_one()
and of_irq_parse_raw().

The first issue is simply that most callers of of_irq_parse_one() and
of_irq_parse_raw() don't call of_node_put() on the returned device node when
they no longer need it.

The second issue is a double get() happening in of_irq_parse_one() when
parsing the "interrupts-extended" properties.

WARNING: I tried to be careful when modifying the callers of
of_irq_parse_one()/of_irq_parse_raw() but haven't test-build all the changes.


Jean-Jacques Hiblot (3):
  of: irq: make callers of of_irq_parse_raw() release the device node
  of: irq: make callers of of_irq_parse_one() release the device node
  of: irq: release the node after looking up for "interrupts-extended"

 .../mach-shmobile/regulator-quirk-rcar-gen2.c |  1 +
 arch/powerpc/platforms/fsl_uli1575.c          |  1 +
 arch/powerpc/sysdev/mpic_msi.c                |  1 +
 drivers/bcma/main.c                           |  5 +++-
 drivers/clocksource/timer-clint.c             |  1 +
 drivers/irqchip/irq-mchp-eic.c                |  1 +
 drivers/irqchip/irq-owl-sirq.c                |  1 +
 drivers/irqchip/irq-renesas-rzg2l.c           |  1 +
 drivers/irqchip/irq-sifive-plic.c             |  1 +
 drivers/irqchip/irq-sun6i-r.c                 |  2 ++
 drivers/of/irq.c                              | 30 ++++++++++++++-----
 drivers/of/unittest.c                         |  7 +++++
 drivers/pci/of.c                              |  6 +++-
 drivers/soc/ti/knav_qmss_queue.c              |  3 ++
 drivers/usb/host/xhci-tegra.c                 |  1 +
 15 files changed, 53 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH 1/3] of: irq: make callers of of_irq_parse_raw() release the device node

[Jean-Jacques Hiblot]

of_irq_parse_raw() does a get() on the device node returned in out_irq->np.
Callers of of_irq_parse_raw() must do a put() when they are done with it.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 arch/powerpc/platforms/fsl_uli1575.c | 1 +
 drivers/bcma/main.c                  | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/fsl_uli1575.c b/arch/powerpc/platforms/fsl_uli1575.c
index 84afae7a25617..ba104f6474bc8 100644
--- a/arch/powerpc/platforms/fsl_uli1575.c
+++ b/arch/powerpc/platforms/fsl_uli1575.c
@@ -334,6 +334,7 @@ static void hpcd_final_uli5288(struct pci_dev *dev)
 	laddr[1] = laddr[2] = 0;
 	of_irq_parse_raw(laddr, &oirq);
 	dev->irq = irq_create_of_mapping(&oirq);
+	of_node_put(oirq.np);
 }
 
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x1575, hpcd_quirk_uli1575);
diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c
index 0a8469e0b13ad..11219dd79d327 100644
--- a/drivers/bcma/main.c
+++ b/drivers/bcma/main.c
@@ -193,7 +193,10 @@ static unsigned int bcma_of_get_irq(struct device *parent,
 		return 0;
 	}
 
-	return irq_create_of_mapping(&out_irq);
+	ret = irq_create_of_mapping(&out_irq);
+	of_node_put(out_irq.np);
+
+	return ret;
 }
 
 static void bcma_of_fill_device(struct device *parent,
-- 
2.25.1

[PATCH 2/3] of: irq: make callers of of_irq_parse_one() release the device node

[Jean-Jacques Hiblot]

of_irq_parse_one() does a get() on the device node returned in out_irq->np.
Callers of of_irq_parse_one() must do a put() when they are done with it.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 .../mach-shmobile/regulator-quirk-rcar-gen2.c |  1 +
 arch/powerpc/sysdev/mpic_msi.c                |  1 +
 drivers/clocksource/timer-clint.c             |  1 +
 drivers/irqchip/irq-mchp-eic.c                |  1 +
 drivers/irqchip/irq-owl-sirq.c                |  1 +
 drivers/irqchip/irq-renesas-rzg2l.c           |  1 +
 drivers/irqchip/irq-sifive-plic.c             |  1 +
 drivers/irqchip/irq-sun6i-r.c                 |  2 ++
 drivers/of/irq.c                              | 22 ++++++++++++++-----
 drivers/of/unittest.c                         |  7 ++++++
 drivers/pci/of.c                              |  6 ++++-
 drivers/soc/ti/knav_qmss_queue.c              |  3 +++
 drivers/usb/host/xhci-tegra.c                 |  1 +
 13 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
index 117e7b07995b9..af1c04f0705aa 100644
--- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
@@ -184,6 +184,7 @@ static int __init rcar_gen2_regulator_quirk(void)
 			kfree(quirk);
 			continue;
 		}
+		of_node_put(argsa->np);
 
 		list_for_each_entry(pos, &quirk_list, list) {
 			argsb = &pos->irq_args;
diff --git a/arch/powerpc/sysdev/mpic_msi.c b/arch/powerpc/sysdev/mpic_msi.c
index 34246c8e01c2b..bac8b12291a87 100644
--- a/arch/powerpc/sysdev/mpic_msi.c
+++ b/arch/powerpc/sysdev/mpic_msi.c
@@ -63,6 +63,7 @@ static int __init mpic_msi_reserve_u3_hwirqs(struct mpic *mpic)
 			ops->xlate(mpic->irqhost, NULL, oirq.args,
 						oirq.args_count, &hwirq, &flags);
 			msi_bitmap_reserve_hwirq(&mpic->msi_bitmap, hwirq);
+			of_node_put(oirq.np);
 		}
 	}
 
diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
index 6cfe2ab73eb0c..126912674610c 100644
--- a/drivers/clocksource/timer-clint.c
+++ b/drivers/clocksource/timer-clint.c
@@ -175,6 +175,7 @@ static int __init clint_timer_init_dt(struct device_node *np)
 		    oirq.args[0] == RV_IRQ_TIMER &&
 		    irq_find_host(oirq.np))
 			clint_timer_irq = irq_of_parse_and_map(np, i);
+		of_node_put(oirq.np);
 	}
 
 	/* If CLINT timer irq not found then fail */
diff --git a/drivers/irqchip/irq-mchp-eic.c b/drivers/irqchip/irq-mchp-eic.c
index c726a19837d29..f020d28efc6a4 100644
--- a/drivers/irqchip/irq-mchp-eic.c
+++ b/drivers/irqchip/irq-mchp-eic.c
@@ -239,6 +239,7 @@ static int mchp_eic_init(struct device_node *node, struct device_node *parent)
 		ret = of_irq_parse_one(node, i, &irq);
 		if (ret)
 			goto clk_unprepare;
+		of_node_put(irq.np);
 
 		if (WARN_ON(irq.args_count != 3)) {
 			ret = -EINVAL;
diff --git a/drivers/irqchip/irq-owl-sirq.c b/drivers/irqchip/irq-owl-sirq.c
index 6e4127465094f..1ac31acb8e792 100644
--- a/drivers/irqchip/irq-owl-sirq.c
+++ b/drivers/irqchip/irq-owl-sirq.c
@@ -311,6 +311,7 @@ static int __init owl_sirq_init(const struct owl_sirq_params *params,
 			pr_err("%pOF: failed to parse interrupt %d\n", node, i);
 			goto out_unmap;
 		}
+		of_node_put(irq.np);
 
 		if (WARN_ON(irq.args_count != 3)) {
 			ret = -EINVAL;
diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 25fd8ee66565b..095d2263e2dac 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -310,6 +310,7 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
 			return ret;
 		of_phandle_args_to_fwspec(np, map.args, map.args_count,
 					  &priv->fwspec[i]);
+		of_node_put(map.np);
 	}
 
 	return 0;
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index ff47bd0dec455..1a863ab2abd60 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -420,6 +420,7 @@ static int __init __plic_init(struct device_node *node,
 				irq_set_chained_handler(plic_parent_irq,
 							plic_handle_irq);
 		}
+		of_node_put(parent.np);
 
 		/*
 		 * When running in M-mode we need to ignore the S-mode handler.
diff --git a/drivers/irqchip/irq-sun6i-r.c b/drivers/irqchip/irq-sun6i-r.c
index a01e440494154..29e5917d1fc9d 100644
--- a/drivers/irqchip/irq-sun6i-r.c
+++ b/drivers/irqchip/irq-sun6i-r.c
@@ -317,6 +317,8 @@ static int __init sun6i_r_intc_init(struct device_node *node,
 	ret = of_irq_parse_one(node, 0, &nmi_parent);
 	if (ret)
 		return ret;
+	of_node_put(nmi_parent.np);
+
 	if (nmi_parent.args_count < 3 ||
 	    nmi_parent.args[0] != GIC_SPI ||
 	    nmi_parent.args[2] != IRQ_TYPE_LEVEL_HIGH)
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index e9bf5236ed892..95da943fcf075 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -35,12 +35,16 @@
  */
 unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
 {
+	unsigned int res;
 	struct of_phandle_args oirq;
 
 	if (of_irq_parse_one(dev, index, &oirq))
 		return 0;
 
-	return irq_create_of_mapping(&oirq);
+	res = irq_create_of_mapping(&oirq);
+	of_node_put(oirq.np);
+
+	return res;
 }
 EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
 
@@ -438,10 +442,16 @@ int of_irq_get(struct device_node *dev, int index)
 		return rc;
 
 	domain = irq_find_host(oirq.np);
-	if (!domain)
-		return -EPROBE_DEFER;
+	if (!domain) {
+		rc = -EPROBE_DEFER;
+		goto out;
+	}
 
-	return irq_create_of_mapping(&oirq);
+	rc = irq_create_of_mapping(&oirq);
+
+out:
+	of_node_put(oirq.np);
+	return rc;
 }
 EXPORT_SYMBOL_GPL(of_irq_get);
 
@@ -478,8 +488,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) {
 		nr++;
+		of_node_put(irq.np);
+	}
 
 	return nr;
 }
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index bc0f1e50a4bee..801a42c20fc79 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1021,6 +1021,8 @@ static void __init of_unittest_parse_interrupts(void)
 
 		memset(&args, 0, sizeof(args));
 		rc = of_irq_parse_one(np, i, &args);
+		if (!rc)
+			of_node_put(args.np);
 
 		passed &= !rc;
 		passed &= (args.args_count == 1);
@@ -1028,6 +1030,7 @@ static void __init of_unittest_parse_interrupts(void)
 
 		unittest(passed, "index %i - data error on node %pOF rc=%i\n",
 			 i, args.np, rc);
+
 	}
 	of_node_put(np);
 
@@ -1042,6 +1045,8 @@ static void __init of_unittest_parse_interrupts(void)
 
 		memset(&args, 0, sizeof(args));
 		rc = of_irq_parse_one(np, i, &args);
+		if (!rc)
+			of_node_put(args.np);
 
 		/* Test the values from tests-phandle.dtsi */
 		switch (i) {
@@ -1098,6 +1103,8 @@ static void __init of_unittest_parse_interrupts_extended(void)
 
 		memset(&args, 0, sizeof(args));
 		rc = of_irq_parse_one(np, i, &args);
+		if (!rc)
+			of_node_put(args.np);
 
 		/* Test the values from tests-phandle.dtsi */
 		switch (i) {
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 196834ed44fe8..a3a047f57e439 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -535,7 +535,11 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin)
 	if (ret)
 		return 0; /* Proper return code 0 == NO_IRQ */
 
-	return irq_create_of_mapping(&oirq);
+	ret = irq_create_of_mapping(&oirq);
+
+	of_node_put(oirq.np);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(of_irq_parse_and_map_pci);
 #endif	/* CONFIG_OF_IRQ */
diff --git a/drivers/soc/ti/knav_qmss_queue.c b/drivers/soc/ti/knav_qmss_queue.c
index 8fb76908be704..0ac4a0f219127 100644
--- a/drivers/soc/ti/knav_qmss_queue.c
+++ b/drivers/soc/ti/knav_qmss_queue.c
@@ -1240,6 +1240,9 @@ static int knav_setup_queue_range(struct knav_device *kdev,
 			break;
 
 		range->irqs[i].irq = irq_create_of_mapping(&oirq);
+
+		of_node_put(oirq.np);
+
 		if (range->irqs[i].irq == IRQ_NONE)
 			break;
 
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index bdb776553826b..538eedac1c71e 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -1458,6 +1458,7 @@ static int tegra_xusb_probe(struct platform_device *pdev)
 	/* Older device-trees don't have padctrl interrupt */
 	err = of_irq_parse_one(np, 0, &args);
 	if (!err) {
+		of_node_put(args.np);
 		tegra->padctl_irq = of_irq_get(np, 0);
 		if (tegra->padctl_irq <= 0) {
 			err = (tegra->padctl_irq == 0) ? -ENODEV : tegra->padctl_irq;
-- 
2.25.1

[PATCH 3/3] of: irq: release the node after looking up for "interrupts-extended"

[Jean-Jacques Hiblot]

When of_parse_phandle_with_args() succeeds, a get() is performed on
out_irq->np. And another get() is performed in of_irq_parse_raw(),
resulting in the refcount being incremented twice.
Fixing this by calling put() after of_irq_parse_raw().

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
---
 drivers/of/irq.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 95da943fcf075..244f240bc4ac4 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -349,8 +349,12 @@ 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, out_irq);
+	if (!res) {
+		p = out_irq->np;
+		res = of_irq_parse_raw(addr, out_irq);
+		of_node_put(p);
+		return res;
+	}
 
 	/* Look for the interrupt parent. */
 	p = of_irq_find_parent(device);
-- 
2.25.1

Re: [PATCH 3/3] of: irq: release the node after looking up for "interrupts-extended"

[Rob Herring]

On Wed, Mar 1, 2023 at 12:53 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
>
> When of_parse_phandle_with_args() succeeds, a get() is performed on
> out_irq->np. And another get() is performed in of_irq_parse_raw(),
> resulting in the refcount being incremented twice.
> Fixing this by calling put() after of_irq_parse_raw().

This looks like a band-aid to me. It only makes sense that the caller
of of_irq_parse_raw() already holds a ref to out_irq->np. So the first
of_node_get() in it looks wrong. It looks like the refcounting was
originally balanced, but commit 2f53a713c4b6 ("of/irq: Fix device_node
refcount in of_irq_parse_raw()") dropped the put on exit after 'got
it!'. I'm not sure if just adding it back would be correct or not
though.

All this needs some test cases to be sure we get things right...

Rob

Re: [PATCH 2/3] of: irq: make callers of of_irq_parse_one() release the device node

[Geert Uytterhoeven]

Hi Jean-Jacques,

Thanks for your patch!

On Wed, Mar 1, 2023 at 7:53 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> of_irq_parse_one() does a get() on the device node returned in out_irq->np.
> Callers of of_irq_parse_one() must do a put() when they are done with it.

What does "be done with it" really mean here?

> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> @@ -184,6 +184,7 @@ static int __init rcar_gen2_regulator_quirk(void)
>                         kfree(quirk);
>                         continue;
>                 }
> +               of_node_put(argsa->np);

The quirk object, which is a container of argsa, is still used below,
and stored in a linked list.  I agree argsa->np is not dereferenced,
but the pointer itself is still compared to other pointers.
IIUIC, calling of_node_put() might cause the reference count to drop to
zero, and the underlying struct node object to be deallocated.
So when a future reference to the same DT node will be taken, a new
struct node object will be allocated, and the pointer comparison below
will fail?

Or am I missing something?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] of: irq: make callers of of_irq_parse_one() release the device node

[Jean-Jacques Hiblot]

On 02/03/2023 08:49, Geert Uytterhoeven wrote:
> Hi Jean-Jacques,
> 
> Thanks for your patch!
> 
> On Wed, Mar 1, 2023 at 7:53 PM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
>> of_irq_parse_one() does a get() on the device node returned in out_irq->np.
>> Callers of of_irq_parse_one() must do a put() when they are done with it.
> 
> What does "be done with it" really mean here?
> 
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> 
>> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
>> @@ -184,6 +184,7 @@ static int __init rcar_gen2_regulator_quirk(void)
>>                          kfree(quirk);
>>                          continue;
>>                  }
>> +               of_node_put(argsa->np);
> 
> The quirk object, which is a container of argsa, is still used below,
> and stored in a linked list.  I agree argsa->np is not dereferenced,
> but the pointer itself is still compared to other pointers.
Hi Geert,

I fail to see when the pointers are compared. It looks to me that only 
the args are compared. Am I missing something ?
In any case, looking more closely at the code, I guess that indeed the
of_node_put() shouldn't be added here because this code expects that the
nodes never go away. That is probably a good assertion in case of PMICs

JJ
> IIUIC, calling of_node_put() might cause the reference count to drop to
> zero, and the underlying struct node object to be deallocated.
> So when a future reference to the same DT node will be taken, a new
> struct node object will be allocated, and the pointer comparison below
> will fail?
> 
> Or am I missing something?
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

Re: [PATCH 2/3] of: irq: make callers of of_irq_parse_one() release the device node

[Geert Uytterhoeven]

Hi Jean-Jacques,

On Sat, Mar 4, 2023 at 11:34 AM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> On 02/03/2023 08:49, Geert Uytterhoeven wrote:
> > On Wed, Mar 1, 2023 at 7:53 PM Jean-Jacques Hiblot
> > <jjhiblot@traphandler.com> wrote:
> >> of_irq_parse_one() does a get() on the device node returned in out_irq->np.
> >> Callers of of_irq_parse_one() must do a put() when they are done with it.
> >
> > What does "be done with it" really mean here?
> >
> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> >
> >> --- a/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> >> +++ b/arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c
> >> @@ -184,6 +184,7 @@ static int __init rcar_gen2_regulator_quirk(void)
> >>                          kfree(quirk);
> >>                          continue;
> >>                  }
> >> +               of_node_put(argsa->np);
> >
> > The quirk object, which is a container of argsa, is still used below,
> > and stored in a linked list.  I agree argsa->np is not dereferenced,
> > but the pointer itself is still compared to other pointers.
>
> I fail to see when the pointers are compared. It looks to me that only
> the args are compared. Am I missing something ?

You're right, in upstream, there is no such check.
In my local tree, I have converted the comparisons below to use a new
helper of_phandle_args_eq() (which does compare the np member, too),
but that change never went upstream, as the other user of that helper
was rejected.

> In any case, looking more closely at the code, I guess that indeed the
> of_node_put() shouldn't be added here because this code expects that the
> nodes never go away. That is probably a good assertion in case of PMICs

OK.

> > IIUIC, calling of_node_put() might cause the reference count to drop to
> > zero, and the underlying struct node object to be deallocated.
> > So when a future reference to the same DT node will be taken, a new
> > struct node object will be allocated, and the pointer comparison below
> > will fail?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds