[PATCH 0/3] drivers/perf: arm_pmu: Small bug fixes in the driver

[PATCH 0/3] drivers/perf: arm_pmu: Small bug fixes in the driver

[Julien Grall]

Hello all,

This patch series contains a series of small bug fixes for the arm_pmu
drivers.

I think all the patch should be backported to stable but I have not figured
out up to which version.

Julien Grall (3):
  drivers/perf: arm_pmu: Fix reference count of a device_node in
    of_pmu_irq_cfg
  drivers/perf: arm_pmu: Defer the setting of __oprofile_cpu_pmu
  drivers/perf: arm_pmu: Avoid leaking pmu->irq_affinity on error

 drivers/perf/arm_pmu.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH 1/3] drivers/perf: arm_pmu: Fix reference count of a device_node in of_pmu_irq_cfg

[Julien Grall]

The only function called by of_pmu_irq_cfg that will increment the
reference count on dn is of_parse_phandle.

Each time we successfully parse a possible CPU from an
interrupt-affinity property, we increment the refcount of that CPU node
once via of_parse_handle. After validating the CPU is possible, we
decrement the refcount once. Subsequently, we decrement the refcount
again, either as part of an early break if we don't have a matching SPI,
or as part of the end of the loop body.

This will lead to decrementing twice the refcounnt.
Remove the second pairs of call to of_node_put as nobody is using dn
between the first and second call to of_node_put.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
 drivers/perf/arm_pmu.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index f2d01d4..6401f0c 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -950,17 +950,14 @@ static int of_pmu_irq_cfg(struct arm_pmu *pmu)
 
 		/* For SPIs, we need to track the affinity per IRQ */
 		if (using_spi) {
-			if (i >= pdev->num_resources) {
-				of_node_put(dn);
+			if (i >= pdev->num_resources)
 				break;
-			}
 
 			irqs[i] = cpu;
 		}
 
 		/* Keep track of the CPUs containing this PMU type */
 		cpumask_set_cpu(cpu, &pmu->supported_cpus);
-		of_node_put(dn);
 		i++;
 	} while (1);
 
-- 
1.9.1

[PATCH 2/3] drivers/perf: arm_pmu: Defer the setting of __oprofile_cpu_pmu

[Julien Grall]

The global variable __oprofile_cpu_pmu is set before the PMU is fully
initialized. If an error occurs before the end of the initialization,
the PMU will be freed and the variable will contain an invalid pointer.

This will result in a kernel crash when perf will be used.

Fix it by moving the setting of __oprofile_cpu_pmu when the PMU is fully
initialized (i.e when it is no longer possible to fail).

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
 drivers/perf/arm_pmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index 6401f0c..95614d2 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -992,9 +992,6 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 
 	armpmu_init(pmu);
 
-	if (!__oprofile_cpu_pmu)
-		__oprofile_cpu_pmu = pmu;
-
 	pmu->plat_device = pdev;
 
 	if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
@@ -1030,6 +1027,9 @@ int arm_pmu_device_probe(struct platform_device *pdev,
 	if (ret)
 		goto out_destroy;
 
+	if (!__oprofile_cpu_pmu)
+		__oprofile_cpu_pmu = pmu;
+
 	pr_info("enabled with %s PMU driver, %d counters available\n",
 			pmu->name, pmu->num_events);
 
-- 
1.9.1

[PATCH 3/3] drivers/perf: arm_pmu: Avoid leaking pmu->irq_affinity on error

[Julien Grall]

pmu->irq_affinity will not be freed if an error occurred within
arm_pmu_device_probe after of_pmu_irq_cfg has been called.

Note that in the case of_pmu_irq_cfg is returning an error,
pmu->irq_affinity will not be set, but it should be NULL as pmu was
kzalloc'd. Therefore the result kfree(NULL) is benign.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
 drivers/perf/arm_pmu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index 95614d2..1b8304e 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -1040,6 +1040,7 @@ out_destroy:
 out_free:
 	pr_info("%s: failed to register PMU devices!\n",
 		of_node_full_name(node));
+	kfree(pmu->irq_affinity);
 	kfree(pmu);
 	return ret;
 }
