[PATCH v2] perf: report/annotate: fix segfault problem.

[PATCH v2] perf: report/annotate: fix segfault problem.

[Wang Nan]

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.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---

Patch v1 doesn't consider module named as [aaa.bbb].

---
 tools/perf/util/dso.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index fc0ddd5..08d7aaf 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -214,12 +214,23 @@ 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 example: [aaa.bbb] is a
+	 * valid module name. '[' should have higher priority than
+	 * '.ko' suffix.
+	 */
+	if ((name[0] == '[') &&	(strncmp(name, "[vdso]", 6) != 0)) {
+		m->kmod = true;
+		is_simple_name = 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;
-- 
1.8.3.4

Re: [PATCH v2] perf: report/annotate: fix segfault problem.

[Ingo Molnar]

* Wang Nan <wangnan0@huawei.com> wrote:

> 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.

Could we also make the place that generated the segfault more robust, 
to protect against future mishaps of this type? I suppose the 
non-initialized value was NULL?

Thanks,

	Ingo

[PATCH] perf: kmaps: enforce usage of kmaps to protect futher bugs.

[Wang Nan]

This patch add checkings on every place where uses map__kmap and get
kmaps from struct kmap. Error messages are added at map__kmap to warn
invalud accessing of kmap (for the case of !map->dso->kernel, kmap(map)
is not exists at all). Also, introduces map__kmaps() to warn
uninitialized kmaps.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 tools/perf/util/machine.c     |  5 ++++-
 tools/perf/util/map.h         | 15 +++++++++++++++
 tools/perf/util/probe-event.c |  2 ++
 tools/perf/util/session.c     |  3 +++
 tools/perf/util/symbol-elf.c  | 16 +++++++++++-----
 tools/perf/util/symbol.c      | 34 ++++++++++++++++++++++++++++------
 6 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index e335330..9c7feec 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -679,6 +679,9 @@ int __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 			machine->vmlinux_maps[type]->unmap_ip =
 				identity__map_ip;
 		kmap = map__kmap(machine->vmlinux_maps[type]);
+		if (!kmap)
+			return -1;
+
 		kmap->kmaps = &machine->kmaps;
 		map_groups__insert(&machine->kmaps,
 				   machine->vmlinux_maps[type]);
@@ -700,7 +703,7 @@ void machine__destroy_kernel_maps(struct machine *machine)
 		kmap = map__kmap(machine->vmlinux_maps[type]);
 		map_groups__remove(&machine->kmaps,
 				   machine->vmlinux_maps[type]);
-		if (kmap->ref_reloc_sym) {
+		if (kmap && kmap->ref_reloc_sym) {
 			/*
 			 * ref_reloc_sym is shared among all maps, so free just
 			 * on one of them.
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 0e42438..2ba83c8 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -78,9 +78,24 @@ void map_groups__put(struct map_groups *mg);
 
 static inline struct kmap *map__kmap(struct map *map)
 {
+	if (!map->dso || !map->dso->kernel) {
+		pr_err("Internal error: map__kmap with a non-kernel map\n");
+		return NULL;
+	}
 	return (struct kmap *)(map + 1);
 }
 
+static inline struct map_groups *kmaps map__kmaps(struct map *map)
+{
+	struct kmap *kmap = map__kmap(map);
+
+	if (!kmap || !kmap->kmaps) {
+		pr_err("Internal error: map__kmaps with a non-kernel map\n");
+		return NULL;
+	}
+	return kmap->kmaps;
+}
+
 static inline u64 map__map_ip(struct map *map, u64 ip)
 {
 	return ip - map->start + map->pgoff;
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 8feac07..4fd49f0 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -135,6 +135,8 @@ static struct ref_reloc_sym *kernel_get_ref_reloc_sym(void)
 		return NULL;
 
 	kmap = map__kmap(host_machine->vmlinux_maps[MAP__FUNCTION]);
+	if (!kmap)
+		return NULL;
 	return kmap->ref_reloc_sym;
 }
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index adf0740..7caedb4 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1456,6 +1456,9 @@ int maps__set_kallsyms_ref_reloc_sym(struct map **maps,
 
 	for (i = 0; i < MAP__NR_TYPES; ++i) {
 		struct kmap *kmap = map__kmap(maps[i]);
+
+		if (!kmap)
+			continue;
 		kmap->ref_reloc_sym = ref;
 	}
 
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 476268c..a7ab606 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -776,6 +776,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 		  symbol_filter_t filter, int kmodule)
 {
 	struct kmap *kmap = dso->kernel ? map__kmap(map) : NULL;
+	struct map_groups *kmaps = kmap ? map__kmaps(map) : NULL;
 	struct map *curr_map = map;
 	struct dso *curr_dso = dso;
 	Elf_Data *symstrs, *secstrs;
@@ -791,6 +792,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
 	int nr = 0;
 	bool remap_kernel = false, adjust_kernel_syms = false;
 
+	if (kmap && !kmaps)
+		return -1;
+
 	dso->symtab_type = syms_ss->type;
 	dso->is_64_bit = syms_ss->is_64_bit;
 	dso->rel = syms_ss->ehdr.e_type == ET_REL;
@@ -958,8 +962,10 @@ int dso__load_sym(struct dso *dso, struct map *map,
 					map->map_ip = map__map_ip;
 					map->unmap_ip = map__unmap_ip;
 					/* Ensure maps are correctly ordered */
-					map_groups__remove(kmap->kmaps, map);
-					map_groups__insert(kmap->kmaps, map);
+					if (kmaps) {
+						map_groups__remove(kmaps, map);
+						map_groups__insert(kmaps, map);
+					}
 				}
 
 				/*
@@ -983,7 +989,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 			snprintf(dso_name, sizeof(dso_name),
 				 "%s%s", dso->short_name, section_name);
 
-			curr_map = map_groups__find_by_name(kmap->kmaps, map->type, dso_name);
+			curr_map = map_groups__find_by_name(kmaps, map->type, dso_name);
 			if (curr_map == NULL) {
 				u64 start = sym.st_value;
 
@@ -1013,7 +1019,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 					curr_map->unmap_ip = identity__map_ip;
 				}
 				curr_dso->symtab_type = dso->symtab_type;
-				map_groups__insert(kmap->kmaps, curr_map);
+				map_groups__insert(kmaps, curr_map);
 				/*
 				 * The new DSO should go to the kernel DSOS
 				 */
@@ -1075,7 +1081,7 @@ new_symbol:
 			 * We need to fixup this here too because we create new
 			 * maps here, for things like vsyscall sections.
 			 */
-			__map_groups__fixup_end(kmap->kmaps, map->type);
+			__map_groups__fixup_end(kmaps, map->type);
 		}
 	}
 	err = nr;
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index fddeb90..201f6c4c 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -630,13 +630,16 @@ static int dso__load_all_kallsyms(struct dso *dso, const char *filename,
 static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map,
 					 symbol_filter_t filter)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
+	struct map_groups *kmaps = map__kmaps(map);
 	struct map *curr_map;
 	struct symbol *pos;
 	int count = 0, moved = 0;
 	struct rb_root *root = &dso->symbols[map->type];
 	struct rb_node *next = rb_first(root);
 
+	if (!kmaps)
+		return -1;
+
 	while (next) {
 		char *module;
 
@@ -682,8 +685,8 @@ static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map,
 static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta,
 			       symbol_filter_t filter)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
-	struct machine *machine = kmaps->machine;
+	struct map_groups *kmaps = map__kmaps(map);
+	struct machine *machine;
 	struct map *curr_map = map;
 	struct symbol *pos;
 	int count = 0, moved = 0;
@@ -691,6 +694,11 @@ static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta,
 	struct rb_node *next = rb_first(root);
 	int kernel_range = 0;
 
