* [PATCH] perf: drop module reference on event init failure
@ 2015-01-07 14:56 Mark Rutland
2015-01-08 11:32 ` Will Deacon
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Mark Rutland @ 2015-01-07 14:56 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Rutland, Will Deacon, Peter Zijlstra, Paul Mackerras,
Ingo Molnar, Arnaldo Carvalho de Melo
When initialising an event, perf_init_event will call try_module_get to
ensure that the PMU's module cannot be removed for the lifetime of the
event, with __free_event dropping the reference when the event is
finally destroyed. If something fails after the event has been
initialised, but before the event is installed, perf_event_alloc will
drop the reference on the module.
However, if we fail to initialise an event for some reason (e.g. we ask
an uncore PMU to perform sampling, and it refuses to initialise the
event), we do not drop the refcount. If we try to open such a bogus
event without a precise IDR type, we will loop over each PMU in the pmus
list, incrementing each of their refcounts without decrementing them.
This patch adds a module_put when pmu->event_init(event) fails, ensuring
that the refcounts are balanced in failure cases. As the innards of the
precise and search based initialisation look very similar, this logic is
hoisted out into a new helper function. While the early return for the
failed try_module_get is removed from the search case, this is handled
by the remaining return when ret is not -ENOENT.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
---
kernel/events/core.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 4c1ee7f..4faccf3 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6857,6 +6857,20 @@ void perf_pmu_unregister(struct pmu *pmu)
}
EXPORT_SYMBOL_GPL(perf_pmu_unregister);
+static int perf_try_init_event(struct pmu *pmu, struct perf_event *event)
+{
+ int ret;
+
+ if (!try_module_get(pmu->module))
+ return -ENODEV;
+ event->pmu = pmu;
+ ret = pmu->event_init(event);
+ if (ret)
+ module_put(pmu->module);
+
+ return ret;
+}
+
struct pmu *perf_init_event(struct perf_event *event)
{
struct pmu *pmu = NULL;
@@ -6869,24 +6883,14 @@ struct pmu *perf_init_event(struct perf_event *event)
pmu = idr_find(&pmu_idr, event->attr.type);
rcu_read_unlock();
if (pmu) {
- if (!try_module_get(pmu->module)) {
- pmu = ERR_PTR(-ENODEV);
- goto unlock;
- }
- event->pmu = pmu;
- ret = pmu->event_init(event);
+ ret = perf_try_init_event(pmu, event);
if (ret)
pmu = ERR_PTR(ret);
goto unlock;
}
list_for_each_entry_rcu(pmu, &pmus, entry) {
- if (!try_module_get(pmu->module)) {
- pmu = ERR_PTR(-ENODEV);
- goto unlock;
- }
- event->pmu = pmu;
- ret = pmu->event_init(event);
+ ret = perf_try_init_event(pmu, event);
if (!ret)
goto unlock;
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] perf: drop module reference on event init failure
2015-01-07 14:56 [PATCH] perf: drop module reference on event init failure Mark Rutland
@ 2015-01-08 11:32 ` Will Deacon
2015-01-08 12:09 ` Mark Rutland
2015-01-16 12:26 ` Mark Rutland
2015-02-04 14:40 ` [tip:perf/core] perf: Drop " tip-bot for Mark Rutland
2 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2015-01-08 11:32 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-kernel, Peter Zijlstra, Paul Mackerras, Ingo Molnar,
Arnaldo Carvalho de Melo
On Wed, Jan 07, 2015 at 02:56:51PM +0000, Mark Rutland wrote:
> When initialising an event, perf_init_event will call try_module_get to
> ensure that the PMU's module cannot be removed for the lifetime of the
> event, with __free_event dropping the reference when the event is
> finally destroyed. If something fails after the event has been
> initialised, but before the event is installed, perf_event_alloc will
> drop the reference on the module.
>
> However, if we fail to initialise an event for some reason (e.g. we ask
> an uncore PMU to perform sampling, and it refuses to initialise the
> event), we do not drop the refcount. If we try to open such a bogus
> event without a precise IDR type, we will loop over each PMU in the pmus
> list, incrementing each of their refcounts without decrementing them.
>
> This patch adds a module_put when pmu->event_init(event) fails, ensuring
> that the refcounts are balanced in failure cases. As the innards of the
> precise and search based initialisation look very similar, this logic is
> hoisted out into a new helper function. While the early return for the
> failed try_module_get is removed from the search case, this is handled
> by the remaining return when ret is not -ENOENT.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> ---
> kernel/events/core.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
Looks like a straightforward fix to me -- does it need to go to stable too?
Acked-by: Will Deacon <will.deacon@arm.com>
Will
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4c1ee7f..4faccf3 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6857,6 +6857,20 @@ void perf_pmu_unregister(struct pmu *pmu)
> }
> EXPORT_SYMBOL_GPL(perf_pmu_unregister);
>
> +static int perf_try_init_event(struct pmu *pmu, struct perf_event *event)
> +{
> + int ret;
> +
> + if (!try_module_get(pmu->module))
> + return -ENODEV;
> + event->pmu = pmu;
> + ret = pmu->event_init(event);
> + if (ret)
> + module_put(pmu->module);
> +
> + return ret;
> +}
> +
> struct pmu *perf_init_event(struct perf_event *event)
> {
> struct pmu *pmu = NULL;
> @@ -6869,24 +6883,14 @@ struct pmu *perf_init_event(struct perf_event *event)
> pmu = idr_find(&pmu_idr, event->attr.type);
> rcu_read_unlock();
> if (pmu) {
> - if (!try_module_get(pmu->module)) {
> - pmu = ERR_PTR(-ENODEV);
> - goto unlock;
> - }
> - event->pmu = pmu;
> - ret = pmu->event_init(event);
> + ret = perf_try_init_event(pmu, event);
> if (ret)
> pmu = ERR_PTR(ret);
> goto unlock;
> }
>
> list_for_each_entry_rcu(pmu, &pmus, entry) {
> - if (!try_module_get(pmu->module)) {
> - pmu = ERR_PTR(-ENODEV);
> - goto unlock;
> - }
> - event->pmu = pmu;
> - ret = pmu->event_init(event);
> + ret = perf_try_init_event(pmu, event);
> if (!ret)
> goto unlock;
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf: drop module reference on event init failure
2015-01-08 11:32 ` Will Deacon
@ 2015-01-08 12:09 ` Mark Rutland
0 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2015-01-08 12:09 UTC (permalink / raw)
To: Will Deacon
Cc: linux-kernel, Peter Zijlstra, Paul Mackerras, Ingo Molnar,
Arnaldo Carvalho de Melo
On Thu, Jan 08, 2015 at 11:32:13AM +0000, Will Deacon wrote:
> On Wed, Jan 07, 2015 at 02:56:51PM +0000, Mark Rutland wrote:
> > When initialising an event, perf_init_event will call try_module_get to
> > ensure that the PMU's module cannot be removed for the lifetime of the
> > event, with __free_event dropping the reference when the event is
> > finally destroyed. If something fails after the event has been
> > initialised, but before the event is installed, perf_event_alloc will
> > drop the reference on the module.
> >
> > However, if we fail to initialise an event for some reason (e.g. we ask
> > an uncore PMU to perform sampling, and it refuses to initialise the
> > event), we do not drop the refcount. If we try to open such a bogus
> > event without a precise IDR type, we will loop over each PMU in the pmus
> > list, incrementing each of their refcounts without decrementing them.
> >
> > This patch adds a module_put when pmu->event_init(event) fails, ensuring
> > that the refcounts are balanced in failure cases. As the innards of the
> > precise and search based initialisation look very similar, this logic is
> > hoisted out into a new helper function. While the early return for the
> > failed try_module_get is removed from the search case, this is handled
> > by the remaining return when ret is not -ENOENT.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > ---
> > kernel/events/core.c | 28 ++++++++++++++++------------
> > 1 file changed, 16 insertions(+), 12 deletions(-)
>
> Looks like a straightforward fix to me -- does it need to go to stable too?
Assuming people are using PMU drivers built as modules on stable
kernels, then yes. Otherwise it's not strictly necessary as far as I can
see.
The only such driver I'm immediately aware of is for the ARM CCN, but
it's entirely possible I missed another.
I guess this fixes c464c76eec4be587 (perf: Allow building PMU drivers as
modules).
> Acked-by: Will Deacon <will.deacon@arm.com>
Cheers.
Mark.
>
> Will
>
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 4c1ee7f..4faccf3 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -6857,6 +6857,20 @@ void perf_pmu_unregister(struct pmu *pmu)
> > }
> > EXPORT_SYMBOL_GPL(perf_pmu_unregister);
> >
> > +static int perf_try_init_event(struct pmu *pmu, struct perf_event *event)
> > +{
> > + int ret;
> > +
> > + if (!try_module_get(pmu->module))
> > + return -ENODEV;
> > + event->pmu = pmu;
> > + ret = pmu->event_init(event);
> > + if (ret)
> > + module_put(pmu->module);
> > +
> > + return ret;
> > +}
> > +
> > struct pmu *perf_init_event(struct perf_event *event)
> > {
> > struct pmu *pmu = NULL;
> > @@ -6869,24 +6883,14 @@ struct pmu *perf_init_event(struct perf_event *event)
> > pmu = idr_find(&pmu_idr, event->attr.type);
> > rcu_read_unlock();
> > if (pmu) {
> > - if (!try_module_get(pmu->module)) {
> > - pmu = ERR_PTR(-ENODEV);
> > - goto unlock;
> > - }
> > - event->pmu = pmu;
> > - ret = pmu->event_init(event);
> > + ret = perf_try_init_event(pmu, event);
> > if (ret)
> > pmu = ERR_PTR(ret);
> > goto unlock;
> > }
> >
> > list_for_each_entry_rcu(pmu, &pmus, entry) {
> > - if (!try_module_get(pmu->module)) {
> > - pmu = ERR_PTR(-ENODEV);
> > - goto unlock;
> > - }
> > - event->pmu = pmu;
> > - ret = pmu->event_init(event);
> > + ret = perf_try_init_event(pmu, event);
> > if (!ret)
> > goto unlock;
> >
> > --
> > 1.9.1
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf: drop module reference on event init failure
2015-01-07 14:56 [PATCH] perf: drop module reference on event init failure Mark Rutland
2015-01-08 11:32 ` Will Deacon
@ 2015-01-16 12:26 ` Mark Rutland
2015-01-16 12:49 ` Peter Zijlstra
2015-02-04 14:40 ` [tip:perf/core] perf: Drop " tip-bot for Mark Rutland
2 siblings, 1 reply; 6+ messages in thread
From: Mark Rutland @ 2015-01-16 12:26 UTC (permalink / raw)
To: linux-kernel
Cc: Will Deacon, Peter Zijlstra, Paul Mackerras, Ingo Molnar,
Arnaldo Carvalho de Melo
On Wed, Jan 07, 2015 at 02:56:51PM +0000, Mark Rutland wrote:
> When initialising an event, perf_init_event will call try_module_get to
> ensure that the PMU's module cannot be removed for the lifetime of the
> event, with __free_event dropping the reference when the event is
> finally destroyed. If something fails after the event has been
> initialised, but before the event is installed, perf_event_alloc will
> drop the reference on the module.
>
> However, if we fail to initialise an event for some reason (e.g. we ask
> an uncore PMU to perform sampling, and it refuses to initialise the
> event), we do not drop the refcount. If we try to open such a bogus
> event without a precise IDR type, we will loop over each PMU in the pmus
> list, incrementing each of their refcounts without decrementing them.
>
> This patch adds a module_put when pmu->event_init(event) fails, ensuring
> that the refcounts are balanced in failure cases. As the innards of the
> precise and search based initialisation look very similar, this logic is
> hoisted out into a new helper function. While the early return for the
> failed try_module_get is removed from the search case, this is handled
> by the remaining return when ret is not -ENOENT.
I hate to ping, but is anyone going to look at this?
I'm happy to rework if required.
Thanks,
Mark.
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> ---
> kernel/events/core.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4c1ee7f..4faccf3 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6857,6 +6857,20 @@ void perf_pmu_unregister(struct pmu *pmu)
> }
> EXPORT_SYMBOL_GPL(perf_pmu_unregister);
>
> +static int perf_try_init_event(struct pmu *pmu, struct perf_event *event)
> +{
> + int ret;
> +
> + if (!try_module_get(pmu->module))
> + return -ENODEV;
> + event->pmu = pmu;
> + ret = pmu->event_init(event);
> + if (ret)
> + module_put(pmu->module);
> +
> + return ret;
> +}
> +
> struct pmu *perf_init_event(struct perf_event *event)
> {
> struct pmu *pmu = NULL;
> @@ -6869,24 +6883,14 @@ struct pmu *perf_init_event(struct perf_event *event)
> pmu = idr_find(&pmu_idr, event->attr.type);
> rcu_read_unlock();
> if (pmu) {
> - if (!try_module_get(pmu->module)) {
> - pmu = ERR_PTR(-ENODEV);
> - goto unlock;
> - }
> - event->pmu = pmu;
> - ret = pmu->event_init(event);
> + ret = perf_try_init_event(pmu, event);
> if (ret)
> pmu = ERR_PTR(ret);
> goto unlock;
> }
>
> list_for_each_entry_rcu(pmu, &pmus, entry) {
> - if (!try_module_get(pmu->module)) {
> - pmu = ERR_PTR(-ENODEV);
> - goto unlock;
> - }
> - event->pmu = pmu;
> - ret = pmu->event_init(event);
> + ret = perf_try_init_event(pmu, event);
> if (!ret)
> goto unlock;
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf: drop module reference on event init failure
2015-01-16 12:26 ` Mark Rutland
@ 2015-01-16 12:49 ` Peter Zijlstra
0 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2015-01-16 12:49 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-kernel, Will Deacon, Paul Mackerras, Ingo Molnar,
Arnaldo Carvalho de Melo
On Fri, Jan 16, 2015 at 12:26:03PM +0000, Mark Rutland wrote:
> On Wed, Jan 07, 2015 at 02:56:51PM +0000, Mark Rutland wrote:
> > When initialising an event, perf_init_event will call try_module_get to
> > ensure that the PMU's module cannot be removed for the lifetime of the
> > event, with __free_event dropping the reference when the event is
> > finally destroyed. If something fails after the event has been
> > initialised, but before the event is installed, perf_event_alloc will
> > drop the reference on the module.
> >
> > However, if we fail to initialise an event for some reason (e.g. we ask
> > an uncore PMU to perform sampling, and it refuses to initialise the
> > event), we do not drop the refcount. If we try to open such a bogus
> > event without a precise IDR type, we will loop over each PMU in the pmus
> > list, incrementing each of their refcounts without decrementing them.
> >
> > This patch adds a module_put when pmu->event_init(event) fails, ensuring
> > that the refcounts are balanced in failure cases. As the innards of the
> > precise and search based initialisation look very similar, this logic is
> > hoisted out into a new helper function. While the early return for the
> > failed try_module_get is removed from the search case, this is handled
> > by the remaining return when ret is not -ENOENT.
>
> I hate to ping, but is anyone going to look at this?
>
Thanks for reminding me; the inbox is still a horrid mess.
Looks good, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:perf/core] perf: Drop module reference on event init failure
2015-01-07 14:56 [PATCH] perf: drop module reference on event init failure Mark Rutland
2015-01-08 11:32 ` Will Deacon
2015-01-16 12:26 ` Mark Rutland
@ 2015-02-04 14:40 ` tip-bot for Mark Rutland
2 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Mark Rutland @ 2015-02-04 14:40 UTC (permalink / raw)
To: linux-tip-commits
Cc: tglx, hpa, will.deacon, peterz, mingo, acme, torvalds,
linux-kernel, mark.rutland
Commit-ID: cc34b98bacb0e102fb720d95a25fed5c6090a70d
Gitweb: http://git.kernel.org/tip/cc34b98bacb0e102fb720d95a25fed5c6090a70d
Author: Mark Rutland <mark.rutland@arm.com>
AuthorDate: Wed, 7 Jan 2015 14:56:51 +0000
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 4 Feb 2015 08:07:14 +0100
perf: Drop module reference on event init failure
When initialising an event, perf_init_event will call try_module_get() to
ensure that the PMU's module cannot be removed for the lifetime of the
event, with __free_event() dropping the reference when the event is
finally destroyed. If something fails after the event has been
initialised, but before the event is installed, perf_event_alloc will
drop the reference on the module.
However, if we fail to initialise an event for some reason (e.g. we ask
an uncore PMU to perform sampling, and it refuses to initialise the
event), we do not drop the refcount. If we try to open such a bogus
event without a precise IDR type, we will loop over each PMU in the pmus
list, incrementing each of their refcounts without decrementing them.
This patch adds a module_put when pmu->event_init(event) fails, ensuring
that the refcounts are balanced in failure cases. As the innards of the
precise and search based initialisation look very similar, this logic is
hoisted out into a new helper function. While the early return for the
failed try_module_get is removed from the search case, this is handled
by the remaining return when ret is not -ENOENT.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1420642611-22667-1-git-send-email-mark.rutland@arm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index f773fa1..37cc20e 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7027,6 +7027,20 @@ void perf_pmu_unregister(struct pmu *pmu)
}
EXPORT_SYMBOL_GPL(perf_pmu_unregister);
+static int perf_try_init_event(struct pmu *pmu, struct perf_event *event)
+{
+ int ret;
+
+ if (!try_module_get(pmu->module))
+ return -ENODEV;
+ event->pmu = pmu;
+ ret = pmu->event_init(event);
+ if (ret)
+ module_put(pmu->module);
+
+ return ret;
+}
+
struct pmu *perf_init_event(struct perf_event *event)
{
struct pmu *pmu = NULL;
@@ -7039,24 +7053,14 @@ struct pmu *perf_init_event(struct perf_event *event)
pmu = idr_find(&pmu_idr, event->attr.type);
rcu_read_unlock();
if (pmu) {
- if (!try_module_get(pmu->module)) {
- pmu = ERR_PTR(-ENODEV);
- goto unlock;
- }
- event->pmu = pmu;
- ret = pmu->event_init(event);
+ ret = perf_try_init_event(pmu, event);
if (ret)
pmu = ERR_PTR(ret);
goto unlock;
}
list_for_each_entry_rcu(pmu, &pmus, entry) {
- if (!try_module_get(pmu->module)) {
- pmu = ERR_PTR(-ENODEV);
- goto unlock;
- }
- event->pmu = pmu;
- ret = pmu->event_init(event);
+ ret = perf_try_init_event(pmu, event);
if (!ret)
goto unlock;
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-02-04 14:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-07 14:56 [PATCH] perf: drop module reference on event init failure Mark Rutland
2015-01-08 11:32 ` Will Deacon
2015-01-08 12:09 ` Mark Rutland
2015-01-16 12:26 ` Mark Rutland
2015-01-16 12:49 ` Peter Zijlstra
2015-02-04 14:40 ` [tip:perf/core] perf: Drop " tip-bot for Mark Rutland
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.