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 <[email protected]>
---
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
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")
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> > ---
> > 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,
On Sun, Jul 14, 2019 at 1:55 PM Jiri Olsa <[email protected]> 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 <[email protected]>
> > > ---
> > > 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,
On Sun, Jul 14, 2019 at 02:36:42PM -0700, Stephane Eranian wrote:
> On Sun, Jul 14, 2019 at 1:55 PM Jiri Olsa <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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,
On Mon, Jul 15, 2019 at 12:59 AM Jiri Olsa <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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,
On Mon, Jul 15, 2019 at 01:14:59AM -0700, Stephane Eranian wrote:
> On Mon, Jul 15, 2019 at 12:59 AM Jiri Olsa <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > ---
> > > > > > 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
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 <[email protected]>
Link: http://lkml.kernel.org/n/[email protected]
Signed-off-by: Jiri Olsa <[email protected]>
---
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
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
Commit-ID: 08ef3af1579d0446db1c1bd08e2c42565addf10f
Gitweb: https://git.kernel.org/tip/08ef3af1579d0446db1c1bd08e2c42565addf10f
Author: Jiri Olsa <[email protected]>
AuthorDate: Mon, 15 Jul 2019 16:21:21 +0200
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Ian Rogers <[email protected]>
Cc: Mark Drayton <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Song Liu <[email protected]>
Cc: Stephane Eranian <[email protected]>
Link: http://lkml.kernel.org/r/20190715142121.GC6032@krava
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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,