From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752712AbdHKLTS (ORCPT <rfc822;w@1wt.eu>);
        Fri, 11 Aug 2017 07:19:18 -0400
Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:43196 "EHLO
        foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751955AbdHKLTR (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 11 Aug 2017 07:19:17 -0400
Date: Fri, 11 Aug 2017 12:18:07 +0100
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}
Message-ID: <20170811111807.GD12985@leverpostej>
References: <20170810173551.GD12812@leverpostej>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170810173551.GD12812@leverpostej>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.