From: Mark Rutland <mark.rutland@arm.com>
To: linux-kernel@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>
Cc: Andi Kleen <ak@linux.intel.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Jiri Olsa <jolsa@kernel.org>,
Stephane Eranian <eranian@google.com>,
Thomas Gleixner <tglx@linutronix.de>,
Vince Weaver <vince@deater.net>, Ingo Molnar <mingo@kernel.org>
Subject: Re: pmu::read() called erroneously in v4.13-rc{3,4}
Date: Fri, 11 Aug 2017 12:18:07 +0100 [thread overview]
Message-ID: <20170811111807.GD12985@leverpostej> (raw)
In-Reply-To: <20170810173551.GD12812@leverpostej>
On Thu, Aug 10, 2017 at 06:35:51PM +0100, Mark Rutland wrote:
> Hi,
>
> While running Vince's perf fuzzer on arm64 v4.13-rc3, I found we call
> pmu::read() for an event whose event::cpu != smp_processor_id(), and
> event::oncpu == -1, violating the usual pmu::read() requirements.
It looks like I have an event that wasn't entirely detached from its
group_leader in perf_group_detach().
The below diff seems to get rid of the problem, though I think this is
masking some futher issues, noted below.
----
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 407dad6..cac84b6 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1724,6 +1724,7 @@ static void perf_group_detach(struct perf_event *event)
if (event->group_leader != event) {
list_del_init(&event->group_entry);
event->group_leader->nr_siblings--;
+ event->group_leader = event;
goto out;
}
----
If perf_group_detach() iterates over siblings, it re-inits each of their
group_leader entries, but doesn't do this if provided a sibling directly.
Is that deliberate?
It looks like without the above, we could get into
perf_output_read_group(), and follow a stale event->group_leader,
read()ing that without checking its state. We check the state of
siblings, so shouldn't we check the leader, too?
I'm also confused by perf_output_read_group() when event == leader.
AFAICT, in that case we won't read() the event at all, and we'll only
read() the siblings. Is that right?
Thanks,
Mark.
next prev parent reply other threads:[~2017-08-11 11:19 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-10 17:35 pmu::read() called erroneously in v4.13-rc{3,4} Mark Rutland
2017-08-10 19:03 ` Andi Kleen
2017-08-11 9:32 ` Mark Rutland
2017-08-11 11:18 ` Mark Rutland [this message]
2017-08-21 14:46 ` 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=20170811111807.GD12985@leverpostej \
--to=mark.rutland@arm.com \
--cc=acme@kernel.org \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=eranian@google.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=vince@deater.net \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).