2015-06-03 07:52:10

by Wang Nan

Subject: [PATCH v7] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly

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->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 <[email protected]>

---

In this version I rebase it on tip/perf/core and improve commit
message.

---
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 | 16 +++++++++-
5 files changed, 134 insertions(+), 11 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 b335db3..87f5796 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);
+ /* caller should pass a masked cpumode. Mask again for safety. */
+ 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 24a507a..ba2d90e 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 851143a..ac5aaae 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 2ed61f5..c7af9a8 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1149,7 +1149,21 @@ 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))
+ /*
+ * 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;
--
1.8.3.4

2015-06-03 08:47:38

by Jiri Olsa

Subject: Re: [PATCH v7] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly

On Wed, Jun 03, 2015 at 07:49:23AM +0000, Wang Nan wrote:

SNIP

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index b335db3..87f5796 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);
> + /* caller should pass a masked cpumode. Mask again for safety. */

no need for this comment then ^^^

jirka

> + 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;
> + }
> + }

SNIP

2015-06-03 08:57:40

by Wang Nan

Subject: [PATCH v8] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly

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->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 <[email protected]>
---

Remove unneeded comments.

---
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 | 16 +++++++++-
5 files changed, 134 insertions(+), 11 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 b335db3..5ec9e89 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 24a507a..ba2d90e 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 851143a..ac5aaae 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 2ed61f5..c7af9a8 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1149,7 +1149,21 @@ 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))
+ /*
+ * 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;
--
1.8.3.4

2015-06-03 09:12:16

by Jiri Olsa

Subject: Re: [PATCH v8] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly

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 <[email protected]>

thanks,
jirka

2015-06-03 13:10:21

by Arnaldo Carvalho de Melo

Subject: Re: [PATCH v8] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly

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 <[email protected]>

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 <[email protected]>
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 <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Zefan Li <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
[ 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 <[email protected]>
---
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

2015-06-04 14:12:27

by tip-bot for Vasyl Gomonovych

Subject: [tip:perf/core] perf tools: Deal with kernel module names in '[]' correctly

Commit-ID: 1f121b03d058dd07199d8924373d3c52a207f63b
Gitweb: http://git.kernel.org/tip/1f121b03d058dd07199d8924373d3c52a207f63b
Author: Wang Nan <[email protected]>
AuthorDate: Wed, 3 Jun 2015 08:52:21 +0000
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 3 Jun 2015 10:02:38 -0300

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 <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Zefan Li <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
[ 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 <[email protected]>
---
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 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 b335db3..5ec9e89 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 24a507a..ba2d90e 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 851143a..ac5aaae 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 4e29e80..9e02c86 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;
}