LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Fix perf stat repeat segfault
@ 2019-07-10 20:45 Numfor Mbiziwo-Tiapo
  2019-07-11  4:44 ` Ravi Bangoria
  2019-07-14 20:44 ` Jiri Olsa
  0 siblings, 2 replies; 11+ messages in thread
From: Numfor Mbiziwo-Tiapo @ 2019-07-10 20:45 UTC (permalink / raw)
  To: peterz, mingo, acme, alexander.shishkin, jolsa, namhyung,
	songliubraving, mbd
  Cc: linux-kernel, irogers, eranian, Numfor Mbiziwo-Tiapo

When perf stat is called with event groups and the repeat option,
a segfault occurs because the cpu ids are stored on each iteration
of the repeat, when they should only be stored on the first iteration,
which causes a buffer overflow.

This can be replicated by running (from the tip directory):

make -C tools/perf

then running:

tools/perf/perf stat -e '{cycles,instructions}' -r 10 ls

Since run_idx keeps track of the current iteration of the repeat,
only storing the cpu ids on the first iteration (when run_idx < 1)
fixes this issue.

Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
---
 tools/perf/builtin-stat.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 63a3afc7f32b..92d6694367e4 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -378,9 +378,10 @@ static void workload_exec_failed_signal(int signo __maybe_unused, siginfo_t *inf
 	workload_exec_errno = info->si_value.sival_int;
 }
 
