Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751173AbbDUFWB (ORCPT ); Tue, 21 Apr 2015 01:22:01 -0400 Received: from lgeamrelo02.lge.com ([156.147.1.126]:34040 "EHLO lgeamrelo02.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750709AbbDUFV7 (ORCPT ); Tue, 21 Apr 2015 01:21:59 -0400 X-Original-SENDERIP: 10.177.220.203 X-Original-MAILFROM: namhyung@kernel.org Date: Tue, 21 Apr 2015 14:16:49 +0900 From: Namhyung Kim To: Wang Nan Cc: jolsa@redhat.com, acme@kernel.org, jolsa@kernel.org, mingo@redhat.com, lizefan@huawei.com, pi3orama@163.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly. Message-ID: <20150421051649.GA1905@sejong> References: <1429587190-208370-1-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1429587190-208370-1-git-send-email-wangnan0@huawei.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: 14706 Lines: 347 Hi Wang, On Tue, Apr 21, 2015 at 03:33:10AM +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: > > # 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 of > it will be treated as kernel, not kernel module. Sorry if I missed some prior discussion on it, but any chance to treat them as modules instead of kernel binaries? Thanks, Namhyung > It will then be passed to > dso__load_kernel_sym() -> dso__load_kcore() because of --kallsyms > argument. > > 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 > --- > Improves commit messages. > > Since ba92732e9808df679ddf75c5ea1c0caae6d7dce2 is already in -tip tree, > segfault will not be triggered even without this patch. > --- > tools/perf/tests/kmod-path.c | 72 ++++++++++++++++++++++++++++++++++++++++++++ > tools/perf/util/dso.c | 42 +++++++++++++++++++++++--- > tools/perf/util/dso.h | 2 +- > tools/perf/util/header.c | 8 ++--- > tools/perf/util/machine.c | 16 +++++++++- > 5 files changed, 130 insertions(+), 10 deletions(-) > > diff --git a/tools/perf/tests/kmod-path.c b/tools/perf/tests/kmod-path.c > index e8d7cbb..08c433b 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 fc0ddd5..e9d4ae4 100644 > --- a/tools/perf/util/dso.c > +++ b/tools/perf/util/dso.c > @@ -165,13 +165,25 @@ 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; > + /* caller should pass a masked cpumode. Mask again for safety. */ > + switch (cpumode & PERF_RECORD_MISC_CPUMODE_MASK) { > + case PERF_RECORD_MISC_USER: > + case PERF_RECORD_MISC_HYPERVISOR: > + case PERF_RECORD_MISC_GUEST_USER: > + return false; > + /* Regard 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; > } > > @@ -214,12 +226,34 @@ 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 e0901b4..cc3797c 100644 > --- a/tools/perf/util/dso.h > +++ b/tools/perf/util/dso.h > @@ -216,7 +216,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 918fd8a..caeea35 100644 > --- a/tools/perf/util/header.c > +++ b/tools/perf/util/header.c > @@ -1220,7 +1220,7 @@ static int __event_process_build_id(struct build_id_event *bev, > int err = -1; > struct dsos *dsos; > struct machine *machine; > - u16 misc; > + u16 cpumode; > struct dso *dso; > enum dso_kernel_type dso_type; > > @@ -1228,9 +1228,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; > dsos = &machine->kernel_dsos; > @@ -1254,7 +1254,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 527e032..fe56c9db 100644 > --- a/tools/perf/util/machine.c > +++ b/tools/perf/util/machine.c > @@ -1110,7 +1110,21 @@ static int machine__process_kernel_mmap_event(struct machine *machine, > struct dso *dso; > > list_for_each_entry(dso, &machine->kernel_dsos.head, node) { > - if (is_kernel_module(dso->long_name)) > + /* > + * 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 (is_kernel_module(dso->long_name, > + PERF_RECORD_MISC_CPUMODE_UNKNOWN)) > continue; > > kernel = dso; > -- > 1.8.3.4 > > -- > 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/ -- 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/