linux-kernel - Re: [PATCH perf/core v8 02/16] perf symbol: Cleanup the code flow of dso__find

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: <20160516133052.GB5475@kernel.org>
Date:	Mon, 16 May 2016 10:30:52 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Masami Hiramatsu <mhiramat@...nel.org>
Cc:	linux-kernel@...r.kernel.org, Namhyung Kim <namhyung@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Hemant Kumar <hemant@...ux.vnet.ibm.com>,
	Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>,
	Brendan Gregg <brendan.d.gregg@...il.com>
Subject: Re: [PATCH perf/core v8 02/16] perf symbol: Cleanup the code flow of
 dso__find_kallsyms

Em Sun, May 15, 2016 at 12:19:50PM +0900, Masami Hiramatsu escreveu:
> Cleanup the code flow of dso__find_kallsyms() to remove
> redundant checking code and add some comment for readability.

Thanks for adding the comment, I'll wait for Hemant to review this, if
you respin it, please break this patch into multiple ones, for
instance, the first should do just the filename__readable() part, then
try to check if you could split it further, to ease reviewing.

I applied the first patch already.

- Arnaldo
 
> Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
> ---
>  Changes in v8:
>   - Add comments why we can't use access(R_OK) for /proc/kcore access.
> ---
>  tools/perf/util/symbol.c |   68 ++++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 2252b54..abda8ba 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1641,6 +1641,20 @@ static int find_matching_kcore(struct map *map, char *dir, size_t dir_sz)
>  	return ret;
>  }
>  
> +/*
> + * Use open(O_RDONLY) to check readability directly instead of access(R_OK)
> + * since access(R_OK) only checks with real UID/GID but open() use effective
> + * UID/GID and actual capabilities (e.g. /proc/kcore requires CAP_SYS_RAWIO).
> + */
> +static bool filename__readable(const char *file)
> +{
> +	int fd = open(file, O_RDONLY);
> +	if (fd < 0)
> +		return false;
> +	close(fd);
> +	return true;
> +}
> +
>  static char *dso__find_kallsyms(struct dso *dso, struct map *map)
>  {
>  	u8 host_build_id[BUILD_ID_SIZE];
> @@ -1660,45 +1674,36 @@ static char *dso__find_kallsyms(struct dso *dso, struct map *map)
>  				 sizeof(host_build_id)) == 0)
>  		is_host = dso__build_id_equal(dso, host_build_id);
>  
> -	build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id);
> -
> -	scnprintf(path, sizeof(path), "%s/%s/%s", buildid_dir,
> -		  DSO__NAME_KCORE, sbuild_id);
> -
> -	/* Use /proc/kallsyms if possible */
> +	/* Try a fast path for /proc/kallsyms if possible */
>  	if (is_host) {
> -		DIR *d;
> -		int fd;
> -
> -		/* If no cached kcore go with /proc/kallsyms */
> -		d = opendir(path);
> -		if (!d)
> -			goto proc_kallsyms;
> -		closedir(d);
> -
>  		/*
> -		 * Do not check the build-id cache, until we know we cannot use
> -		 * /proc/kcore.
> +		 * Do not check the build-id cache, unless we know we cannot use
> +		 * /proc/kcore or module maps don't match to /proc/kallsyms.
> +		 * To check readability of /proc/kcore, do not use access(R_OK)
> +		 * since /proc/kcore requires CAP_SYS_RAWIO to read and access
> +		 * can't check it.
>  		 */
> -		fd = open("/proc/kcore", O_RDONLY);
> -		if (fd != -1) {
> -			close(fd);
> -			/* If module maps match go with /proc/kallsyms */
> -			if (!validate_kcore_addresses("/proc/kallsyms", map))
> -				goto proc_kallsyms;
> -		}
> -
> -		/* Find kallsyms in build-id cache with kcore */
> -		if (!find_matching_kcore(map, path, sizeof(path)))
> -			return strdup(path);
> -
> -		goto proc_kallsyms;
> +		if (filename__readable("/proc/kcore") &&
> +		    !validate_kcore_addresses("/proc/kallsyms", map))
> +			goto proc_kallsyms;
>  	}
>  
> +	build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id);
> +
>  	/* Find kallsyms in build-id cache with kcore */
> +	scnprintf(path, sizeof(path), "%s/%s/%s",
> +		  buildid_dir, DSO__NAME_KCORE, sbuild_id);
> +
>  	if (!find_matching_kcore(map, path, sizeof(path)))
>  		return strdup(path);
>  
> +	/* Use current /proc/kallsyms if possible */
> +	if (is_host) {
> +proc_kallsyms:
> +		return strdup("/proc/kallsyms");
> +	}
> +
> +	/* Finally, find a cache of kallsyms */
>  	scnprintf(path, sizeof(path), "%s/%s/%s",
>  		  buildid_dir, DSO__NAME_KALLSYMS, sbuild_id);
>  
> @@ -1709,9 +1714,6 @@ static char *dso__find_kallsyms(struct dso *dso, struct map *map)
>  	}
>  
>  	return strdup(path);
> -
> -proc_kallsyms:
> -	return strdup("/proc/kallsyms");
>  }
>  
>  static int dso__load_kernel_sym(struct dso *dso, struct map *map,