* [PATCH 0/3] drivers/perf: arm_pmu: Small bug fixes in the driver
@ 2016-05-31 11:41 ` Julien Grall
0 siblings, 0 replies; 12+ messages in thread
From: Julien Grall @ 2016-05-31 11:41 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 0/3] drivers/perf: arm_pmu: Small bug fixes in the driver @ 2016-05-31 11:41 ` Julien Grall 0 siblings, 0 replies; 12+ messages in thread From: Julien Grall @ 2016-05-31 11:41 UTC (permalink / raw) To: will.deacon, mark.rutland; +Cc: linux-kernel, linux-arm-kernel, 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] drivers/perf: arm_pmu: Fix reference count of a device_node in of_pmu_irq_cfg 2016-05-31 11:41 ` Julien Grall @ 2016-05-31 11:41 ` Julien Grall -1 siblings, 0 replies; 12+ messages in thread From: Julien Grall @ 2016-05-31 11:41 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 1/3] drivers/perf: arm_pmu: Fix reference count of a device_node in of_pmu_irq_cfg @ 2016-05-31 11:41 ` Julien Grall 0 siblings, 0 replies; 12+ messages in thread From: Julien Grall @ 2016-05-31 11:41 UTC (permalink / raw) To: will.deacon, mark.rutland; +Cc: linux-kernel, linux-arm-kernel, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] drivers/perf: arm_pmu: Defer the setting of __oprofile_cpu_pmu 2016-05-31 11:41 ` Julien Grall @ 2016-05-31 11:41 ` Julien Grall -1 siblings, 0 replies; 12+ messages in thread From: Julien Grall @ 2016-05-31 11:41 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] drivers/perf: arm_pmu: Defer the setting of __oprofile_cpu_pmu @ 2016-05-31 11:41 ` Julien Grall 0 siblings, 0 replies; 12+ messages in thread From: Julien Grall @ 2016-05-31 11:41 UTC (permalink / raw) To: will.deacon, mark.rutland; +Cc: linux-kernel, linux-arm-kernel, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] drivers/perf: arm_pmu: Defer the setting of __oprofile_cpu_pmu 2016-05-31 11:41 ` Julien Grall @ 2016-05-31 16:28 ` Will Deacon -1 siblings, 0 replies; 12+ messages in thread From: Will Deacon @ 2016-05-31 16:28 UTC (permalink / raw) To: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drivers/perf: arm_pmu: Defer the setting of __oprofile_cpu_pmu @ 2016-05-31 16:28 ` Will Deacon 0 siblings, 0 replies; 12+ messages in thread From: Will Deacon @ 2016-05-31 16:28 UTC (permalink / raw) To: Julien Grall; +Cc: mark.rutland, linux-kernel, linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] drivers/perf: arm_pmu: Defer the setting of __oprofile_cpu_pmu 2016-05-31 16:28 ` Will Deacon @ 2016-05-31 17:29 ` Mark Rutland -1 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2016-05-31 17:29 UTC (permalink / raw) To: linux-arm-kernel 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 > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drivers/perf: arm_pmu: Defer the setting of __oprofile_cpu_pmu @ 2016-05-31 17:29 ` Mark Rutland 0 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2016-05-31 17:29 UTC (permalink / raw) To: Will Deacon; +Cc: Julien Grall, linux-kernel, linux-arm-kernel 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 > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] drivers/perf: arm_pmu: Avoid leaking pmu->irq_affinity on error 2016-05-31 11:41 ` Julien Grall @ 2016-05-31 11:41 ` Julien Grall -1 siblings, 0 replies; 12+ messages in thread From: Julien Grall @ 2016-05-31 11:41 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] drivers/perf: arm_pmu: Avoid leaking pmu->irq_affinity on error @ 2016-05-31 11:41 ` Julien Grall 0 siblings, 0 replies; 12+ messages in thread From: Julien Grall @ 2016-05-31 11:41 UTC (permalink / raw) To: will.deacon, mark.rutland; +Cc: linux-kernel, linux-arm-kernel, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-05-31 17:30 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-05-31 11:41 [PATCH 0/3] drivers/perf: arm_pmu: Small bug fixes in the driver Julien Grall 2016-05-31 11:41 ` Julien Grall 2016-05-31 11:41 ` [PATCH 1/3] drivers/perf: arm_pmu: Fix reference count of a device_node in of_pmu_irq_cfg Julien Grall 2016-05-31 11:41 ` Julien Grall 2016-05-31 11:41 ` [PATCH 2/3] drivers/perf: arm_pmu: Defer the setting of __oprofile_cpu_pmu Julien Grall 2016-05-31 11:41 ` Julien Grall 2016-05-31 16:28 ` Will Deacon 2016-05-31 16:28 ` Will Deacon 2016-05-31 17:29 ` Mark Rutland 2016-05-31 17:29 ` Mark Rutland 2016-05-31 11:41 ` [PATCH 3/3] drivers/perf: arm_pmu: Avoid leaking pmu->irq_affinity on error Julien Grall 2016-05-31 11:41 ` Julien Grall
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.