[PATCH 0/2] perf stat: fix memory corruption and core dump

[PATCH 0/2] perf stat: fix memory corruption and core dump

[Stephane Eranian]

This short patch series fixes two problems:
 - segfault  for perf stat -e cpu/event=0x3c ls
 - memory corruption via xyarray of evsel->fd.

The patch is relative to tip.get


Stephane Eranian (2):
  perf stat: fix NULL pointer reference bug with event unit
  perf stat: fix memory corruption of xyarray when cpumask is used

 tools/perf/util/evlist.c       |    7 +++++--
 tools/perf/util/evsel.c        |    2 --
 tools/perf/util/parse-events.c |    2 +-
 tools/perf/util/pmu.c          |   24 ++++++++++++++++++++----
 tools/perf/util/pmu.h          |    2 +-
 5 files changed, 27 insertions(+), 10 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] perf stat: fix NULL pointer reference bug with event unit

[Stephane Eranian]

This patch fixes a problem with the handling of the newly
introduced optional event unit. The following cmdline
caused a segfault:
 $ perf stat -e cpu/event-0x3c/ ls

This patch fixes the problem with the default setting
for alias->unit which was eventually causing the segfault.

Signed-off-by: Stephane Eranian <eranian@google.com>
---
 tools/perf/util/parse-events.c |    2 +-
 tools/perf/util/pmu.c          |   24 ++++++++++++++++++++----
 tools/perf/util/pmu.h          |    2 +-
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index a7f1b6a..d248fca 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -635,7 +635,7 @@ int parse_events_add_pmu(struct list_head *list, int *idx,
 	struct perf_event_attr attr;
 	struct perf_pmu *pmu;
 	struct perf_evsel *evsel;
-	char *unit;
+	const char *unit;
 	double scale;
 
 	pmu = perf_pmu__find(name);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index d9cab4d..b752ecb 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -105,7 +105,7 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
 	char scale[128];
 	int fd, ret = -1;
 	char path[PATH_MAX];
-	char *lc;
+	const char *lc;
 
 	snprintf(path, PATH_MAX, "%s/%s.scale", dir, name);
 
@@ -609,7 +609,7 @@ static struct perf_pmu_alias *pmu_find_alias(struct perf_pmu *pmu,
 
 
 static int check_unit_scale(struct perf_pmu_alias *alias,
-			    char **unit, double *scale)
+			    const char **unit, double *scale)
 {
 	/*
 	 * Only one term in event definition can
@@ -634,14 +634,18 @@ static int check_unit_scale(struct perf_pmu_alias *alias,
  * defined for the alias
  */
 int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
-			  char **unit, double *scale)
+			  const char **unit, double *scale)
 {
 	struct parse_events_term *term, *h;
 	struct perf_pmu_alias *alias;
 	int ret;
 
+	/*
+	 * Mark unit and scale as not set
+	 * (different from default values, see below)
+	 */
 	*unit   = NULL;
-	*scale  = 0;
+	*scale  = 0.0;
 
 	list_for_each_entry_safe(term, h, head_terms, list) {
 		alias = pmu_find_alias(pmu, term);
@@ -658,6 +662,18 @@ int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
 		list_del(&term->list);
 		free(term);
 	}
+
+	/*
+	 * if no unit or scale foundin aliases, then
+	 * set defaults as for evsel
+	 * unit cannot left to NULL
+	 */
+	if (*unit == NULL)
+		*unit   = "";
+
+	if (*scale == 0.0)
+		*scale  = 1.0;
+
 	return 0;
 }
 
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 9183380..8b64125 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -29,7 +29,7 @@ int perf_pmu__config_terms(struct list_head *formats,
 			   struct perf_event_attr *attr,
 			   struct list_head *head_terms);
 int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
-			  char **unit, double *scale);
+			  const char **unit, double *scale);
 struct list_head *perf_pmu__alias(struct perf_pmu *pmu,
 				  struct list_head *head_terms);
 int perf_pmu_wrap(void);
-- 
1.7.9.5

[PATCH 2/2] perf stat: fix memory corruption of xyarray when cpumask is used

[Stephane Eranian]

