From: Peter Zijlstra <peterz@infradead.org>
To: Jiri Olsa <jolsa@kernel.org>
Cc: lkml <linux-kernel@vger.kernel.org>,
Ingo Molnar <mingo@kernel.org>, Andi Kleen <andi@firstfloor.org>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>
Subject: Re: [PATCH] perf: Fix locking for children siblings group read
Date: Thu, 20 Jul 2017 16:41:56 +0200 [thread overview]
Message-ID: <20170720144156.a4zjaw6uncczkfql@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20170720141455.2106-1-jolsa@kernel.org>
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
>
next prev parent reply other threads:[~2017-07-20 14:42 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-20 14:14 [PATCH] perf: Fix locking for children siblings group read Jiri Olsa
2017-07-20 14:41 ` Peter Zijlstra [this message]
2017-07-21 9:37 ` [tip:perf/urgent] perf/core: " tip-bot for Jiri Olsa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170720144156.a4zjaw6uncczkfql@hirez.programming.kicks-ass.net \
--to=peterz@infradead.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=andi@firstfloor.org \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.