Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760303AbbFCNKV (ORCPT ); Wed, 3 Jun 2015 09:10:21 -0400 Received: from mail.kernel.org ([198.145.29.136]:53608 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756160AbbFCNKG (ORCPT ); Wed, 3 Jun 2015 09:10:06 -0400 Date: Wed, 3 Jun 2015 10:09:59 -0300 From: Arnaldo Carvalho de Melo To: Jiri Olsa Cc: Wang Nan , namhyung@kernel.org, jolsa@kernel.org, mingo@redhat.com, lizefan@huawei.com, pi3orama@163.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v8] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly Message-ID: <20150603130959.GU624@kernel.org> References: <20150603084726.GC24863@krava.redhat.com> <1433321541-170245-1-git-send-email-wangnan0@huawei.com> <20150603091204.GA1828@krava.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150603091204.GA1828@krava.redhat.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 16150 Lines: 416 Em Wed, Jun 03, 2015 at 11:12:04AM +0200, Jiri Olsa escreveu: > On Wed, Jun 03, 2015 at 08:52:21AM +0000, Wang Nan wrote: > > Before patch ba92732e9808df679ddf75c5ea1c0caae6d7dce2 ('perf kmaps: > > Check kmaps to make code more robust'), perf report and perf annotate > > will segfault if trace data contains kernel module information like > > this: > > Acked-by: Jiri Olsa Had to fix the merge with one hunk I had from a fix I made, please check, full patch below, but the clash was in this loop: +++ b/tools/perf/util/machine.c @@ -1149,9 +1149,29 @@ static int machine__process_kernel_mmap_event(struct machine *machine, struct dso *dso; list_for_each_entry(dso, &machine->dsos.head, node) { - if (!dso->kernel || is_kernel_module(dso->long_name)) + + /* + * The cpumode passed to is_kernel_module is not the + * cpumode of *this* event. If we insist on passing + * correct cpumode to is_kernel_module, we should + * record the cpumode when we adding this dso to the + * linked list. + * + * However we don't really need passing correct + * cpumode. We know the correct cpumode must be kernel + * mode (if not, we should not link it onto kernel_dsos + * list). + * + * Therefore, we pass PERF_RECORD_MISC_CPUMODE_UNKNOWN. + * is_kernel_module() treats it as a kernel cpumode. + */ + + if (!dso->kernel || + is_kernel_module(dso->long_name, + PERF_RECORD_MISC_CPUMODE_UNKNOWN)) continue; + kernel = dso; break; } - ------------------------------- I.e. before it was: if (dso->kernel && is_kernel_module(dso->long_name)) But then, since now there are !dso->kernel entries in this list, it could get as the 'kernel' a user DSO. So I changed it to: if (!dso->kernel || is_kernel_module(dso->long_name)) To discard user and kernel module DSOs. Applied the patch below, let me know if you disagree. - Arnaldo >From 1f121b03d058dd07199d8924373d3c52a207f63b Mon Sep 17 00:00:00 2001 From: Wang Nan Date: Wed, 3 Jun 2015 08:52:21 +0000 Subject: [PATCH 1/1] perf tools: Deal with kernel module names in '[]' correctly Before patch ba92732e9808 ('perf kmaps: Check kmaps to make code more robust'), 'perf report' and 'perf annotate' will segfault if trace data contains kernel module information like this: # perf report -D -i ./perf.data ... 0 0 0x188 [0x50]: PERF_RECORD_MMAP -1/0: [0xffffffbff1018000(0xf068000) @ 0]: x [test_module] ... # perf report -i ./perf.data --objdump=/path/to/objdump --kallsyms=/path/to/kallsyms perf: Segmentation fault -------- backtrace -------- /path/to/perf[0x503478] /lib64/libc.so.6(+0x3545f)[0x7fb201f3745f] /path/to/perf[0x499b56] /path/to/perf(dso__load_kallsyms+0x13c)[0x49b56c] /path/to/perf(dso__load+0x72e)[0x49c21e] /path/to/perf(map__load+0x6e)[0x4ae9ee] /path/to/perf(thread__find_addr_map+0x24c)[0x47deec] /path/to/perf(perf_event__preprocess_sample+0x88)[0x47e238] /path/to/perf[0x43ad02] /path/to/perf[0x4b55bc] /path/to/perf(ordered_events__flush+0xca)[0x4b57ea] /path/to/perf[0x4b1a01] /path/to/perf(perf_session__process_events+0x3be)[0x4b428e] /path/to/perf(cmd_report+0xf11)[0x43bfc1] /path/to/perf[0x474702] /path/to/perf(main+0x5f5)[0x42de95] /lib64/libc.so.6(__libc_start_main+0xf4)[0x7fb201f23bd4] /path/to/perf[0x42dfc4] This is because __kmod_path__parse treats '[' leading names as kernel name instead of names of kernel module. If perf.data contains build information and the buildid of such modules can be found, the dso->kernel of it will be set to DSO_TYPE_KERNEL by __event_process_build_id(), not kernel module. It will then be passed to dso__load() -> dso__load_kernel_sym() -> dso__load_kcore() if --kallsyms is provided. The refered patch adds NULL pointer checker to avoid segfault. However, such kernel modules are still processed incorrectly. This patch fixes __kmod_path__parse, makes it treat names like '[test_module]' as kernel modules. kmod-path.c is also update to reflect the above changes. Signed-off-by: Wang Nan Acked-by: Jiri Olsa Cc: Namhyung Kim Cc: Zefan Li Link: http://lkml.kernel.org/r/1433321541-170245-1-git-send-email-wangnan0@huawei.com [ Fixed the merged with 0443f36b0de0 ("perf machine: Fix the search for the kernel DSO on the unified list" ] Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/tests/kmod-path.c | 72 ++++++++++++++++++++++++++++++++++++++++++++ tools/perf/util/dso.c | 47 ++++++++++++++++++++++++++--- tools/perf/util/dso.h | 2 +- tools/perf/util/header.c | 8 ++--- tools/perf/util/machine.c | 22 +++++++++++++- 5 files changed, 140 insertions(+), 11 deletions(-) diff --git a/tools/perf/tests/kmod-path.c b/tools/perf/tests/kmod-path.c index e8d7cbb9320c..08c433b4bf4f 100644 --- a/tools/perf/tests/kmod-path.c +++ b/tools/perf/tests/kmod-path.c @@ -34,9 +34,21 @@ static int test(const char *path, bool alloc_name, bool alloc_ext, return 0; } +static int test_is_kernel_module(const char *path, int cpumode, bool expect) +{ + TEST_ASSERT_VAL("is_kernel_module", + (!!is_kernel_module(path, cpumode)) == (!!expect)); + pr_debug("%s (cpumode: %d) - is_kernel_module: %s\n", + path, cpumode, expect ? "true" : "false"); + return 0; +} + #define T(path, an, ae, k, c, n, e) \ TEST_ASSERT_VAL("failed", !test(path, an, ae, k, c, n, e)) +#define M(path, c, e) \ + TEST_ASSERT_VAL("failed", !test_is_kernel_module(path, c, e)) + int test__kmod_path__parse(void) { /* path alloc_name alloc_ext kmod comp name ext */ @@ -44,30 +56,90 @@ int test__kmod_path__parse(void) T("/xxxx/xxxx/x-x.ko", false , true , true, false, NULL , NULL); T("/xxxx/xxxx/x-x.ko", true , false , true, false, "[x_x]", NULL); T("/xxxx/xxxx/x-x.ko", false , false , true, false, NULL , NULL); + M("/xxxx/xxxx/x-x.ko", PERF_RECORD_MISC_CPUMODE_UNKNOWN, true); + M("/xxxx/xxxx/x-x.ko", PERF_RECORD_MISC_KERNEL, true); + M("/xxxx/xxxx/x-x.ko", PERF_RECORD_MISC_USER, false); /* path alloc_name alloc_ext kmod comp name ext */ T("/xxxx/xxxx/x.ko.gz", true , true , true, true, "[x]", "gz"); T("/xxxx/xxxx/x.ko.gz", false , true , true, true, NULL , "gz"); T("/xxxx/xxxx/x.ko.gz", true , false , true, true, "[x]", NULL); T("/xxxx/xxxx/x.ko.gz", false , false , true, true, NULL , NULL); + M("/xxxx/xxxx/x.ko.gz", PERF_RECORD_MISC_CPUMODE_UNKNOWN, true); + M("/xxxx/xxxx/x.ko.gz", PERF_RECORD_MISC_KERNEL, true); + M("/xxxx/xxxx/x.ko.gz", PERF_RECORD_MISC_USER, false); /* path alloc_name alloc_ext kmod comp name ext */ T("/xxxx/xxxx/x.gz", true , true , false, true, "x.gz" ,"gz"); T("/xxxx/xxxx/x.gz", false , true , false, true, NULL ,"gz"); T("/xxxx/xxxx/x.gz", true , false , false, true, "x.gz" , NULL); T("/xxxx/xxxx/x.gz", false , false , false, true, NULL , NULL); + M("/xxxx/xxxx/x.gz", PERF_RECORD_MISC_CPUMODE_UNKNOWN, false); + M("/xxxx/xxxx/x.gz", PERF_RECORD_MISC_KERNEL, false); + M("/xxxx/xxxx/x.gz", PERF_RECORD_MISC_USER, false); /* path alloc_name alloc_ext kmod comp name ext */ T("x.gz", true , true , false, true, "x.gz", "gz"); T("x.gz", false , true , false, true, NULL , "gz"); T("x.gz", true , false , false, true, "x.gz", NULL); T("x.gz", false , false , false, true, NULL , NULL); + M("x.gz", PERF_RECORD_MISC_CPUMODE_UNKNOWN, false); + M("x.gz", PERF_RECORD_MISC_KERNEL, false); + M("x.gz", PERF_RECORD_MISC_USER, false); /* path alloc_name alloc_ext kmod comp name ext */ T("x.ko.gz", true , true , true, true, "[x]", "gz"); T("x.ko.gz", false , true , true, true, NULL , "gz"); T("x.ko.gz", true , false , true, true, "[x]", NULL); T("x.ko.gz", false , false , true, true, NULL , NULL); + M("x.ko.gz", PERF_RECORD_MISC_CPUMODE_UNKNOWN, true); + M("x.ko.gz", PERF_RECORD_MISC_KERNEL, true); + M("x.ko.gz", PERF_RECORD_MISC_USER, false); + + /* path alloc_name alloc_ext kmod comp name ext */ + T("[test_module]", true , true , true, false, "[test_module]", NULL); + T("[test_module]", false , true , true, false, NULL , NULL); + T("[test_module]", true , false , true, false, "[test_module]", NULL); + T("[test_module]", false , false , true, false, NULL , NULL); + M("[test_module]", PERF_RECORD_MISC_CPUMODE_UNKNOWN, true); + M("[test_module]", PERF_RECORD_MISC_KERNEL, true); + M("[test_module]", PERF_RECORD_MISC_USER, false); + + /* path alloc_name alloc_ext kmod comp name ext */ + T("[test.module]", true , true , true, false, "[test.module]", NULL); + T("[test.module]", false , true , true, false, NULL , NULL); + T("[test.module]", true , false , true, false, "[test.module]", NULL); + T("[test.module]", false , false , true, false, NULL , NULL); + M("[test.module]", PERF_RECORD_MISC_CPUMODE_UNKNOWN, true); + M("[test.module]", PERF_RECORD_MISC_KERNEL, true); + M("[test.module]", PERF_RECORD_MISC_USER, false); + + /* path alloc_name alloc_ext kmod comp name ext */ + T("[vdso]", true , true , false, false, "[vdso]", NULL); + T("[vdso]", false , true , false, false, NULL , NULL); + T("[vdso]", true , false , false, false, "[vdso]", NULL); + T("[vdso]", false , false , false, false, NULL , NULL); + M("[vdso]", PERF_RECORD_MISC_CPUMODE_UNKNOWN, false); + M("[vdso]", PERF_RECORD_MISC_KERNEL, false); + M("[vdso]", PERF_RECORD_MISC_USER, false); + + /* path alloc_name alloc_ext kmod comp name ext */ + T("[vsyscall]", true , true , false, false, "[vsyscall]", NULL); + T("[vsyscall]", false , true , false, false, NULL , NULL); + T("[vsyscall]", true , false , false, false, "[vsyscall]", NULL); + T("[vsyscall]", false , false , false, false, NULL , NULL); + M("[vsyscall]", PERF_RECORD_MISC_CPUMODE_UNKNOWN, false); + M("[vsyscall]", PERF_RECORD_MISC_KERNEL, false); + M("[vsyscall]", PERF_RECORD_MISC_USER, false); + + /* path alloc_name alloc_ext kmod comp name ext */ + T("[kernel.kallsyms]", true , true , false, false, "[kernel.kallsyms]", NULL); + T("[kernel.kallsyms]", false , true , false, false, NULL , NULL); + T("[kernel.kallsyms]", true , false , false, false, "[kernel.kallsyms]", NULL); + T("[kernel.kallsyms]", false , false , false, false, NULL , NULL); + M("[kernel.kallsyms]", PERF_RECORD_MISC_CPUMODE_UNKNOWN, false); + M("[kernel.kallsyms]", PERF_RECORD_MISC_KERNEL, false); + M("[kernel.kallsyms]", PERF_RECORD_MISC_USER, false); return 0; } diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c index b335db3532a2..5ec9e892c89b 100644 --- a/tools/perf/util/dso.c +++ b/tools/perf/util/dso.c @@ -166,12 +166,28 @@ bool is_supported_compression(const char *ext) return false; } -bool is_kernel_module(const char *pathname) +bool is_kernel_module(const char *pathname, int cpumode) { struct kmod_path m; - - if (kmod_path__parse(&m, pathname)) - return NULL; + int mode = cpumode & PERF_RECORD_MISC_CPUMODE_MASK; + + WARN_ONCE(mode != cpumode, + "Internal error: passing unmasked cpumode (%x) to is_kernel_module", + cpumode); + + switch (mode) { + case PERF_RECORD_MISC_USER: + case PERF_RECORD_MISC_HYPERVISOR: + case PERF_RECORD_MISC_GUEST_USER: + return false; + /* Treat PERF_RECORD_MISC_CPUMODE_UNKNOWN as kernel */ + default: + if (kmod_path__parse(&m, pathname)) { + pr_err("Failed to check whether %s is a kernel module or not. Assume it is.", + pathname); + return true; + } + } return m.kmod; } @@ -215,12 +231,33 @@ int __kmod_path__parse(struct kmod_path *m, const char *path, { const char *name = strrchr(path, '/'); const char *ext = strrchr(path, '.'); + bool is_simple_name = false; memset(m, 0x0, sizeof(*m)); name = name ? name + 1 : path; + /* + * '.' is also a valid character for module name. For example: + * [aaa.bbb] is a valid module name. '[' should have higher + * priority than '.ko' suffix. + * + * The kernel names are from machine__mmap_name. Such + * name should belong to kernel itself, not kernel module. + */ + if (name[0] == '[') { + is_simple_name = true; + if ((strncmp(name, "[kernel.kallsyms]", 17) == 0) || + (strncmp(name, "[guest.kernel.kallsyms", 22) == 0) || + (strncmp(name, "[vdso]", 6) == 0) || + (strncmp(name, "[vsyscall]", 10) == 0)) { + m->kmod = false; + + } else + m->kmod = true; + } + /* No extension, just return name. */ - if (ext == NULL) { + if ((ext == NULL) || is_simple_name) { if (alloc_name) { m->name = strdup(name); return m->name ? 0 : -ENOMEM; diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h index 24a507a54147..ba2d90ed881f 100644 --- a/tools/perf/util/dso.h +++ b/tools/perf/util/dso.h @@ -220,7 +220,7 @@ char dso__symtab_origin(const struct dso *dso); int dso__read_binary_type_filename(const struct dso *dso, enum dso_binary_type type, char *root_dir, char *filename, size_t size); bool is_supported_compression(const char *ext); -bool is_kernel_module(const char *pathname); +bool is_kernel_module(const char *pathname, int cpumode); bool decompress_to_file(const char *ext, const char *filename, int output_fd); bool dso__needs_decompress(struct dso *dso); diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c index 851143a7988d..ac5aaaeed7ff 100644 --- a/tools/perf/util/header.c +++ b/tools/perf/util/header.c @@ -1239,7 +1239,7 @@ static int __event_process_build_id(struct build_id_event *bev, { int err = -1; struct machine *machine; - u16 misc; + u16 cpumode; struct dso *dso; enum dso_kernel_type dso_type; @@ -1247,9 +1247,9 @@ static int __event_process_build_id(struct build_id_event *bev, if (!machine) goto out; - misc = bev->header.misc & PERF_RECORD_MISC_CPUMODE_MASK; + cpumode = bev->header.misc & PERF_RECORD_MISC_CPUMODE_MASK; - switch (misc) { + switch (cpumode) { case PERF_RECORD_MISC_KERNEL: dso_type = DSO_TYPE_KERNEL; break; @@ -1270,7 +1270,7 @@ static int __event_process_build_id(struct build_id_event *bev, dso__set_build_id(dso, &bev->build_id); - if (!is_kernel_module(filename)) + if (!is_kernel_module(filename, cpumode)) dso->kernel = dso_type; build_id__sprintf(dso->build_id, sizeof(dso->build_id), diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index 4e29e80932e5..9e02c86f39f5 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -1149,9 +1149,29 @@ static int machine__process_kernel_mmap_event(struct machine *machine, struct dso *dso; list_for_each_entry(dso, &machine->dsos.head, node) { - if (!dso->kernel || is_kernel_module(dso->long_name)) + + /* + * The cpumode passed to is_kernel_module is not the + * cpumode of *this* event. If we insist on passing + * correct cpumode to is_kernel_module, we should + * record the cpumode when we adding this dso to the + * linked list. + * + * However we don't really need passing correct + * cpumode. We know the correct cpumode must be kernel + * mode (if not, we should not link it onto kernel_dsos + * list). + * + * Therefore, we pass PERF_RECORD_MISC_CPUMODE_UNKNOWN. + * is_kernel_module() treats it as a kernel cpumode. + */ + + if (!dso->kernel || + is_kernel_module(dso->long_name, + PERF_RECORD_MISC_CPUMODE_UNKNOWN)) continue; + kernel = dso; break; } -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/