lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <552DBE67.50500@huawei.com>
Date:	Wed, 15 Apr 2015 09:27:03 +0800
From:	Wang Nan <wangnan0@...wei.com>
To:	<jolsa@...hat.com>
CC:	<acme@...nel.org>, <jolsa@...nel.org>, <namhyung@...nel.org>,
	<mingo@...hat.com>, <lizefan@...wei.com>, <pi3orama@....com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 2/2] perf: report/annotate: fix segfault problem.

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@...wei.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