| Message ID | 20171112122850.30804-1-johan@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: keystone: fix interrupt-controller-node lookup | expand |
Hi Johan, On Sun, Nov 12, 2017 at 01:28:50PM +0100, Johan Hovold wrote: > Fix child-node lookup during initialisation, which ended up searching > the whole device tree depth-first starting at the parent rather than > just matching on its children. > > To make things worse, the parent pci node was prematurely freed, while > the child interrupt-controller node was leaked. Thanks for fixing this. I would kindly ask you please to split the patch in two since there are two bugs you are fixing at once. Murali: please test/ack accordingly. Thanks ! Lorenzo > Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver") > Cc: stable <stable@vger.kernel.org> # 3.18 > Cc: Murali Karicheri <m-karicheri2@ti.com> > --- > drivers/pci/dwc/pci-keystone.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/dwc/pci-keystone.c b/drivers/pci/dwc/pci-keystone.c > index 5bee3af47588..39405598b22d 100644 > --- a/drivers/pci/dwc/pci-keystone.c > +++ b/drivers/pci/dwc/pci-keystone.c > @@ -178,7 +178,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > } > > /* interrupt controller is in a child node */ > - *np_temp = of_find_node_by_name(np_pcie, controller); > + *np_temp = of_get_child_by_name(np_pcie, controller); > if (!(*np_temp)) { > dev_err(dev, "Node for %s is absent\n", controller); > return -EINVAL; > @@ -187,6 +187,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > temp = of_irq_count(*np_temp); > if (!temp) { > dev_err(dev, "No IRQ entries in %s\n", controller); > + of_node_put(*np_temp); > return -EINVAL; > } > > @@ -204,6 +205,8 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > break; > } > > + of_node_put(*np_temp); > + > if (temp) { > *num_irqs = temp; > return 0; > -- > 2.15.0 >
-----Original Message----- From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com] Sent: Monday, November 13, 2017 5:43 AM To: Johan Hovold Cc: Karicheri, Muralidharan; Bjorn Helgaas; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; stable Subject: Re: [PATCH] PCI: keystone: fix interrupt-controller-node lookup Hi Johan, On Sun, Nov 12, 2017 at 01:28:50PM +0100, Johan Hovold wrote: > Fix child-node lookup during initialisation, which ended up searching > the whole device tree depth-first starting at the parent rather than > just matching on its children. > > To make things worse, the parent pci node was prematurely freed, while > the child interrupt-controller node was leaked. Thanks for fixing this. I would kindly ask you please to split the patch in two since there are two bugs you are fixing at once. Murali: please test/ack accordingly. Thanks! Acked-by: Murali Karicheri <m-karicheri2@ti.com> Thanks ! Lorenzo > Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver") > Cc: stable <stable@vger.kernel.org> # 3.18 > Cc: Murali Karicheri <m-karicheri2@ti.com> > --- > drivers/pci/dwc/pci-keystone.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/dwc/pci-keystone.c > b/drivers/pci/dwc/pci-keystone.c index 5bee3af47588..39405598b22d > 100644 > --- a/drivers/pci/dwc/pci-keystone.c > +++ b/drivers/pci/dwc/pci-keystone.c > @@ -178,7 +178,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > } > > /* interrupt controller is in a child node */ > - *np_temp = of_find_node_by_name(np_pcie, controller); > + *np_temp = of_get_child_by_name(np_pcie, controller); > if (!(*np_temp)) { > dev_err(dev, "Node for %s is absent\n", controller); > return -EINVAL; > @@ -187,6 +187,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > temp = of_irq_count(*np_temp); > if (!temp) { > dev_err(dev, "No IRQ entries in %s\n", controller); > + of_node_put(*np_temp); > return -EINVAL; > } > > @@ -204,6 +205,8 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > break; > } > > + of_node_put(*np_temp); > + > if (temp) { > *num_irqs = temp; > return 0; > -- > 2.15.0 >
On Mon, Nov 13, 2017 at 10:43:06AM +0000, Lorenzo Pieralisi wrote: > Hi Johan, > > On Sun, Nov 12, 2017 at 01:28:50PM +0100, Johan Hovold wrote: > > Fix child-node lookup during initialisation, which ended up searching > > the whole device tree depth-first starting at the parent rather than > > just matching on its children. > > > > To make things worse, the parent pci node was prematurely freed, while > > the child interrupt-controller node was leaked. > > Thanks for fixing this. I would kindly ask you please to split the > patch in two since there are two bugs you are fixing at once. I guess it depends on how you look at it. I'm fixing the child-node lookup which just happens to broken in several ways: tree-wide search, parent-node put-imbalance and node leaks in both the error and success paths. Fixing that in two or even three patches seems a bit excessive, especially as the first patch would in a sense be broken as the of_get_child_by_name() does indeed (also) return a refcounted node. And furthermore, this broken lookup, in all of its aspects, was introduced by a single commit. But if you insist, I'll split it up of course. Thanks, Johan > > Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver") > > Cc: stable <stable@vger.kernel.org> # 3.18 > > Cc: Murali Karicheri <m-karicheri2@ti.com> > > --- > > drivers/pci/dwc/pci-keystone.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/dwc/pci-keystone.c b/drivers/pci/dwc/pci-keystone.c > > index 5bee3af47588..39405598b22d 100644 > > --- a/drivers/pci/dwc/pci-keystone.c > > +++ b/drivers/pci/dwc/pci-keystone.c > > @@ -178,7 +178,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > > } > > > > /* interrupt controller is in a child node */ > > - *np_temp = of_find_node_by_name(np_pcie, controller); > > + *np_temp = of_get_child_by_name(np_pcie, controller); > > if (!(*np_temp)) { > > dev_err(dev, "Node for %s is absent\n", controller); > > return -EINVAL; > > @@ -187,6 +187,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > > temp = of_irq_count(*np_temp); > > if (!temp) { > > dev_err(dev, "No IRQ entries in %s\n", controller); > > + of_node_put(*np_temp); > > return -EINVAL; > > } > > > > @@ -204,6 +205,8 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > > break; > > } > > > > + of_node_put(*np_temp); > > + > > if (temp) { > > *num_irqs = temp; > > return 0; > > -- > > 2.15.0 > >
On Tue, Nov 14, 2017 at 06:34:37PM +0100, Johan Hovold wrote: > On Mon, Nov 13, 2017 at 10:43:06AM +0000, Lorenzo Pieralisi wrote: > > Hi Johan, > > > > On Sun, Nov 12, 2017 at 01:28:50PM +0100, Johan Hovold wrote: > > > Fix child-node lookup during initialisation, which ended up searching > > > the whole device tree depth-first starting at the parent rather than > > > just matching on its children. > > > > > > To make things worse, the parent pci node was prematurely freed, while > > > the child interrupt-controller node was leaked. > > > > Thanks for fixing this. I would kindly ask you please to split the > > patch in two since there are two bugs you are fixing at once. > > I guess it depends on how you look at it. I'm fixing the child-node > lookup which just happens to broken in several ways: tree-wide search, > parent-node put-imbalance and node leaks in both the error and > success paths. > > Fixing that in two or even three patches seems a bit excessive, > especially as the first patch would in a sense be broken as the > of_get_child_by_name() does indeed (also) return a refcounted node. And > furthermore, this broken lookup, in all of its aspects, was introduced > by a single commit. > > But if you insist, I'll split it up of course. No, your analysis is correct - I will have a final look and apply with Murali's ACK. Thanks, Lorenzo > Thanks, > Johan > > > > Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver") > > > Cc: stable <stable@vger.kernel.org> # 3.18 > > > Cc: Murali Karicheri <m-karicheri2@ti.com> > > > --- > > > drivers/pci/dwc/pci-keystone.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/pci/dwc/pci-keystone.c b/drivers/pci/dwc/pci-keystone.c > > > index 5bee3af47588..39405598b22d 100644 > > > --- a/drivers/pci/dwc/pci-keystone.c > > > +++ b/drivers/pci/dwc/pci-keystone.c > > > @@ -178,7 +178,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > > > } > > > > > > /* interrupt controller is in a child node */ > > > - *np_temp = of_find_node_by_name(np_pcie, controller); > > > + *np_temp = of_get_child_by_name(np_pcie, controller); > > > if (!(*np_temp)) { > > > dev_err(dev, "Node for %s is absent\n", controller); > > > return -EINVAL; > > > @@ -187,6 +187,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > > > temp = of_irq_count(*np_temp); > > > if (!temp) { > > > dev_err(dev, "No IRQ entries in %s\n", controller); > > > + of_node_put(*np_temp); > > > return -EINVAL; > > > } > > > > > > @@ -204,6 +205,8 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > > > break; > > > } > > > > > > + of_node_put(*np_temp); > > > + > > > if (temp) { > > > *num_irqs = temp; > > > return 0; > > > -- > > > 2.15.0 > > >
Hi Johan, On Sun, Nov 12, 2017 at 01:28:50PM +0100, Johan Hovold wrote: > Fix child-node lookup during initialisation, which ended up searching > the whole device tree depth-first starting at the parent rather than > just matching on its children. > > To make things worse, the parent pci node was prematurely freed, while > the child interrupt-controller node was leaked. I think you should explain that of_find_node_by_name() drops a reference to the from pointer, it is not clear from the log. More importantly: are you saying that all of_find_node_by_name() usages with a (from* != NULL) are broken unless they bump up the from node (if != NULL) ref count ? Is there a reason why of_find_node_by_name() behaviour can't be changed ? > Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver") > Cc: stable <stable@vger.kernel.org> # 3.18 Do we really want to send this to stable kernels straight away ? There is not any specific bug report - it should be safe but I wanted to ask. > Cc: Murali Karicheri <m-karicheri2@ti.com> > --- > drivers/pci/dwc/pci-keystone.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) With an update log: Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > diff --git a/drivers/pci/dwc/pci-keystone.c b/drivers/pci/dwc/pci-keystone.c > index 5bee3af47588..39405598b22d 100644 > --- a/drivers/pci/dwc/pci-keystone.c > +++ b/drivers/pci/dwc/pci-keystone.c > @@ -178,7 +178,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > } > > /* interrupt controller is in a child node */ > - *np_temp = of_find_node_by_name(np_pcie, controller); > + *np_temp = of_get_child_by_name(np_pcie, controller); > if (!(*np_temp)) { > dev_err(dev, "Node for %s is absent\n", controller); > return -EINVAL; > @@ -187,6 +187,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > temp = of_irq_count(*np_temp); > if (!temp) { > dev_err(dev, "No IRQ entries in %s\n", controller); > + of_node_put(*np_temp); > return -EINVAL; > } > > @@ -204,6 +205,8 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, > break; > } > > + of_node_put(*np_temp); > + > if (temp) { > *num_irqs = temp; > return 0; > -- > 2.15.0 >
On Fri, Nov 17, 2017 at 11:19:10AM +0000, Lorenzo Pieralisi wrote: > Hi Johan, > > On Sun, Nov 12, 2017 at 01:28:50PM +0100, Johan Hovold wrote: > > Fix child-node lookup during initialisation, which ended up searching > > the whole device tree depth-first starting at the parent rather than > > just matching on its children. > > > > To make things worse, the parent pci node was prematurely freed, while > > the child interrupt-controller node was leaked. > > I think you should explain that of_find_node_by_name() drops a reference > to the from pointer, it is not clear from the log. Sure, I'll amend the log. > More importantly: are you saying that all of_find_node_by_name() usages > with a (from* != NULL) are broken unless they bump up the from node (if > != NULL) ref count ? > > Is there a reason why of_find_node_by_name() behaviour can't be changed ? The problem here is that this driver is using the wrong helper; of_find_node_by_name() is used for tree-wide searches, and for which its semantics makes sense. Here we only want to match on child nodes, and then of_get_child_by_name() is what you need to use. We had some 10-20 drivers that were using the wrong helper and which I've now fixed up. I'm gonna see about amending the kernel docs to minimise the risk of these bugs from reoccurring further. > > Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver") > > Cc: stable <stable@vger.kernel.org> # 3.18 > > Do we really want to send this to stable kernels straight away ? > > There is not any specific bug report - it should be safe but I > wanted to ask. The unbalanced put can lead to some nasty use-after-free issues (e.g. after a couple of probe deferrals). But feel to free to drop the stable tags if you deem this to be safe for this particular driver (most of the other fixes have gone in with a stable tag). > > Cc: Murali Karicheri <m-karicheri2@ti.com> > > --- > > drivers/pci/dwc/pci-keystone.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > With an update log: > > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Thanks for the ack. I'll respin a v2 with an updated commit message. Johan
diff --git a/drivers/pci/dwc/pci-keystone.c b/drivers/pci/dwc/pci-keystone.c index 5bee3af47588..39405598b22d 100644 --- a/drivers/pci/dwc/pci-keystone.c +++ b/drivers/pci/dwc/pci-keystone.c @@ -178,7 +178,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, } /* interrupt controller is in a child node */ - *np_temp = of_find_node_by_name(np_pcie, controller); + *np_temp = of_get_child_by_name(np_pcie, controller); if (!(*np_temp)) { dev_err(dev, "Node for %s is absent\n", controller); return -EINVAL; @@ -187,6 +187,7 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, temp = of_irq_count(*np_temp); if (!temp) { dev_err(dev, "No IRQ entries in %s\n", controller); + of_node_put(*np_temp); return -EINVAL; } @@ -204,6 +205,8 @@ static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie, break; } + of_node_put(*np_temp); + if (temp) { *num_irqs = temp; return 0;
Fix child-node lookup during initialisation, which ended up searching the whole device tree depth-first starting at the parent rather than just matching on its children. To make things worse, the parent pci node was prematurely freed, while the child interrupt-controller node was leaked. Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver") Cc: stable <stable@vger.kernel.org> # 3.18 Cc: Murali Karicheri <m-karicheri2@ti.com> --- drivers/pci/dwc/pci-keystone.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)