This patch fixes a memory corruption problem with the xyarray when
the evsel fds get closed at the end of the run_perf_stat() call.

It could be triggered with:
 # perf stat -a -e power/energy-cores/ ls

When cpumask are used by events (.e.g, RAPL or uncores) then
the evsel fds are allocated based on the actual number of CPUs
monitored. That number can be smaller than the total number of
CPUs on the system. The problem arises at the end by perf stat
closes the fds twice. When fds are closed, their entry in the
xyarray are set to -1. The first close() on the evsel is made
from __run_perf_stat() and it uses the actual number of CPUS
for the event which is how the xyarray was allocated for. The
second is from perf_evlist_close() but that one is on the
total number of CPUs in the system, so it assume the xyarray
was allocated to cover it. However it was not, and some writes
corrupt memory.

The fix is in perf_evlist_close() is to first try with the
evsel->cpus if present, if not use the evlist->cpus. That
fixes the problem.

Signed-off-by: Stephane Eranian <eranian@google.com>
---
 tools/perf/util/evlist.c |    7 +++++--
 tools/perf/util/evsel.c  |    2 --
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 40bd2c0..59ef280 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1003,9 +1003,12 @@ void perf_evlist__close(struct perf_evlist *evlist)
 	struct perf_evsel *evsel;
 	int ncpus = cpu_map__nr(evlist->cpus);
 	int nthreads = thread_map__nr(evlist->threads);
+	int n;
 
