From: Wang Nan <wangnan0@huawei.com>
To: <jolsa@redhat.com>
Cc: <acme@kernel.org>, <jolsa@kernel.org>, <namhyung@kernel.org>,
<mingo@redhat.com>, <lizefan@huawei.com>, <pi3orama@163.com>,
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/2] perf: report/annotate: fix segfault problem.
Date: Wed, 15 Apr 2015 09:27:03 +0800 [thread overview]
Message-ID: <552DBE67.50500@huawei.com> (raw)
In-Reply-To: <1428637982-126924-1-git-send-email-wangnan0@huawei.com>
Ping?
On 2015/4/10 11:53, Wang Nan wrote:
> perf report and perf annotate are easy to trigger segfault if trace data
> contain 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 regard '[' leading name as kernel
> instead of kernel module. If perf.data contain build information and
> the buildid of such modules can be found, the DSO of it will be treated
> as kernel, not kernel module. It will then be passed to
> dso__load_kernel_sym() then dso__load_kcore() because of --kallsyms
> argument. The segfault is triggered because the kmap structure is not
> initialized.
>
> Although in --vmlinux case such segfault can be avoided, the symbols in
> the kernel module are unable to be retrived since the attribute of DSO
> is incorrect.
>
> This patch fixes __kmod_path__parse, make it to treat names like
> '[test_module]' as kernel modules.
>
> kmod-path.c is also update to reflect the above changes.
>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>
> Different from v4: checks cpumode in is_kernel_module(), makes code simpler.
> Appends tests of is_kernel_module().
> ---
> 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 fb43215..8c76a23 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1232,7 +1232,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;
>
> @@ -1240,9 +1240,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;
> @@ -1266,7 +1266,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 e335330..3769009 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1109,7 +1109,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() treat it as a kernel cpumode.
> + */
> + if (is_kernel_module(dso->long_name,
> + PERF_RECORD_MISC_CPUMODE_UNKNOWN))
> continue;
>
> kernel = dso;
>
next prev parent reply other threads:[~2015-04-15 1:28 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-03 5:56 [PATCH v2] perf: report/annotate: fix segfault problem Wang Nan
2015-04-03 6:48 ` Ingo Molnar
2015-04-03 8:47 ` [PATCH] perf: kmaps: enforce usage of kmaps to protect futher bugs Wang Nan
2015-04-03 8:49 ` Ingo Molnar
2015-04-03 11:11 ` Jiri Olsa
2015-04-07 10:42 ` Adrian Hunter
2015-04-08 10:50 ` [PATCH v4] " Wang Nan
2015-04-03 9:07 ` [PATCH v2] perf: report/annotate: fix segfault problem Jiri Olsa
2015-04-03 10:57 ` Jiri Olsa
2015-04-06 12:52 ` Arnaldo Carvalho de Melo
2015-04-07 8:22 ` [PATCH v3 0/2] " Wang Nan
2015-04-07 8:22 ` [PATCH v3 1/2] perf: kmaps: enforce usage of kmaps to protect futher bugs Wang Nan
2015-04-08 15:10 ` [tip:perf/core] perf kmaps: Check kmaps to make code more robust tip-bot for Wang Nan
2015-04-07 8:22 ` [PATCH v3 2/2] perf: report/annotate: fix segfault problem Wang Nan
2015-04-07 15:13 ` Arnaldo Carvalho de Melo
2015-04-08 3:49 ` Wang Nan
2015-04-08 3:52 ` [PATCH v4 " Wang Nan
2015-04-08 13:59 ` Jiri Olsa
2015-04-09 7:05 ` Wang Nan
2015-04-09 11:40 ` Jiri Olsa
2015-04-09 11:52 ` Jiri Olsa
2015-04-10 1:57 ` Wang Nan
2015-04-10 2:49 ` Wang Nan
2015-04-10 3:53 ` [PATCH v5 " Wang Nan
2015-04-15 1:27 ` Wang Nan [this message]
2015-04-20 1:18 ` Wang Nan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=552DBE67.50500@huawei.com \
--to=wangnan0@huawei.com \
--cc=acme@kernel.org \
--cc=jolsa@kernel.org \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lizefan@huawei.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=pi3orama@163.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.