+	if (!kmaps)
+		return -1;
+
+	machine = kmaps->machine;
+
 	while (next) {
 		char *module;
 
@@ -1025,9 +1033,12 @@ static bool filename_from_kallsyms_filename(char *filename,
 static int validate_kcore_modules(const char *kallsyms_filename,
 				  struct map *map)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
+	struct map_groups *kmaps = map__kmaps(map);
 	char modules_filename[PATH_MAX];
 
+	if (!kmaps)
+		return -EINVAL;
+
 	if (!filename_from_kallsyms_filename(modules_filename, "modules",
 					     kallsyms_filename))
 		return -EINVAL;
@@ -1043,6 +1054,9 @@ static int validate_kcore_addresses(const char *kallsyms_filename,
 {
 	struct kmap *kmap = map__kmap(map);
 
+	if (!kmap)
+		return -EINVAL;
+
 	if (kmap->ref_reloc_sym && kmap->ref_reloc_sym->name) {
 		u64 start;
 
@@ -1081,8 +1095,8 @@ static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
 static int dso__load_kcore(struct dso *dso, struct map *map,
 			   const char *kallsyms_filename)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
-	struct machine *machine = kmaps->machine;
+	struct map_groups *kmaps = map__kmaps(map);
+	struct machine *machine;
 	struct kcore_mapfn_data md;
 	struct map *old_map, *new_map, *replacement_map = NULL;
 	bool is_64_bit;
@@ -1090,6 +1104,11 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 	char kcore_filename[PATH_MAX];
 	struct symbol *sym;
 
+	if (!kmaps)
+		return -EINVAL;
+
+	machine = kmaps->machine;
+
 	/* This function requires that the map is the kernel map */
 	if (map != machine->vmlinux_maps[map->type])
 		return -EINVAL;
@@ -1202,6 +1221,9 @@ static int kallsyms__delta(struct map *map, const char *filename, u64 *delta)
 	struct kmap *kmap = map__kmap(map);
 	u64 addr;
 
+	if (!kmap)
+		return -1;
+
 	if (!kmap->ref_reloc_sym || !kmap->ref_reloc_sym->name)
 		return 0;
 
-- 
1.8.3.4

Re: [PATCH] perf: kmaps: enforce usage of kmaps to protect futher bugs.

[Ingo Molnar]

* Wang Nan <wangnan0@huawei.com> wrote:

> This patch add checkings on every place where uses map__kmap and get
> kmaps from struct kmap. Error messages are added at map__kmap to warn
> invalud accessing of kmap (for the case of !map->dso->kernel, kmap(map)
> is not exists at all). Also, introduces map__kmaps() to warn
> uninitialized kmaps.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH v2] perf: report/annotate: fix segfault problem.

[Jiri Olsa]

On Fri, Apr 03, 2015 at 05:56:25AM +0000, Wang Nan wrote:
> 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.

hum, I need to go through that backtrace first.. but could you also
update the tests/kmod-path.c, so it's obvious what's the output of
__kmod_path__parse for [test_module]

thanks,
jirka

Re: [PATCH v2] perf: report/annotate: fix segfault problem.

[Jiri Olsa]

On Fri, Apr 03, 2015 at 05:56:25AM +0000, Wang Nan wrote:
> 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]

oops, I was wondering how you'd get such a MMAP record, because we lookup
modules real paths and store it.. I haven't realized that if we dont find
it we keep the '[mod]' name :-\

SNIP

> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
> 
> Patch v1 doesn't consider module named as [aaa.bbb].
> 
> ---
>  tools/perf/util/dso.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index fc0ddd5..08d7aaf 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -214,12 +214,23 @@ 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 example: [aaa.bbb] is a
> +	 * valid module name. '[' should have higher priority than
> +	 * '.ko' suffix.
> +	 */


> +	if ((name[0] == '[') &&	(strncmp(name, "[vdso]", 6) != 0)) {

maybe also check for '[vsyscall]' ?

> +		m->kmod = true;
> +		is_simple_name = 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;

otherwise it looks ok to me.. please update also the tests/kmod-path.c ;-)

thanks,
jirka

Re: [PATCH] perf: kmaps: enforce usage of kmaps to protect futher bugs.

[Jiri Olsa]

On Fri, Apr 03, 2015 at 08:47:57AM +0000, Wang Nan wrote:
> This patch add checkings on every place where uses map__kmap and get
> kmaps from struct kmap. Error messages are added at map__kmap to warn
> invalud accessing of kmap (for the case of !map->dso->kernel, kmap(map)
> is not exists at all). Also, introduces map__kmaps() to warn
> uninitialized kmaps.

looks ok, CC-ing Adrian

thanks,
jirka

> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  tools/perf/util/machine.c     |  5 ++++-
>  tools/perf/util/map.h         | 15 +++++++++++++++
>  tools/perf/util/probe-event.c |  2 ++
>  tools/perf/util/session.c     |  3 +++
>  tools/perf/util/symbol-elf.c  | 16 +++++++++++-----
>  tools/perf/util/symbol.c      | 34 ++++++++++++++++++++++++++++------
>  6 files changed, 63 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index e335330..9c7feec 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -679,6 +679,9 @@ int __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
>  			machine->vmlinux_maps[type]->unmap_ip =
>  				identity__map_ip;
>  		kmap = map__kmap(machine->vmlinux_maps[type]);
> +		if (!kmap)
> +			return -1;
> +
>  		kmap->kmaps = &machine->kmaps;
>  		map_groups__insert(&machine->kmaps,
>  				   machine->vmlinux_maps[type]);
> @@ -700,7 +703,7 @@ void machine__destroy_kernel_maps(struct machine *machine)
>  		kmap = map__kmap(machine->vmlinux_maps[type]);
>  		map_groups__remove(&machine->kmaps,
>  				   machine->vmlinux_maps[type]);
> -		if (kmap->ref_reloc_sym) {
> +		if (kmap && kmap->ref_reloc_sym) {
>  			/*
>  			 * ref_reloc_sym is shared among all maps, so free just
>  			 * on one of them.
> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
> index 0e42438..2ba83c8 100644
> --- a/tools/perf/util/map.h
> +++ b/tools/perf/util/map.h
> @@ -78,9 +78,24 @@ void map_groups__put(struct map_groups *mg);
>  
>  static inline struct kmap *map__kmap(struct map *map)
>  {
> +	if (!map->dso || !map->dso->kernel) {
> +		pr_err("Internal error: map__kmap with a non-kernel map\n");
> +		return NULL;
> +	}
>  	return (struct kmap *)(map + 1);
>  }
>  
> +static inline struct map_groups *kmaps map__kmaps(struct map *map)
> +{
> +	struct kmap *kmap = map__kmap(map);
> +
> +	if (!kmap || !kmap->kmaps) {
> +		pr_err("Internal error: map__kmaps with a non-kernel map\n");
> +		return NULL;
> +	}
> +	return kmap->kmaps;
> +}
> +
>  static inline u64 map__map_ip(struct map *map, u64 ip)
>  {
>  	return ip - map->start + map->pgoff;
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 8feac07..4fd49f0 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -135,6 +135,8 @@ static struct ref_reloc_sym *kernel_get_ref_reloc_sym(void)
>  		return NULL;
>  
>  	kmap = map__kmap(host_machine->vmlinux_maps[MAP__FUNCTION]);
> +	if (!kmap)
> +		return NULL;
>  	return kmap->ref_reloc_sym;
>  }
>  
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index adf0740..7caedb4 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1456,6 +1456,9 @@ int maps__set_kallsyms_ref_reloc_sym(struct map **maps,
>  
>  	for (i = 0; i < MAP__NR_TYPES; ++i) {
>  		struct kmap *kmap = map__kmap(maps[i]);
> +
> +		if (!kmap)
> +			continue;
>  		kmap->ref_reloc_sym = ref;
>  	}
>  
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 476268c..a7ab606 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -776,6 +776,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
>  		  symbol_filter_t filter, int kmodule)
>  {
>  	struct kmap *kmap = dso->kernel ? map__kmap(map) : NULL;
> +	struct map_groups *kmaps = kmap ? map__kmaps(map) : NULL;
>  	struct map *curr_map = map;
>  	struct dso *curr_dso = dso;
>  	Elf_Data *symstrs, *secstrs;
> @@ -791,6 +792,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
>  	int nr = 0;
>  	bool remap_kernel = false, adjust_kernel_syms = false;
>  
> +	if (kmap && !kmaps)
> +		return -1;
> +
>  	dso->symtab_type = syms_ss->type;
>  	dso->is_64_bit = syms_ss->is_64_bit;
>  	dso->rel = syms_ss->ehdr.e_type == ET_REL;
> @@ -958,8 +962,10 @@ int dso__load_sym(struct dso *dso, struct map *map,
>  					map->map_ip = map__map_ip;
>  					map->unmap_ip = map__unmap_ip;
>  					/* Ensure maps are correctly ordered */
> -					map_groups__remove(kmap->kmaps, map);
> -					map_groups__insert(kmap->kmaps, map);
> +					if (kmaps) {
> +						map_groups__remove(kmaps, map);
> +						map_groups__insert(kmaps, map);
> +					}
>  				}
>  
>  				/*
> @@ -983,7 +989,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
>  			snprintf(dso_name, sizeof(dso_name),
>  				 "%s%s", dso->short_name, section_name);
>  
> -			curr_map = map_groups__find_by_name(kmap->kmaps, map->type, dso_name);
> +			curr_map = map_groups__find_by_name(kmaps, map->type, dso_name);
>  			if (curr_map == NULL) {
>  				u64 start = sym.st_value;
>  
> @@ -1013,7 +1019,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
>  					curr_map->unmap_ip = identity__map_ip;
>  				}
>  				curr_dso->symtab_type = dso->symtab_type;
> -				map_groups__insert(kmap->kmaps, curr_map);
> +				map_groups__insert(kmaps, curr_map);
>  				/*
>  				 * The new DSO should go to the kernel DSOS
>  				 */
> @@ -1075,7 +1081,7 @@ new_symbol:
>  			 * We need to fixup this here too because we create new
>  			 * maps here, for things like vsyscall sections.
>  			 */
> -			__map_groups__fixup_end(kmap->kmaps, map->type);
> +			__map_groups__fixup_end(kmaps, map->type);
>  		}
>  	}
>  	err = nr;
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index fddeb90..201f6c4c 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -630,13 +630,16 @@ static int dso__load_all_kallsyms(struct dso *dso, const char *filename,
>  static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map,
>  					 symbol_filter_t filter)
>  {
> -	struct map_groups *kmaps = map__kmap(map)->kmaps;
> +	struct map_groups *kmaps = map__kmaps(map);
>  	struct map *curr_map;
>  	struct symbol *pos;
>  	int count = 0, moved = 0;
>  	struct rb_root *root = &dso->symbols[map->type];
>  	struct rb_node *next = rb_first(root);
>  
> +	if (!kmaps)
> +		return -1;
> +
>  	while (next) {
>  		char *module;
>  
> @@ -682,8 +685,8 @@ static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map,
>  static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta,
>  			       symbol_filter_t filter)
>  {
> -	struct map_groups *kmaps = map__kmap(map)->kmaps;
> -	struct machine *machine = kmaps->machine;
> +	struct map_groups *kmaps = map__kmaps(map);
> +	struct machine *machine;
>  	struct map *curr_map = map;
>  	struct symbol *pos;
>  	int count = 0, moved = 0;
> @@ -691,6 +694,11 @@ static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta,
>  	struct rb_node *next = rb_first(root);
>  	int kernel_range = 0;
>  
> +	if (!kmaps)
> +		return -1;
> +
> +	machine = kmaps->machine;
> +
>  	while (next) {
>  		char *module;
>  
> @@ -1025,9 +1033,12 @@ static bool filename_from_kallsyms_filename(char *filename,
>  static int validate_kcore_modules(const char *kallsyms_filename,
>  				  struct map *map)
>  {
> -	struct map_groups *kmaps = map__kmap(map)->kmaps;
> +	struct map_groups *kmaps = map__kmaps(map);
>  	char modules_filename[PATH_MAX];
>  
> +	if (!kmaps)
> +		return -EINVAL;
> +
>  	if (!filename_from_kallsyms_filename(modules_filename, "modules",
>  					     kallsyms_filename))
>  		return -EINVAL;
> @@ -1043,6 +1054,9 @@ static int validate_kcore_addresses(const char *kallsyms_filename,
>  {
>  	struct kmap *kmap = map__kmap(map);
>  
> +	if (!kmap)
> +		return -EINVAL;
> +
>  	if (kmap->ref_reloc_sym && kmap->ref_reloc_sym->name) {
>  		u64 start;
>  
> @@ -1081,8 +1095,8 @@ static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
>  static int dso__load_kcore(struct dso *dso, struct map *map,
>  			   const char *kallsyms_filename)
>  {
> -	struct map_groups *kmaps = map__kmap(map)->kmaps;
> -	struct machine *machine = kmaps->machine;
> +	struct map_groups *kmaps = map__kmaps(map);
> +	struct machine *machine;
>  	struct kcore_mapfn_data md;
>  	struct map *old_map, *new_map, *replacement_map = NULL;
>  	bool is_64_bit;
> @@ -1090,6 +1104,11 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>  	char kcore_filename[PATH_MAX];
>  	struct symbol *sym;
>  
> +	if (!kmaps)
> +		return -EINVAL;
> +
> +	machine = kmaps->machine;
> +
>  	/* This function requires that the map is the kernel map */
>  	if (map != machine->vmlinux_maps[map->type])
>  		return -EINVAL;
> @@ -1202,6 +1221,9 @@ static int kallsyms__delta(struct map *map, const char *filename, u64 *delta)
>  	struct kmap *kmap = map__kmap(map);
>  	u64 addr;
>  
> +	if (!kmap)
> +		return -1;
> +
>  	if (!kmap->ref_reloc_sym || !kmap->ref_reloc_sym->name)
>  		return 0;
>  
> -- 
> 1.8.3.4
> 

Re: [PATCH v2] perf: report/annotate: fix segfault problem.

[Arnaldo Carvalho de Melo]

Em Fri, Apr 03, 2015 at 12:57:21PM +0200, Jiri Olsa escreveu:
> On Fri, Apr 03, 2015 at 05:56:25AM +0000, Wang Nan wrote:
> > 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]

> oops, I was wondering how you'd get such a MMAP record, because we lookup
> modules real paths and store it.. I haven't realized that if we dont find
> it we keep the '[mod]' name :-\
 
> SNIP
 
> > Patch v1 doesn't consider module named as [aaa.bbb].

> >  tools/perf/util/dso.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > @@ -214,12 +214,23 @@ 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 example: [aaa.bbb] is a
> > +	 * valid module name. '[' should have higher priority than
> > +	 * '.ko' suffix.
> > +	 */
> 
> 
> > +	if ((name[0] == '[') &&	(strncmp(name, "[vdso]", 6) != 0)) {
> 
> maybe also check for '[vsyscall]' ?

I wonder if there are more? Possibly we should look at some other field
to figure it out if this is kernel code, perhaps use this that is set
when synthesizing a module:

        if (machine__is_host(machine))
                event->header.misc = PERF_RECORD_MISC_KERNEL;
        else
                event->header.misc = PERF_RECORD_MISC_GUEST_KERNEL;

?
 
> > +		m->kmod = true;
> > +		is_simple_name = 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;
> 
> otherwise it looks ok to me.. please update also the tests/kmod-path.c ;-)
> 
> thanks,
> jirka

[PATCH v3 0/2] perf: report/annotate: fix segfault problem.

[Wang Nan]

Patch v3 updates tests/kmod-path.c. It also check cpumode to filter out
strings like '[vdso]' and '[vsyscalls]' instead of directly match
against them.

'perf: kmaps: enforce usage of kmaps to protect futher bugs.' is also
posted in this series (1/2). It has a small fix: moving map__kmap() and
newly introduced map__kmaps() from map.h to map.c, because they require
pr_err and struct dso but their existance are not ensure in map.h.

Wang Nan (2):
  perf: kmaps: enforce usage of kmaps to protect futher bugs.
  perf: report/annotate: fix segfault problem.

 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     |  9 ++++++--
 tools/perf/util/map.c         | 20 ++++++++++++++++++
 tools/perf/util/map.h         |  6 ++----
 tools/perf/util/probe-event.c |  2 ++
 tools/perf/util/session.c     |  3 +++
 tools/perf/util/symbol-elf.c  | 16 +++++++++-----
 tools/perf/util/symbol.c      | 34 ++++++++++++++++++++++++------
 11 files changed, 163 insertions(+), 33 deletions(-)

-- 
1.8.3.4

[PATCH v3 1/2] perf: kmaps: enforce usage of kmaps to protect futher bugs.

[Wang Nan]

This patch add checkings on places where use map__kmap and get
kmaps from struct kmap. Error messages are added at map__kmap to warn
invalud accessing of kmap (for the case of !map->dso->kernel, kmap(map)
is not exists at all). Also, introduces map__kmaps() to warn
uninitialized kmaps.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 tools/perf/util/machine.c     |  5 ++++-
 tools/perf/util/map.c         | 20 ++++++++++++++++++++
 tools/perf/util/map.h         |  6 ++----
 tools/perf/util/probe-event.c |  2 ++
 tools/perf/util/session.c     |  3 +++
 tools/perf/util/symbol-elf.c  | 16 +++++++++++-----
 tools/perf/util/symbol.c      | 34 ++++++++++++++++++++++++++++------
 7 files changed, 70 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index e335330..9c7feec 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -679,6 +679,9 @@ int __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 			machine->vmlinux_maps[type]->unmap_ip =
 				identity__map_ip;
 		kmap = map__kmap(machine->vmlinux_maps[type]);
+		if (!kmap)
+			return -1;
+
 		kmap->kmaps = &machine->kmaps;
 		map_groups__insert(&machine->kmaps,
 				   machine->vmlinux_maps[type]);
@@ -700,7 +703,7 @@ void machine__destroy_kernel_maps(struct machine *machine)
 		kmap = map__kmap(machine->vmlinux_maps[type]);
 		map_groups__remove(&machine->kmaps,
 				   machine->vmlinux_maps[type]);
-		if (kmap->ref_reloc_sym) {
+		if (kmap && kmap->ref_reloc_sym) {
 			/*
 			 * ref_reloc_sym is shared among all maps, so free just
 			 * on one of them.
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 62ca9f2..a14f08f 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -778,3 +778,23 @@ struct map *maps__next(struct map *map)
 		return rb_entry(next, struct map, rb_node);
 	return NULL;
 }
+
+struct kmap *map__kmap(struct map *map)
+{
+	if (!map->dso || !map->dso->kernel) {
+		pr_err("Internal error: map__kmap with a non-kernel map\n");
+		return NULL;
+	}
+	return (struct kmap *)(map + 1);
+}
+
+struct map_groups *map__kmaps(struct map *map)
+{
+	struct kmap *kmap = map__kmap(map);
+
+	if (!kmap || !kmap->kmaps) {
+		pr_err("Internal error: map__kmaps with a non-kernel map\n");
+		return NULL;
+	}
+	return kmap->kmaps;
+}
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 0e42438..ec19c59 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -76,10 +76,8 @@ static inline struct map_groups *map_groups__get(struct map_groups *mg)
 
 void map_groups__put(struct map_groups *mg);
 
-static inline struct kmap *map__kmap(struct map *map)
-{
-	return (struct kmap *)(map + 1);
-}
+struct kmap *map__kmap(struct map *map);
+struct map_groups *map__kmaps(struct map *map);
 
 static inline u64 map__map_ip(struct map *map, u64 ip)
 {
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 8feac07..4fd49f0 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -135,6 +135,8 @@ static struct ref_reloc_sym *kernel_get_ref_reloc_sym(void)
 		return NULL;
 
 	kmap = map__kmap(host_machine->vmlinux_maps[MAP__FUNCTION]);
+	if (!kmap)
+		return NULL;
 	return kmap->ref_reloc_sym;
 }
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index adf0740..7caedb4 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1456,6 +1456,9 @@ int maps__set_kallsyms_ref_reloc_sym(struct map **maps,
 
 	for (i = 0; i < MAP__NR_TYPES; ++i) {
 		struct kmap *kmap = map__kmap(maps[i]);
+
+		if (!kmap)
+			continue;
 		kmap->ref_reloc_sym = ref;
 	}
 
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 476268c..a7ab606 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -776,6 +776,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 		  symbol_filter_t filter, int kmodule)
 {
 	struct kmap *kmap = dso->kernel ? map__kmap(map) : NULL;
+	struct map_groups *kmaps = kmap ? map__kmaps(map) : NULL;
 	struct map *curr_map = map;
 	struct dso *curr_dso = dso;
 	Elf_Data *symstrs, *secstrs;
@@ -791,6 +792,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
 	int nr = 0;
 	bool remap_kernel = false, adjust_kernel_syms = false;
 
+	if (kmap && !kmaps)
+		return -1;
+
 	dso->symtab_type = syms_ss->type;
 	dso->is_64_bit = syms_ss->is_64_bit;
 	dso->rel = syms_ss->ehdr.e_type == ET_REL;
@@ -958,8 +962,10 @@ int dso__load_sym(struct dso *dso, struct map *map,
 					map->map_ip = map__map_ip;
 					map->unmap_ip = map__unmap_ip;
 					/* Ensure maps are correctly ordered */
-					map_groups__remove(kmap->kmaps, map);
-					map_groups__insert(kmap->kmaps, map);
+					if (kmaps) {
+						map_groups__remove(kmaps, map);
+						map_groups__insert(kmaps, map);
+					}
 				}
 
 				/*
@@ -983,7 +989,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 			snprintf(dso_name, sizeof(dso_name),
 				 "%s%s", dso->short_name, section_name);
 
-			curr_map = map_groups__find_by_name(kmap->kmaps, map->type, dso_name);
+			curr_map = map_groups__find_by_name(kmaps, map->type, dso_name);
 			if (curr_map == NULL) {
 				u64 start = sym.st_value;
 
@@ -1013,7 +1019,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 					curr_map->unmap_ip = identity__map_ip;
 				}
 				curr_dso->symtab_type = dso->symtab_type;
-				map_groups__insert(kmap->kmaps, curr_map);
+				map_groups__insert(kmaps, curr_map);
 				/*
 				 * The new DSO should go to the kernel DSOS
 				 */
@@ -1075,7 +1081,7 @@ new_symbol:
 			 * We need to fixup this here too because we create new
 			 * maps here, for things like vsyscall sections.
 			 */
-			__map_groups__fixup_end(kmap->kmaps, map->type);
+			__map_groups__fixup_end(kmaps, map->type);
 		}
 	}
 	err = nr;
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index fddeb90..201f6c4 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -630,13 +630,16 @@ static int dso__load_all_kallsyms(struct dso *dso, const char *filename,
 static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map,
 					 symbol_filter_t filter)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
+	struct map_groups *kmaps = map__kmaps(map);
 	struct map *curr_map;
 	struct symbol *pos;
 	int count = 0, moved = 0;
 	struct rb_root *root = &dso->symbols[map->type];
 	struct rb_node *next = rb_first(root);
 
+	if (!kmaps)
+		return -1;
+
 	while (next) {
 		char *module;
 
@@ -682,8 +685,8 @@ static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map,
 static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta,
 			       symbol_filter_t filter)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
-	struct machine *machine = kmaps->machine;
+	struct map_groups *kmaps = map__kmaps(map);
+	struct machine *machine;
 	struct map *curr_map = map;
 	struct symbol *pos;
 	int count = 0, moved = 0;
@@ -691,6 +694,11 @@ static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta,
 	struct rb_node *next = rb_first(root);
 	int kernel_range = 0;
 
+	if (!kmaps)
+		return -1;
+
+	machine = kmaps->machine;
+
 	while (next) {
 		char *module;
 
@@ -1025,9 +1033,12 @@ static bool filename_from_kallsyms_filename(char *filename,
 static int validate_kcore_modules(const char *kallsyms_filename,
 				  struct map *map)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
+	struct map_groups *kmaps = map__kmaps(map);
 	char modules_filename[PATH_MAX];
 
+	if (!kmaps)
+		return -EINVAL;
+
 	if (!filename_from_kallsyms_filename(modules_filename, "modules",
 					     kallsyms_filename))
 		return -EINVAL;
@@ -1043,6 +1054,9 @@ static int validate_kcore_addresses(const char *kallsyms_filename,
 {
 	struct kmap *kmap = map__kmap(map);
 
+	if (!kmap)
+		return -EINVAL;
+
 	if (kmap->ref_reloc_sym && kmap->ref_reloc_sym->name) {
 		u64 start;
 
@@ -1081,8 +1095,8 @@ static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
 static int dso__load_kcore(struct dso *dso, struct map *map,
 			   const char *kallsyms_filename)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
-	struct machine *machine = kmaps->machine;
+	struct map_groups *kmaps = map__kmaps(map);
+	struct machine *machine;
 	struct kcore_mapfn_data md;
 	struct map *old_map, *new_map, *replacement_map = NULL;
 	bool is_64_bit;
@@ -1090,6 +1104,11 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 	char kcore_filename[PATH_MAX];
 	struct symbol *sym;
 
+	if (!kmaps)
+		return -EINVAL;
+
+	machine = kmaps->machine;
+
 	/* This function requires that the map is the kernel map */
 	if (map != machine->vmlinux_maps[map->type])
 		return -EINVAL;
@@ -1202,6 +1221,9 @@ static int kallsyms__delta(struct map *map, const char *filename, u64 *delta)
 	struct kmap *kmap = map__kmap(map);
 	u64 addr;
 
+	if (!kmap)
+		return -1;
+
 	if (!kmap->ref_reloc_sym || !kmap->ref_reloc_sym->name)
 		return 0;
 
-- 
1.8.3.4

[PATCH v3 2/2] perf: report/annotate: fix segfault problem.

[Wang Nan]

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

Re: [PATCH] perf: kmaps: enforce usage of kmaps to protect futher bugs.

[Adrian Hunter]

On 03/04/15 14:11, Jiri Olsa wrote:
> On Fri, Apr 03, 2015 at 08:47:57AM +0000, Wang Nan wrote:
>> This patch add checkings on every place where uses map__kmap and get
>> kmaps from struct kmap. Error messages are added at map__kmap to warn
>> invalud accessing of kmap (for the case of !map->dso->kernel, kmap(map)
>> is not exists at all). Also, introduces map__kmaps() to warn
>> uninitialized kmaps.
> 
> looks ok, CC-ing Adrian

Look Ok to me too

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Re: [PATCH v3 2/2] perf: report/annotate: fix segfault problem.

[Arnaldo Carvalho de Melo]

Em Tue, Apr 07, 2015 at 08:22:46AM +0000, Wang Nan escreveu:
> 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]

So, with this patch applied I am seeing this:

[root@ssdandy ~]# perf sched record -a
Kmod (modules.order) and cpumode (0) inconsistent
Kmod (modules.builtin) and cpumode (0) inconsistent
Kmod (modules.dep) and cpumode (0) inconsistent
Kmod (modules.dep.bin) and cpumode (0) inconsistent
Kmod (modules.alias) and cpumode (0) inconsistent
Kmod (modules.alias.bin) and cpumode (0) inconsistent
Kmod (modules.softdep) and cpumode (0) inconsistent
Kmod (modules.symbols) and cpumode (0) inconsistent
Kmod (modules.symbols.bin) and cpumode (0) inconsistent
Kmod (modules.builtin.bin) and cpumode (0) inconsistent
Kmod (modules.devname) and cpumode (0) inconsistent
^C[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.465 MB perf.data (1595 samples) ]

[root@ssdandy ~]#o

Can you please check this?

Why do we get these warning for these files?


Applied the other patch:

-> 28 T 04/07 Wang Nan (8,8K) [PATCH v3 1/2] perf: kmaps: enforce usage of kmaps to protect futher bugs.


- Arnaldo

Re: [PATCH v3 2/2] perf: report/annotate: fix segfault problem.

[Wang Nan]

On 2015/4/7 23:13, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 07, 2015 at 08:22:46AM +0000, Wang Nan escreveu:
>> 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]
> 
> So, with this patch applied I am seeing this:
> 
> [root@ssdandy ~]# perf sched record -a
> Kmod (modules.order) and cpumode (0) inconsistent
> Kmod (modules.builtin) and cpumode (0) inconsistent
> Kmod (modules.dep) and cpumode (0) inconsistent
> Kmod (modules.dep.bin) and cpumode (0) inconsistent
> Kmod (modules.alias) and cpumode (0) inconsistent
> Kmod (modules.alias.bin) and cpumode (0) inconsistent
> Kmod (modules.softdep) and cpumode (0) inconsistent
> Kmod (modules.symbols) and cpumode (0) inconsistent
> Kmod (modules.symbols.bin) and cpumode (0) inconsistent
> Kmod (modules.builtin.bin) and cpumode (0) inconsistent
> Kmod (modules.devname) and cpumode (0) inconsistent
> ^C[ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.465 MB perf.data (1595 samples) ]
> 
> [root@ssdandy ~]#o
> 
> Can you please check this?
> 
> Why do we get these warning for these files?
> 

This is because in map_groups__set_modules_path_dir() we enumerate over each file
in /lib/modules/`uname -a`/ and use kmod_path__parse_name() to parse their names.
In this v3 patch, I update __kmod_path__parse() to check consistency between m.kmod and
cpumode. It will warn if one passes a kernel module name with user
space cpumode, or passes kernel space cpumode with a non-kmodule name.

I'll post a patch v4 to avoid such warning if passed cpumode is not explicitly set to
user or kernel.

Difference from v3:

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index fb1ed7d..875f7c9 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -215,7 +215,7 @@ 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;
-       bool cpu_mode_kernel, is_kernel = false;
+       bool cpu_mode_kernel, cpu_mode_unknown = false, is_kernel = false;

        /* treat PERF_RECORD_MISC_CPUMODE_UNKNOWN as kernel. */
        switch (cpumode & PERF_RECORD_MISC_CPUMODE_MASK) {
@@ -224,6 +224,9 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
        case PERF_RECORD_MISC_GUEST_USER:
                cpu_mode_kernel = false;
                break;
+       case PERF_RECORD_MISC_CPUMODE_UNKNOWN:
+               cpu_mode_unknown = true;
+               /* fall through */
        default:
                cpu_mode_kernel = true;
        }
@@ -288,10 +291,12 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
                }
        }
 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);
+       if (!cpu_mode_unknown) {
+               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;
 }

> 
> Applied the other patch:
> 
> -> 28 T 04/07 Wang Nan (8,8K) [PATCH v3 1/2] perf: kmaps: enforce usage of kmaps to protect futher bugs.
> 
> 
> - Arnaldo
> 

[PATCH v4 2/2] perf: report/annotate: fix segfault problem.

[Wang Nan]

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>
---

Different from v3:

Don't warn if cpumode is unknown.

---

 tools/perf/tests/kmod-path.c | 43 +++++++++++++++++++++++++++++++----
 tools/perf/util/dso.c        | 54 +++++++++++++++++++++++++++++++++++++++-----
 tools/perf/util/dso.h        | 12 ++++++----
 tools/perf/util/header.c     |  2 +-
 tools/perf/util/machine.c    |  4 +++-
 5 files changed, 98 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..875f7c9 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,56 @@ 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, cpu_mode_unknown = false, 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;
+	case PERF_RECORD_MISC_CPUMODE_UNKNOWN:
+		cpu_mode_unknown = true;
+		/* fall through */
+	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 +290,13 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
 			return -ENOMEM;
 		}
 	}
+out:
+	if (!cpu_mode_unknown) {
+		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 e45c8f3..e084943 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1109,7 +1109,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

[PATCH v4] perf: kmaps: enforce usage of kmaps to protect futher bugs.

[Wang Nan]

This patch add checkings on places where use map__kmap and get
kmaps from struct kmap. Error messages are added at map__kmap to warn
invalud accessing of kmap (for the case of !map->dso->kernel, kmap(map)
is not exists at all). Also, introduces map__kmaps() to warn
uninitialized kmaps.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---

Using this patch I found a problem in report__warn_kptr_restrict(),
in which map__kmap(NULL) is possible. This v3 patch add NULL checking
to map__kmap() to avoid segfault directly:

 -	if (!map->dso || !map->dso->kernel) {
 +	if (!map || !map->dso || !map->dso->kernel) {

---
 tools/perf/util/machine.c     |  5 ++++-
 tools/perf/util/map.c         | 20 ++++++++++++++++++++
 tools/perf/util/map.h         |  6 ++----
 tools/perf/util/probe-event.c |  2 ++
 tools/perf/util/session.c     |  3 +++
 tools/perf/util/symbol-elf.c  | 16 +++++++++++-----
 tools/perf/util/symbol.c      | 34 ++++++++++++++++++++++++++++------
 7 files changed, 70 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index e084943..18e911b 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -679,6 +679,9 @@ int __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 			machine->vmlinux_maps[type]->unmap_ip =
 				identity__map_ip;
 		kmap = map__kmap(machine->vmlinux_maps[type]);
+		if (!kmap)
+			return -1;
+
 		kmap->kmaps = &machine->kmaps;
 		map_groups__insert(&machine->kmaps,
 				   machine->vmlinux_maps[type]);
@@ -700,7 +703,7 @@ void machine__destroy_kernel_maps(struct machine *machine)
 		kmap = map__kmap(machine->vmlinux_maps[type]);
 		map_groups__remove(&machine->kmaps,
 				   machine->vmlinux_maps[type]);
-		if (kmap->ref_reloc_sym) {
+		if (kmap && kmap->ref_reloc_sym) {
 			/*
 			 * ref_reloc_sym is shared among all maps, so free just
 			 * on one of them.
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 62ca9f2..f46e5a7 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -778,3 +778,23 @@ struct map *maps__next(struct map *map)
 		return rb_entry(next, struct map, rb_node);
 	return NULL;
 }
+
+struct kmap *map__kmap(struct map *map)
+{
+	if (!map || !map->dso || !map->dso->kernel) {
+		pr_err("Internal error: map__kmap with a non-kernel map\n");
+		return NULL;
+	}
+	return (struct kmap *)(map + 1);
+}
+
+struct map_groups *map__kmaps(struct map *map)
+{
+	struct kmap *kmap = map__kmap(map);
+
+	if (!kmap || !kmap->kmaps) {
+		pr_err("Internal error: map__kmaps with a non-kernel map\n");
+		return NULL;
+	}
+	return kmap->kmaps;
+}
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 0e42438..ec19c59 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -76,10 +76,8 @@ static inline struct map_groups *map_groups__get(struct map_groups *mg)
 
 void map_groups__put(struct map_groups *mg);
 
-static inline struct kmap *map__kmap(struct map *map)
-{
-	return (struct kmap *)(map + 1);
-}
+struct kmap *map__kmap(struct map *map);
+struct map_groups *map__kmaps(struct map *map);
 
 static inline u64 map__map_ip(struct map *map, u64 ip)
 {
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 3dacec9..b788517 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -135,6 +135,8 @@ static struct ref_reloc_sym *kernel_get_ref_reloc_sym(void)
 		return NULL;
 
 	kmap = map__kmap(host_machine->vmlinux_maps[MAP__FUNCTION]);
+	if (!kmap)
+		return NULL;
 	return kmap->ref_reloc_sym;
 }
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index dfacf1d..0c74012 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1466,6 +1466,9 @@ int maps__set_kallsyms_ref_reloc_sym(struct map **maps,
 
 	for (i = 0; i < MAP__NR_TYPES; ++i) {
 		struct kmap *kmap = map__kmap(maps[i]);
+
+		if (!kmap)
+			continue;
 		kmap->ref_reloc_sym = ref;
 	}
 
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 476268c..a7ab606 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -776,6 +776,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 		  symbol_filter_t filter, int kmodule)
 {
 	struct kmap *kmap = dso->kernel ? map__kmap(map) : NULL;
+	struct map_groups *kmaps = kmap ? map__kmaps(map) : NULL;
 	struct map *curr_map = map;
 	struct dso *curr_dso = dso;
 	Elf_Data *symstrs, *secstrs;
@@ -791,6 +792,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
 	int nr = 0;
 	bool remap_kernel = false, adjust_kernel_syms = false;
 
+	if (kmap && !kmaps)
+		return -1;
+
 	dso->symtab_type = syms_ss->type;
 	dso->is_64_bit = syms_ss->is_64_bit;
 	dso->rel = syms_ss->ehdr.e_type == ET_REL;
@@ -958,8 +962,10 @@ int dso__load_sym(struct dso *dso, struct map *map,
 					map->map_ip = map__map_ip;
 					map->unmap_ip = map__unmap_ip;
 					/* Ensure maps are correctly ordered */
-					map_groups__remove(kmap->kmaps, map);
-					map_groups__insert(kmap->kmaps, map);
+					if (kmaps) {
+						map_groups__remove(kmaps, map);
+						map_groups__insert(kmaps, map);
+					}
 				}
 
 				/*
@@ -983,7 +989,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 			snprintf(dso_name, sizeof(dso_name),
 				 "%s%s", dso->short_name, section_name);
 
-			curr_map = map_groups__find_by_name(kmap->kmaps, map->type, dso_name);
+			curr_map = map_groups__find_by_name(kmaps, map->type, dso_name);
 			if (curr_map == NULL) {
 				u64 start = sym.st_value;
 
@@ -1013,7 +1019,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 					curr_map->unmap_ip = identity__map_ip;
 				}
 				curr_dso->symtab_type = dso->symtab_type;
-				map_groups__insert(kmap->kmaps, curr_map);
+				map_groups__insert(kmaps, curr_map);
 				/*
 				 * The new DSO should go to the kernel DSOS
 				 */
@@ -1075,7 +1081,7 @@ new_symbol:
 			 * We need to fixup this here too because we create new
 			 * maps here, for things like vsyscall sections.
 			 */
-			__map_groups__fixup_end(kmap->kmaps, map->type);
+			__map_groups__fixup_end(kmaps, map->type);
 		}
 	}
 	err = nr;
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index fddeb90..201f6c4c 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -630,13 +630,16 @@ static int dso__load_all_kallsyms(struct dso *dso, const char *filename,
 static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map,
 					 symbol_filter_t filter)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
+	struct map_groups *kmaps = map__kmaps(map);
 	struct map *curr_map;
 	struct symbol *pos;
 	int count = 0, moved = 0;
 	struct rb_root *root = &dso->symbols[map->type];
 	struct rb_node *next = rb_first(root);
 
+	if (!kmaps)
+		return -1;
+
 	while (next) {
 		char *module;
 
@@ -682,8 +685,8 @@ static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map,
 static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta,
 			       symbol_filter_t filter)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
-	struct machine *machine = kmaps->machine;
+	struct map_groups *kmaps = map__kmaps(map);
+	struct machine *machine;
 	struct map *curr_map = map;
 	struct symbol *pos;
 	int count = 0, moved = 0;
@@ -691,6 +694,11 @@ static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta,
 	struct rb_node *next = rb_first(root);
 	int kernel_range = 0;
 
+	if (!kmaps)
+		return -1;
+
+	machine = kmaps->machine;
+
 	while (next) {
 		char *module;
 
@@ -1025,9 +1033,12 @@ static bool filename_from_kallsyms_filename(char *filename,
 static int validate_kcore_modules(const char *kallsyms_filename,
 				  struct map *map)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
+	struct map_groups *kmaps = map__kmaps(map);
 	char modules_filename[PATH_MAX];
 
+	if (!kmaps)
+		return -EINVAL;
+
 	if (!filename_from_kallsyms_filename(modules_filename, "modules",
 					     kallsyms_filename))
 		return -EINVAL;
@@ -1043,6 +1054,9 @@ static int validate_kcore_addresses(const char *kallsyms_filename,
 {
 	struct kmap *kmap = map__kmap(map);
 
+	if (!kmap)
+		return -EINVAL;
+
 	if (kmap->ref_reloc_sym && kmap->ref_reloc_sym->name) {
 		u64 start;
 
@@ -1081,8 +1095,8 @@ static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
 static int dso__load_kcore(struct dso *dso, struct map *map,
 			   const char *kallsyms_filename)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
-	struct machine *machine = kmaps->machine;
+	struct map_groups *kmaps = map__kmaps(map);
+	struct machine *machine;
 	struct kcore_mapfn_data md;
 	struct map *old_map, *new_map, *replacement_map = NULL;
 	bool is_64_bit;
@@ -1090,6 +1104,11 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 	char kcore_filename[PATH_MAX];
 	struct symbol *sym;
 
+	if (!kmaps)
+		return -EINVAL;
+
+	machine = kmaps->machine;
+
 	/* This function requires that the map is the kernel map */
 	if (map != machine->vmlinux_maps[map->type])
 		return -EINVAL;
@@ -1202,6 +1221,9 @@ static int kallsyms__delta(struct map *map, const char *filename, u64 *delta)
 	struct kmap *kmap = map__kmap(map);
 	u64 addr;
 
+	if (!kmap)
+		return -1;
+
 	if (!kmap->ref_reloc_sym || !kmap->ref_reloc_sym->name)
 		return 0;
 
-- 
1.8.3.4

Re: [PATCH v4 2/2] perf: report/annotate: fix segfault problem.

[Jiri Olsa]

On Wed, Apr 08, 2015 at 03:52:18AM +0000, Wang Nan wrote:
> 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]

hum, I'm strugling to reproduce this, but I guess it's
because the other patch is already in:

  37e9744467d7 perf kmaps: Check kmaps to make code more robust

> 
> 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.

could you please be more detailed on describing the segfault code path?

I can see that we should parse [xxx] as a module, however I fail to see
how it could cause the scenario you described above.

If it's no longer the case, because of the above patch is in, please
describe current issue..

thanks,
jirka

> 
> 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>
> ---
> 
> Different from v3:
> 
> Don't warn if cpumode is unknown.
> 
> ---
> 
>  tools/perf/tests/kmod-path.c | 43 +++++++++++++++++++++++++++++++----
>  tools/perf/util/dso.c        | 54 +++++++++++++++++++++++++++++++++++++++-----
>  tools/perf/util/dso.h        | 12 ++++++----
>  tools/perf/util/header.c     |  2 +-
>  tools/perf/util/machine.c    |  4 +++-
>  5 files changed, 98 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..875f7c9 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,56 @@ 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, cpu_mode_unknown = false, 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;
> +	case PERF_RECORD_MISC_CPUMODE_UNKNOWN:
> +		cpu_mode_unknown = true;
> +		/* fall through */
> +	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 +290,13 @@ int __kmod_path__parse(struct kmod_path *m, const char *path,
>  			return -ENOMEM;
>  		}
>  	}
> +out:
> +	if (!cpu_mode_unknown) {
> +		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 e45c8f3..e084943 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1109,7 +1109,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
> 

[tip:perf/core] perf kmaps: Check kmaps to make code more robust

[tip-bot for Wang Nan]

Commit-ID:  ba92732e9808df679ddf75c5ea1c0caae6d7dce2
Gitweb:     http://git.kernel.org/tip/ba92732e9808df679ddf75c5ea1c0caae6d7dce2
Author:     Wang Nan <wangnan0@huawei.com>
AuthorDate: Tue, 7 Apr 2015 08:22:45 +0000
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 8 Apr 2015 09:07:03 -0300

perf kmaps: Check kmaps to make code more robust

This patch add checks in places where map__kmap is used to get kmaps
from struct kmap.

Error messages are added at map__kmap to warn invalid accessing of kmap
(for the case of !map->dso->kernel, kmap(map) does not exists at all).

Also, introduces map__kmaps() to warn uninitialized kmaps.

Reviewed-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: pi3orama@163.com
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Zefan Li <lizefan@huawei.com>
Link: http://lkml.kernel.org/r/1428394966-131044-2-git-send-email-wangnan0@huawei.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c     |  5 ++++-
 tools/perf/util/map.c         | 20 ++++++++++++++++++++
 tools/perf/util/map.h         |  6 ++----
 tools/perf/util/probe-event.c |  2 ++
 tools/perf/util/session.c     |  3 +++
 tools/perf/util/symbol-elf.c  | 16 +++++++++++-----
 tools/perf/util/symbol.c      | 34 ++++++++++++++++++++++++++++------
 7 files changed, 70 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index e45c8f3..9c380a2 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -679,6 +679,9 @@ int __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 			machine->vmlinux_maps[type]->unmap_ip =
 				identity__map_ip;
 		kmap = map__kmap(machine->vmlinux_maps[type]);
+		if (!kmap)
+			return -1;
+
 		kmap->kmaps = &machine->kmaps;
 		map_groups__insert(&machine->kmaps,
 				   machine->vmlinux_maps[type]);
@@ -700,7 +703,7 @@ void machine__destroy_kernel_maps(struct machine *machine)
 		kmap = map__kmap(machine->vmlinux_maps[type]);
 		map_groups__remove(&machine->kmaps,
 				   machine->vmlinux_maps[type]);
-		if (kmap->ref_reloc_sym) {
+		if (kmap && kmap->ref_reloc_sym) {
 			/*
 			 * ref_reloc_sym is shared among all maps, so free just
 			 * on one of them.
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 62ca9f2..a14f08f 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -778,3 +778,23 @@ struct map *maps__next(struct map *map)
 		return rb_entry(next, struct map, rb_node);
 	return NULL;
 }
+
+struct kmap *map__kmap(struct map *map)
+{
+	if (!map->dso || !map->dso->kernel) {
+		pr_err("Internal error: map__kmap with a non-kernel map\n");
+		return NULL;
+	}
+	return (struct kmap *)(map + 1);
+}
+
+struct map_groups *map__kmaps(struct map *map)
+{
+	struct kmap *kmap = map__kmap(map);
+
+	if (!kmap || !kmap->kmaps) {
+		pr_err("Internal error: map__kmaps with a non-kernel map\n");
+		return NULL;
+	}
+	return kmap->kmaps;
+}
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 0e42438..ec19c59 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -76,10 +76,8 @@ static inline struct map_groups *map_groups__get(struct map_groups *mg)
 
 void map_groups__put(struct map_groups *mg);
 
-static inline struct kmap *map__kmap(struct map *map)
-{
-	return (struct kmap *)(map + 1);
-}
+struct kmap *map__kmap(struct map *map);
+struct map_groups *map__kmaps(struct map *map);
 
 static inline u64 map__map_ip(struct map *map, u64 ip)
 {
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 8feac07..4fd49f0 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -135,6 +135,8 @@ static struct ref_reloc_sym *kernel_get_ref_reloc_sym(void)
 		return NULL;
 
 	kmap = map__kmap(host_machine->vmlinux_maps[MAP__FUNCTION]);
+	if (!kmap)
+		return NULL;
 	return kmap->ref_reloc_sym;
 }
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index dfacf1d..0c74012 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1466,6 +1466,9 @@ int maps__set_kallsyms_ref_reloc_sym(struct map **maps,
 
 	for (i = 0; i < MAP__NR_TYPES; ++i) {
 		struct kmap *kmap = map__kmap(maps[i]);
+
+		if (!kmap)
+			continue;
 		kmap->ref_reloc_sym = ref;
 	}
 
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 476268c..a7ab606 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -776,6 +776,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 		  symbol_filter_t filter, int kmodule)
 {
 	struct kmap *kmap = dso->kernel ? map__kmap(map) : NULL;
+	struct map_groups *kmaps = kmap ? map__kmaps(map) : NULL;
 	struct map *curr_map = map;
 	struct dso *curr_dso = dso;
 	Elf_Data *symstrs, *secstrs;
@@ -791,6 +792,9 @@ int dso__load_sym(struct dso *dso, struct map *map,
 	int nr = 0;
 	bool remap_kernel = false, adjust_kernel_syms = false;
 
+	if (kmap && !kmaps)
+		return -1;
+
 	dso->symtab_type = syms_ss->type;
 	dso->is_64_bit = syms_ss->is_64_bit;
 	dso->rel = syms_ss->ehdr.e_type == ET_REL;
@@ -958,8 +962,10 @@ int dso__load_sym(struct dso *dso, struct map *map,
 					map->map_ip = map__map_ip;
 					map->unmap_ip = map__unmap_ip;
 					/* Ensure maps are correctly ordered */
-					map_groups__remove(kmap->kmaps, map);
-					map_groups__insert(kmap->kmaps, map);
+					if (kmaps) {
+						map_groups__remove(kmaps, map);
+						map_groups__insert(kmaps, map);
+					}
 				}
 
 				/*
@@ -983,7 +989,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 			snprintf(dso_name, sizeof(dso_name),
 				 "%s%s", dso->short_name, section_name);
 
-			curr_map = map_groups__find_by_name(kmap->kmaps, map->type, dso_name);
+			curr_map = map_groups__find_by_name(kmaps, map->type, dso_name);
 			if (curr_map == NULL) {
 				u64 start = sym.st_value;
 
@@ -1013,7 +1019,7 @@ int dso__load_sym(struct dso *dso, struct map *map,
 					curr_map->unmap_ip = identity__map_ip;
 				}
 				curr_dso->symtab_type = dso->symtab_type;
-				map_groups__insert(kmap->kmaps, curr_map);
+				map_groups__insert(kmaps, curr_map);
 				/*
 				 * The new DSO should go to the kernel DSOS
 				 */
@@ -1075,7 +1081,7 @@ new_symbol:
 			 * We need to fixup this here too because we create new
 			 * maps here, for things like vsyscall sections.
 			 */
-			__map_groups__fixup_end(kmap->kmaps, map->type);
+			__map_groups__fixup_end(kmaps, map->type);
 		}
 	}
 	err = nr;
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index fddeb90..201f6c4c 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -630,13 +630,16 @@ static int dso__load_all_kallsyms(struct dso *dso, const char *filename,
 static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map,
 					 symbol_filter_t filter)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
+	struct map_groups *kmaps = map__kmaps(map);
 	struct map *curr_map;
 	struct symbol *pos;
 	int count = 0, moved = 0;
 	struct rb_root *root = &dso->symbols[map->type];
 	struct rb_node *next = rb_first(root);
 
+	if (!kmaps)
+		return -1;
+
 	while (next) {
 		char *module;
 
@@ -682,8 +685,8 @@ static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map,
 static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta,
 			       symbol_filter_t filter)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
-	struct machine *machine = kmaps->machine;
+	struct map_groups *kmaps = map__kmaps(map);
+	struct machine *machine;
 	struct map *curr_map = map;
 	struct symbol *pos;
 	int count = 0, moved = 0;
@@ -691,6 +694,11 @@ static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta,
 	struct rb_node *next = rb_first(root);
 	int kernel_range = 0;
 
+	if (!kmaps)
+		return -1;
+
+	machine = kmaps->machine;
+
 	while (next) {
 		char *module;
 
@@ -1025,9 +1033,12 @@ static bool filename_from_kallsyms_filename(char *filename,
 static int validate_kcore_modules(const char *kallsyms_filename,
 				  struct map *map)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
+	struct map_groups *kmaps = map__kmaps(map);
 	char modules_filename[PATH_MAX];
 
+	if (!kmaps)
+		return -EINVAL;
+
 	if (!filename_from_kallsyms_filename(modules_filename, "modules",
 					     kallsyms_filename))
 		return -EINVAL;
@@ -1043,6 +1054,9 @@ static int validate_kcore_addresses(const char *kallsyms_filename,
 {
 	struct kmap *kmap = map__kmap(map);
 
+	if (!kmap)
+		return -EINVAL;
+
 	if (kmap->ref_reloc_sym && kmap->ref_reloc_sym->name) {
 		u64 start;
 
@@ -1081,8 +1095,8 @@ static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
 static int dso__load_kcore(struct dso *dso, struct map *map,
 			   const char *kallsyms_filename)
 {
-	struct map_groups *kmaps = map__kmap(map)->kmaps;
-	struct machine *machine = kmaps->machine;
+	struct map_groups *kmaps = map__kmaps(map);
+	struct machine *machine;
 	struct kcore_mapfn_data md;
 	struct map *old_map, *new_map, *replacement_map = NULL;
 	bool is_64_bit;
@@ -1090,6 +1104,11 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 	char kcore_filename[PATH_MAX];
 	struct symbol *sym;
 
+	if (!kmaps)
+		return -EINVAL;
+
+	machine = kmaps->machine;
+
 	/* This function requires that the map is the kernel map */
 	if (map != machine->vmlinux_maps[map->type])
 		return -EINVAL;
@@ -1202,6 +1221,9 @@ static int kallsyms__delta(struct map *map, const char *filename, u64 *delta)
 	struct kmap *kmap = map__kmap(map);
 	u64 addr;
 
+	if (!kmap)
+		return -1;
+
 	if (!kmap->ref_reloc_sym || !kmap->ref_reloc_sym->name)
 		return 0;
 

Re: [PATCH v4 2/2] perf: report/annotate: fix segfault problem.

[Wang Nan]

On 2015/4/8 21:59, Jiri Olsa wrote:
> On Wed, Apr 08, 2015 at 03:52:18AM +0000, Wang Nan wrote:
>> 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]
> 
> hum, I'm strugling to reproduce this, but I guess it's
> because the other patch is already in:
> 
>   37e9744467d7 perf kmaps: Check kmaps to make code more robust
> 
>>
>> 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.
> 
> could you please be more detailed on describing the segfault code path?
> 
> I can see that we should parse [xxx] as a module, however I fail to see
> how it could cause the scenario you described above.
> 
> If it's no longer the case, because of the above patch is in, please
> describe current issue..
> 
> thanks,
> jirka
>

Sorry, I think some infors are missed in the description above:
to reproduce such segfault, perf.data you use should have buildid
infos for [test_module].

* Reproducing the segfault *

I think you have already have a perf.data which contains sth like:

 ...
 0 0 0x188 [0x50]: PERF_RECORD_MMAP -1/0: [0xffffffbff1018000(0xf068000) @ 0]: x [test_module]
 ...

In my case, perf.data also contains a HEADER_BUILD_ID feature in its header. Which contains
build_ids of vmlinux and all modules retrived by perf_session__create_kernel_maps() during
recording, and written into perf.data. Therefore, during reporting, __event_process_build_id()
will be called for [test_module]:

 $ perf report -i ./test-annotation/perf.data -v  --stdio --header-only
  build id event received for [kernel.kallsyms]: xxxxxxxxxxxxxxxxxxxxxxxx
  build id event received for [test_module]: xxxxxxxxxxxxxxxxxxxxxxxx
  ...
  # ========
  # captured on: Fri Apr  3 02:59:11 2015
  # hostname : ...
  # os release : ...
  # perf version : 4.0.rc5.g9f66
  # ...


If we don't have such buildid in our buildid-cache, perf is unable to find symbols from [test_module]
but the bug won't be trigger. However, if that module can be found in our buildid-cache, something bad
will happen.

To help you reproduce this problem, I suggest you to remove your buildid cache using

 # rm -rf ~/.debug

then add test_module.ko into it:

 # perf buildid-cache -a ./path-to-module/test_module.ko

During report, use --kallsyms explicitly:

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

Then you will see the segfault:

perf: Segmentation fault
-------- backtrace --------
/path/to/perf[0x502958]
/lib64/libc.so.6(+0x3545f)[0x7f820bdab45f]
/path/to/perf[0x499ab6]
/path/to/perf(dso__load_kallsyms+0x13c)[0x49b4cc]
/path/to/perf(dso__load+0x72e)[0x49c17e]
/path/to/perf(map__load+0x6e)[0x4adf5e]
/path/to/perf(thread__find_addr_map+0x24c)[0x47de4c]
/path/to/perf(perf_event__preprocess_sample+0x88)[0x47e198]
/path/to/perf[0x43ac92]
/path/to/perf[0x4b4b2c]
/path/to/perf(ordered_events__flush+0xca)[0x4b4d5a]
/path/to/perf[0x4b0f71]
/path/to/perf(perf_session__process_events+0x3be)[0x4b37fe]
/path/to/perf(cmd_report+0xed9)[0x43bf19]
/path/to/perf[0x474662]
/path/to/perf(main+0x5f5)[0x42de25]
/lib64/libc.so.6(__libc_start_main+0xf4)[0x7f820bd97bd4]
/path/to/perf[0x42df54]

Using gdb I can get following call stack:

Program received signal SIGSEGV, Segmentation fault.
dso__load_kcore (dso=dso@entry=0x959710, map=map@entry=0x9a4410,
    kallsyms_filename=kallsyms_filename@entry=0x7fffffffe2ed "./test-annotation/kallsyms") at util/symbol.c:1094
1094		if (map != machine->vmlinux_maps[map->type])
(gdb) bt
#0  dso__load_kcore (dso=dso@entry=0x959710, map=map@entry=0x9a4410,
    kallsyms_filename=kallsyms_filename@entry=0x7fffffffe2ed "./test-annotation/kallsyms") at util/symbol.c:1094
#1  0x000000000049b4cd in dso__load_kallsyms (dso=dso@entry=0x959710,
    filename=filename@entry=0x7fffffffe2ed "./test-annotation/kallsyms", map=map@entry=0x9a4410,
    filter=filter@entry=0x476070 <symbol__annotate_init>) at util/symbol.c:1239
#2  0x000000000049c17f in dso__load_kernel_sym (filter=0x476070 <symbol__annotate_init>, map=0x9a4410, dso=0x959710)
    at util/symbol.c:1715
#3  dso__load (dso=0x959710, map=map@entry=0x9a4410, filter=filter@entry=0x476070 <symbol__annotate_init>) at util/symbol.c:1364
#4  0x00000000004adf5f in map__load (map=0x9a4410, filter=0x476070 <symbol__annotate_init>) at util/map.c:259
#5  0x000000000047de4d in thread__find_addr_map (thread=<optimized out>, cpumode=<optimized out>, type=<optimized out>,
    addr=<optimized out>, al=0x7fffffffbdf0) at util/event.c:819
#6  0x000000000047e199 in perf_event__preprocess_sample (event=event@entry=0x7ffff3fe51e0, machine=0x9587f0,
    al=al@entry=0x7fffffffbdf0, sample=sample@entry=0x7fffffffbf00) at util/event.c:860
#7  0x000000000043ac93 in process_sample_event (tool=0x7fffffffc730, event=0x7ffff3fe51e0, sample=0x7fffffffbf00, evsel=0x9593c0,
    machine=<optimized out>) at builtin-report.c:145
#8  0x00000000004b4b2d in __ordered_events__flush (oe=oe@entry=0x958920) at util/ordered-events.c:214
#9  0x00000000004b4d5b in ordered_events__flush (oe=0x958920, how=OE_FLUSH__ROUND) at util/ordered-events.c:279
#10 0x00000000004b0f72 in perf_session__process_user_event (file_offset=2539904, event=0x7ffff4242180, session=0x958730)
    at util/session.c:972
#11 perf_session__process_event (session=session@entry=0x958730, event=event@entry=0x7ffff4242180,
    file_offset=file_offset@entry=2539904) at util/session.c:1076
#12 0x00000000004b37ff in __perf_session__process_events (file_size=2539912, data_size=<optimized out>, data_offset=<optimized out>,
    session=0x958730) at util/session.c:1369
#13 perf_session__process_events (session=session@entry=0x958730) at util/session.c:1412
#14 0x000000000043bf1a in __cmd_report (rep=0x7fffffffc730) at builtin-report.c:485
#15 cmd_report (argc=0, argv=0x7fffffffdea0, prefix=<optimized out>) at builtin-report.c:867
#16 0x0000000000474663 in run_builtin (p=p@entry=0x89cae8 <commands+168>, argc=argc@entry=4, argv=argv@entry=0x7fffffffdea0)
    at perf.c:370
#17 0x000000000042de26 in handle_internal_command (argv=0x7fffffffdea0, argc=4) at perf.c:429
#18 run_argv (argv=0x7fffffffdc20, argcp=0x7fffffffdc2c) at perf.c:473
#19 main (argc=4, argv=0x7fffffffdea0) at perf.c:588
(gdb)

* Path to the failure *

I'll explain what lead to the segfault.

Step 1: during __event_process_build_id():
        ....
        misc = bev->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;

        switch (misc) {
        case PERF_RECORD_MISC_KERNEL:
                dso_type = DSO_TYPE_KERNEL;
                dsos = &machine->kernel_dsos;
                break;
        ....
                if (!is_kernel_module(filename))
                        dso->kernel = dso_type;
        ....

Here, is_kernel_module("[test_module]") is false, and cpumode here is PERF_RECORD_MISC_KERNEL,
so dso->kernel will become DSO_TYPE_KERNEL, which means the kernel itself. This is the root cause
of everything.

Step 2: machine__process_mmap_event(), which creates map structure for both kernel and modules.
        struct map is allocated using map__new2(). Since according dso ->type is kernel,
        the extra kmap structure is allocated. However, the only place initializes
        map__kmap(map)->kmaps is __machine__create_kernel_maps(), which is called by
        machine__process_kernel_mmap_event():


static int machine__process_kernel_mmap_event(struct machine *machine,
                                              union perf_event *event)
{
        ...
        is_kernel_mmap = memcmp(event->mmap.filename,
                                kmmap_prefix,
                                strlen(kmmap_prefix) - 1) == 0;
        if (event->mmap.filename[0] == '/' ||
            (!is_kernel_mmap && event->mmap.filename[0] == '[')) {
                map = machine__new_module(machine, event->mmap.start,
                                          event->mmap.filename);
                if (map == NULL)
                        goto out_problem;

                map->end = map->start + event->mmap.len;
        } else if (is_kernel_mmap) {
                ...
                if (__machine__create_kernel_maps(machine, kernel) < 0)
                        goto out_problem;
                ...
        }
        ...
}

When mmap event at kernel address space occures, it distinguishes kernel and kernel module
according to the name in mmap event. Unfortunately, '[test_module]' won't be regarded as kernel
at this time, so according map__kmap(map)->kmaps won't get initialized.

If we don't have buildid-cache for [test_module], machine__process_kernel_mmap_event -->
machine__new_module --> machine__module_dso --> dsos__find will return failuer, and a new
struct dso is created using dsos__addnew, its default dso->kernel is DSO_TYPE_USER.

Step 3: process_sample_event -> map__load: a sample with kernel IP is arise, perf
        tries to load DSO of such address.

Step 4: map__load -> dso__load: in dso__load():

int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter)
{
	...
	if (dso->kernel == DSO_TYPE_KERNEL)
                return dso__load_kernel_sym(dso, map, filter);
	...
}

Because dso->kernel of "[test_module]" is incorrectly set to DSO_TYPE_KERNEL, it goes to
dso__load_kernel_sym. Where, since we use --kallsyms explicitly, dso__load_kallsyms() will
be called.

Step 5: dso__load_kallsyms -> dso__load_kcore:

dso__load_kallsyms tries to call dso__load_kcore. In my case it should fail because I don't have
/proc/kcore at all. However,

static int dso__load_kcore(struct dso *dso, struct map *map,
                           const char *kallsyms_filename)
{
        struct map_groups *kmaps = map__kmap(map)->kmaps;
        struct machine *machine = kmaps->machine;
        ...
}

dso__load_kcore requires both map__kmap(map) and map__kmap(map)->kmaps to be valid.

However, although '[test_module]' is regarded to kernel itself, its map__kmap(map)->kmaps is invalid.
See step 2.

* Conclusion *

According to the above discussion, we can conclude that, the segfault is caused by mismatching
of dso and map for [test_module]. In most cases, map and dso of [test_module] is incorrectly be
treated as kernel itself. However, in machine__process_kernel_mmap_event(), it is treated like
a kernel module.

We can also see that the code in dso__load_kcore() is also dangerous. If there is no such mmap
in perf.data, "machine = kmaps->machine" will also causes segfault.

Re: [PATCH v4 2/2] perf: report/annotate: fix segfault problem.

[Jiri Olsa]

On Thu, Apr 09, 2015 at 03:05:27PM +0800, Wang Nan wrote:

SNIP

> When mmap event at kernel address space occures, it distinguishes kernel and kernel module
> according to the name in mmap event. Unfortunately, '[test_module]' won't be regarded as kernel
> at this time, so according map__kmap(map)->kmaps won't get initialized.
> 
> If we don't have buildid-cache for [test_module], machine__process_kernel_mmap_event -->
> machine__new_module --> machine__module_dso --> dsos__find will return failuer, and a new
> struct dso is created using dsos__addnew, its default dso->kernel is DSO_TYPE_USER.
> 
> Step 3: process_sample_event -> map__load: a sample with kernel IP is arise, perf
>         tries to load DSO of such address.
> 
> Step 4: map__load -> dso__load: in dso__load():
> 
> int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter)
> {
> 	...
> 	if (dso->kernel == DSO_TYPE_KERNEL)
>                 return dso__load_kernel_sym(dso, map, filter);
> 	...
> }
> 
> Because dso->kernel of "[test_module]" is incorrectly set to DSO_TYPE_KERNEL, it goes to
> dso__load_kernel_sym. Where, since we use --kallsyms explicitly, dso__load_kallsyms() will
> be called.
> 
> Step 5: dso__load_kallsyms -> dso__load_kcore:
> 
> dso__load_kallsyms tries to call dso__load_kcore. In my case it should fail because I don't have
> /proc/kcore at all. However,
> 
> static int dso__load_kcore(struct dso *dso, struct map *map,
>                            const char *kallsyms_filename)
> {
>         struct map_groups *kmaps = map__kmap(map)->kmaps;
>         struct machine *machine = kmaps->machine;
>         ...

ok, here it is.. I've got 'lucky' and hit the '!kmap->kmaps'
check in the map__kmaps function.. so I've got pr_err
output instead which I succesfully ignored ;-)

I'll comment in the patch v4

nice explanation! thanks 


jirka

Re: [PATCH v4 2/2] perf: report/annotate: fix segfault problem.

[Jiri Olsa]

On Wed, Apr 08, 2015 at 03:52:18AM +0000, Wang Nan wrote:
SNIP

>  
> -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,56 @@ 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, cpu_mode_unknown = false, 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;
> +	case PERF_RECORD_MISC_CPUMODE_UNKNOWN:
> +		cpu_mode_unknown = true;
> +		/* fall through */
> +	default:
> +		cpu_mode_kernel = true;
> +	}

so the cpumode is valid only for the is_kernel_module caller,
the rest of the users use PERF_RECORD_MISC_CPUMODE_UNKNOWN

could you move this logic into is_kernel_module function
and let __kmod_path__parse do only parsing work..?

also new test for is_kernel_module functionality would be nice ;-)

>  
>  	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)) {

these checks would stay in here together with checks
for [vdso] and [vsyscall] right?

SNIP

>  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))

please pass either whole misc or just cpumode like you do below
in machine__process_kernel_mmap_event

however the is_kernel_module declaration says cpumode so
I'd expect cpumode, not complete 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 e45c8f3..e084943 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1109,7 +1109,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
> 

Re: [PATCH v4 2/2] perf: report/annotate: fix segfault problem.

[Wang Nan]

On 2015/4/9 19:52, Jiri Olsa wrote:
> On Wed, Apr 08, 2015 at 03:52:18AM +0000, Wang Nan wrote:
> SNIP
> 
>>  
>> -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;

Find another problem here: return NULL for bool function. Will fix in v5.

>>  
>>  	return m.kmod;
>> @@ -210,21 +210,56 @@ 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, cpu_mode_unknown = false, 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;
>> +	case PERF_RECORD_MISC_CPUMODE_UNKNOWN:
>> +		cpu_mode_unknown = true;
>> +		/* fall through */
>> +	default:
>> +		cpu_mode_kernel = true;
>> +	}
> 
> so the cpumode is valid only for the is_kernel_module caller,
> the rest of the users use PERF_RECORD_MISC_CPUMODE_UNKNOWN
> 
> could you move this logic into is_kernel_module function
> and let __kmod_path__parse do only parsing work..?
> 
> also new test for is_kernel_module functionality would be nice ;-)
> 
>>  
>>  	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)) {
> 
> these checks would stay in here together with checks
> for [vdso] and [vsyscall] right?
> 
> SNIP
> 
>>  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))
> 
> please pass either whole misc or just cpumode like you do below
> in machine__process_kernel_mmap_event
> 
> however the is_kernel_module declaration says cpumode so
> I'd expect cpumode, not complete 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 e45c8f3..e084943 100644
>> --- a/tools/perf/util/machine.c
>> +++ b/tools/perf/util/machine.c
>> @@ -1109,7 +1109,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
>>

