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

[Jiri Olsa <jolsa@redhat.com>]

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
>