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

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: <20150409115231.GC1321@krava.brq.redhat.com>
Date:	Thu, 9 Apr 2015 13:52:31 +0200
From:	Jiri Olsa <jolsa@...hat.com>
To:	Wang Nan <wangnan0@...wei.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 v4 2/2] perf: report/annotate: fix segfault problem.

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