Re: [PATCH] perf report: Fix invalid memory accessing

From: Wangnan (F)
Date: Mon Sep 07 2015 - 09:08:58 EST




On 2015/9/7 21:03, Jiri Olsa wrote:
On Mon, Sep 07, 2015 at 12:51:55PM +0000, Wang Nan wrote:
Commit e1e499aba570a2ea84d29822b7ea637ac41d9a51 (perf tools: Add
processor socket info to hist_entry and addr_location) reads env->cpu
array for each sample using index al.cpu. However, al.cpu can be -1 if
sample doesn't select PERF_SAMPLE_CPU. Also, env->cpu can be invalid if
feature CPU_TOPOLOGY not selected. We should validate env->cpu and al.cpu
before setting al.socket.

Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
Cc: Kan Liang <kan.liang@xxxxxxxxx>
Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
Cc: Stephane Eranian <eranian@xxxxxxxxxx>
Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
---

Although theoretically CPU_TOPOLOGY feature should always be selected by
'perf record', I did generate a perf.data without that feature. It has
header like this:

# perf report -i ./bad.perf.data --header-only
# ========
# captured on: Thu Jan 8 09:30:15 2009
# hostname : localhost
# os release : 3.10.49-gd672fc4
# perf version : 4.2.gc9df
# arch : aarch64
# nrcpus online : 8
# nrcpus avail : 8
# total memory : 1850768 kB
# cmdline : /system/bin/perf record -e sync:sync_timeline -e kgsl:kgsl_register_event -g -a sleep 5
# event : name = sync:sync_timeline, , id = { 1107, 1108, 1109, 1110, 1111, 1112 }, type = 2, size = 112, config = 0x3e7, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, mmap = 1, comm = 1, task = 1, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1
# event : name = kgsl:kgsl_register_event, , id = { 1113, 1114, 1115, 1116, 1117, 1118 }, type = 2, size = 112, config = 0x350, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, sample_id_all = 1, exclude_guest = 1
# pmu mappings: cpu = 4, software = 1, tracepoint = 2
# ========
#

It should be:

# ========
# captured on: Thu Jan 8 11:26:41 2009
...
# HEADER_CPU_TOPOLOGY info available, use -I to display
# pmu mappings: cpu = 4, software = 1, tracepoint = 2
# ========

However, bad perf.data appears randomly. I can't stably reproduce it, so I
guess there might have another invalid memory accessing.

---
tools/perf/builtin-report.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 4b43245..16d097d 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -158,8 +158,16 @@ static int process_sample_event(struct perf_tool *tool,
return -1;
}
- /* read socket id from perf.data for perf report */
- al.socket = env->cpu[al.cpu].socket_id;
+ /*
+ * read socket id from perf.data for perf report
+ * al.cpu is invalid if PERF_SAMPLE_CPU is not selected by this
+ * sample.
+ * env->cpu is invalid if CPU_TOPOLOGY feature is not set in
+ * header.
+ */
+ al.socket = -1;
+ if (env->cpu && al.cpu >= 0)
+ al.socket = env->cpu[al.cpu].socket_id;
perf_event__preprocess_sample initializes al.socket from current system

No. For 'perf report' it initializes al.cpu from sample.


Commit message of e1e499aba570a2ea84d29822b7ea637ac41d9a51:

Finor 'perf report', the socket id info is from perf.data.

For others, the socket id info is from current system.

And at least checking of env->cpu is essential. I'm looking the problem I reported.
Looks like build_cpu_topology() is possible to fail.

Thank you.
do we want to move this over there?

also this change is just report specific, and we could need
this in at least perf top

jirka


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/