* [PATCH] perf: Fix locking for children siblings group read
@ 2017-07-20 14:14 Jiri Olsa
2017-07-20 14:41 ` Peter Zijlstra
2017-07-21 9:37 ` [tip:perf/urgent] perf/core: " tip-bot for Jiri Olsa
0 siblings, 2 replies; 3+ messages in thread
From: Jiri Olsa @ 2017-07-20 14:14 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: lkml, Ingo Molnar, Andi Kleen, Alexander Shishkin
We're missing ctx lock when iterating children siblings
within the perf_read path for group reading. Following
race and crash can happen:
User space doing read syscall on event group leader:
T1:
perf_read
lock event->ctx->mutex
perf_read_group
lock leader->child_mutex
__perf_read_group_add(child)
list_for_each_entry(sub, &leader->sibling_list, group_entry)
----> sub might be invalid at this point, because it could
get removed via perf_event_exit_task_context in T2
Child exiting and cleaning up its events:
T2:
perf_event_exit_task_context
lock ctx->mutex
list_for_each_entry_safe(child_event, next, &child_ctx->event_list,...
perf_event_exit_event(child)
lock ctx->lock
perf_group_detach(child)
unlock ctx->lock
----> child is removed from sibling_list without any sync
with T1 path above
...
free_event(child)
Before the child is removed from the leader's child_list,
(and thus is omitted from perf_read_group processing), we
need to ensure that perf_read_group touches child's
siblings under its ctx->lock.
Tested-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
kernel/events/core.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9747e422ab20..585955b41489 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4365,7 +4365,9 @@ EXPORT_SYMBOL_GPL(perf_event_read_value);
static int __perf_read_group_add(struct perf_event *leader,
u64 read_format, u64 *values)
{
+ struct perf_event_context *ctx = leader->ctx;
struct perf_event *sub;
+ unsigned long flags;
int n = 1; /* skip @nr */
int ret;
@@ -4395,12 +4397,15 @@ static int __perf_read_group_add(struct perf_event *leader,
if (read_format & PERF_FORMAT_ID)
values[n++] = primary_event_id(leader);
+ raw_spin_lock_irqsave(&ctx->lock, flags);
+
list_for_each_entry(sub, &leader->sibling_list, group_entry) {
values[n++] += perf_event_count(sub);
if (read_format & PERF_FORMAT_ID)
values[n++] = primary_event_id(sub);
}
+ raw_spin_unlock_irqrestore(&ctx->lock, flags);
return 0;
}
--
2.9.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] perf: Fix locking for children siblings group read
2017-07-20 14:14 [PATCH] perf: Fix locking for children siblings group read Jiri Olsa
@ 2017-07-20 14:41 ` Peter Zijlstra
2017-07-21 9:37 ` [tip:perf/urgent] perf/core: " tip-bot for Jiri Olsa
1 sibling, 0 replies; 3+ messages in thread
From: Peter Zijlstra @ 2017-07-20 14:41 UTC (permalink / raw)
To: Jiri Olsa; +Cc: lkml, Ingo Molnar, Andi Kleen, Alexander Shishkin
On Thu, Jul 20, 2017 at 04:14:55PM +0200, Jiri Olsa wrote:
> We're missing ctx lock when iterating children siblings
> within the perf_read path for group reading. Following
> race and crash can happen:
>
> User space doing read syscall on event group leader:
>
> T1:
> perf_read
> lock event->ctx->mutex
> perf_read_group
> lock leader->child_mutex
> __perf_read_group_add(child)
> list_for_each_entry(sub, &leader->sibling_list, group_entry)
>
> ----> sub might be invalid at this point, because it could
> get removed via perf_event_exit_task_context in T2
>
> Child exiting and cleaning up its events:
>
> T2:
> perf_event_exit_task_context
> lock ctx->mutex
> list_for_each_entry_safe(child_event, next, &child_ctx->event_list,...
> perf_event_exit_event(child)
> lock ctx->lock
> perf_group_detach(child)
> unlock ctx->lock
>
> ----> child is removed from sibling_list without any sync
> with T1 path above
>
> ...
> free_event(child)
>
> Before the child is removed from the leader's child_list,
> (and thus is omitted from perf_read_group processing), we
> need to ensure that perf_read_group touches child's
> siblings under its ctx->lock.
One additional note; this bug got exposed by commit:
ba5213ae6b88 ("perf/core: Correct event creation with PERF_FORMAT_GROUP")
which made it possible to actually trigger this code-path.
So while it doesn't fix a bug in that commit per-se, we should maybe
have a Fixes: tag with that commit in because that commit exposed it and
this should be added to any kernel that commit goes into.
> Tested-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Thanks Jiri!
> ---
> kernel/events/core.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 9747e422ab20..585955b41489 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -4365,7 +4365,9 @@ EXPORT_SYMBOL_GPL(perf_event_read_value);
> static int __perf_read_group_add(struct perf_event *leader,
> u64 read_format, u64 *values)
> {
> + struct perf_event_context *ctx = leader->ctx;
> struct perf_event *sub;
> + unsigned long flags;
> int n = 1; /* skip @nr */
> int ret;
>
> @@ -4395,12 +4397,15 @@ static int __perf_read_group_add(struct perf_event *leader,
> if (read_format & PERF_FORMAT_ID)
> values[n++] = primary_event_id(leader);
>
> + raw_spin_lock_irqsave(&ctx->lock, flags);
> +
> list_for_each_entry(sub, &leader->sibling_list, group_entry) {
> values[n++] += perf_event_count(sub);
> if (read_format & PERF_FORMAT_ID)
> values[n++] = primary_event_id(sub);
> }
>
> + raw_spin_unlock_irqrestore(&ctx->lock, flags);
> return 0;
> }
>
> --
> 2.9.4
>
^ permalink raw reply [flat|nested] 3+ messages in thread* [tip:perf/urgent] perf/core: Fix locking for children siblings group read
2017-07-20 14:14 [PATCH] perf: Fix locking for children siblings group read Jiri Olsa
2017-07-20 14:41 ` Peter Zijlstra
@ 2017-07-21 9:37 ` tip-bot for Jiri Olsa
1 sibling, 0 replies; 3+ messages in thread
From: tip-bot for Jiri Olsa @ 2017-07-21 9:37 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, peterz, acme, jolsa, alexander.shishkin, hpa,
a.p.zijlstra, jolsa, torvalds, ak, tglx, mingo
Commit-ID: 2aeb1883547626d82c597cce2c99f0b9c62e2425
Gitweb: http://git.kernel.org/tip/2aeb1883547626d82c597cce2c99f0b9c62e2425
Author: Jiri Olsa <jolsa@kernel.org>
AuthorDate: Thu, 20 Jul 2017 16:14:55 +0200
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 21 Jul 2017 09:54:23 +0200
perf/core: Fix locking for children siblings group read
We're missing ctx lock when iterating children siblings
within the perf_read path for group reading. Following
race and crash can happen:
User space doing read syscall on event group leader:
T1:
perf_read
lock event->ctx->mutex
perf_read_group
lock leader->child_mutex
__perf_read_group_add(child)
list_for_each_entry(sub, &leader->sibling_list, group_entry)
----> sub might be invalid at this point, because it could
get removed via perf_event_exit_task_context in T2
Child exiting and cleaning up its events:
T2:
perf_event_exit_task_context
lock ctx->mutex
list_for_each_entry_safe(child_event, next, &child_ctx->event_list,...
perf_event_exit_event(child)
lock ctx->lock
perf_group_detach(child)
unlock ctx->lock
----> child is removed from sibling_list without any sync
with T1 path above
...
free_event(child)
Before the child is removed from the leader's child_list,
(and thus is omitted from perf_read_group processing), we
need to ensure that perf_read_group touches child's
siblings under its ctx->lock.
Peter further notes:
| One additional note; this bug got exposed by commit:
|
| ba5213ae6b88 ("perf/core: Correct event creation with PERF_FORMAT_GROUP")
|
| which made it possible to actually trigger this code-path.
Tested-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: ba5213ae6b88 ("perf/core: Correct event creation with PERF_FORMAT_GROUP")
Link: http://lkml.kernel.org/r/20170720141455.2106-1-jolsa@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/events/core.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index c9cdbd3..c17c088 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4372,7 +4372,9 @@ EXPORT_SYMBOL_GPL(perf_event_read_value);
static int __perf_read_group_add(struct perf_event *leader,
u64 read_format, u64 *values)
{
+ struct perf_event_context *ctx = leader->ctx;
struct perf_event *sub;
+ unsigned long flags;
int n = 1; /* skip @nr */
int ret;
@@ -4402,12 +4404,15 @@ static int __perf_read_group_add(struct perf_event *leader,
if (read_format & PERF_FORMAT_ID)
values[n++] = primary_event_id(leader);
+ raw_spin_lock_irqsave(&ctx->lock, flags);
+
list_for_each_entry(sub, &leader->sibling_list, group_entry) {
values[n++] += perf_event_count(sub);
if (read_format & PERF_FORMAT_ID)
values[n++] = primary_event_id(sub);
}
+ raw_spin_unlock_irqrestore(&ctx->lock, flags);
return 0;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-07-21 9:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-20 14:14 [PATCH] perf: Fix locking for children siblings group read Jiri Olsa
2017-07-20 14:41 ` Peter Zijlstra
2017-07-21 9:37 ` [tip:perf/urgent] perf/core: " tip-bot for Jiri Olsa
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.