Re: [PATCH v4 2/2] perf: report/annotate: fix segfault problem.

[Wang Nan]

On 2015/4/9 19:52, Jiri Olsa wrote:
> On Wed, Apr 08, 2015 at 03:52:18AM +0000, Wang Nan wrote:
> SNIP
> 
>>  
>> -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,56 @@ 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, cpu_mode_unknown = false, 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;
>> +	case PERF_RECORD_MISC_CPUMODE_UNKNOWN:
>> +		cpu_mode_unknown = true;
>> +		/* fall through */
>> +	default:
>> +		cpu_mode_kernel = true;
>> +	}
> 
> so the cpumode is valid only for the is_kernel_module caller,
> the rest of the users use PERF_RECORD_MISC_CPUMODE_UNKNOWN
> 
> could you move this logic into is_kernel_module function
> and let __kmod_path__parse do only parsing work..?
> 
> also new test for is_kernel_module functionality would be nice ;-)
> 
>>  
>>  	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)) {
> 
> these checks would stay in here together with checks
> for [vdso] and [vsyscall] right?
> 
> SNIP
> 
>>  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))
> 
> please pass either whole misc or just cpumode like you do below
> in machine__process_kernel_mmap_event
> 

misc = bev->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;

So misc is cpumode.

I'll rename it to cpumode in v5 patch.

