All of lore.kernel.org
 help / color / mirror / Atom feed
From: Wang Nan <wangnan0@huawei.com>
To: <acme@kernel.org>, <jolsa@kernel.org>, <namhyung@kernel.org>,
	<mingo@redhat.com>
Cc: <lizefan@huawei.com>, <pi3orama@163.com>, <linux-kernel@vger.kernel.org>
Subject: [PATCH v3 2/2] perf: report/annotate: fix segfault problem.
Date: Tue, 7 Apr 2015 08:22:46 +0000	[thread overview]
Message-ID: <1428394966-131044-3-git-send-email-wangnan0@huawei.com> (raw)
In-Reply-To: <1428394966-131044-1-git-send-email-wangnan0@huawei.com>

perf report and perf annotate are easy to trigger segfault if trace data
contain kernel module information like this:

 # perf report -D -i ./perf.data
 ...
 0 0 0x188 [0x50]: PERF_RECORD_MMAP -1/0: [0xffffffbff1018000(0xf068000) @ 0]: x [test_module]
 ...

 # perf report -i ./perf.data --objdump=/path/to/objdump --kallsyms=/path/to/kallsyms

 perf: Segmentation fault
 -------- backtrace --------
 /path/to/perf[0x503478]
 /lib64/libc.so.6(+0x3545f)[0x7fb201f3745f]
 /path/to/perf[0x499b56]
 /path/to/perf(dso__load_kallsyms+0x13c)[0x49b56c]
 /path/to/perf(dso__load+0x72e)[0x49c21e]
 /path/to/perf(map__load+0x6e)[0x4ae9ee]
 /path/to/perf(thread__find_addr_map+0x24c)[0x47deec]
 /path/to/perf(perf_event__preprocess_sample+0x88)[0x47e238]
 /path/to/perf[0x43ad02]
 /path/to/perf[0x4b55bc]
 /path/to/perf(ordered_events__flush+0xca)[0x4b57ea]
 /path/to/perf[0x4b1a01]
 /path/to/perf(perf_session__process_events+0x3be)[0x4b428e]
 /path/to/perf(cmd_report+0xf11)[0x43bfc1]
 /path/to/perf[0x474702]
 /path/to/perf(main+0x5f5)[0x42de95]
 /lib64/libc.so.6(__libc_start_main+0xf4)[0x7fb201f23bd4]
 /path/to/perf[0x42dfc4]

This is because __kmod_path__parse regard '[' leading name as kernel
instead of kernel module. The DSO will then be passed to
dso__load_kernel_sym() then dso__load_kcore() because of --kallsyms
argument. The segfault is triggered because the kmap structure is not
initialized.

Although in --vmlinux case such segfault can be avoided, the symbols in
the kernel module are unable to be retrived since the attribute of DSO
is incorrect.

This patch fixes __kmod_path__parse, make it regard names like
'[test_module]' as kernel module.

According to suggestion from Arnaldo Carvalho de Melo (
https://lkml.org/lkml/2015/4/6/90), appends cpumode argument to
__kmod_path__parse() to ensure the passed path is a kernel mmap.

kmod-path.c is also update to reflect the above changes.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 tools/perf/tests/kmod-path.c | 43 ++++++++++++++++++++++++++++++++++----
 tools/perf/util/dso.c        | 49 ++++++++++++++++++++++++++++++++++++++------
 tools/perf/util/dso.h        | 12 ++++++-----
 tools/perf/util/header.c     |  2 +-
 tools/perf/util/machine.c    |  4 +++-
 5 files changed, 93 insertions(+), 17 deletions(-)

diff --git a/tools/perf/tests/kmod-path.c b/tools/perf/tests/kmod-path.c
index e8d7cbb..5d57df6 100644
--- a/tools/perf/tests/kmod-path.c
+++ b/tools/perf/tests/kmod-path.c
@@ -4,14 +4,14 @@
 #include "debug.h"
 
 static int test(const char *path, bool alloc_name, bool alloc_ext,
-		bool kmod, bool comp, const char *name, const char *ext)
+		bool kmod, bool comp, const char *name, const char *ext, int cpumode)
 {
 	struct kmod_path m;
 
 	memset(&m, 0x0, sizeof(m));
 
 	TEST_ASSERT_VAL("kmod_path__parse",
-			!__kmod_path__parse(&m, path, alloc_name, alloc_ext));
+			!__kmod_path__parse(&m, path, cpumode, alloc_name, alloc_ext));
 
 	pr_debug("%s - alloc name %d, alloc ext %d, kmod %d, comp %d, name '%s', ext '%s'\n",
 		 path, alloc_name, alloc_ext, m.kmod, m.comp, m.name, m.ext);
