[PATCH 0/2] perf probe: Report permission errors

[PATCH 0/2] perf probe: Report permission errors

[Masami Hiramatsu]

Hi Arnaldo and Ravi,

Here is a couple of patches to improve error message for the permission
errors on perf probe, which we talked in the following thread;

 https://lore.kernel.org/lkml/20210525043744.193297-1-ravi.bangoria@linux.ibm.com/

Thank you,

---

Masami Hiramatsu (2):
      perf/probe: Report possible permission error for map__load failure
      perf/probe: Report permission error for tracefs error


 tools/perf/util/probe-event.c |   25 ++++++++++++++++++++++---
 tools/perf/util/probe-file.c  |    4 ++++
 2 files changed, 26 insertions(+), 3 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH 1/2] perf/probe: Report possible permission error for map__load failure

[Masami Hiramatsu]

Report possible permission error including kptr_restrict setting
for map__load() failure. This can happen when non-superuser runs
perf probe.

With this patch, perf probe shows the following message.

 $ perf probe vfs_read
 Failed to load symbols from /proc/kallsyms
 Please ensure you can read the /proc/kallsyms symbol addresses.
 If the /proc/sys/kernel/kptr_restrict is '2', you can not read
 kernel symbol address even if you are a superuser. Please change
 it to '1'. If kptr_restrict is '1', the superuser can read the
 symbol addresses.
 In that case, please run this command again with sudo.
   Error: Failed to add events.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/probe-event.c |   25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 3a7649835ec9..8fe179d671c3 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2936,7 +2936,7 @@ static int find_probe_functions(struct map *map, char *name,
 	bool cut_version = true;
 
 	if (map__load(map) < 0)
-		return 0;
+		return -EACCES;	/* Possible permission error to load symbols */
 
 	/* If user gives a version, don't cut off the version from symbols */
 	if (strchr(name, '@'))
@@ -2975,6 +2975,17 @@ void __weak arch__fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
 				struct map *map __maybe_unused,
 				struct symbol *sym __maybe_unused) { }
 
+
+static void pr_kallsyms_access_error(void)
+{
+	pr_err("Please ensure you can read the /proc/kallsyms symbol addresses.\n"
+	       "If the /proc/sys/kernel/kptr_restrict is '2', you can not read\n"
+	       "kernel symbol address even if you are a superuser. Please change\n"
+	       "it to '1'. If kptr_restrict is '1', the superuser can read the\n"
+	       "symbol addresses.\n"
+	       "In that case, please run this command again with sudo.\n");
+}
+
 /*
  * Find probe function addresses from map.
  * Return an error or the number of found probe_trace_event
@@ -3011,8 +3022,16 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	 */
 	num_matched_functions = find_probe_functions(map, pp->function, syms);
 	if (num_matched_functions <= 0) {
-		pr_err("Failed to find symbol %s in %s\n", pp->function,
-			pev->target ? : "kernel");
+		if (num_matched_functions == -EACCES) {
+			pr_err("Failed to load symbols from %s\n",
+			       pev->target ?: "/proc/kallsyms");
+			if (pev->target)
+				pr_err("Please ensure the file is not stripped.\n");
+			else
+				pr_kallsyms_access_error();
+		} else
+			pr_err("Failed to find symbol %s in %s\n", pp->function,
+				pev->target ? : "kernel");
 		ret = -ENOENT;
 		goto out;
 	} else if (num_matched_functions > probe_conf.max_probes) {

[PATCH 2/2] perf/probe: Report permission error for tracefs error

[Masami Hiramatsu]

Report permission error for the tracefs access error.
This can happen when non-superuser runs perf probe.
With this patch, perf probe shows the following message.

  $ perf probe -l
  No permission to access tracefs. Please run this command again with sudo.
    Error: Failed to show event list.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/perf/util/probe-file.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 52273542e6ef..52d878f5a44d 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -48,6 +48,8 @@ static void print_open_warning(int err, bool uprobe)
 			   uprobe ? 'u' : 'k', config);
 	} else if (err == -ENOTSUP)
 		pr_warning("Tracefs or debugfs is not mounted.\n");
+	else if (err == -EACCES)
+		pr_warning("No permission to access tracefs. Please run this command again with sudo.\n");
 	else
 		pr_warning("Failed to open %cprobe_events: %s\n",
 			   uprobe ? 'u' : 'k',
@@ -62,6 +64,8 @@ static void print_both_open_warning(int kerr, int uerr)
 	else if (kerr == -ENOENT && uerr == -ENOENT)
 		pr_warning("Please rebuild kernel with CONFIG_KPROBE_EVENTS "
 			   "or/and CONFIG_UPROBE_EVENTS.\n");
+	else if (kerr == -EACCES && uerr == -EACCES)
+		pr_warning("No permission to access tracefs. Please run this command again with sudo.\n");
 	else {
 		char sbuf[STRERR_BUFSIZE];
 		pr_warning("Failed to open kprobe events: %s.\n",

Re: [PATCH 2/2] perf/probe: Report permission error for tracefs error

[Arnaldo Carvalho de Melo]

Em Sat, Jun 05, 2021 at 12:31:08AM +0900, Masami Hiramatsu escreveu:
> Report permission error for the tracefs access error.
> This can happen when non-superuser runs perf probe.
> With this patch, perf probe shows the following message.
> 
>   $ perf probe -l
>   No permission to access tracefs. Please run this command again with sudo.
>     Error: Failed to show event list.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  tools/perf/util/probe-file.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 52273542e6ef..52d878f5a44d 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -48,6 +48,8 @@ static void print_open_warning(int err, bool uprobe)
>  			   uprobe ? 'u' : 'k', config);
>  	} else if (err == -ENOTSUP)
>  		pr_warning("Tracefs or debugfs is not mounted.\n");
> +	else if (err == -EACCES)
> +		pr_warning("No permission to access tracefs. Please run this command again with sudo.\n");
>  	else
>  		pr_warning("Failed to open %cprobe_events: %s\n",
>  			   uprobe ? 'u' : 'k',
> @@ -62,6 +64,8 @@ static void print_both_open_warning(int kerr, int uerr)
>  	else if (kerr == -ENOENT && uerr == -ENOENT)
>  		pr_warning("Please rebuild kernel with CONFIG_KPROBE_EVENTS "
>  			   "or/and CONFIG_UPROBE_EVENTS.\n");
> +	else if (kerr == -EACCES && uerr == -EACCES)
> +		pr_warning("No permission to access tracefs. Please run this command again with sudo.\n");
>  	else {
>  		char sbuf[STRERR_BUFSIZE];
>  		pr_warning("Failed to open kprobe events: %s.\n",

This one doesn't look so helpful, as running as root usually will allow
things to proceed.

'perf trace' does:

⬢[acme@toolbox pahole]$ perf trace ls
Error:	No permissions to read /sys/kernel/tracing/events/raw_syscalls/sys_(enter|exit)
Hint:	Try 'sudo mount -o remount,mode=755 /sys/kernel/tracing/'

⬢[acme@toolbox pahole]$

Which would be less drastic than requiring full superuser access.

- Arnaldo

Re: [PATCH 1/2] perf/probe: Report possible permission error for map__load failure

[Arnaldo Carvalho de Melo]

Em Sat, Jun 05, 2021 at 12:30:58AM +0900, Masami Hiramatsu escreveu:
> Report possible permission error including kptr_restrict setting
> for map__load() failure. This can happen when non-superuser runs
> perf probe.
> 
> With this patch, perf probe shows the following message.
> 
>  $ perf probe vfs_read
>  Failed to load symbols from /proc/kallsyms
>  Please ensure you can read the /proc/kallsyms symbol addresses.
>  If the /proc/sys/kernel/kptr_restrict is '2', you can not read
>  kernel symbol address even if you are a superuser. Please change
>  it to '1'. If kptr_restrict is '1', the superuser can read the
>  symbol addresses.
>  In that case, please run this command again with sudo.
>    Error: Failed to add events.

Thanks, applied.

- Arnaldo

 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  tools/perf/util/probe-event.c |   25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 3a7649835ec9..8fe179d671c3 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2936,7 +2936,7 @@ static int find_probe_functions(struct map *map, char *name,
>  	bool cut_version = true;
>  
>  	if (map__load(map) < 0)
> -		return 0;
> +		return -EACCES;	/* Possible permission error to load symbols */
>  
>  	/* If user gives a version, don't cut off the version from symbols */
>  	if (strchr(name, '@'))
> @@ -2975,6 +2975,17 @@ void __weak arch__fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
>  				struct map *map __maybe_unused,
>  				struct symbol *sym __maybe_unused) { }
>  
> +
> +static void pr_kallsyms_access_error(void)
> +{
> +	pr_err("Please ensure you can read the /proc/kallsyms symbol addresses.\n"
> +	       "If the /proc/sys/kernel/kptr_restrict is '2', you can not read\n"
> +	       "kernel symbol address even if you are a superuser. Please change\n"
> +	       "it to '1'. If kptr_restrict is '1', the superuser can read the\n"
> +	       "symbol addresses.\n"
> +	       "In that case, please run this command again with sudo.\n");
> +}
> +
>  /*
>   * Find probe function addresses from map.
>   * Return an error or the number of found probe_trace_event
> @@ -3011,8 +3022,16 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  	 */
>  	num_matched_functions = find_probe_functions(map, pp->function, syms);
>  	if (num_matched_functions <= 0) {
> -		pr_err("Failed to find symbol %s in %s\n", pp->function,
> -			pev->target ? : "kernel");
> +		if (num_matched_functions == -EACCES) {
> +			pr_err("Failed to load symbols from %s\n",
> +			       pev->target ?: "/proc/kallsyms");
> +			if (pev->target)
> +				pr_err("Please ensure the file is not stripped.\n");
> +			else
> +				pr_kallsyms_access_error();
> +		} else
> +			pr_err("Failed to find symbol %s in %s\n", pp->function,
> +				pev->target ? : "kernel");
>  		ret = -ENOENT;
>  		goto out;
>  	} else if (num_matched_functions > probe_conf.max_probes) {
> 

-- 

- Arnaldo

Re: [PATCH 2/2] perf/probe: Report permission error for tracefs error

[Masami Hiramatsu]

Hi Arnaldo,

On Fri, 4 Jun 2021 16:18:34 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Sat, Jun 05, 2021 at 12:31:08AM +0900, Masami Hiramatsu escreveu:
> > Report permission error for the tracefs access error.
> > This can happen when non-superuser runs perf probe.
> > With this patch, perf probe shows the following message.
> > 
> >   $ perf probe -l
> >   No permission to access tracefs. Please run this command again with sudo.
> >     Error: Failed to show event list.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  tools/perf/util/probe-file.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> > index 52273542e6ef..52d878f5a44d 100644
> > --- a/tools/perf/util/probe-file.c
> > +++ b/tools/perf/util/probe-file.c
> > @@ -48,6 +48,8 @@ static void print_open_warning(int err, bool uprobe)
> >  			   uprobe ? 'u' : 'k', config);
> >  	} else if (err == -ENOTSUP)
> >  		pr_warning("Tracefs or debugfs is not mounted.\n");
> > +	else if (err == -EACCES)
> > +		pr_warning("No permission to access tracefs. Please run this command again with sudo.\n");
> >  	else
> >  		pr_warning("Failed to open %cprobe_events: %s\n",
> >  			   uprobe ? 'u' : 'k',
> > @@ -62,6 +64,8 @@ static void print_both_open_warning(int kerr, int uerr)
> >  	else if (kerr == -ENOENT && uerr == -ENOENT)
> >  		pr_warning("Please rebuild kernel with CONFIG_KPROBE_EVENTS "
> >  			   "or/and CONFIG_UPROBE_EVENTS.\n");
> > +	else if (kerr == -EACCES && uerr == -EACCES)
> > +		pr_warning("No permission to access tracefs. Please run this command again with sudo.\n");
> >  	else {
> >  		char sbuf[STRERR_BUFSIZE];
> >  		pr_warning("Failed to open kprobe events: %s.\n",
> 
> This one doesn't look so helpful, as running as root usually will allow
> things to proceed.
> 
> 'perf trace' does:
> 
> ⬢[acme@toolbox pahole]$ perf trace ls
> Error:	No permissions to read /sys/kernel/tracing/events/raw_syscalls/sys_(enter|exit)
> Hint:	Try 'sudo mount -o remount,mode=755 /sys/kernel/tracing/'
> 
> ⬢[acme@toolbox pahole]$
> 
> Which would be less drastic than requiring full superuser access.

Hmm, perf trace only read access to the tracefs, so that is easy to
suggest user to do remount it. However, perf probe usually requires
a write access. (Only perf probe -l requires read access)

I'll change this patch to check whether the read or write access and
switch the message. But if it is a write access, anyway it has to
requests superuser.

Let me update this patch.

Thank you,

> 
> - Arnaldo


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 1/2] perf/probe: Report possible permission error for map__load failure

[Namhyung Kim]

Hi Arnaldo and Masami,

On Fri, Jun 4, 2021 at 12:18 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Sat, Jun 05, 2021 at 12:30:58AM +0900, Masami Hiramatsu escreveu:
> > Report possible permission error including kptr_restrict setting
> > for map__load() failure. This can happen when non-superuser runs
> > perf probe.
> >
> > With this patch, perf probe shows the following message.
> >
> >  $ perf probe vfs_read
> >  Failed to load symbols from /proc/kallsyms
> >  Please ensure you can read the /proc/kallsyms symbol addresses.
> >  If the /proc/sys/kernel/kptr_restrict is '2', you can not read
> >  kernel symbol address even if you are a superuser. Please change
> >  it to '1'. If kptr_restrict is '1', the superuser can read the
> >  symbol addresses.
> >  In that case, please run this command again with sudo.
> >    Error: Failed to add events.

Maybe we can read the kptr_restrict file and (e)uid to suggest
the specific message for the situation only.

Thanks,
Namhyung

>
>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  tools/perf/util/probe-event.c |   25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 3a7649835ec9..8fe179d671c3 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -2936,7 +2936,7 @@ static int find_probe_functions(struct map *map, char *name,
> >       bool cut_version = true;
> >
> >       if (map__load(map) < 0)
> > -             return 0;
> > +             return -EACCES; /* Possible permission error to load symbols */
> >
> >       /* If user gives a version, don't cut off the version from symbols */
> >       if (strchr(name, '@'))
> > @@ -2975,6 +2975,17 @@ void __weak arch__fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
> >                               struct map *map __maybe_unused,
> >                               struct symbol *sym __maybe_unused) { }
> >
> > +
> > +static void pr_kallsyms_access_error(void)
> > +{
> > +     pr_err("Please ensure you can read the /proc/kallsyms symbol addresses.\n"
> > +            "If the /proc/sys/kernel/kptr_restrict is '2', you can not read\n"
> > +            "kernel symbol address even if you are a superuser. Please change\n"
> > +            "it to '1'. If kptr_restrict is '1', the superuser can read the\n"
> > +            "symbol addresses.\n"
> > +            "In that case, please run this command again with sudo.\n");
> > +}
> > +
> >  /*
> >   * Find probe function addresses from map.
> >   * Return an error or the number of found probe_trace_event
> > @@ -3011,8 +3022,16 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
> >        */
> >       num_matched_functions = find_probe_functions(map, pp->function, syms);
> >       if (num_matched_functions <= 0) {
> > -             pr_err("Failed to find symbol %s in %s\n", pp->function,
> > -                     pev->target ? : "kernel");
> > +             if (num_matched_functions == -EACCES) {
> > +                     pr_err("Failed to load symbols from %s\n",
> > +                            pev->target ?: "/proc/kallsyms");
> > +                     if (pev->target)
> > +                             pr_err("Please ensure the file is not stripped.\n");
> > +                     else
> > +                             pr_kallsyms_access_error();
> > +             } else
> > +                     pr_err("Failed to find symbol %s in %s\n", pp->function,
> > +                             pev->target ? : "kernel");
> >               ret = -ENOENT;
> >               goto out;
> >       } else if (num_matched_functions > probe_conf.max_probes) {
> >
>
> --
>
> - Arnaldo

[PATCH v2] perf/probe: Report permission error for tracefs access

[Masami Hiramatsu]

Report permission error for the tracefs open and rewrite
whole the error message code around it.
You'll see a hint according to what you want to do with
perf probe as below.

  $ perf probe -l
  No permission to read tracefs.
  Please try 'sudo mount -o remount,mode=755 /sys/kernel/tracing/'
    Error: Failed to show event list.

  $ perf probe -d \*
  No permission to write tracefs.
  Please run this command again with sudo.
    Error: Failed to delete events.

This also fixes -ENOTSUP checking for mounting tracefs/debugfs.
Actually open returns -ENOENT in that case and we have to check
it with current mount point list. If we unmount debugfs and tracefs
perf probe shows correct message as below.

  $ perf probe -l
  Debugfs or tracefs is not mounted
  Please try 'sudo mount -t tracefs nodev /sys/kernel/tracing/'
    Error: Failed to show event list.


Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v2:
  - Rewrite whole the error message for the tracefs access.
---
 tools/perf/util/probe-file.c |   95 +++++++++++++++++++++++++++++-------------
 1 file changed, 65 insertions(+), 30 deletions(-)

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 52273542e6ef..f9a6cbcd6415 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -22,6 +22,7 @@
 #include "symbol.h"
 #include "strbuf.h"
 #include <api/fs/tracing_path.h>
+#include <api/fs/fs.h>
 #include "probe-event.h"
 #include "probe-file.h"
 #include "session.h"
@@ -31,44 +32,78 @@
 /* 4096 - 2 ('\n' + '\0') */
 #define MAX_CMDLEN 4094
 
-static void print_open_warning(int err, bool uprobe)
+static bool print_common_warning(int err, bool readwrite)
 {
-	char sbuf[STRERR_BUFSIZE];
+	if (err == -EACCES)
+		pr_warning("No permission to %s tracefs.\nPlease %s\n",
+			   readwrite ? "write" : "read",
+			   readwrite ? "run this command again with sudo." :
+				       "try 'sudo mount -o remount,mode=755 /sys/kernel/tracing/'");
+	else
+		return false;
 
-	if (err == -ENOENT) {
-		const char *config;
+	return true;
+}
 
-		if (uprobe)
-			config = "CONFIG_UPROBE_EVENTS";
-		else
-			config = "CONFIG_KPROBE_EVENTS";
+static bool print_configure_probe_event(int kerr, int uerr)
+{
+	const char *config, *file;
+
+	if (kerr == -ENOENT && uerr == -ENOENT) {
+		file = "{k,u}probe_events";
+		config = "CONFIG_KPROBE_EVENTS=y and CONFIG_UPROBE_EVENTS=y";
+	} else if (kerr == -ENOENT) {
+		file = "kprobe_events";
+		config = "CONFIG_KPROBE_EVENTS=y";
+	} else if (uerr == -ENOENT) {
+		file = "uprobe_events";
+		config = "CONFIG_UPROBE_EVENTS=y";
+	} else
+		return false;
 
-		pr_warning("%cprobe_events file does not exist"
-			   " - please rebuild kernel with %s.\n",
-			   uprobe ? 'u' : 'k', config);
-	} else if (err == -ENOTSUP)
-		pr_warning("Tracefs or debugfs is not mounted.\n");
+	if (!debugfs__configured() && !tracefs__configured())
+		pr_warning("Debugfs or tracefs is not mounted\n"
+			   "Please try 'sudo mount -t tracefs nodev /sys/kernel/tracing/'\n");
 	else
-		pr_warning("Failed to open %cprobe_events: %s\n",
-			   uprobe ? 'u' : 'k',
-			   str_error_r(-err, sbuf, sizeof(sbuf)));
+		pr_warning("%s/%s does not exist.\nPlease rebuild kernel with %s.\n",
+			   tracing_path_mount(), file, config);
+
+	return true;
+}
+
+static void print_open_warning(int err, bool uprobe, bool readwrite)
+{
+	char sbuf[STRERR_BUFSIZE];
+
+	if (print_common_warning(err, readwrite))
+		return;
+
+	if (print_configure_probe_event(uprobe ? 0 : err, uprobe ? err : 0))
+		return;
+
+	pr_warning("Failed to open %s/%cprobe_events: %s\n",
+		   tracing_path_mount(), uprobe ? 'u' : 'k',
+		   str_error_r(-err, sbuf, sizeof(sbuf)));
 }
 
-static void print_both_open_warning(int kerr, int uerr)
+static void print_both_open_warning(int kerr, int uerr, bool readwrite)
 {
-	/* Both kprobes and uprobes are disabled, warn it. */
-	if (kerr == -ENOTSUP && uerr == -ENOTSUP)
-		pr_warning("Tracefs or debugfs is not mounted.\n");
-	else if (kerr == -ENOENT && uerr == -ENOENT)
-		pr_warning("Please rebuild kernel with CONFIG_KPROBE_EVENTS "
-			   "or/and CONFIG_UPROBE_EVENTS.\n");
-	else {
-		char sbuf[STRERR_BUFSIZE];
-		pr_warning("Failed to open kprobe events: %s.\n",
+	char sbuf[STRERR_BUFSIZE];
+
+	if (kerr == uerr && print_common_warning(kerr, readwrite))
+		return;
+
+	if (print_configure_probe_event(kerr, uerr))
+		return;
+
+	if (kerr < 0)
+		pr_warning("Failed to open %s/kprobe_events: %s.\n",
+			   tracing_path_mount(),
 			   str_error_r(-kerr, sbuf, sizeof(sbuf)));
-		pr_warning("Failed to open uprobe events: %s.\n",
+	if (uerr < 0)
+		pr_warning("Failed to open %s/uprobe_events: %s.\n",
+			   tracing_path_mount(),
 			   str_error_r(-uerr, sbuf, sizeof(sbuf)));
-	}
 }
 
 int open_trace_file(const char *trace_file, bool readwrite)
@@ -109,7 +144,7 @@ int probe_file__open(int flag)
 	else
 		fd = open_kprobe_events(flag & PF_FL_RW);
 	if (fd < 0)
-		print_open_warning(fd, flag & PF_FL_UPROBE);
+		print_open_warning(fd, flag & PF_FL_UPROBE, flag & PF_FL_RW);
 
 	return fd;
 }
@@ -122,7 +157,7 @@ int probe_file__open_both(int *kfd, int *ufd, int flag)
 	*kfd = open_kprobe_events(flag & PF_FL_RW);
 	*ufd = open_uprobe_events(flag & PF_FL_RW);
 	if (*kfd < 0 && *ufd < 0) {
-		print_both_open_warning(*kfd, *ufd);
+		print_both_open_warning(*kfd, *ufd, flag & PF_FL_RW);
 		return *kfd;
 	}
 

Re: [PATCH 1/2] perf/probe: Report possible permission error for map__load failure

[Masami Hiramatsu]

On Sun, 6 Jun 2021 00:11:31 -0700
Namhyung Kim <namhyung@kernel.org> wrote:

> Hi Arnaldo and Masami,
> 
> On Fri, Jun 4, 2021 at 12:18 PM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Sat, Jun 05, 2021 at 12:30:58AM +0900, Masami Hiramatsu escreveu:
> > > Report possible permission error including kptr_restrict setting
> > > for map__load() failure. This can happen when non-superuser runs
> > > perf probe.
> > >
> > > With this patch, perf probe shows the following message.
> > >
> > >  $ perf probe vfs_read
> > >  Failed to load symbols from /proc/kallsyms
> > >  Please ensure you can read the /proc/kallsyms symbol addresses.
> > >  If the /proc/sys/kernel/kptr_restrict is '2', you can not read
> > >  kernel symbol address even if you are a superuser. Please change
> > >  it to '1'. If kptr_restrict is '1', the superuser can read the
> > >  symbol addresses.
> > >  In that case, please run this command again with sudo.
> > >    Error: Failed to add events.
> 
> Maybe we can read the kptr_restrict file and (e)uid to suggest
> the specific message for the situation only.

Agreed, and that should be done in map__load(), since it returns -1
for any error. Caller needs to estimate the reason.

If each caller does it, those have to repeat same estimation and
similar messages in different place. Maybe we need to have a global
error and hint object so that caller can show it when it detects
an error.

Thank you,


> Thanks,
> Namhyung
> 
> >
> >
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > ---
> > >  tools/perf/util/probe-event.c |   25 ++++++++++++++++++++++---
> > >  1 file changed, 22 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > > index 3a7649835ec9..8fe179d671c3 100644
> > > --- a/tools/perf/util/probe-event.c
> > > +++ b/tools/perf/util/probe-event.c
> > > @@ -2936,7 +2936,7 @@ static int find_probe_functions(struct map *map, char *name,
> > >       bool cut_version = true;
> > >
> > >       if (map__load(map) < 0)
> > > -             return 0;
> > > +             return -EACCES; /* Possible permission error to load symbols */
> > >
> > >       /* If user gives a version, don't cut off the version from symbols */
> > >       if (strchr(name, '@'))
> > > @@ -2975,6 +2975,17 @@ void __weak arch__fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
> > >                               struct map *map __maybe_unused,
> > >                               struct symbol *sym __maybe_unused) { }
> > >
> > > +
> > > +static void pr_kallsyms_access_error(void)
> > > +{
> > > +     pr_err("Please ensure you can read the /proc/kallsyms symbol addresses.\n"
> > > +            "If the /proc/sys/kernel/kptr_restrict is '2', you can not read\n"
> > > +            "kernel symbol address even if you are a superuser. Please change\n"
> > > +            "it to '1'. If kptr_restrict is '1', the superuser can read the\n"
> > > +            "symbol addresses.\n"
> > > +            "In that case, please run this command again with sudo.\n");
> > > +}
> > > +
> > >  /*
> > >   * Find probe function addresses from map.
> > >   * Return an error or the number of found probe_trace_event
> > > @@ -3011,8 +3022,16 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
> > >        */
> > >       num_matched_functions = find_probe_functions(map, pp->function, syms);
> > >       if (num_matched_functions <= 0) {
> > > -             pr_err("Failed to find symbol %s in %s\n", pp->function,
> > > -                     pev->target ? : "kernel");
> > > +             if (num_matched_functions == -EACCES) {
> > > +                     pr_err("Failed to load symbols from %s\n",
> > > +                            pev->target ?: "/proc/kallsyms");
> > > +                     if (pev->target)
> > > +                             pr_err("Please ensure the file is not stripped.\n");
> > > +                     else
> > > +                             pr_kallsyms_access_error();
> > > +             } else
> > > +                     pr_err("Failed to find symbol %s in %s\n", pp->function,
> > > +                             pev->target ? : "kernel");
> > >               ret = -ENOENT;
> > >               goto out;
> > >       } else if (num_matched_functions > probe_conf.max_probes) {
> > >
> >
> > --
> >
> > - Arnaldo


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 1/2] perf/probe: Report possible permission error for map__load failure

[Arnaldo Carvalho de Melo]

Em Mon, Jun 07, 2021 at 09:00:34AM +0900, Masami Hiramatsu escreveu:
> On Sun, 6 Jun 2021 00:11:31 -0700
> Namhyung Kim <namhyung@kernel.org> wrote:
> 
> > Hi Arnaldo and Masami,
> > 
> > On Fri, Jun 4, 2021 at 12:18 PM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > Em Sat, Jun 05, 2021 at 12:30:58AM +0900, Masami Hiramatsu escreveu:
> > > > Report possible permission error including kptr_restrict setting
> > > > for map__load() failure. This can happen when non-superuser runs
> > > > perf probe.
> > > >
> > > > With this patch, perf probe shows the following message.
> > > >
> > > >  $ perf probe vfs_read
> > > >  Failed to load symbols from /proc/kallsyms
> > > >  Please ensure you can read the /proc/kallsyms symbol addresses.
> > > >  If the /proc/sys/kernel/kptr_restrict is '2', you can not read
> > > >  kernel symbol address even if you are a superuser. Please change
> > > >  it to '1'. If kptr_restrict is '1', the superuser can read the
> > > >  symbol addresses.
> > > >  In that case, please run this command again with sudo.
> > > >    Error: Failed to add events.
> > 
> > Maybe we can read the kptr_restrict file and (e)uid to suggest
> > the specific message for the situation only.
> 
> Agreed, and that should be done in map__load(), since it returns -1
> for any error. Caller needs to estimate the reason.

Then we need to change it to return a errno so that the caller can react
accordingly.

- Arnaldo
 
> If each caller does it, those have to repeat same estimation and
> similar messages in different place. Maybe we need to have a global
> error and hint object so that caller can show it when it detects
> an error.
> 
> Thank you,
> 
> 
> > Thanks,
> > Namhyung
> > 
> > >
> > >
> > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > > ---
> > > >  tools/perf/util/probe-event.c |   25 ++++++++++++++++++++++---
> > > >  1 file changed, 22 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > > > index 3a7649835ec9..8fe179d671c3 100644
> > > > --- a/tools/perf/util/probe-event.c
> > > > +++ b/tools/perf/util/probe-event.c
> > > > @@ -2936,7 +2936,7 @@ static int find_probe_functions(struct map *map, char *name,
> > > >       bool cut_version = true;
> > > >
> > > >       if (map__load(map) < 0)
> > > > -             return 0;
> > > > +             return -EACCES; /* Possible permission error to load symbols */
> > > >
> > > >       /* If user gives a version, don't cut off the version from symbols */
> > > >       if (strchr(name, '@'))
> > > > @@ -2975,6 +2975,17 @@ void __weak arch__fix_tev_from_maps(struct perf_probe_event *pev __maybe_unused,
> > > >                               struct map *map __maybe_unused,
> > > >                               struct symbol *sym __maybe_unused) { }
> > > >
> > > > +
> > > > +static void pr_kallsyms_access_error(void)
> > > > +{
> > > > +     pr_err("Please ensure you can read the /proc/kallsyms symbol addresses.\n"
> > > > +            "If the /proc/sys/kernel/kptr_restrict is '2', you can not read\n"
> > > > +            "kernel symbol address even if you are a superuser. Please change\n"
> > > > +            "it to '1'. If kptr_restrict is '1', the superuser can read the\n"
> > > > +            "symbol addresses.\n"
> > > > +            "In that case, please run this command again with sudo.\n");
> > > > +}
> > > > +
> > > >  /*
> > > >   * Find probe function addresses from map.
> > > >   * Return an error or the number of found probe_trace_event
> > > > @@ -3011,8 +3022,16 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
> > > >        */
> > > >       num_matched_functions = find_probe_functions(map, pp->function, syms);
> > > >       if (num_matched_functions <= 0) {
> > > > -             pr_err("Failed to find symbol %s in %s\n", pp->function,
> > > > -                     pev->target ? : "kernel");
> > > > +             if (num_matched_functions == -EACCES) {
> > > > +                     pr_err("Failed to load symbols from %s\n",
> > > > +                            pev->target ?: "/proc/kallsyms");
> > > > +                     if (pev->target)
> > > > +                             pr_err("Please ensure the file is not stripped.\n");
> > > > +                     else
> > > > +                             pr_kallsyms_access_error();
> > > > +             } else
> > > > +                     pr_err("Failed to find symbol %s in %s\n", pp->function,
> > > > +                             pev->target ? : "kernel");
> > > >               ret = -ENOENT;
> > > >               goto out;
> > > >       } else if (num_matched_functions > probe_conf.max_probes) {
> > > >
> > >
> > > --
> > >
> > > - Arnaldo
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

-- 

- Arnaldo

Re: [PATCH v2] perf/probe: Report permission error for tracefs access

[Arnaldo Carvalho de Melo]

Em Mon, Jun 07, 2021 at 12:49:28AM +0900, Masami Hiramatsu escreveu:
> Report permission error for the tracefs open and rewrite
> whole the error message code around it.
> You'll see a hint according to what you want to do with
> perf probe as below.
> 
>   $ perf probe -l
>   No permission to read tracefs.
>   Please try 'sudo mount -o remount,mode=755 /sys/kernel/tracing/'
>     Error: Failed to show event list.
> 
>   $ perf probe -d \*
>   No permission to write tracefs.
>   Please run this command again with sudo.
>     Error: Failed to delete events.
> 
> This also fixes -ENOTSUP checking for mounting tracefs/debugfs.
> Actually open returns -ENOENT in that case and we have to check
> it with current mount point list. If we unmount debugfs and tracefs
> perf probe shows correct message as below.
> 
>   $ perf probe -l
>   Debugfs or tracefs is not mounted
>   Please try 'sudo mount -t tracefs nodev /sys/kernel/tracing/'
>     Error: Failed to show event list.

Thanks, applied.

- Arnaldo

 
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  Changes in v2:
>   - Rewrite whole the error message for the tracefs access.
> ---
>  tools/perf/util/probe-file.c |   95 +++++++++++++++++++++++++++++-------------
>  1 file changed, 65 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 52273542e6ef..f9a6cbcd6415 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -22,6 +22,7 @@
>  #include "symbol.h"
>  #include "strbuf.h"
>  #include <api/fs/tracing_path.h>
> +#include <api/fs/fs.h>
>  #include "probe-event.h"
>  #include "probe-file.h"
>  #include "session.h"
> @@ -31,44 +32,78 @@
>  /* 4096 - 2 ('\n' + '\0') */
>  #define MAX_CMDLEN 4094
>  
> -static void print_open_warning(int err, bool uprobe)
> +static bool print_common_warning(int err, bool readwrite)
>  {
> -	char sbuf[STRERR_BUFSIZE];
> +	if (err == -EACCES)
> +		pr_warning("No permission to %s tracefs.\nPlease %s\n",
> +			   readwrite ? "write" : "read",
> +			   readwrite ? "run this command again with sudo." :
> +				       "try 'sudo mount -o remount,mode=755 /sys/kernel/tracing/'");
> +	else
> +		return false;
>  
> -	if (err == -ENOENT) {
> -		const char *config;
> +	return true;
> +}
>  
> -		if (uprobe)
> -			config = "CONFIG_UPROBE_EVENTS";
> -		else
> -			config = "CONFIG_KPROBE_EVENTS";
> +static bool print_configure_probe_event(int kerr, int uerr)
> +{
> +	const char *config, *file;
> +
> +	if (kerr == -ENOENT && uerr == -ENOENT) {
> +		file = "{k,u}probe_events";
> +		config = "CONFIG_KPROBE_EVENTS=y and CONFIG_UPROBE_EVENTS=y";
> +	} else if (kerr == -ENOENT) {
> +		file = "kprobe_events";
> +		config = "CONFIG_KPROBE_EVENTS=y";
> +	} else if (uerr == -ENOENT) {
> +		file = "uprobe_events";
> +		config = "CONFIG_UPROBE_EVENTS=y";
> +	} else
> +		return false;
>  
> -		pr_warning("%cprobe_events file does not exist"
> -			   " - please rebuild kernel with %s.\n",
> -			   uprobe ? 'u' : 'k', config);
> -	} else if (err == -ENOTSUP)
> -		pr_warning("Tracefs or debugfs is not mounted.\n");
> +	if (!debugfs__configured() && !tracefs__configured())
> +		pr_warning("Debugfs or tracefs is not mounted\n"
> +			   "Please try 'sudo mount -t tracefs nodev /sys/kernel/tracing/'\n");
>  	else
> -		pr_warning("Failed to open %cprobe_events: %s\n",
> -			   uprobe ? 'u' : 'k',
> -			   str_error_r(-err, sbuf, sizeof(sbuf)));
> +		pr_warning("%s/%s does not exist.\nPlease rebuild kernel with %s.\n",
> +			   tracing_path_mount(), file, config);
> +
> +	return true;
> +}
> +
> +static void print_open_warning(int err, bool uprobe, bool readwrite)
> +{
> +	char sbuf[STRERR_BUFSIZE];
> +
> +	if (print_common_warning(err, readwrite))
> +		return;
> +
> +	if (print_configure_probe_event(uprobe ? 0 : err, uprobe ? err : 0))
> +		return;
> +
> +	pr_warning("Failed to open %s/%cprobe_events: %s\n",
> +		   tracing_path_mount(), uprobe ? 'u' : 'k',
> +		   str_error_r(-err, sbuf, sizeof(sbuf)));
>  }
>  
> -static void print_both_open_warning(int kerr, int uerr)
> +static void print_both_open_warning(int kerr, int uerr, bool readwrite)
>  {
> -	/* Both kprobes and uprobes are disabled, warn it. */
> -	if (kerr == -ENOTSUP && uerr == -ENOTSUP)
> -		pr_warning("Tracefs or debugfs is not mounted.\n");
> -	else if (kerr == -ENOENT && uerr == -ENOENT)
> -		pr_warning("Please rebuild kernel with CONFIG_KPROBE_EVENTS "
> -			   "or/and CONFIG_UPROBE_EVENTS.\n");
> -	else {
> -		char sbuf[STRERR_BUFSIZE];
> -		pr_warning("Failed to open kprobe events: %s.\n",
> +	char sbuf[STRERR_BUFSIZE];
> +
> +	if (kerr == uerr && print_common_warning(kerr, readwrite))
> +		return;
> +
> +	if (print_configure_probe_event(kerr, uerr))
> +		return;
> +
> +	if (kerr < 0)
> +		pr_warning("Failed to open %s/kprobe_events: %s.\n",
> +			   tracing_path_mount(),
>  			   str_error_r(-kerr, sbuf, sizeof(sbuf)));
> -		pr_warning("Failed to open uprobe events: %s.\n",
> +	if (uerr < 0)
> +		pr_warning("Failed to open %s/uprobe_events: %s.\n",
> +			   tracing_path_mount(),
>  			   str_error_r(-uerr, sbuf, sizeof(sbuf)));
> -	}
>  }
>  
>  int open_trace_file(const char *trace_file, bool readwrite)
> @@ -109,7 +144,7 @@ int probe_file__open(int flag)
>  	else
>  		fd = open_kprobe_events(flag & PF_FL_RW);
>  	if (fd < 0)
> -		print_open_warning(fd, flag & PF_FL_UPROBE);
> +		print_open_warning(fd, flag & PF_FL_UPROBE, flag & PF_FL_RW);
>  
>  	return fd;
>  }
> @@ -122,7 +157,7 @@ int probe_file__open_both(int *kfd, int *ufd, int flag)
>  	*kfd = open_kprobe_events(flag & PF_FL_RW);
>  	*ufd = open_uprobe_events(flag & PF_FL_RW);
>  	if (*kfd < 0 && *ufd < 0) {
> -		print_both_open_warning(*kfd, *ufd);
> +		print_both_open_warning(*kfd, *ufd, flag & PF_FL_RW);
>  		return *kfd;
>  	}
>  
> 

-- 

- Arnaldo