| Message ID | 20230301185209.274134-3-jjhiblot@traphandler.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | of: irq: Fixes refcount issues with of_irq_parse_one()/of_irq_parse_raw() | expand |
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
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 >
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
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;
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(-)