[PATCH] Fix perf stat repeat segfault

[PATCH] Fix perf stat repeat segfault

[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

Re: [PATCH] Fix perf stat repeat segfault

[Ravi Bangoria]

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

Re: [PATCH] Fix perf stat repeat segfault

[Jiri Olsa]

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
> 

Re: [PATCH] Fix perf stat repeat segfault

[Jiri Olsa]

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,

Re: [PATCH] Fix perf stat repeat segfault

[Stephane Eranian]

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,

Re: [PATCH] Fix perf stat repeat segfault

[Jiri Olsa]

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,

Re: [PATCH] Fix perf stat repeat segfault

[Stephane Eranian]

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,

Re: [PATCH] Fix perf stat repeat segfault

[Jiri Olsa]

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

[PATCH] perf stat: Fix segfault for event group in repeat mode

[Jiri Olsa]

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

Re: [PATCH] perf stat: Fix segfault for event group in repeat mode

[Arnaldo Carvalho de Melo]

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

[tip:perf/urgent] perf stat: Fix segfault for event group in repeat mode

[tip-bot for Jiri Olsa]

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,

[PATCH] Fix perf stat repeat segfault

[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..00a13ce17fd9 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