[PATCH] PCI: keystone: fix interrupt-controller-node lookup

[PATCH] PCI: keystone: fix interrupt-controller-node lookup

[Johan Hovold]

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(-)

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

Re: [PATCH] PCI: keystone: fix interrupt-controller-node lookup

[Lorenzo Pieralisi]

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
> 

RE: [PATCH] PCI: keystone: fix interrupt-controller-node lookup

[Karicheri, Muralidharan]

-----Original Message-----
From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@arm.com]=20
Sent: Monday, November 13, 2017 5:43 AM
To: Johan Hovold
Cc: Karicheri, Muralidharan; Bjorn Helgaas; linux-pci@vger.kernel.org; linu=
x-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=20
> the whole device tree depth-first starting at the parent rather than=20
> just matching on its children.
>=20
> To make things worse, the parent pci node was prematurely freed, while=20
> 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(-)
>=20
> diff --git a/drivers/pci/dwc/pci-keystone.c=20
> b/drivers/pci/dwc/pci-keystone.c index 5bee3af47588..39405598b22d=20
> 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 key=
stone_pcie *ks_pcie,
>  	}
> =20
>  	/* interrupt controller is in a child node */
> -	*np_temp =3D of_find_node_by_name(np_pcie, controller);
> +	*np_temp =3D 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 key=
stone_pcie *ks_pcie,
>  	temp =3D of_irq_count(*np_temp);
>  	if (!temp) {
>  		dev_err(dev, "No IRQ entries in %s\n", controller);
> +		of_node_put(*np_temp);
>  		return -EINVAL;
>  	}
> =20
> @@ -204,6 +205,8 @@ static int ks_pcie_get_irq_controller_info(struct key=
stone_pcie *ks_pcie,
>  			break;
>  	}
> =20
> +	of_node_put(*np_temp);
> +
>  	if (temp) {
>  		*num_irqs =3D temp;
>  		return 0;
> --
> 2.15.0
>=20

Re: [PATCH] PCI: keystone: fix interrupt-controller-node lookup

[Johan Hovold]

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
> > 

Re: [PATCH] PCI: keystone: fix interrupt-controller-node lookup

[Lorenzo Pieralisi]

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
> > > 

Re: [PATCH] PCI: keystone: fix interrupt-controller-node lookup

[Lorenzo Pieralisi]

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
> 

Re: [PATCH] PCI: keystone: fix interrupt-controller-node lookup

[Johan Hovold]

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