-static bool perf_evsel__should_store_id(struct perf_evsel *counter)
+static bool perf_evsel__should_store_id(struct perf_evsel *counter, int run_idx)
 {
-	return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID;
+	return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID
+		&& run_idx < 1;
 }
 
 static bool is_target_alive(struct target *_target,
@@ -503,7 +504,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		if (l > stat_config.unit_width)
 			stat_config.unit_width = l;
 
-		if (perf_evsel__should_store_id(counter) &&
+		if (perf_evsel__should_store_id(counter, run_idx) &&
 		    perf_evsel__store_ids(counter, evsel_list))
 			return -1;
 	}
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix perf stat repeat segfault
  2019-07-10 20:45 [PATCH] Fix perf stat repeat segfault Numfor Mbiziwo-Tiapo
@ 2019-07-11  4:44 ` Ravi Bangoria
  2019-07-14 20:44 ` Jiri Olsa
  1 sibling, 0 replies; 11+ messages in thread
From: Ravi Bangoria @ 2019-07-11  4:44 UTC (permalink / raw)
  To: Numfor Mbiziwo-Tiapo
  Cc: peterz, mingo, acme, alexander.shishkin, jolsa, namhyung,
	songliubraving, mbd, linux-kernel, irogers, eranian

Hi Numfor,

On 7/11/19 2:15 AM, Numfor Mbiziwo-Tiapo wrote:
> -static bool perf_evsel__should_store_id(struct perf_evsel *counter)
> +static bool perf_evsel__should_store_id(struct perf_evsel *counter, int run_idx)
>  {
> -	return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID;
> +	return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID
> +		&& run_idx < 1;
>  }

Build fails for me:

builtin-stat.c: In function ‘perf_evsel__should_store_id’:
builtin-stat.c:395:3: error: suggest parentheses around ‘&&’ within ‘||’ [-Werror=parentheses]
  return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID
                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   && run_idx < 1;
   ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors

And probably,
Fixes: 82bf311e15d2 ("perf stat: Use group read for event groups")


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix perf stat repeat segfault
  2019-07-10 20:45 [PATCH] Fix perf stat repeat segfault Numfor Mbiziwo-Tiapo
  2019-07-11  4:44 ` Ravi Bangoria
@ 2019-07-14 20:44 ` Jiri Olsa
  2019-07-14 20:55   ` Jiri Olsa
  1 sibling, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2019-07-14 20:44 UTC (permalink / raw)
  To: Numfor Mbiziwo-Tiapo
  Cc: peterz, mingo, acme, alexander.shishkin, namhyung,
	songliubraving, mbd, linux-kernel, irogers, eranian

On Wed, Jul 10, 2019 at 01:45:40PM -0700, Numfor Mbiziwo-Tiapo wrote:
> When perf stat is called with event groups and the repeat option,
> a segfault occurs because the cpu ids are stored on each iteration
> of the repeat, when they should only be stored on the first iteration,
> which causes a buffer overflow.
> 
> This can be replicated by running (from the tip directory):
> 
> make -C tools/perf
> 
> then running:
> 
> tools/perf/perf stat -e '{cycles,instructions}' -r 10 ls
> 
> Since run_idx keeps track of the current iteration of the repeat,
> only storing the cpu ids on the first iteration (when run_idx < 1)
> fixes this issue.
> 
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
>  tools/perf/builtin-stat.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 63a3afc7f32b..92d6694367e4 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -378,9 +378,10 @@ static void workload_exec_failed_signal(int signo __maybe_unused, siginfo_t *inf
>  	workload_exec_errno = info->si_value.sival_int;
>  }
>  
> -static bool perf_evsel__should_store_id(struct perf_evsel *counter)
> +static bool perf_evsel__should_store_id(struct perf_evsel *counter, int run_idx)
>  {
> -	return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID;
> +	return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID
> +		&& run_idx < 1;

we create counters for every iteration, so this can't be
based on iteration

I think that's just a workaround for memory corruption,
that's happening for repeating groupped events stats,
I'll check on this

jirka


>  }
>  
>  static bool is_target_alive(struct target *_target,
> @@ -503,7 +504,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  		if (l > stat_config.unit_width)
>  			stat_config.unit_width = l;
>  
> -		if (perf_evsel__should_store_id(counter) &&
> +		if (perf_evsel__should_store_id(counter, run_idx) &&
>  		    perf_evsel__store_ids(counter, evsel_list))
>  			return -1;
>  	}
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix perf stat repeat segfault
  2019-07-14 20:44 ` Jiri Olsa
@ 2019-07-14 20:55   ` Jiri Olsa
  2019-07-14 21:36     ` Stephane Eranian
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2019-07-14 20:55 UTC (permalink / raw)
  To: Numfor Mbiziwo-Tiapo
  Cc: peterz, mingo, acme, alexander.shishkin, namhyung,
	songliubraving, mbd, linux-kernel, irogers, eranian

On Sun, Jul 14, 2019 at 10:44:36PM +0200, Jiri Olsa wrote:
> On Wed, Jul 10, 2019 at 01:45:40PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > When perf stat is called with event groups and the repeat option,
> > a segfault occurs because the cpu ids are stored on each iteration
> > of the repeat, when they should only be stored on the first iteration,
> > which causes a buffer overflow.
> > 
> > This can be replicated by running (from the tip directory):
> > 
> > make -C tools/perf
> > 
> > then running:
> > 
> > tools/perf/perf stat -e '{cycles,instructions}' -r 10 ls
> > 
> > Since run_idx keeps track of the current iteration of the repeat,
> > only storing the cpu ids on the first iteration (when run_idx < 1)
> > fixes this issue.
> > 
> > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> > ---
> >  tools/perf/builtin-stat.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 63a3afc7f32b..92d6694367e4 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -378,9 +378,10 @@ static void workload_exec_failed_signal(int signo __maybe_unused, siginfo_t *inf
> >  	workload_exec_errno = info->si_value.sival_int;
> >  }
> >  
> > -static bool perf_evsel__should_store_id(struct perf_evsel *counter)
> > +static bool perf_evsel__should_store_id(struct perf_evsel *counter, int run_idx)
> >  {
> > -	return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID;
> > +	return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID
> > +		&& run_idx < 1;
> 
> we create counters for every iteration, so this can't be
> based on iteration
> 
> I think that's just a workaround for memory corruption,
> that's happening for repeating groupped events stats,
> I'll check on this

how about something like this? we did not cleanup
ids on evlist close, so it kept on raising and
causing corruption in next iterations

jirka


---
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index ebb46da4dfe5..52459dd5ad0c 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1291,6 +1291,7 @@ static void perf_evsel__free_id(struct perf_evsel *evsel)
 	xyarray__delete(evsel->sample_id);
 	evsel->sample_id = NULL;
 	zfree(&evsel->id);
+	evsel->ids = 0;
 }
 
 static void perf_evsel__free_config_terms(struct perf_evsel *evsel)
@@ -2077,6 +2078,7 @@ void perf_evsel__close(struct perf_evsel *evsel)
 
 	perf_evsel__close_fd(evsel);
 	perf_evsel__free_fd(evsel);
+	perf_evsel__free_id(evsel);
 }
 
 int perf_evsel__open_per_cpu(struct perf_evsel *evsel,

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix perf stat repeat segfault
  2019-07-14 20:55   ` Jiri Olsa
@ 2019-07-14 21:36     ` Stephane Eranian
  2019-07-15  7:59       ` Jiri Olsa
  0 siblings, 1 reply; 11+ messages in thread
From: Stephane Eranian @ 2019-07-14 21:36 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Numfor Mbiziwo-Tiapo, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Namhyung Kim,
	Song Liu, mbd, LKML, Ian Rogers

On Sun, Jul 14, 2019 at 1:55 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Sun, Jul 14, 2019 at 10:44:36PM +0200, Jiri Olsa wrote:
> > On Wed, Jul 10, 2019 at 01:45:40PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > > When perf stat is called with event groups and the repeat option,
> > > a segfault occurs because the cpu ids are stored on each iteration
> > > of the repeat, when they should only be stored on the first iteration,
> > > which causes a buffer overflow.
> > >
> > > This can be replicated by running (from the tip directory):
> > >
> > > make -C tools/perf
> > >
> > > then running:
> > >
> > > tools/perf/perf stat -e '{cycles,instructions}' -r 10 ls
> > >
> > > Since run_idx keeps track of the current iteration of the repeat,
> > > only storing the cpu ids on the first iteration (when run_idx < 1)
> > > fixes this issue.
> > >
> > > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> > > ---
> > >  tools/perf/builtin-stat.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > > index 63a3afc7f32b..92d6694367e4 100644
> > > --- a/tools/perf/builtin-stat.c
> > > +++ b/tools/perf/builtin-stat.c
> > > @@ -378,9 +378,10 @@ static void workload_exec_failed_signal(int signo __maybe_unused, siginfo_t *inf
> > >     workload_exec_errno = info->si_value.sival_int;
> > >  }
> > >
> > > -static bool perf_evsel__should_store_id(struct perf_evsel *counter)
> > > +static bool perf_evsel__should_store_id(struct perf_evsel *counter, int run_idx)
> > >  {
> > > -   return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID;
> > > +   return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID
> > > +           && run_idx < 1;
> >
> > we create counters for every iteration, so this can't be
> > based on iteration
> >
> > I think that's just a workaround for memory corruption,
> > that's happening for repeating groupped events stats,
> > I'll check on this
>
> how about something like this? we did not cleanup
> ids on evlist close, so it kept on raising and
> causing corruption in next iterations
>
not sure, that would realloc on each iteration of the repeats.

>
> jirka
>
>
> ---
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index ebb46da4dfe5..52459dd5ad0c 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1291,6 +1291,7 @@ static void perf_evsel__free_id(struct perf_evsel *evsel)
>         xyarray__delete(evsel->sample_id);
>         evsel->sample_id = NULL;
>         zfree(&evsel->id);
> +       evsel->ids = 0;
>  }
>
>  static void perf_evsel__free_config_terms(struct perf_evsel *evsel)
> @@ -2077,6 +2078,7 @@ void perf_evsel__close(struct perf_evsel *evsel)
>
>         perf_evsel__close_fd(evsel);
>         perf_evsel__free_fd(evsel);
> +       perf_evsel__free_id(evsel);
>  }
>
>  int perf_evsel__open_per_cpu(struct perf_evsel *evsel,

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix perf stat repeat segfault
  2019-07-14 21:36     ` Stephane Eranian
@ 2019-07-15  7:59       ` Jiri Olsa
  2019-07-15  8:14         ` Stephane Eranian
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2019-07-15  7:59 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: Numfor Mbiziwo-Tiapo, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Namhyung Kim,
	Song Liu, mbd, LKML, Ian Rogers

On Sun, Jul 14, 2019 at 02:36:42PM -0700, Stephane Eranian wrote:
> On Sun, Jul 14, 2019 at 1:55 PM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Sun, Jul 14, 2019 at 10:44:36PM +0200, Jiri Olsa wrote:
> > > On Wed, Jul 10, 2019 at 01:45:40PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > > > When perf stat is called with event groups and the repeat option,
> > > > a segfault occurs because the cpu ids are stored on each iteration
> > > > of the repeat, when they should only be stored on the first iteration,
> > > > which causes a buffer overflow.
> > > >
> > > > This can be replicated by running (from the tip directory):
> > > >
> > > > make -C tools/perf
> > > >
> > > > then running:
> > > >
> > > > tools/perf/perf stat -e '{cycles,instructions}' -r 10 ls
> > > >
> > > > Since run_idx keeps track of the current iteration of the repeat,
> > > > only storing the cpu ids on the first iteration (when run_idx < 1)
> > > > fixes this issue.
> > > >
> > > > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> > > > ---
> > > >  tools/perf/builtin-stat.c | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > > > index 63a3afc7f32b..92d6694367e4 100644
> > > > --- a/tools/perf/builtin-stat.c
> > > > +++ b/tools/perf/builtin-stat.c
> > > > @@ -378,9 +378,10 @@ static void workload_exec_failed_signal(int signo __maybe_unused, siginfo_t *inf
> > > >     workload_exec_errno = info->si_value.sival_int;
> > > >  }
> > > >
> > > > -static bool perf_evsel__should_store_id(struct perf_evsel *counter)
> > > > +static bool perf_evsel__should_store_id(struct perf_evsel *counter, int run_idx)
> > > >  {
> > > > -   return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID;
> > > > +   return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID
> > > > +           && run_idx < 1;
> > >
> > > we create counters for every iteration, so this can't be
> > > based on iteration
> > >
> > > I think that's just a workaround for memory corruption,
> > > that's happening for repeating groupped events stats,
> > > I'll check on this
> >
> > how about something like this? we did not cleanup
> > ids on evlist close, so it kept on raising and
> > causing corruption in next iterations
> >
> not sure, that would realloc on each iteration of the repeats.

well, we need new ids, because we create new events every iteration

jirka

> 
> >
> > jirka
> >
> >
> > ---
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index ebb46da4dfe5..52459dd5ad0c 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -1291,6 +1291,7 @@ static void perf_evsel__free_id(struct perf_evsel *evsel)
> >         xyarray__delete(evsel->sample_id);
> >         evsel->sample_id = NULL;
> >         zfree(&evsel->id);
> > +       evsel->ids = 0;
> >  }
> >
> >  static void perf_evsel__free_config_terms(struct perf_evsel *evsel)
> > @@ -2077,6 +2078,7 @@ void perf_evsel__close(struct perf_evsel *evsel)
> >
> >         perf_evsel__close_fd(evsel);
> >         perf_evsel__free_fd(evsel);
> > +       perf_evsel__free_id(evsel);
> >  }
> >
> >  int perf_evsel__open_per_cpu(struct perf_evsel *evsel,

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix perf stat repeat segfault
  2019-07-15  7:59       ` Jiri Olsa
@ 2019-07-15  8:14         ` Stephane Eranian
  2019-07-15  8:31           ` Jiri Olsa
  0 siblings, 1 reply; 11+ messages in thread
From: Stephane Eranian @ 2019-07-15  8:14 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Numfor Mbiziwo-Tiapo, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Namhyung Kim,
	Song Liu, mbd, LKML, Ian Rogers

On Mon, Jul 15, 2019 at 12:59 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Sun, Jul 14, 2019 at 02:36:42PM -0700, Stephane Eranian wrote:
> > On Sun, Jul 14, 2019 at 1:55 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Sun, Jul 14, 2019 at 10:44:36PM +0200, Jiri Olsa wrote:
> > > > On Wed, Jul 10, 2019 at 01:45:40PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > > > > When perf stat is called with event groups and the repeat option,
> > > > > a segfault occurs because the cpu ids are stored on each iteration
> > > > > of the repeat, when they should only be stored on the first iteration,
> > > > > which causes a buffer overflow.
> > > > >
> > > > > This can be replicated by running (from the tip directory):
> > > > >
> > > > > make -C tools/perf
> > > > >
> > > > > then running:
> > > > >
> > > > > tools/perf/perf stat -e '{cycles,instructions}' -r 10 ls
> > > > >
> > > > > Since run_idx keeps track of the current iteration of the repeat,
> > > > > only storing the cpu ids on the first iteration (when run_idx < 1)
> > > > > fixes this issue.
> > > > >
> > > > > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> > > > > ---
> > > > >  tools/perf/builtin-stat.c | 7 ++++---
> > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > > > > index 63a3afc7f32b..92d6694367e4 100644
> > > > > --- a/tools/perf/builtin-stat.c
> > > > > +++ b/tools/perf/builtin-stat.c
> > > > > @@ -378,9 +378,10 @@ static void workload_exec_failed_signal(int signo __maybe_unused, siginfo_t *inf
> > > > >     workload_exec_errno = info->si_value.sival_int;
> > > > >  }
> > > > >
> > > > > -static bool perf_evsel__should_store_id(struct perf_evsel *counter)
> > > > > +static bool perf_evsel__should_store_id(struct perf_evsel *counter, int run_idx)
> > > > >  {
> > > > > -   return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID;
> > > > > +   return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID
> > > > > +           && run_idx < 1;
> > > >
> > > > we create counters for every iteration, so this can't be
> > > > based on iteration
> > > >
> > > > I think that's just a workaround for memory corruption,
> > > > that's happening for repeating groupped events stats,
> > > > I'll check on this
> > >
> > > how about something like this? we did not cleanup
> > > ids on evlist close, so it kept on raising and
> > > causing corruption in next iterations
> > >
> > not sure, that would realloc on each iteration of the repeats.
>
> well, we need new ids, because we create new events every iteration
>
If you recreate them, then agreed.
It is not clear to me why you need ids when not running is STAT_RECORD mode.

> jirka
>
> >
> > >
> > > jirka
> > >
> > >
> > > ---
> > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > index ebb46da4dfe5..52459dd5ad0c 100644
> > > --- a/tools/perf/util/evsel.c
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -1291,6 +1291,7 @@ static void perf_evsel__free_id(struct perf_evsel *evsel)
> > >         xyarray__delete(evsel->sample_id);
> > >         evsel->sample_id = NULL;
> > >         zfree(&evsel->id);
> > > +       evsel->ids = 0;
> > >  }
> > >
> > >  static void perf_evsel__free_config_terms(struct perf_evsel *evsel)
> > > @@ -2077,6 +2078,7 @@ void perf_evsel__close(struct perf_evsel *evsel)
> > >
> > >         perf_evsel__close_fd(evsel);
> > >         perf_evsel__free_fd(evsel);
> > > +       perf_evsel__free_id(evsel);
> > >  }
> > >
> > >  int perf_evsel__open_per_cpu(struct perf_evsel *evsel,

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix perf stat repeat segfault
  2019-07-15  8:14         ` Stephane Eranian
@ 2019-07-15  8:31           ` Jiri Olsa
  2019-07-15 14:21             ` [PATCH] perf stat: Fix segfault for event group in repeat mode Jiri Olsa
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2019-07-15  8:31 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: Numfor Mbiziwo-Tiapo, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Alexander Shishkin, Namhyung Kim,
	Song Liu, mbd, LKML, Ian Rogers

On Mon, Jul 15, 2019 at 01:14:59AM -0700, Stephane Eranian wrote:
> On Mon, Jul 15, 2019 at 12:59 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Sun, Jul 14, 2019 at 02:36:42PM -0700, Stephane Eranian wrote:
> > > On Sun, Jul 14, 2019 at 1:55 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > > >
> > > > On Sun, Jul 14, 2019 at 10:44:36PM +0200, Jiri Olsa wrote:
> > > > > On Wed, Jul 10, 2019 at 01:45:40PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > > > > > When perf stat is called with event groups and the repeat option,
> > > > > > a segfault occurs because the cpu ids are stored on each iteration
> > > > > > of the repeat, when they should only be stored on the first iteration,
> > > > > > which causes a buffer overflow.
> > > > > >
> > > > > > This can be replicated by running (from the tip directory):
> > > > > >
> > > > > > make -C tools/perf
> > > > > >
> > > > > > then running:
> > > > > >
> > > > > > tools/perf/perf stat -e '{cycles,instructions}' -r 10 ls
> > > > > >
> > > > > > Since run_idx keeps track of the current iteration of the repeat,
> > > > > > only storing the cpu ids on the first iteration (when run_idx < 1)
> > > > > > fixes this issue.
> > > > > >
> > > > > > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> > > > > > ---
> > > > > >  tools/perf/builtin-stat.c | 7 ++++---
> > > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > > > > > index 63a3afc7f32b..92d6694367e4 100644
> > > > > > --- a/tools/perf/builtin-stat.c
> > > > > > +++ b/tools/perf/builtin-stat.c
> > > > > > @@ -378,9 +378,10 @@ static void workload_exec_failed_signal(int signo __maybe_unused, siginfo_t *inf
> > > > > >     workload_exec_errno = info->si_value.sival_int;
> > > > > >  }
> > > > > >
> > > > > > -static bool perf_evsel__should_store_id(struct perf_evsel *counter)
> > > > > > +static bool perf_evsel__should_store_id(struct perf_evsel *counter, int run_idx)
> > > > > >  {
> > > > > > -   return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID;
> > > > > > +   return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID
> > > > > > +           && run_idx < 1;
> > > > >
> > > > > we create counters for every iteration, so this can't be
> > > > > based on iteration
> > > > >
> > > > > I think that's just a workaround for memory corruption,
> > > > > that's happening for repeating groupped events stats,
> > > > > I'll check on this
> > > >
> > > > how about something like this? we did not cleanup
> > > > ids on evlist close, so it kept on raising and
> > > > causing corruption in next iterations
> > > >
> > > not sure, that would realloc on each iteration of the repeats.
> >
> > well, we need new ids, because we create new events every iteration
> >
> If you recreate them, then agreed.
> It is not clear to me why you need ids when not running is STAT_RECORD mode.

it's for faster reading of group events, see:
  82bf311e15d2 perf stat: Use group read for event groups

jirka

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] perf stat: Fix segfault for event group in repeat mode
  2019-07-15  8:31           ` Jiri Olsa
@ 2019-07-15 14:21             ` Jiri Olsa
  2019-07-16 18:48               ` Arnaldo Carvalho de Melo
  2019-07-23 21:49               ` [tip:perf/urgent] " tip-bot for Jiri Olsa
  0 siblings, 2 replies; 11+ messages in thread
From: Jiri Olsa @ 2019-07-15 14:21 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Numfor Mbiziwo-Tiapo, Stephane Eranian, Peter Zijlstra,
	Ingo Molnar, Arnaldo Carvalho de Melo, Alexander Shishkin,
	Namhyung Kim, Song Liu, mbd, LKML, Ian Rogers

Numfor Mbiziwo-Tiapo reported segfault on stat of event
group in repeat mode:

  # perf stat -e '{cycles,instructions}' -r 10 ls

It's caused by memory corruption due to not cleaned
evsel's id array and index, which needs to be rebuilt
in every stat iteration. Currently the ids index grows,
while the array (which is also not freed) has the same
size.

Fixing this by releasing id array and zeroing ids index
in perf_evsel__close function.

We also need to keep the evsel_list alive for stat
record (which is disabled in repeat mode).

Reported-by: Numfor Mbiziwo-Tiapo <nums@google.com>
Link: http://lkml.kernel.org/n/tip-07t75chgdhcr00uerh51hb6j@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-stat.c | 9 ++++++++-
 tools/perf/util/evsel.c   | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index b55a534b4de0..352cf39d7c2f 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -607,7 +607,13 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	 * group leaders.
 	 */
 	read_counters(&(struct timespec) { .tv_nsec = t1-t0 });
-	perf_evlist__close(evsel_list);
+
+	/*
+	 * We need to keep evsel_list alive, because it's processed
+	 * later the evsel_list will be closed after.
+	 */
+	if (!STAT_RECORD)
+		perf_evlist__close(evsel_list);
 
 	return WEXITSTATUS(status);
 }
