Re: [PATCH v6] perf: __kmod_path__parse: deal with kernel module names in '[]' correctly.
From: Wangnan (F)
Date: Thu May 28 2015 - 03:32:11 EST
Ping? Does anyone still concern this patch?
On 2015/5/28 15:23, www-data 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. It will then be passed to
dso__load_kernel_sym() -> dso__load_kcore() because of --kallsyms
argument.
The refered patch adds NULL pointer checker to avoid segfault. However,
such kernel modules are still processed incorrectly.
This patch fixes __kmod_path__parse, makes it treat names like
'[test_module]' as kernel modules.
kmod-path.c is also update to reflect the above changes.
Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
---
Improves commit messages.
Since ba92732e9808df679ddf75c5ea1c0caae6d7dce2 is already in -tip tree,
segfault will not be triggered even without this patch.
---
tools/perf/tests/kmod-path.c | 72 ++++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/dso.c | 42 +++++++++++++++++++++++---
tools/perf/util/dso.h | 2 +-
tools/perf/util/header.c | 8 ++---
tools/perf/util/machine.c | 16 +++++++++-
5 files changed, 130 insertions(+), 10 deletions(-)
diff --git a/tools/perf/tests/kmod-path.c b/tools/perf/tests/kmod-path.c
index e8d7cbb..08c433b 100644
--- a/tools/perf/tests/kmod-path.c
+++ b/tools/perf/tests/kmod-path.c
@@ -34,9 +34,21 @@ static int test(const char *path, bool alloc_name, bool alloc_ext,
return 0;
}
+static int test_is_kernel_module(const char *path, int cpumode, bool expect)
+{
+ TEST_ASSERT_VAL("is_kernel_module",
+ (!!is_kernel_module(path, cpumode)) == (!!expect));
+ pr_debug("%s (cpumode: %d) - is_kernel_module: %s\n",
+ path, cpumode, expect ? "true" : "false");
+ return 0;
+}
+
#define T(path, an, ae, k, c, n, e) \
TEST_ASSERT_VAL("failed", !test(path, an, ae, k, c, n, e))
+#define M(path, c, e) \
+ TEST_ASSERT_VAL("failed", !test_is_kernel_module(path, c, e))
+
int test__kmod_path__parse(void)
{
/* path alloc_name alloc_ext kmod comp name ext */
@@ -44,30 +56,90 @@ int test__kmod_path__parse(void)
T("/xxxx/xxxx/x-x.ko", false , true , true, false, NULL , NULL);
T("/xxxx/xxxx/x-x.ko", true , false , true, false, "[x_x]", NULL);
T("/xxxx/xxxx/x-x.ko", false , false , true, false, NULL , NULL);
+ M("/xxxx/xxxx/x-x.ko", PERF_RECORD_MISC_CPUMODE_UNKNOWN, true);
+ M("/xxxx/xxxx/x-x.ko", PERF_RECORD_MISC_KERNEL, true);
+ M("/xxxx/xxxx/x-x.ko", PERF_RECORD_MISC_USER, false);
/* path alloc_name alloc_ext kmod comp name ext */
T("/xxxx/xxxx/x.ko.gz", true , true , true, true, "[x]", "gz");
T("/xxxx/xxxx/x.ko.gz", false , true , true, true, NULL , "gz");
T("/xxxx/xxxx/x.ko.gz", true , false , true, true, "[x]", NULL);
T("/xxxx/xxxx/x.ko.gz", false , false , true, true, NULL , NULL);
+ M("/xxxx/xxxx/x.ko.gz", PERF_RECORD_MISC_CPUMODE_UNKNOWN, true);
+ M("/xxxx/xxxx/x.ko.gz", PERF_RECORD_MISC_KERNEL, true);
+ M("/xxxx/xxxx/x.ko.gz", PERF_RECORD_MISC_USER, false);
/* path alloc_name alloc_ext kmod comp name ext */
T("/xxxx/xxxx/x.gz", true , true , false, true, "x.gz" ,"gz");
T("/xxxx/xxxx/x.gz", false , true , false, true, NULL ,"gz");
T("/xxxx/xxxx/x.gz", true , false , false, true, "x.gz" , NULL);
T("/xxxx/xxxx/x.gz", false , false , false, true, NULL , NULL);
+ M("/xxxx/xxxx/x.gz", PERF_RECORD_MISC_CPUMODE_UNKNOWN, false);
+ M("/xxxx/xxxx/x.gz", PERF_RECORD_MISC_KERNEL, false);
+ M("/xxxx/xxxx/x.gz", PERF_RECORD_MISC_USER, false);
/* path alloc_name alloc_ext kmod comp name ext */
T("x.gz", true , true , false, true, "x.gz", "gz");
T("x.gz", false , true , false, true, NULL , "gz");
T("x.gz", true , false , false, true, "x.gz", NULL);
T("x.gz", false , false , false, true, NULL , NULL);
+ M("x.gz", PERF_RECORD_MISC_CPUMODE_UNKNOWN, false);
+ M("x.gz", PERF_RECORD_MISC_KERNEL, false);
+ M("x.gz", PERF_RECORD_MISC_USER, false);
/* path alloc_name alloc_ext kmod comp name ext */
T("x.ko.gz", true , true , true, true, "[x]", "gz");
T("x.ko.gz", false , true , true, true, NULL , "gz");
T("x.ko.gz", true , false , true, true, "[x]", NULL);
T("x.ko.gz", false , false , true, true, NULL , NULL);
+ M("x.ko.gz", PERF_RECORD_MISC_CPUMODE_UNKNOWN, true);
+ M("x.ko.gz", PERF_RECORD_MISC_KERNEL, true);
+ M("x.ko.gz", PERF_RECORD_MISC_USER, false);
+
+ /* path alloc_name alloc_ext kmod comp name ext */
+ T("[test_module]", true , true , true, false, "[test_module]", NULL);
+ T("[test_module]", false , true , true, false, NULL , NULL);
+ T("[test_module]", true , false , true, false, "[test_module]", NULL);
+ T("[test_module]", false , false , true, false, NULL , NULL);
+ M("[test_module]", PERF_RECORD_MISC_CPUMODE_UNKNOWN, true);
+ M("[test_module]", PERF_RECORD_MISC_KERNEL, true);
+ M("[test_module]", PERF_RECORD_MISC_USER, false);
+
+ /* path alloc_name alloc_ext kmod comp name ext */
+ T("[test.module]", true , true , true, false, "[test.module]", NULL);
+ T("[test.module]", false , true , true, false, NULL , NULL);
+ T("[test.module]", true , false , true, false, "[test.module]", NULL);
+ T("[test.module]", false , false , true, false, NULL , NULL);
+ M("[test.module]", PERF_RECORD_MISC_CPUMODE_UNKNOWN, true);
+ M("[test.module]", PERF_RECORD_MISC_KERNEL, true);
+ M("[test.module]", PERF_RECORD_MISC_USER, false);
+
+ /* path alloc_name alloc_ext kmod comp name ext */
+ T("[vdso]", true , true , false, false, "[vdso]", NULL);
+ T("[vdso]", false , true , false, false, NULL , NULL);
+ T("[vdso]", true , false , false, false, "[vdso]", NULL);
+ T("[vdso]", false , false , false, false, NULL , NULL);
+ M("[vdso]", PERF_RECORD_MISC_CPUMODE_UNKNOWN, false);
+ M("[vdso]", PERF_RECORD_MISC_KERNEL, false);
+ M("[vdso]", PERF_RECORD_MISC_USER, false);
+
+ /* path alloc_name alloc_ext kmod comp name ext */
+ T("[vsyscall]", true , true , false, false, "[vsyscall]", NULL);
+ T("[vsyscall]", false , true , false, false, NULL , NULL);
+ T("[vsyscall]", true , false , false, false, "[vsyscall]", NULL);
+ T("[vsyscall]", false , false , false, false, NULL , NULL);
+ M("[vsyscall]", PERF_RECORD_MISC_CPUMODE_UNKNOWN, false);
+ M("[vsyscall]", PERF_RECORD_MISC_KERNEL, false);
+ M("[vsyscall]", PERF_RECORD_MISC_USER, false);
+
+ /* path alloc_name alloc_ext kmod comp name ext */
+ T("[kernel.kallsyms]", true , true , false, false, "[kernel.kallsyms]", NULL);
+ T("[kernel.kallsyms]", false , true , false, false, NULL , NULL);
+ T("[kernel.kallsyms]", true , false , false, false, "[kernel.kallsyms]", NULL);
+ T("[kernel.kallsyms]", false , false , false, false, NULL , NULL);
+ M("[kernel.kallsyms]", PERF_RECORD_MISC_CPUMODE_UNKNOWN, false);
+ M("[kernel.kallsyms]", PERF_RECORD_MISC_KERNEL, false);
+ M("[kernel.kallsyms]", PERF_RECORD_MISC_USER, false);
return 0;
}
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index fc0ddd5..e9d4ae4 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -165,13 +165,25 @@ bool is_supported_compression(const char *ext)
return false;
}
-bool is_kernel_module(const char *pathname)
+bool is_kernel_module(const char *pathname, int cpumode)
{
struct kmod_path m;
- if (kmod_path__parse(&m, pathname))
- return NULL;
+ /* caller should pass a masked cpumode. Mask again for safety. */
+ switch (cpumode & PERF_RECORD_MISC_CPUMODE_MASK) {
+ case PERF_RECORD_MISC_USER:
+ case PERF_RECORD_MISC_HYPERVISOR:
+ case PERF_RECORD_MISC_GUEST_USER:
+ return false;
+ /* Regard PERF_RECORD_MISC_CPUMODE_UNKNOWN as kernel */
+ default:
+ if (kmod_path__parse(&m, pathname)) {
+ pr_err("Failed to check whether %s is a kernel module or not. Assume it is.",
+ pathname);
+ return true;
+ }
+ }
return m.kmod;
}
@@ -214,12 +226,34 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
{
const char *name = strrchr(path, '/');
const char *ext = strrchr(path, '.');
+ bool is_simple_name = false;
memset(m, 0x0, sizeof(*m));
name = name ? name + 1 : path;
+ /*
+ * '.' is also a valid character for module name. For example:
+ * [aaa.bbb] is a valid module name. '[' should have higher
+ * priority than '.ko' suffix.
+ *
+ * The kernel names are from machine__mmap_name. Such
+ * name should belong to kernel itself, not kernel module.
+ */
+ if (name[0] == '[') {
+ is_simple_name = true;
+ if ((strncmp(name, "[kernel.kallsyms]", 17) == 0) ||
+ (strncmp(name, "[guest.kernel.kallsyms", 22) == 0) ||
+ (strncmp(name, "[vdso]", 6) == 0) ||
+ (strncmp(name, "[vsyscall]", 10) == 0)) {
+ m->kmod = false;
+
+ } else
+ m->kmod = true;
+ }
+
+
/* No extension, just return name. */
- if (ext == NULL) {
+ if ((ext == NULL) || is_simple_name) {
if (alloc_name) {
m->name = strdup(name);
return m->name ? 0 : -ENOMEM;
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index e0901b4..cc3797c 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -216,7 +216,7 @@ char dso__symtab_origin(const struct dso *dso);
int dso__read_binary_type_filename(const struct dso *dso, enum dso_binary_type type,
char *root_dir, char *filename, size_t size);
bool is_supported_compression(const char *ext);
-bool is_kernel_module(const char *pathname);
+bool is_kernel_module(const char *pathname, int cpumode);
bool decompress_to_file(const char *ext, const char *filename, int output_fd);
bool dso__needs_decompress(struct dso *dso);
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 918fd8a..caeea35 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1220,7 +1220,7 @@ static int __event_process_build_id(struct build_id_event *bev,
int err = -1;
struct dsos *dsos;
struct machine *machine;
- u16 misc;
+ u16 cpumode;
struct dso *dso;
enum dso_kernel_type dso_type;
@@ -1228,9 +1228,9 @@ static int __event_process_build_id(struct build_id_event *bev,
if (!machine)
goto out;
- misc = bev->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
+ cpumode = bev->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
- switch (misc) {
+ switch (cpumode) {
case PERF_RECORD_MISC_KERNEL:
dso_type = DSO_TYPE_KERNEL;
dsos = &machine->kernel_dsos;
@@ -1254,7 +1254,7 @@ static int __event_process_build_id(struct build_id_event *bev,
dso__set_build_id(dso, &bev->build_id);
- if (!is_kernel_module(filename))
+ if (!is_kernel_module(filename, cpumode))
dso->kernel = dso_type;
build_id__sprintf(dso->build_id, sizeof(dso->build_id),
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 527e032..fe56c9db 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1110,7 +1110,21 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
struct dso *dso;
list_for_each_entry(dso, &machine->kernel_dsos.head, node) {
- if (is_kernel_module(dso->long_name))
+ /*
+ * cpumode passed to is_kernel_module is not the
+ * cpumode of *this* event. If we insist on passing
+ * correct cpumode to is_kernel_module, we should record
+ * the cpumode when we adding this dso to the linked list.
+ *
+ * However we don't really need passing correct cpumode.
+ * We know the correct cpumode must be kernel mode
+ * (if not, we should not link it onto kernel_dsos list).
+ *
+ * Therefore, we pass PERF_RECORD_MISC_CPUMODE_UNKNOWN.
+ * is_kernel_module() treats it as a kernel cpumode.
+ */
+ if (is_kernel_module(dso->long_name,
+ PERF_RECORD_MISC_CPUMODE_UNKNOWN))
continue;
kernel = dso;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/