@@ -34,8 +34,14 @@ static int test(const char *path, bool alloc_name, bool alloc_ext,
 	return 0;
 }
 
-#define T(path, an, ae, k, c, n, e) \
-	TEST_ASSERT_VAL("failed", !test(path, an, ae, k, c, n, e))
+
+#define _T(path, an, ae, k, c, n, e, m) \
+	TEST_ASSERT_VAL("failed", !test(path, an, ae, k, c, n, e, m))
+
+#define T(path, an, ae, k, c, n, e) _T(path, an, ae, k, c, n, e, \
+		PERF_RECORD_MISC_CPUMODE_UNKNOWN)
+#define TU(path, an, ae, k, c, n, e) _T(path, an, ae, k, c, n, e, \
+		PERF_RECORD_MISC_USER)
 
 int test__kmod_path__parse(void)
 {
@@ -69,5 +75,34 @@ int test__kmod_path__parse(void)
 	T("x.ko.gz", true      , false    , true, true, "[x]", NULL);
 	T("x.ko.gz", false     , false    , true, true, NULL , NULL);
 
+	/* 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);
+
+	/* 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);
+
+	/* path     alloc_name     alloc_ext kmod  comp   name       ext */
+	TU("[vdso]", true         , true    , false, false, "[vdso]" , NULL);
+	TU("[vdso]", false        , true    , false, false, NULL     , NULL);
+	TU("[vdso]", true         , false   , false, false, "[vdso]" , NULL);
+	TU("[vdso]", false        , false   , false, false, NULL     , NULL);
+
+	/* path     alloc_name     alloc_ext kmod  comp   name           ext */
+	TU("[vsyscall]", true     , true    , false, false, "[vsyscall]" , NULL);
+	TU("[vsyscall]", false    , true    , false, false, NULL         , NULL);
+	TU("[vsyscall]", true     , false   , false, false, "[vsyscall]" , NULL);
+	TU("[vsyscall]", false    , false   , false, false, NULL         , NULL);
+
+	/* 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);
 	return 0;
 }
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index fc0ddd5..fb1ed7d 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -165,11 +165,11 @@ 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))
+	if (kmod_path__parse(&m, pathname, cpumode))
 		return NULL;
 
 	return m.kmod;
@@ -210,21 +210,53 @@ bool dso__needs_decompress(struct dso *dso)
  * Returns 0 if there's no strdup error, -ENOMEM otherwise.
  */
 int __kmod_path__parse(struct kmod_path *m, const char *path,
-		       bool alloc_name, bool alloc_ext)
+		       int cpumode, bool alloc_name, bool alloc_ext)
 {
 	const char *name = strrchr(path, '/');
 	const char *ext  = strrchr(path, '.');
+	bool is_simple_name = false;
+	bool cpu_mode_kernel, is_kernel = false;
+
+	/* treat PERF_RECORD_MISC_CPUMODE_UNKNOWN as kernel. */
+	switch (cpumode & PERF_RECORD_MISC_CPUMODE_MASK) {
+	case PERF_RECORD_MISC_USER:
+	case PERF_RECORD_MISC_HYPERVISOR:
+	case PERF_RECORD_MISC_GUEST_USER:
+		cpu_mode_kernel = false;
+		break;
+	default:
+		cpu_mode_kernel = true;
+	}
 
 	memset(m, 0x0, sizeof(*m));
 	name = name ? name + 1 : path;
 
+	/*
+	 * '.' is also a valid character. 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)) {
+			m->kmod = false;
+			is_kernel = true;
+		} else
+			m->kmod = cpu_mode_kernel;
+	}
+
 	/* 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;
+			if (!m->name)
+				return -ENOMEM;
 		}
-		return 0;
+		goto out;
 	}
 
 	if (is_supported_compression(ext + 1)) {
@@ -255,6 +287,11 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
 			return -ENOMEM;
 		}
 	}
+out:
+	if ((m->kmod && !cpu_mode_kernel) ||
+			(cpu_mode_kernel && !m->kmod && !is_kernel))
+		pr_warning("Kmod (%s) and cpumode (%d) inconsistent\n",
+				path, cpumode);
 
 	return 0;
 }
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index e0901b4..0079b2b 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);
 
@@ -228,11 +228,13 @@ struct kmod_path {
 };
 
 int __kmod_path__parse(struct kmod_path *m, const char *path,
-		     bool alloc_name, bool alloc_ext);
+		     int cpumode, bool alloc_name, bool alloc_ext);
 
-#define kmod_path__parse(__m, __p)      __kmod_path__parse(__m, __p, false, false)
-#define kmod_path__parse_name(__m, __p) __kmod_path__parse(__m, __p, true , false)
-#define kmod_path__parse_ext(__m, __p)  __kmod_path__parse(__m, __p, false, true)
+#define kmod_path__parse(__m, __p, __c)      __kmod_path__parse(__m, __p, __c, false, false)
+#define kmod_path__parse_name(__m, __p) __kmod_path__parse(__m, __p, \
+		PERF_RECORD_MISC_CPUMODE_UNKNOWN, true , false)
+#define kmod_path__parse_ext(__m, __p)       __kmod_path__parse(__m, __p, \
+		PERF_RECORD_MISC_CPUMODE_UNKNOWN, false, true)
 
 /*
  * The dso__data_* external interface provides following functions:
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index fb43215..d106e12 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1266,7 +1266,7 @@ static int __event_process_build_id(struct build_id_event *bev,
 
 		dso__set_build_id(dso, &bev->build_id);
 
-		if (!is_kernel_module(filename))
+		if (!is_kernel_module(filename, misc))
 			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 9c7feec..c9e7e57 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1112,7 +1112,9 @@ 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))
+			if (is_kernel_module(dso->long_name,
+					     event->header.misc &
+					     PERF_RECORD_MISC_CPUMODE_MASK))
 				continue;
 
 			kernel = dso;
-- 
1.8.3.4


  parent reply	other threads:[~2015-04-07  8:23 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-04-03  5:56 [PATCH v2] perf: report/annotate: fix segfault problem Wang Nan
2015-04-03  6:48 ` Ingo Molnar
2015-04-03  8:47   ` [PATCH] perf: kmaps: enforce usage of kmaps to protect futher bugs Wang Nan
2015-04-03  8:49     ` Ingo Molnar
2015-04-03 11:11     ` Jiri Olsa
2015-04-07 10:42       ` Adrian Hunter
2015-04-08 10:50         ` [PATCH v4] " Wang Nan
2015-04-03  9:07 ` [PATCH v2] perf: report/annotate: fix segfault problem Jiri Olsa
2015-04-03 10:57 ` Jiri Olsa
2015-04-06 12:52   ` Arnaldo Carvalho de Melo
2015-04-07  8:22     ` [PATCH v3 0/2] " Wang Nan
2015-04-07  8:22       ` [PATCH v3 1/2] perf: kmaps: enforce usage of kmaps to protect futher bugs Wang Nan
2015-04-08 15:10         ` [tip:perf/core] perf kmaps: Check kmaps to make code more robust tip-bot for Wang Nan
2015-04-07  8:22       ` Wang Nan [this message]
2015-04-07 15:13         ` [PATCH v3 2/2] perf: report/annotate: fix segfault problem Arnaldo Carvalho de Melo
2015-04-08  3:49           ` Wang Nan
2015-04-08  3:52           ` [PATCH v4 " Wang Nan
2015-04-08 13:59             ` Jiri Olsa
2015-04-09  7:05               ` Wang Nan
2015-04-09 11:40                 ` Jiri Olsa
2015-04-09 11:52             ` Jiri Olsa
2015-04-10  1:57               ` Wang Nan
2015-04-10  2:49               ` Wang Nan
2015-04-10  3:53               ` [PATCH v5 " Wang Nan
2015-04-15  1:27                 ` Wang Nan
2015-04-20  1:18                   ` Wang Nan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1428394966-131044-3-git-send-email-wangnan0@huawei.com \
    --to=wangnan0@huawei.com \
    --cc=acme@kernel.org \
    --cc=jolsa@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lizefan@huawei.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=pi3orama@163.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.