@@ -1997,6 +2003,7 @@ int cmd_stat(int argc, const char **argv)
 			perf_session__write_header(perf_stat.session, evsel_list, fd, true);
 		}
 
+		perf_evlist__close(evsel_list);
 		perf_session__delete(perf_stat.session);
 	}
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index ebb46da4dfe5..52459dd5ad0c 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1291,6 +1291,7 @@ static void perf_evsel__free_id(struct perf_evsel *evsel)
 	xyarray__delete(evsel->sample_id);
 	evsel->sample_id = NULL;
 	zfree(&evsel->id);
+	evsel->ids = 0;
 }
 
 static void perf_evsel__free_config_terms(struct perf_evsel *evsel)
@@ -2077,6 +2078,7 @@ void perf_evsel__close(struct perf_evsel *evsel)
 
 	perf_evsel__close_fd(evsel);
 	perf_evsel__free_fd(evsel);
+	perf_evsel__free_id(evsel);
 }
 
 int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
-- 
2.21.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] perf stat: Fix segfault for event group in repeat mode
  2019-07-15 14:21             ` [PATCH] perf stat: Fix segfault for event group in repeat mode Jiri Olsa
@ 2019-07-16 18:48               ` Arnaldo Carvalho de Melo
  2019-07-23 21:49               ` [tip:perf/urgent] " tip-bot for Jiri Olsa
  1 sibling, 0 replies; 11+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-07-16 18:48 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Numfor Mbiziwo-Tiapo, Stephane Eranian, Peter Zijlstra,
	Ingo Molnar, Alexander Shishkin, Namhyung Kim, Song Liu, mbd,
	LKML, Ian Rogers

Em Mon, Jul 15, 2019 at 04:21:21PM +0200, Jiri Olsa escreveu:
> Numfor Mbiziwo-Tiapo reported segfault on stat of event
> group in repeat mode:
> 
>   # perf stat -e '{cycles,instructions}' -r 10 ls
> 
> It's caused by memory corruption due to not cleaned
> evsel's id array and index, which needs to be rebuilt
> in every stat iteration. Currently the ids index grows,
> while the array (which is also not freed) has the same
> size.
> 
> Fixing this by releasing id array and zeroing ids index
> in perf_evsel__close function.
> 
> We also need to keep the evsel_list alive for stat
> record (which is disabled in repeat mode).

Thanks, applied.

- Arnaldo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tip:perf/urgent] perf stat: Fix segfault for event group in repeat mode
  2019-07-15 14:21             ` [PATCH] perf stat: Fix segfault for event group in repeat mode Jiri Olsa
  2019-07-16 18:48               ` Arnaldo Carvalho de Melo
@ 2019-07-23 21:49               ` tip-bot for Jiri Olsa
  1 sibling, 0 replies; 11+ messages in thread