-- 
1.9.1

Re: [PATCH 2/3] drivers/perf: arm_pmu: Defer the setting of __oprofile_cpu_pmu

[Will Deacon]

On Tue, May 31, 2016 at 12:41:22PM +0100, Julien Grall wrote:
> The global variable __oprofile_cpu_pmu is set before the PMU is fully
> initialized. If an error occurs before the end of the initialization,
> the PMU will be freed and the variable will contain an invalid pointer.
> 
> This will result in a kernel crash when perf will be used.
> 
> Fix it by moving the setting of __oprofile_cpu_pmu when the PMU is fully
> initialized (i.e when it is no longer possible to fail).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  drivers/perf/arm_pmu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Should this one go to -stable too?

Will

> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 6401f0c..95614d2 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -992,9 +992,6 @@ int arm_pmu_device_probe(struct platform_device *pdev,
>  
>  	armpmu_init(pmu);
>  
> -	if (!__oprofile_cpu_pmu)
> -		__oprofile_cpu_pmu = pmu;
> -
>  	pmu->plat_device = pdev;
>  
>  	if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
> @@ -1030,6 +1027,9 @@ int arm_pmu_device_probe(struct platform_device *pdev,
>  	if (ret)
>  		goto out_destroy;
>  
> +	if (!__oprofile_cpu_pmu)
> +		__oprofile_cpu_pmu = pmu;
> +
>  	pr_info("enabled with %s PMU driver, %d counters available\n",
>  			pmu->name, pmu->num_events);
>  
> -- 
> 1.9.1
> 

Re: [PATCH 2/3] drivers/perf: arm_pmu: Defer the setting of __oprofile_cpu_pmu

[Mark Rutland]

On Tue, May 31, 2016 at 05:28:34PM +0100, Will Deacon wrote:
> On Tue, May 31, 2016 at 12:41:22PM +0100, Julien Grall wrote:
> > The global variable __oprofile_cpu_pmu is set before the PMU is fully
> > initialized. If an error occurs before the end of the initialization,
> > the PMU will be freed and the variable will contain an invalid pointer.
> > 
> > This will result in a kernel crash when perf will be used.
> > 
> > Fix it by moving the setting of __oprofile_cpu_pmu when the PMU is fully
> > initialized (i.e when it is no longer possible to fail).
> > 
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > Acked-by: Mark Rutland <mark.rutland@arm.com>
> > ---
> >  drivers/perf/arm_pmu.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Should this one go to -stable too?

I think so.

The bug has been there at least since 76b8a0e4c8bda5f0 ("ARM: perf:
handle armpmu_register failing"), in v3.8...

Prior to that we wouldn't free the PMU, but it might not have been
initialised correctly.

Thanks,
Mark.
 
> Will
> 
> > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> > index 6401f0c..95614d2 100644
> > --- a/drivers/perf/arm_pmu.c
> > +++ b/drivers/perf/arm_pmu.c
> > @@ -992,9 +992,6 @@ int arm_pmu_device_probe(struct platform_device *pdev,
> >  
> >  	armpmu_init(pmu);
> >  
> > -	if (!__oprofile_cpu_pmu)
> > -		__oprofile_cpu_pmu = pmu;
> > -
> >  	pmu->plat_device = pdev;
> >  
> >  	if (node && (of_id = of_match_node(of_table, pdev->dev.of_node))) {
> > @@ -1030,6 +1027,9 @@ int arm_pmu_device_probe(struct platform_device *pdev,
> >  	if (ret)
> >  		goto out_destroy;
> >  
> > +	if (!__oprofile_cpu_pmu)
> > +		__oprofile_cpu_pmu = pmu;
> > +
> >  	pr_info("enabled with %s PMU driver, %d counters available\n",
> >  			pmu->name, pmu->num_events);
> >  
> > -- 
> > 1.9.1
> > 
>