-	evlist__for_each_reverse(evlist, evsel)
-		perf_evsel__close(evsel, ncpus, nthreads);
+	evlist__for_each_reverse(evlist, evsel) {
+		n = evsel->cpus ? evsel->cpus->nr : ncpus;
+		perf_evsel__close(evsel, n, nthreads);
+	}
 }
 
 int perf_evlist__open(struct perf_evlist *evlist)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 22e18a2..0a878db 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -667,7 +667,6 @@ int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
 {
 	int cpu, thread;
 	evsel->fd = xyarray__new(ncpus, nthreads, sizeof(int));
-
 	if (evsel->fd) {
 		for (cpu = 0; cpu < ncpus; cpu++) {
 			for (thread = 0; thread < nthreads; thread++) {
@@ -1081,7 +1080,6 @@ void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads)
 
 	perf_evsel__close_fd(evsel, ncpus, nthreads);
 	perf_evsel__free_fd(evsel);
-	evsel->fd = NULL;
 }
 
 static struct {
-- 
1.7.9.5

Re: [PATCH 2/2] perf stat: fix memory corruption of xyarray when cpumask is used

[Arnaldo Carvalho de Melo]

Em Fri, Jan 17, 2014 at 04:34:06PM +0100, Stephane Eranian escreveu:
> This patch fixes a memory corruption problem with the xyarray when
> the evsel fds get closed at the end of the run_perf_stat() call.
> 
> It could be triggered with:
>  # perf stat -a -e power/energy-cores/ ls
 
> When cpumask are used by events (.e.g, RAPL or uncores) then the evsel
> fds are allocated based on the actual number of CPUs monitored. That
> number can be smaller than the total number of CPUs on the system. The
> problem arises at the end by perf stat closes the fds twice. When fds
> are closed, their entry in the xyarray are set to -1. The first
> close() on the evsel is made from __run_perf_stat() and it uses the
> actual number of CPUS for the event which is how the xyarray was
> allocated for. The second is from perf_evlist_close() but that one is
> on the total number of CPUs in the system, so it assume the xyarray
> was allocated to cover it. However it was not, and some writes corrupt
> memory.
 
> The fix is in perf_evlist_close() is to first try with the
> evsel->cpus if present, if not use the evlist->cpus. That
> fixes the problem.

Humm, if there is an evsel->cpus, why does perf_evsel__close needs to
get that ncpus parameter?

My first reaction is that evsel->cpus needs to point to what is being
used for its evsel->fd member, that way we will never need to pass a
ncpus, because evsel->cpus->nr will be the size of ots evsel->fd
xyarray.

Now to figure out why is that we pass ncpus when we have evsel->cpus,
bet ->cpus came after ->fd, i.e. the assumption was that we don't ever
need to have a per evsel cpu map, as it would use the one in the evlist
that contains said evsel, but for some reason we grew evsel->cpus anyway
abd forgot to use its cpus->nr to ditch the ncpus evsel__close() parm.

I'll apply your patch, as it fixes the issue, but the above analysis
probably will led us to remove that.

But just a moment, why have you removed the evsel->fd zeroing at the end
of perf_evsel__close()? Since we called perf_evsel__free_fd(), ok, that
is because perf_evsel__free_fd() does that already, no need to zero it
again, ok, moving that to a separate, prep patch.

Thanks for looking into this, I'll get this to Ingo soon.

- Arnaldo
 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> ---
>  tools/perf/util/evlist.c |    7 +++++--
>  tools/perf/util/evsel.c  |    2 --
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 40bd2c0..59ef280 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1003,9 +1003,12 @@ void perf_evlist__close(struct perf_evlist *evlist)
>  	struct perf_evsel *evsel;
>  	int ncpus = cpu_map__nr(evlist->cpus);
>  	int nthreads = thread_map__nr(evlist->threads);
> +	int n;
>  
> -	evlist__for_each_reverse(evlist, evsel)
> -		perf_evsel__close(evsel, ncpus, nthreads);
> +	evlist__for_each_reverse(evlist, evsel) {
> +		n = evsel->cpus ? evsel->cpus->nr : ncpus;
> +		perf_evsel__close(evsel, n, nthreads);
> +	}
>  }
>  
>  int perf_evlist__open(struct perf_evlist *evlist)
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 22e18a2..0a878db 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -667,7 +667,6 @@ int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)
>  {
>  	int cpu, thread;
>  	evsel->fd = xyarray__new(ncpus, nthreads, sizeof(int));
> -
>  	if (evsel->fd) {
>  		for (cpu = 0; cpu < ncpus; cpu++) {
>  			for (thread = 0; thread < nthreads; thread++) {
> @@ -1081,7 +1080,6 @@ void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads)
>  
>  	perf_evsel__close_fd(evsel, ncpus, nthreads);
>  	perf_evsel__free_fd(evsel);
> -	evsel->fd = NULL;
>  }
>  
>  static struct {
> -- 
> 1.7.9.5

[tip:perf/urgent] perf stat: fix NULL pointer reference bug with event unit

[tip-bot for Stephane Eranian]

Commit-ID:  8a398897ff21f73cb8b15a19514660f032926882
Gitweb:     http://git.kernel.org/tip/8a398897ff21f73cb8b15a19514660f032926882
Author:     Stephane Eranian <eranian@google.com>
AuthorDate: Fri, 17 Jan 2014 16:34:05 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 20 Jan 2014 16:19:08 -0300

perf stat: fix NULL pointer reference bug with event unit

This patch fixes a problem with the handling of the newly introduced
optional event unit. The following cmdline caused a segfault:

 $ perf stat -e cpu/event-0x3c/ ls

This patch fixes the problem with the default setting for alias->unit
which was eventually causing the segfault.

Signed-off-by: Stephane Eranian <eranian@google.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1389972846-6566-2-git-send-email-eranian@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/parse-events.c |  2 +-
 tools/perf/util/pmu.c          | 24 ++++++++++++++++++++----
 tools/perf/util/pmu.h          |  2 +-
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index a7f1b6a..d248fca 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -635,7 +635,7 @@ int parse_events_add_pmu(struct list_head *list, int *idx,
 	struct perf_event_attr attr;
 	struct perf_pmu *pmu;
 	struct perf_evsel *evsel;
-	char *unit;
+	const char *unit;
 	double scale;
 
 	pmu = perf_pmu__find(name);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index d9cab4d..b752ecb 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -105,7 +105,7 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
 	char scale[128];
 	int fd, ret = -1;
 	char path[PATH_MAX];
-	char *lc;
+	const char *lc;
 
 	snprintf(path, PATH_MAX, "%s/%s.scale", dir, name);
 
@@ -609,7 +609,7 @@ static struct perf_pmu_alias *pmu_find_alias(struct perf_pmu *pmu,
 
 
 static int check_unit_scale(struct perf_pmu_alias *alias,
-			    char **unit, double *scale)
+			    const char **unit, double *scale)
 {
 	/*
 	 * Only one term in event definition can
@@ -634,14 +634,18 @@ static int check_unit_scale(struct perf_pmu_alias *alias,
  * defined for the alias
  */
 int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
-			  char **unit, double *scale)
+			  const char **unit, double *scale)
 {
 	struct parse_events_term *term, *h;
 	struct perf_pmu_alias *alias;
 	int ret;
 
+	/*
+	 * Mark unit and scale as not set
+	 * (different from default values, see below)
+	 */
 	*unit   = NULL;
-	*scale  = 0;
+	*scale  = 0.0;
 
 	list_for_each_entry_safe(term, h, head_terms, list) {
 		alias = pmu_find_alias(pmu, term);
@@ -658,6 +662,18 @@ int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
 		list_del(&term->list);
 		free(term);
 	}
+
+	/*
+	 * if no unit or scale foundin aliases, then
+	 * set defaults as for evsel
+	 * unit cannot left to NULL
+	 */
+	if (*unit == NULL)
+		*unit   = "";
+
+	if (*scale == 0.0)
+		*scale  = 1.0;
+
 	return 0;
 }
 
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 9183380..8b64125 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -29,7 +29,7 @@ int perf_pmu__config_terms(struct list_head *formats,
 			   struct perf_event_attr *attr,
 			   struct list_head *head_terms);
 int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
-			  char **unit, double *scale);
+			  const char **unit, double *scale);
 struct list_head *perf_pmu__alias(struct perf_pmu *pmu,
 				  struct list_head *head_terms);
 int perf_pmu_wrap(void);

[tip:perf/urgent] perf stat: Fix memory corruption of xyarray when cpumask is used

[tip-bot for Stephane Eranian]

Commit-ID:  8ad9219e08af12a5652892e273336dbd31b25b03
Gitweb:     http://git.kernel.org/tip/8ad9219e08af12a5652892e273336dbd31b25b03
Author:     Stephane Eranian <eranian@google.com>
AuthorDate: Fri, 17 Jan 2014 16:34:06 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 20 Jan 2014 16:19:09 -0300

perf stat: Fix memory corruption of xyarray when cpumask is used

This patch fixes a memory corruption problem with the xyarray when the
evsel fds get closed at the end of the run_perf_stat() call.

It could be triggered with:

 # perf stat -a -e power/energy-cores/ ls

When cpumask are used by events (.e.g, RAPL or uncores) then the evsel
fds are allocated based on the actual number of CPUs monitored. That
number can be smaller than the total number of CPUs on the system.

The problem arises at the end by perf stat closes the fds twice. When
fds are closed, their entry in the xyarray are set to -1.

The first close() on the evsel is made from __run_perf_stat() and it
uses the actual number of CPUS for the event which is how the xyarray
was allocated for.

The second is from perf_evlist_close() but that one is on the total
number of CPUs in the system, so it assume the xyarray was allocated to
cover it. However it was not, and some writes corrupt memory.

The fix is in perf_evlist_close() is to first try with the evsel->cpus
if present, if not use the evlist->cpus. That fixes the problem.

Signed-off-by: Stephane Eranian <eranian@google.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1389972846-6566-3-git-send-email-eranian@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 40bd2c0..59ef280 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1003,9 +1003,12 @@ void perf_evlist__close(struct perf_evlist *evlist)
 	struct perf_evsel *evsel;
 	int ncpus = cpu_map__nr(evlist->cpus);
 	int nthreads = thread_map__nr(evlist->threads);
+	int n;
 
-	evlist__for_each_reverse(evlist, evsel)
-		perf_evsel__close(evsel, ncpus, nthreads);
+	evlist__for_each_reverse(evlist, evsel) {
+		n = evsel->cpus ? evsel->cpus->nr : ncpus;
+		perf_evsel__close(evsel, n, nthreads);
+	}
 }
 
 int perf_evlist__open(struct perf_evlist *evlist)