From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S638184AbdDZOvf (ORCPT ); Wed, 26 Apr 2017 10:51:35 -0400 Received: from foss.arm.com ([217.140.101.70]:57066 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S638169AbdDZOv0 (ORCPT ); Wed, 26 Apr 2017 10:51:26 -0400 Date: Wed, 26 Apr 2017 15:50:57 +0100 From: Mark Rutland To: Ganapatrao Kulkarni Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Will.Deacon@arm.com, catalin.marinas@arm.com, acme@kernel.org, alexander.shishkin@linux.intel.com, peterz@infradead.org, mingo@redhat.com, jnair@caviumnetworks.com, gpkulkarni@gmail.com Subject: Re: [PATCH] perf evsel: Fix to perf-stat malloc corruption on arm64 platforms Message-ID: <20170426145057.GK27156@leverpostej> References: <1493198780-25415-1-git-send-email-ganapatrao.kulkarni@cavium.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1493198780-25415-1-git-send-email-ganapatrao.kulkarni@cavium.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ganapatrao, Thanks for tracking this down. On Wed, Apr 26, 2017 at 02:56:20PM +0530, Ganapatrao Kulkarni wrote: > In some cases, ncpus used for perf_evsel__alloc_fd and for > perf_evsel__close are not the same, this is causing memory > overwrite/corruption. It would be good if we could enumerate when this occurs. >>From what I can tell, the problem occurs when opening a thread-bound event on PMU with a cpus/cpumask in sysfs. For perf-stat we create events using create_perf_stat_counter(). There we see !target_has_cpu(), so we call perf_evsel__open_per_thread(). Thus perf_evsel__open() is passed NULL cpus, and creates an empty cpu_map. As cpus->nr = 1, we get 1 * nthreads fds allocated, and open events for each of these. Later, we try to close events using perf_evlist__close(). This doesn't take target_has_cpu() into account, but sees evsel->cpus is non-NULL (since the PMU had a cpus/cpumask file), and tries to close events for cpus->nr * nthreads, and goes out-of-bounds of the fd array. > > Fixing issue by using same ncpus in perf_evsel__alloc_fd. > > This bug is more evident on arm64 platforms, which uses > cpu_map(cpus) for PMU core devices. > > Signed-off-by: Ganapatrao Kulkarni > --- > tools/perf/util/evsel.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index ac59710..0dc94d7 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -1489,7 +1489,9 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus, > nthreads = threads->nr; > > if (evsel->fd == NULL && > - perf_evsel__alloc_fd(evsel, cpus->nr, nthreads) < 0) > + perf_evsel__alloc_fd(evsel, > + evsel->cpus ? evsel->cpus->nr : cpus->nr, > + nthreads) < 0) Unfortunately, I don't think this is the right fix. Looking at the logic I added in commit: 9f21b815be863218 ("perf evlist: Only open events on CPUs an evsel permit"). ... in some cases (e.g. when using perf record with cpu-bound events), evsel->cpus may contain a subset of evlist->cpus, and thus the use of evsel->cpus->nr here may lower the number of entries allocated, such that the manipulation of fds will go out-of-bounds. I think that to properly solve this, we need a more invasive rework, ensuring that open/manipulation/close always deal with the same set of cpus and threads for a given evsel. I'm taking a look into that now. Thanks, Mark.