* [PATCH 0/2] perf stat: fix memory corruption and core dump @ 2014-01-17 15:34 Stephane Eranian 2014-01-17 15:34 ` [PATCH 1/2] perf stat: fix NULL pointer reference bug with event unit Stephane Eranian 2014-01-17 15:34 ` [PATCH 2/2] perf stat: fix memory corruption of xyarray when cpumask is used Stephane Eranian 0 siblings, 2 replies; 6+ messages in thread From: Stephane Eranian @ 2014-01-17 15:34 UTC (permalink / raw) To: linux-kernel; +Cc: peterz, mingo, acme, jolsa, dsahern, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] perf stat: fix NULL pointer reference bug with event unit 2014-01-17 15:34 [PATCH 0/2] perf stat: fix memory corruption and core dump Stephane Eranian @ 2014-01-17 15:34 ` Stephane Eranian 2014-01-23 17:03 ` [tip:perf/urgent] " tip-bot for Stephane Eranian 2014-01-17 15:34 ` [PATCH 2/2] perf stat: fix memory corruption of xyarray when cpumask is used Stephane Eranian 1 sibling, 1 reply; 6+ messages in thread From: Stephane Eranian @ 2014-01-17 15:34 UTC (permalink / raw) To: linux-kernel; +Cc: peterz, mingo, acme, jolsa, dsahern, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:perf/urgent] perf stat: fix NULL pointer reference bug with event unit 2014-01-17 15:34 ` [PATCH 1/2] perf stat: fix NULL pointer reference bug with event unit Stephane Eranian @ 2014-01-23 17:03 ` tip-bot for Stephane Eranian 0 siblings, 0 replies; 6+ messages in thread From: tip-bot for Stephane Eranian @ 2014-01-23 17:03 UTC (permalink / raw) To: linux-tip-commits Cc: acme, linux-kernel, eranian, hpa, mingo, peterz, jolsa, dsahern, tglx, mingo 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); ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] perf stat: fix memory corruption of xyarray when cpumask is used 2014-01-17 15:34 [PATCH 0/2] perf stat: fix memory corruption and core dump Stephane Eranian 2014-01-17 15:34 ` [PATCH 1/2] perf stat: fix NULL pointer reference bug with event unit Stephane Eranian @ 2014-01-17 15:34 ` Stephane Eranian 2014-01-20 19:06 ` Arnaldo Carvalho de Melo 2014-01-23 17:04 ` [tip:perf/urgent] perf stat: Fix " tip-bot for Stephane Eranian 1 sibling, 2 replies; 6+ messages in thread From: Stephane Eranian @ 2014-01-17 15:34 UTC (permalink / raw) To: linux-kernel; +Cc: peterz, mingo, acme, jolsa, dsahern, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] perf stat: fix memory corruption of xyarray when cpumask is used 2014-01-17 15:34 ` [PATCH 2/2] perf stat: fix memory corruption of xyarray when cpumask is used Stephane Eranian @ 2014-01-20 19:06 ` Arnaldo Carvalho de Melo 2014-01-23 17:04 ` [tip:perf/urgent] perf stat: Fix " tip-bot for Stephane Eranian 1 sibling, 0 replies; 6+ messages in thread From: Arnaldo Carvalho de Melo @ 2014-01-20 19:06 UTC (permalink / raw) To: Stephane Eranian; +Cc: linux-kernel, peterz, mingo, jolsa, dsahern 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:perf/urgent] perf stat: Fix memory corruption of xyarray when cpumask is used 2014-01-17 15:34 ` [PATCH 2/2] perf stat: fix memory corruption of xyarray when cpumask is used Stephane Eranian 2014-01-20 19:06 ` Arnaldo Carvalho de Melo @ 2014-01-23 17:04 ` tip-bot for Stephane Eranian 1 sibling, 0 replies; 6+ messages in thread From: tip-bot for Stephane Eranian @ 2014-01-23 17:04 UTC (permalink / raw) To: linux-tip-commits Cc: acme, linux-kernel, eranian, hpa, mingo, peterz, jolsa, dsahern, tglx, mingo 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) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-01-23 17:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-01-17 15:34 [PATCH 0/2] perf stat: fix memory corruption and core dump Stephane Eranian 2014-01-17 15:34 ` [PATCH 1/2] perf stat: fix NULL pointer reference bug with event unit Stephane Eranian 2014-01-23 17:03 ` [tip:perf/urgent] " tip-bot for Stephane Eranian 2014-01-17 15:34 ` [PATCH 2/2] perf stat: fix memory corruption of xyarray when cpumask is used Stephane Eranian 2014-01-20 19:06 ` Arnaldo Carvalho de Melo 2014-01-23 17:04 ` [tip:perf/urgent] perf stat: Fix " tip-bot for Stephane Eranian
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome