Re: [PATCH v2 1/1] perf tools: Check access permission when reading /proc/kcore file.

Re: [PATCH v2 1/1] perf tools: Check access permission when reading /proc/kcore file.

[Sukadev Bhattiprolu]

Li Zhang [zhlcindy@linux.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@linux.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(). 

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

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

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?

| -- 
| 2.1.0

Re: [PATCH v2 1/1] perf tools: Check access permission when reading /proc/kcore file.

[Li Zhang]

On 2015年06月17日 14:09, Sukadev Bhattiprolu wrote:
> Li Zhang [zhlcindy@linux.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@linux.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@vger.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

Re: [PATCH v2 1/1] perf tools: Check access permission when reading /proc/kcore file.

[Sukadev Bhattiprolu]

Li Zhang [zhlcindy@linux.vnet.ibm.com] wrote:

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

That seems to be the way perf currently operates - silent by default for
non-fatal errors. -v or -vvv increases verbosity and reports non-fatal
warnings/errors also.

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

Maybe the error message could hint that CAP_SYS_RAWIO would be needed.
| 
| >
| >|  	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?

Well, if profiling applications, user may not care about kernel symbols,
so being unable to open /proc/kcore would be ok? If OTOH, user specifies
--kallsyms, then they care about the kenrel symbols so we should treat
the open() error () as fatal.

Re: [PATCH v2 1/1] perf tools: Check access permission when reading /proc/kcore file.

[Li Zhang]

On 2015年06月18日 13:56, Sukadev Bhattiprolu wrote:
> Li Zhang [zhlcindy@linux.vnet.ibm.com] wrote:
>
> | >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.
>
> That seems to be the way perf currently operates - silent by default for
> non-fatal errors. -v or -vvv increases verbosity and reports non-fatal
> warnings/errors also.

OK, I will follow the way perf operates.

>
> |
> | >
> | >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.
>
> Maybe the error message could hint that CAP_SYS_RAWIO would be needed.

I will check this in my V3 patch.
> |
> | >
> | >|  	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?
>
> Well, if profiling applications, user may not care about kernel symbols,
> so being unable to open /proc/kcore would be ok? If OTOH, user specifies
> --kallsyms, then they care about the kenrel symbols so we should treat
> the open() error () as fatal.
I see, thanks. I will send out v3 soon.



-- 

Li Zhang
IBM China Linux Technology Centre