* [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly.
@ 2015-04-21 3:33 Wang Nan
2015-04-21 5:16 ` Namhyung Kim
2015-05-28 8:33 ` [PATCH v6] perf: __kmod_path__parse: deal " Jiri Olsa
0 siblings, 2 replies; 11+ messages in thread
From: Wang Nan @ 2015-04-21 3:33 UTC (permalink / raw)
To: jolsa, acme, jolsa, mingo; +Cc: namhyung, lizefan, pi3orama, linux-kernel
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. 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@huawei.com>
---
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 +-
| 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);
--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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly.
2015-04-21 3:33 [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly Wang Nan
@ 2015-04-21 5:16 ` Namhyung Kim
2015-04-21 9:28 ` Wang Nan
2015-05-28 8:33 ` [PATCH v6] perf: __kmod_path__parse: deal " Jiri Olsa
1 sibling, 1 reply; 11+ messages in thread
From: Namhyung Kim @ 2015-04-21 5:16 UTC (permalink / raw)
To: Wang Nan; +Cc: jolsa, acme, jolsa, mingo, lizefan, pi3orama, linux-kernel
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@huawei.com>
> ---
> 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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly.
2015-04-21 5:16 ` Namhyung Kim
@ 2015-04-21 9:28 ` Wang Nan
2015-05-29 0:00 ` Namhyung Kim
0 siblings, 1 reply; 11+ messages in thread
From: Wang Nan @ 2015-04-21 9:28 UTC (permalink / raw)
To: Namhyung Kim; +Cc: jolsa, acme, jolsa, mingo, lizefan, pi3orama, linux-kernel
On 2015/4/21 13:16, Namhyung Kim wrote:
> 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
>
>
Sorry, I tried but failed to understand your question. What this patch do
is to treat them as modules instead of kernel binaries (or binary?
since kernel is a whole binary and kernel modules are DSOs).
[SNIP]
>> + /* 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);
Before this patch, these testcases will return kmod == false result.
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly.
2015-04-21 3:33 [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly Wang Nan
2015-04-21 5:16 ` Namhyung Kim
@ 2015-05-28 8:33 ` Jiri Olsa
1 sibling, 0 replies; 11+ messages in thread
From: Jiri Olsa @ 2015-05-28 8:33 UTC (permalink / raw)
To: Wang Nan; +Cc: acme, jolsa, mingo, namhyung, lizefan, pi3orama, linux-kernel
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:
sry I've lost track on this one.. found it under:
[PATCH v4 2/2] perf: report/annotate: fix segfault problem.
my comments were addressed and other than one nit below
it looks ok to me
Namhyung, I looked up in history that you had question on this one,
could you please recheck Wang's reply?
thanks,
jirka
SNIP
> 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) {
I think it's better to print out if there's unexpected error as
warning rather than hide it.. like we do with WARN_ONCE
or pr_warning stuff
> + 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;
SNIP
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly.
2015-04-21 9:28 ` Wang Nan
@ 2015-05-29 0:00 ` Namhyung Kim
2015-06-03 7:49 ` [PATCH v7] " Wang Nan
0 siblings, 1 reply; 11+ messages in thread
From: Namhyung Kim @ 2015-05-29 0:00 UTC (permalink / raw)
To: Wang Nan; +Cc: jolsa, acme, jolsa, mingo, lizefan, pi3orama, linux-kernel
Hi,
Sorry for being late, I forgot about this email..
On Tue, Apr 21, 2015 at 05:28:51PM +0800, Wang Nan wrote:
> On 2015/4/21 13:16, Namhyung Kim wrote:
> > 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?
>
> Sorry, I tried but failed to understand your question. What this patch do
> is to treat them as modules instead of kernel binaries (or binary?
> since kernel is a whole binary and kernel modules are DSOs).
It seems I misunderstood your point. So this patch makes DSOs have
correct dso_kernel_type, right? Then I'm fine with this change..
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v7] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly
2015-05-29 0:00 ` Namhyung Kim
@ 2015-06-03 7:49 ` Wang Nan
2015-06-03 8:47 ` Jiri Olsa
0 siblings, 1 reply; 11+ messages in thread
From: Wang Nan @ 2015-06-03 7:49 UTC (permalink / raw)
To: namhyung, jolsa, acme, jolsa; +Cc: mingo, lizefan, pi3orama, linux-kernel
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 <wangnan0@huawei.com>
---
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 +-
| 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);
--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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v7] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly
2015-06-03 7:49 ` [PATCH v7] " Wang Nan
@ 2015-06-03 8:47 ` Jiri Olsa
2015-06-03 8:52 ` [PATCH v8] " Wang Nan
0 siblings, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2015-06-03 8:47 UTC (permalink / raw)
To: Wang Nan; +Cc: namhyung, acme, jolsa, mingo, lizefan, pi3orama, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v8] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly
2015-06-03 8:47 ` Jiri Olsa
@ 2015-06-03 8:52 ` Wang Nan
2015-06-03 9:12 ` Jiri Olsa
2015-06-04 14:12 ` [tip:perf/core] perf tools: Deal " tip-bot for Wang Nan
0 siblings, 2 replies; 11+ messages in thread
From: Wang Nan @ 2015-06-03 8:52 UTC (permalink / raw)
To: namhyung, jolsa, acme; +Cc: mingo, lizefan, pi3orama, linux-kernel
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 <wangnan0@huawei.com>
---
Remove unneeded comments.
---
tools/perf/tests/kmod-path.c | 72 ++++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/dso.c | 47 ++++++++++++++++++++++++++---
tools/perf/util/dso.h | 2 +-
| 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);
--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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v8] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly
2015-06-03 8:52 ` [PATCH v8] " Wang Nan
@ 2015-06-03 9:12 ` Jiri Olsa
2015-06-03 13:09 ` Arnaldo Carvalho de Melo
2015-06-04 14:12 ` [tip:perf/core] perf tools: Deal " tip-bot for Wang Nan
1 sibling, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2015-06-03 9:12 UTC (permalink / raw)
To: Wang Nan; +Cc: namhyung, jolsa, acme, mingo, lizefan, pi3orama, linux-kernel
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 <jolsa@kernel.org>
thanks,
jirka
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v8] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly
2015-06-03 9:12 ` Jiri Olsa
@ 2015-06-03 13:09 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 11+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-06-03 13:09 UTC (permalink / raw)
To: Jiri Olsa
Cc: Wang Nan, namhyung, jolsa, mingo, lizefan, pi3orama, linux-kernel
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 <jolsa@kernel.org>
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 <wangnan0@huawei.com>
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 <wangnan0@huawei.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Zefan Li <lizefan@huawei.com>
Link: http://lkml.kernel.org/r/1433321541-170245-1-git-send-email-wangnan0@huawei.com
[ Fixed the merged with 0443f36b0de0 ("perf machine: Fix the search
for the kernel DSO on the unified list" ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/tests/kmod-path.c | 72 ++++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/dso.c | 47 ++++++++++++++++++++++++++---
tools/perf/util/dso.h | 2 +-
| 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);
--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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [tip:perf/core] perf tools: Deal with kernel module names in '[]' correctly
2015-06-03 8:52 ` [PATCH v8] " Wang Nan
2015-06-03 9:12 ` Jiri Olsa
@ 2015-06-04 14:12 ` tip-bot for Wang Nan
1 sibling, 0 replies; 11+ messages in thread
From: tip-bot for Wang Nan @ 2015-06-04 14:12 UTC (permalink / raw)
To: linux-tip-commits
Cc: acme, lizefan, linux-kernel, mingo, tglx, jolsa, hpa, namhyung, wangnan0
Commit-ID: 1f121b03d058dd07199d8924373d3c52a207f63b
Gitweb: http://git.kernel.org/tip/1f121b03d058dd07199d8924373d3c52a207f63b
Author: Wang Nan <wangnan0@huawei.com>
AuthorDate: Wed, 3 Jun 2015 08:52:21 +0000
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
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 <wangnan0@huawei.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Zefan Li <lizefan@huawei.com>
Link: http://lkml.kernel.org/r/1433321541-170245-1-git-send-email-wangnan0@huawei.com
[ Fixed the merged with 0443f36b0de0 ("perf machine: Fix the search
for the kernel DSO on the unified list" ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/tests/kmod-path.c | 72 ++++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/dso.c | 47 ++++++++++++++++++++++++++---
tools/perf/util/dso.h | 2 +-
| 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);
--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;
}
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-06-04 14:12 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-21 3:33 [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly Wang Nan
2015-04-21 5:16 ` Namhyung Kim
2015-04-21 9:28 ` Wang Nan
2015-05-29 0:00 ` Namhyung Kim
2015-06-03 7:49 ` [PATCH v7] " Wang Nan
2015-06-03 8:47 ` Jiri Olsa
2015-06-03 8:52 ` [PATCH v8] " Wang Nan
2015-06-03 9:12 ` Jiri Olsa
2015-06-03 13:09 ` Arnaldo Carvalho de Melo
2015-06-04 14:12 ` [tip:perf/core] perf tools: Deal " tip-bot for Wang Nan
2015-05-28 8:33 ` [PATCH v6] perf: __kmod_path__parse: deal " Jiri Olsa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).