Skip to contentSkip to conversation

perf, tools, stat: Avoid memory overrun with -r

PATCH Arnaldo Carvalho de Melo 2 participants 3 messages Mar 19, 2019 2 messages not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

Arnaldo Carvalho de Melo

author
#2 lore raw

Em Thu, Mar 14, 2019 at 03:49:54PM -0700, Andi Kleen escreveu:

Quoted 40 lines

From: Andi Kleen <[email protected]>

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:

Quoted 8 lines
$ 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

#3 lore raw
Quoted 2 lines

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

Quoted 9 lines

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

author
#4 lore raw

Em Tue, Mar 19, 2019 at 12:20:23PM -0700, Andi Kleen escreveu:

Quoted 9 lines
Quoted 2 lines

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