From: tip-bot for Jiri Olsa @ 2019-07-23 21:49 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: jolsa, eranian, peterz, songliubraving, mbd, tglx, mingo,
	irogers, alexander.shishkin, nums, linux-kernel, jolsa, hpa,
	acme, namhyung

Commit-ID:  08ef3af1579d0446db1c1bd08e2c42565addf10f
Gitweb:     https://git.kernel.org/tip/08ef3af1579d0446db1c1bd08e2c42565addf10f
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Mon, 15 Jul 2019 16:21:21 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 23 Jul 2019 09:00:05 -0300

perf stat: Fix segfault for event group in repeat mode

Numfor Mbiziwo-Tiapo reported segfault on stat of event group in repeat
mode:

  # perf stat -e '{cycles,instructions}' -r 10 ls

It's caused by memory corruption due to not cleaned evsel's id array and
index, which needs to be rebuilt in every stat iteration. Currently the
ids index grows, while the array (which is also not freed) has the same
size.

Fixing this by releasing id array and zeroing ids index in
perf_evsel__close function.

We also need to keep the evsel_list alive for stat record (which is
disabled in repeat mode).

Reported-by: Numfor Mbiziwo-Tiapo <nums@google.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Mark Drayton <mbd@fb.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/20190715142121.GC6032@krava
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c | 9 ++++++++-
 tools/perf/util/evsel.c   | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index b55a534b4de0..352cf39d7c2f 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -607,7 +607,13 @@ try_again:
 	 * group leaders.
 	 */
 	read_counters(&(struct timespec) { .tv_nsec = t1-t0 });
