Date:	Thu, 18 Jun 2015 11:31:04 +0800
From:	Li Zhang <zhlcindy@...ux.vnet.ibm.com>
To:	Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>
CC:	acme@...nel.org, mingo@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] perf tools: Check access permission when reading
 /proc/kcore file.

On 2015年06月17日 14:09, Sukadev Bhattiprolu wrote:
> Li Zhang [zhlcindy@...ux.vnet.ibm.com] wrote:
> | When using command perf report --kallsyms=/proc/kallsyms with a non-root
> | user, symbols are resolved. Then select one symbol and annotate it, it
> | reports the error as the following:
> | Can't annotate __clear_user: No vmlinux file with build id xxx was
> | found.
> |
> | The problem is caused by reading /proc/kcore without access permission.
> | It needs to change access permission to allow a specific user to read
> | /proc/kcore or use root to execute the perf command.
> |
> | This patch is to check access permission when reading kcore file.
> |
> | Signed-off-by: Li Zhang <zhlcindy@...ux.vnet.ibm.com>
> | ---
> |  v2 -> v1:
> |     * Report one useful message to users about the access permision,
> |       then go back to the tools. Suggested by Arnaldo Carvalho de Melo.
> |
> |  tools/perf/util/symbol.c | 4 ++++
> |  1 file changed, 4 insertions(+)
> |
> | diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> | index 201f6c4c..1bcd8dc 100644
> | --- a/tools/perf/util/symbol.c
> | +++ b/tools/perf/util/symbol.c
> | @@ -1125,6 +1125,10 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
> |  	md.type = map->type;
> |  	INIT_LIST_HEAD(&md.maps);
> |
> | +	if (access(kcore_filename, R_OK))
> | +		ui__warning("Insufficient permission to access %s.\n",
> | +			    kcore_filename);
> | +
>
> Couple of comments.
>
> For consistency with rest of the file, use pr_warning() or pr_err().

ui_warning can report the message to users directly when this program is 
running.
But if we considered the consistency, pr_warning or pr_err should be better.
And users can get this message by trying another time.

>
> Also, we could drop the access() call and report the error when open()
> fails below?

I think we can drop this access. But /proc/kcore also require the 
process with CAP_SYS_RAWIO
capability. Even if chown this file, access report right result, but 
open still fails.

>
> |  	fd = open(kcore_filename, O_RDONLY);
> |  	if (fd < 0)
> |  		return -EINVAL;
>
> Further, if user specifies the file with --kallsyms and we are not
> able to read it, we should treat it as a fatal error and exit - this
> would be easer when parsing command line args.
I have another patch which checks this files. I will merge it to this patch.

>
> If user did not specify the option and we are proactively trying to
> use /proc/kcore, we should not treat errors as fatal? i.e report
> a warning message and continue without symbols?

In the current program, even if open fails, the program still continue 
to run.
Is it helpful for users to get the address without symbols?

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


-- 

Li Zhang
IBM China Linux Technology Centre

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