Re: [PATCH v2 02/10] perf, tools, stat: Avoid memory overrun with -r

Re: [PATCH v2 02/10] perf, tools, stat: Avoid memory overrun with -r

[Arnaldo Carvalho de Melo]

Em Thu, Mar 14, 2019 at 03:49:54PM -0700, Andi Kleen escreveu:
> From: Andi Kleen <ak@linux.intel.com>
> 
> When -r is used memory would get corrupted because the evsel->id array
> would get overrun. evsel->ids is a running counter of the last id.
> Normally this works fine, but with -r the same event is initialized
> multiple times, but not this counter, so it would keep growing
> beyond the array limit and corrupt random memory.
> 
> Always reinitialize ->ids, and also add an assert to catch
> such overruns in the future.
> 
> This fixes a perf segfault when running it from toplev.
> 
> Before:
> 
> $ valgrind perf stat -r2 -e '{cycles,cycles,cycles,cycles}' true
> ==27012== Memcheck, a memory error detector
> ==27012== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> ==27012== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
> ==27012== Command: perf stat -r2 -e {cycles,cycles,cycles,cycles} true
> ==27012==
> ==27012== Invalid write of size 8
> ==27012==    at 0x33090F: perf_evlist__id_add_fd (in /usr/bin/perf)
> ==27012==    by 0x33C99B: perf_evsel__store_ids (in /usr/bin/perf)
> ==27012==    by 0x2B7E1D: ??? (in /usr/bin/perf)
> ==27012==    by 0x2B97DE: cmd_stat (in /usr/bin/perf)
> ==27012==    by 0x31BFC0: ??? (in /usr/bin/perf)
> ==27012==    by 0x29C7A9: main (in /usr/bin/perf)
> ==27012==  Address 0x13182be8 is 0 bytes after a block of size 8 alloc'd
> ==27012==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
> ==27012==    by 0x33C921: perf_evsel__store_ids (in /usr/bin/perf)
> ==27012==    by 0x2B7E1D: ??? (in /usr/bin/perf)
> ==27012==    by 0x2B97DE: cmd_stat (in /usr/bin/perf)
> ==27012==    by 0x31BFC0: ??? (in /usr/bin/perf)
> ==27012==    by 0x29C7A9: main (in /usr/bin/perf)
> ==27012==
> ...
> 
> After:
> 
> $ valgrind ./perf stat -r2 -e '{cycles,cycles,cycles,cycles}' true
> ==27026== Memcheck, a memory error detector
> ==27026== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> ==27026== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
> ==27026== Command: ./perf stat -r2 -e {cycles,cycles,cycles,cycles} true
> ==27026==
> 
>  Performance counter stats for 'true' (2 runs):

So, this made this break:

[root@quaco perf]# perf test backward
50: Read backward ring buffer                             : FAILED!
[root@quaco perf]# perf test -v backward
50: Read backward ring buffer                             :
--- start ---
test child forked, pid 11127
Using CPUID GenuineIntel-6-8E-A
registering plugin: /root/.traceevent/plugins/plugin_cfg80211.so
registering plugin: /root/.traceevent/plugins/plugin_kvm.so
registering plugin: /root/.traceevent/plugins/plugin_function.so
registering plugin: /root/.traceevent/plugins/plugin_scsi.so
registering plugin: /root/.traceevent/plugins/plugin_jbd2.so
registering plugin: /root/.traceevent/plugins/plugin_sched_switch.so
registering plugin: /root/.traceevent/plugins/plugin_xen.so
registering plugin: /root/.traceevent/plugins/plugin_kmem.so
registering plugin: /root/.traceevent/plugins/plugin_hrtimer.so
registering plugin: /root/.traceevent/plugins/plugin_mac80211.so
mmap size 1052672B
mmap size 8192B
perf: util/evlist.c:533: perf_evlist__id_add: Assertion `evsel->ids < evsel->sample_id->max_x * evsel->sample_id->max_y' failed.
test child interrupted
---- end ----
Read backward ring buffer: FAILED!


I'm removing it till we go thru it to figure out what is the best way to
go, not to get in the way of the other patches going upstream.

- Arnaldo

Re: [PATCH v2 02/10] perf, tools, stat: Avoid memory overrun with -r

[Andi Kleen]

> mmap size 8192B
> perf: util/evlist.c:533: perf_evlist__id_add: Assertion `evsel->ids < evsel->sample_id->max_x * evsel->sample_id->max_y' failed.

I suspect this is a latent bug just caught by my assert.

So it was already broken.

You could of course remove the assert, but it would merely
hide it.

-Andi

> test child interrupted
> ---- end ----
> Read backward ring buffer: FAILED!
> 
> 
> I'm removing it till we go thru it to figure out what is the best way to
> go, not to get in the way of the other patches going upstream.
> 
> - Arnaldo

Re: [PATCH v2 02/10] perf, tools, stat: Avoid memory overrun with -r

[Arnaldo Carvalho de Melo]

Em Tue, Mar 19, 2019 at 12:20:23PM -0700, Andi Kleen escreveu:
> > mmap size 8192B
> > perf: util/evlist.c:533: perf_evlist__id_add: Assertion `evsel->ids < evsel->sample_id->max_x * evsel->sample_id->max_y' failed.
> 
> I suspect this is a latent bug just caught by my assert.
> 
> So it was already broken.
> 
> You could of course remove the assert, but it would merely
> hide it.

I'm removing it just for the moment, I'll definetely revisit this.

- Arnaldo