Re: [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly.

From: Namhyung Kim
Date: Tue Apr 21 2015 - 01:22:12 EST

Next message: Jean Delvare: "Re: [RFC] i2c-tools: i2ctransfer: add new tool"
Previous message: Namhyung Kim: "[PATCH 3/6] perf kmem: Add --live option for current allocation stat"
In reply to: Wang Nan: "[PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly."
Next in thread: Wang Nan: "Re: [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <wangnan0@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Jean Delvare: "Re: [RFC] i2c-tools: i2ctransfer: add new tool"
Previous message: Namhyung Kim: "[PATCH 3/6] perf kmem: Add --live option for current allocation stat"
In reply to: Wang Nan: "[PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly."
Next in thread: Wang Nan: "Re: [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]