> however the is_kernel_module declaration says cpumode so
> I'd expect cpumode, not complete 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 e45c8f3..e084943 100644
>> --- a/tools/perf/util/machine.c
>> +++ b/tools/perf/util/machine.c
>> @@ -1109,7 +1109,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
>>

[PATCH v5 2/2] perf: report/annotate: fix segfault problem.

[Wang Nan]

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. If perf.data contain 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() 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 to 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>
---

Different from v4: checks cpumode in is_kernel_module(), makes code simpler.
                   Appends tests of is_kernel_module().
---
 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 fb43215..8c76a23 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1232,7 +1232,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;
 
@@ -1240,9 +1240,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;
@@ -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, 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 e335330..3769009 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1109,7 +1109,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() treat it as a kernel cpumode.
+			 */
+			if (is_kernel_module(dso->long_name,
+					     PERF_RECORD_MISC_CPUMODE_UNKNOWN))
 				continue;
 
 			kernel = dso;
-- 
1.8.3.4

Re: [PATCH v5 2/2] perf: report/annotate: fix segfault problem.

[Wang Nan]

Ping?

On 2015/4/10 11:53, Wang Nan wrote:
> 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. If perf.data contain 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() 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 to 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>
> ---
> 
> Different from v4: checks cpumode in is_kernel_module(), makes code simpler.
>                    Appends tests of is_kernel_module().
> ---
>  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 fb43215..8c76a23 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1232,7 +1232,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;
>  
> @@ -1240,9 +1240,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;
> @@ -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, 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 e335330..3769009 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1109,7 +1109,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() treat it as a kernel cpumode.
> +			 */
> +			if (is_kernel_module(dso->long_name,
> +					     PERF_RECORD_MISC_CPUMODE_UNKNOWN))
>  				continue;
>  
>  			kernel = dso;
> 

Re: [PATCH v5 2/2] perf: report/annotate: fix segfault problem.

[Wang Nan]

Ping again?
On 2015/4/15 9:27, Wang Nan wrote:
> Ping?
> 
> On 2015/4/10 11:53, Wang Nan wrote:
>> 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. If perf.data contain 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() 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 to 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>
>> ---
>>
>> Different from v4: checks cpumode in is_kernel_module(), makes code simpler.
>>                    Appends tests of is_kernel_module().
>> ---
>>  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 fb43215..8c76a23 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -1232,7 +1232,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;
>>  
>> @@ -1240,9 +1240,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;
>> @@ -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, 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 e335330..3769009 100644
>> --- a/tools/perf/util/machine.c
>> +++ b/tools/perf/util/machine.c
>> @@ -1109,7 +1109,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() treat 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>