-	perf_evlist__close(evsel_list);
+
+	/*
+	 * We need to keep evsel_list alive, because it's processed
+	 * later the evsel_list will be closed after.
+	 */
+	if (!STAT_RECORD)
+		perf_evlist__close(evsel_list);
 
 	return WEXITSTATUS(status);
 }
@@ -1997,6 +2003,7 @@ int cmd_stat(int argc, const char **argv)
 			perf_session__write_header(perf_stat.session, evsel_list, fd, true);
 		}
 
+		perf_evlist__close(evsel_list);
 		perf_session__delete(perf_stat.session);
 	}
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index ebb46da4dfe5..52459dd5ad0c 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1291,6 +1291,7 @@ static void perf_evsel__free_id(struct perf_evsel *evsel)
 	xyarray__delete(evsel->sample_id);
 	evsel->sample_id = NULL;
 	zfree(&evsel->id);
+	evsel->ids = 0;
 }
 
 static void perf_evsel__free_config_terms(struct perf_evsel *evsel)
@@ -2077,6 +2078,7 @@ void perf_evsel__close(struct perf_evsel *evsel)
 
 	perf_evsel__close_fd(evsel);
 	perf_evsel__free_fd(evsel);
+	perf_evsel__free_id(evsel);
 }
 
 int perf_evsel__open_per_cpu(struct perf_evsel *evsel,

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-07-23 21:50 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-10 20:45 [PATCH] Fix perf stat repeat segfault Numfor Mbiziwo-Tiapo
2019-07-11  4:44 ` Ravi Bangoria
2019-07-14 20:44 ` Jiri Olsa
2019-07-14 20:55   ` Jiri Olsa
2019-07-14 21:36     ` Stephane Eranian
2019-07-15  7:59       ` Jiri Olsa
2019-07-15  8:14         ` Stephane Eranian
2019-07-15  8:31           ` Jiri Olsa
2019-07-15 14:21             ` [PATCH] perf stat: Fix segfault for event group in repeat mode Jiri Olsa
2019-07-16 18:48               ` Arnaldo Carvalho de Melo
2019-07-23 21:49               ` [tip:perf/urgent] " tip-bot for Jiri Olsa

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).