| Message ID | 20220526110246.53502-1-linmq006@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: mediatek-gen3: Fix refcount leak in mtk_pcie_init_irq_domains | expand |
On Thu, May 26, 2022 at 03:02:46PM +0400, Miaoqian Lin wrote: > of_get_child_by_name() returns a node pointer with refcount > incremented, we should use of_node_put() on it when not need anymore. > Add missing of_node_put() to avoid refcount leak. > > Fixes: 814cceebba9b ("PCI: mediatek-gen3: Add INTx support") > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > --- > drivers/pci/controller/pcie-mediatek-gen3.c | 1 + > 1 file changed, 1 insertion(+) Acked-by: Rob Herring <robh@kernel.org>
Hi Miaoqian, >Signed-off-by: Miaoqian Lin <linmq006@gmail.com> >--- > drivers/pci/controller/pcie-mediatek-gen3.c | 1 + > 1 file changed, 1 insertion(+) > >diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c >index 3e8d70bfabc6..da8e9db0abdf 100644 >--- a/drivers/pci/controller/pcie-mediatek-gen3.c >+++ b/drivers/pci/controller/pcie-mediatek-gen3.c >@@ -600,6 +600,7 @@ static int mtk_pcie_init_irq_domains(struct mtk_gen3_pcie *pcie) > &intx_domain_ops, pcie); > if (!pcie->intx_domain) { > dev_err(dev, "failed to create INTx IRQ domain\n"); >+ of_node_put(intc_node); > return -ENODEV; > } Thanks for doing this. I checked mtk_pcie_init_irq_domains() and there are multiple exit paths like err_msi_domain and err_msi_bottom_domain and the normal path which also need of_node_put(intc_node). Maybe we can move the of_node_put(intc_node) to #54 below and cover all possible paths? cheers, Miles e.g., static int mtk_pcie_init_irq_domains(struct mtk_gen3_pcie *pcie) { struct device *dev = pcie->dev; struct device_node *intc_node, *node = dev->of_node; int ret; raw_spin_lock_init(&pcie->irq_lock); /* Setup INTx */ intc_node = of_get_child_by_name(node, "interrupt-controller"); if (!intc_node) { dev_err(dev, "missing interrupt-controller node\n"); return -ENODEV; } pcie->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX, &intx_domain_ops, pcie); of_node_put(intc_node); if (!pcie->intx_domain) { dev_err(dev, "failed to create INTx IRQ domain\n"); return -ENODEV; } /* Setup MSI */ mutex_init(&pcie->lock); pcie->msi_bottom_domain = irq_domain_add_linear(node, PCIE_MSI_IRQS_NUM, &mtk_msi_bottom_domain_ops, pcie); if (!pcie->msi_bottom_domain) { dev_err(dev, "failed to create MSI bottom domain\n"); ret = -ENODEV; goto err_msi_bottom_domain; } pcie->msi_domain = pci_msi_create_irq_domain(dev->fwnode, &mtk_msi_domain_info, pcie->msi_bottom_domain); if (!pcie->msi_domain) { dev_err(dev, "failed to create MSI domain\n"); ret = -ENODEV; goto err_msi_domain; } return 0; err_msi_domain: irq_domain_remove(pcie->msi_bottom_domain); err_msi_bottom_domain: irq_domain_remove(pcie->intx_domain); return ret; } > >-- >2.25.1
Hi Miles, On 2022/5/27 16:45, Miles Chen wrote: > Hi Miaoqian, > >> Signed-off-by: Miaoqian Lin <linmq006@gmail.com> >> --- >> drivers/pci/controller/pcie-mediatek-gen3.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c >> index 3e8d70bfabc6..da8e9db0abdf 100644 >> --- a/drivers/pci/controller/pcie-mediatek-gen3.c >> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c >> @@ -600,6 +600,7 @@ static int mtk_pcie_init_irq_domains(struct mtk_gen3_pcie *pcie) >> &intx_domain_ops, pcie); >> if (!pcie->intx_domain) { >> dev_err(dev, "failed to create INTx IRQ domain\n"); >> + of_node_put(intc_node); >> return -ENODEV; >> } > Thanks for doing this. > > I checked mtk_pcie_init_irq_domains() and there are multiple exit paths like > err_msi_domain and err_msi_bottom_domain and the normal path which also > need of_node_put(intc_node). Thanks for your reply, I didn't add of_node_put() in other paths because I am not sure if the reference passed through irq_domain_add_linear(), since intc_node is passed to irq_domain_add_linear(). __irq_domain_add() keeps &node->fwnode in the irq_domain structure. and use fwnode_handle_get() to get the reference of fwnode, but I still uncertain. If the reference don't needed anymore after irq_domain_add_linear(), your suggestion looks fine, and I will submit v2. > Maybe we can move the of_node_put(intc_node) to #54 below and cover > all possible paths? > > > cheers, > Miles > > e.g., > > static int mtk_pcie_init_irq_domains(struct mtk_gen3_pcie *pcie) > { > struct device *dev = pcie->dev; > struct device_node *intc_node, *node = dev->of_node; > int ret; > > raw_spin_lock_init(&pcie->irq_lock); > > /* Setup INTx */ > intc_node = of_get_child_by_name(node, "interrupt-controller"); > if (!intc_node) { > dev_err(dev, "missing interrupt-controller node\n"); > return -ENODEV; > } > > pcie->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX, > &intx_domain_ops, pcie); > of_node_put(intc_node); > if (!pcie->intx_domain) { > dev_err(dev, "failed to create INTx IRQ domain\n"); > return -ENODEV; > } > > /* Setup MSI */ > mutex_init(&pcie->lock); > > pcie->msi_bottom_domain = irq_domain_add_linear(node, PCIE_MSI_IRQS_NUM, > &mtk_msi_bottom_domain_ops, pcie); > if (!pcie->msi_bottom_domain) { > dev_err(dev, "failed to create MSI bottom domain\n"); > ret = -ENODEV; > goto err_msi_bottom_domain; > } > > pcie->msi_domain = pci_msi_create_irq_domain(dev->fwnode, > &mtk_msi_domain_info, > pcie->msi_bottom_domain); > if (!pcie->msi_domain) { > dev_err(dev, "failed to create MSI domain\n"); > ret = -ENODEV; > goto err_msi_domain; > } > > return 0; > > err_msi_domain: > irq_domain_remove(pcie->msi_bottom_domain); > err_msi_bottom_domain: > irq_domain_remove(pcie->intx_domain); > > return ret; > } >> -- >> 2.25.1 >
Hi Miaoqian, >>> &intx_domain_ops, pcie); >>> if (!pcie->intx_domain) { >>> dev_err(dev, "failed to create INTx IRQ domain\n"); >>> + of_node_put(intc_node); >>> return -ENODEV; >>> } >> Thanks for doing this. >> >> I checked mtk_pcie_init_irq_domains() and there are multiple exit paths like >> err_msi_domain and err_msi_bottom_domain and the normal path which also >> need of_node_put(intc_node). > >Thanks for your reply, > >I didn't add of_node_put() in other paths because I am not sure if the reference passed through irq_domain_add_linear(), since intc_node is passed to irq_domain_add_linear(). > >__irq_domain_add() keeps &node->fwnode in the irq_domain structure. > >and use fwnode_handle_get() to get the reference of fwnode, but I still uncertain. > >If the reference don't needed anymore after irq_domain_add_linear(), > >your suggestion looks fine, and I will submit v2. Thanks for your reply, I think we can do similar things like rtl8365mb_irq_setup() in drivers/net/dsa/realtek/rtl8365mb.c Thanks, Miles
Hi, Miles On 2022/5/30 10:19, Miles Chen wrote: > Hi Miaoqian, > >>>> &intx_domain_ops, pcie); >>>> if (!pcie->intx_domain) { >>>> dev_err(dev, "failed to create INTx IRQ domain\n"); >>>> + of_node_put(intc_node); >>>> return -ENODEV; >>>> } >>> Thanks for doing this. >>> >>> I checked mtk_pcie_init_irq_domains() and there are multiple exit paths like >>> err_msi_domain and err_msi_bottom_domain and the normal path which also >>> need of_node_put(intc_node). >> Thanks for your reply, >> >> I didn't add of_node_put() in other paths because I am not sure if the reference passed through irq_domain_add_linear(), since intc_node is passed to irq_domain_add_linear(). >> >> __irq_domain_add() keeps &node->fwnode in the irq_domain structure. >> >> and use fwnode_handle_get() to get the reference of fwnode, but I still uncertain. >> >> If the reference don't needed anymore after irq_domain_add_linear(), >> >> your suggestion looks fine, and I will submit v2. > > Thanks for your reply, I think we can do similar things like > rtl8365mb_irq_setup() in drivers/net/dsa/realtek/rtl8365mb.c I checked rtl8365mb_irq_setup(), it calls of_node_put() by goto statement for error paths. and calls of_node_put() before return 0 in normal path. I didn't see the same problem. > Thanks, > Miles
Hi Miaoqian, >Hi, Miles > >On 2022/5/30 10:19, Miles Chen wrote: >> Hi Miaoqian, >> >>>>> &intx_domain_ops, pcie); >>>>> if (!pcie->intx_domain) { >>>>> dev_err(dev, "failed to create INTx IRQ domain\n"); >>>>> + of_node_put(intc_node); >>>>> return -ENODEV; >>>>> } >>>> Thanks for doing this. >>>> >>>> I checked mtk_pcie_init_irq_domains() and there are multiple exit paths like >>>> err_msi_domain and err_msi_bottom_domain and the normal path which also >>>> need of_node_put(intc_node). >>> Thanks for your reply, >>> >>> I didn't add of_node_put() in other paths because I am not sure if the reference passed through irq_domain_add_linear(), since intc_node is passed to irq_domain_add_linear(). >>> >>> __irq_domain_add() keeps &node->fwnode in the irq_domain structure. >>> >>> and use fwnode_handle_get() to get the reference of fwnode, but I still uncertain. >>> >>> If the reference don't needed anymore after irq_domain_add_linear(), >>> >>> your suggestion looks fine, and I will submit v2. >> >> Thanks for your reply, I think we can do similar things like >> rtl8365mb_irq_setup() in drivers/net/dsa/realtek/rtl8365mb.c > >I checked rtl8365mb_irq_setup(), it calls of_node_put() by goto statement for error paths. > >and calls of_node_put() before return 0 in normal path. I didn't see the same problem. Sorry for the confusing. I meant that we can do the same thing - it calls of_node_put() by goto statement for error paths and calls of_node_put() before return 0 in normal path. :-) Thanks, Miles
Hi, Miles On 2022/5/30 15:35, Miles Chen wrote: > Hi Miaoqian, > >> Hi, Miles >> >> On 2022/5/30 10:19, Miles Chen wrote: >>> Hi Miaoqian, >>> >>>>>> &intx_domain_ops, pcie); >>>>>> if (!pcie->intx_domain) { >>>>>> dev_err(dev, "failed to create INTx IRQ domain\n"); >>>>>> + of_node_put(intc_node); >>>>>> return -ENODEV; >>>>>> } >>>>> Thanks for doing this. >>>>> >>>>> I checked mtk_pcie_init_irq_domains() and there are multiple exit paths like >>>>> err_msi_domain and err_msi_bottom_domain and the normal path which also >>>>> need of_node_put(intc_node). >>>> Thanks for your reply, >>>> >>>> I didn't add of_node_put() in other paths because I am not sure if the reference passed through irq_domain_add_linear(), since intc_node is passed to irq_domain_add_linear(). >>>> >>>> __irq_domain_add() keeps &node->fwnode in the irq_domain structure. >>>> >>>> and use fwnode_handle_get() to get the reference of fwnode, but I still uncertain. >>>> >>>> If the reference don't needed anymore after irq_domain_add_linear(), >>>> >>>> your suggestion looks fine, and I will submit v2. >>> Thanks for your reply, I think we can do similar things like >>> rtl8365mb_irq_setup() in drivers/net/dsa/realtek/rtl8365mb.c >> I checked rtl8365mb_irq_setup(), it calls of_node_put() by goto statement for error paths. >> >> and calls of_node_put() before return 0 in normal path. I didn't see the same problem. > Sorry for the confusing. I meant that we can do the same thing - > it calls of_node_put() by goto statement for error paths > and calls of_node_put() before return 0 in normal path. :-) I'll sent a v2 for this: https://lore.kernel.org/all/20220530064807.34534-1-linmq006@gmail.com/ following your original suggestion. > Thanks, > Miles
diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c index 3e8d70bfabc6..da8e9db0abdf 100644 --- a/drivers/pci/controller/pcie-mediatek-gen3.c +++ b/drivers/pci/controller/pcie-mediatek-gen3.c @@ -600,6 +600,7 @@ static int mtk_pcie_init_irq_domains(struct mtk_gen3_pcie *pcie) &intx_domain_ops, pcie); if (!pcie->intx_domain) { dev_err(dev, "failed to create INTx IRQ domain\n"); + of_node_put(intc_node); return -ENODEV; }
of_get_child_by_name() returns a node pointer with refcount incremented, we should use of_node_put() on it when not need anymore. Add missing of_node_put() to avoid refcount leak. Fixes: 814cceebba9b ("PCI: mediatek-gen3: Add INTx support") Signed-off-by: Miaoqian Lin <linmq006@gmail.com> --- drivers/pci/controller/pcie-mediatek-gen3.c | 1 + 1 file changed, 1 insertion(+)