[PATCH v2] perf cap: Tidy up and improve capability testing

[PATCH v2] perf cap: Tidy up and improve capability testing

[Ian Rogers]

Remove dependence on libcap. libcap is only used to query whether a
capability is supported, which is just 1 capget system call.

If the capget system call fails, fall back on root permission
checking. Previously if libcap fails then the permission is assumed
not present which may be pessimistic/wrong.

Add a used_root out argument to perf_cap__capable to say whether the
fall back root check was used. This allows the correct error message,
"root" vs "users with the CAP_PERFMON or CAP_SYS_ADMIN capability", to
be selected.

Tidy uses of perf_cap__capable so that tests aren't repeated if capget
isn't supported, to reduce calls or refactor similar to:
https://lore.kernel.org/lkml/20240729004127.238611-3-namhyung@kernel.org/
---
v2: fix syscall number and '>' should have been '>='

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/Makefile.config  | 11 -------
 tools/perf/builtin-ftrace.c | 44 ++++++++++++--------------
 tools/perf/util/Build       |  2 +-
 tools/perf/util/cap.c       | 63 ++++++++++++++++++++++++++-----------
 tools/perf/util/cap.h       | 23 ++------------
 tools/perf/util/symbol.c    |  8 ++---
 tools/perf/util/util.c      | 12 +++++--
 7 files changed, 81 insertions(+), 82 deletions(-)

diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index a4829b6532d8..a9517272f80c 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -1018,17 +1018,6 @@ ifndef NO_LIBZSTD
   endif
 endif
 
-ifndef NO_LIBCAP
-  ifeq ($(feature-libcap), 1)
-    CFLAGS += -DHAVE_LIBCAP_SUPPORT
-    EXTLIBS += -lcap
-    $(call detected,CONFIG_LIBCAP)
-  else
-    $(warning No libcap found, disables capability support, please install libcap-devel/libcap-dev)
-    NO_LIBCAP := 1
-  endif
-endif
-
 ifndef NO_BACKTRACE
   ifeq ($(feature-backtrace), 1)
     CFLAGS += -DHAVE_BACKTRACE_SUPPORT
diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index eb30c8eca488..435208288d24 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -560,6 +560,23 @@ static void select_tracer(struct perf_ftrace *ftrace)
 	pr_debug("%s tracer is used\n", ftrace->tracer);
 }
 
+static bool check_ftrace_capable(void)
+{
+	bool used_root;
+
+	if (perf_cap__capable(CAP_PERFMON, &used_root))
+		return true;
+
+	if (!used_root && perf_cap__capable(CAP_SYS_ADMIN, &used_root))
+		return true;
+
+	pr_err("ftrace only works for %s!\n",
+		used_root ? "root"
+			  : "users with the CAP_PERFMON or CAP_SYS_ADMIN capability"
+		);
+	return -1;
+}
+
 static int __cmd_ftrace(struct perf_ftrace *ftrace)
 {
 	char *trace_file;
@@ -569,18 +586,6 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
 		.events = POLLIN,
 	};
 
-	if (!(perf_cap__capable(CAP_PERFMON) ||
-	      perf_cap__capable(CAP_SYS_ADMIN))) {
-		pr_err("ftrace only works for %s!\n",
-#ifdef HAVE_LIBCAP_SUPPORT
-		"users with the CAP_PERFMON or CAP_SYS_ADMIN capability"
-#else
-		"root"
-#endif
-		);
-		return -1;
-	}
-
 	select_tracer(ftrace);
 
 	if (reset_tracing_files(ftrace) < 0) {
@@ -885,18 +890,6 @@ static int __cmd_latency(struct perf_ftrace *ftrace)
 	};
 	int buckets[NUM_BUCKET] = { };
 
-	if (!(perf_cap__capable(CAP_PERFMON) ||
-	      perf_cap__capable(CAP_SYS_ADMIN))) {
-		pr_err("ftrace only works for %s!\n",
-#ifdef HAVE_LIBCAP_SUPPORT
-		"users with the CAP_PERFMON or CAP_SYS_ADMIN capability"
-#else
-		"root"
-#endif
-		);
-		return -1;
-	}
-
 	trace_fd = prepare_func_latency(ftrace);
 	if (trace_fd < 0)
 		goto out;
@@ -1197,6 +1190,9 @@ int cmd_ftrace(int argc, const char **argv)
 	INIT_LIST_HEAD(&ftrace.graph_funcs);
 	INIT_LIST_HEAD(&ftrace.nograph_funcs);
 
+	if (!check_ftrace_capable())
+		return -1;
+
 	signal(SIGINT, sig_handler);
 	signal(SIGUSR1, sig_handler);
 	signal(SIGCHLD, sig_handler);
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 0f18fe81ef0b..91ce0ab4defc 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -220,7 +220,7 @@ perf-util-$(CONFIG_ZLIB) += zlib.o
 perf-util-$(CONFIG_LZMA) += lzma.o
 perf-util-$(CONFIG_ZSTD) += zstd.o
 
-perf-util-$(CONFIG_LIBCAP) += cap.o
+perf-util-y += cap.o
 
 perf-util-$(CONFIG_CXX_DEMANGLE) += demangle-cxx.o
 perf-util-y += demangle-ocaml.o
diff --git a/tools/perf/util/cap.c b/tools/perf/util/cap.c
index c3ba841bbf37..7574a67651bc 100644
--- a/tools/perf/util/cap.c
+++ b/tools/perf/util/cap.c
@@ -3,27 +3,52 @@
  * Capability utilities
  */
 
-#ifdef HAVE_LIBCAP_SUPPORT
-
 #include "cap.h"
-#include <stdbool.h>
-#include <sys/capability.h>
-
-bool perf_cap__capable(cap_value_t cap)
-{
-	cap_flag_value_t val;
-	cap_t caps = cap_get_proc();
+#include "debug.h"
+#include <errno.h>
+#include <string.h>
+#include <unistd.h>
+#include <linux/capability.h>
+#include <sys/syscall.h>
 
-	if (!caps)
-		return false;
+#ifndef SYS_capget
+#define SYS_capget 90
+#endif
 
-	if (cap_get_flag(caps, cap, CAP_EFFECTIVE, &val) != 0)
-		val = CAP_CLEAR;
+#define MAX_LINUX_CAPABILITY_U32S _LINUX_CAPABILITY_U32S_3
 
-	if (cap_free(caps) != 0)
-		return false;
-
-	return val == CAP_SET;
+bool perf_cap__capable(int cap, bool *used_root)
+{
+	struct __user_cap_header_struct header = {
+		.version = _LINUX_CAPABILITY_VERSION_3,
+		.pid = getpid(),
+	};
+	struct __user_cap_data_struct data[MAX_LINUX_CAPABILITY_U32S];
+	__u32 cap_val;
+
+	*used_root = false;
+	while (syscall(SYS_capget, &header, &data[0]) == -1) {
+		/* Retry, first attempt has set the header.version correctly. */
+		if (errno == EINVAL && header.version != _LINUX_CAPABILITY_VERSION_3 &&
+		    header.version == _LINUX_CAPABILITY_VERSION_1)
+			continue;
+
+		pr_debug2("capget syscall failed (%s - %d) fall back on root check\n",
+			  strerror(errno), errno);
+		*used_root = true;
+		return geteuid() == 0;
+	}
+
+	/* Extract the relevant capability bit. */
+	if (cap >= 32) {
+		if (header.version == _LINUX_CAPABILITY_VERSION_3) {
+			cap_val = data[1].effective;
+		} else {
+			/* Capability beyond 32 is requested but only 32 are supported. */
+			return false;
+		}
+	} else {
+		cap_val = data[0].effective;
+	}
+	return (cap_val & (1 << (cap & 0x1f))) != 0;
 }
-
-#endif  /* HAVE_LIBCAP_SUPPORT */
diff --git a/tools/perf/util/cap.h b/tools/perf/util/cap.h
index ae52878c0b2e..0c6a1ff55f07 100644
--- a/tools/perf/util/cap.h
+++ b/tools/perf/util/cap.h
@@ -3,26 +3,6 @@
 #define __PERF_CAP_H
 
 #include <stdbool.h>
-#include <linux/capability.h>
-#include <linux/compiler.h>
-
-#ifdef HAVE_LIBCAP_SUPPORT
-
-#include <sys/capability.h>
-
-bool perf_cap__capable(cap_value_t cap);
-
-#else
-
-#include <unistd.h>
-#include <sys/types.h>
-
-static inline bool perf_cap__capable(int cap __maybe_unused)
-{
-	return geteuid() == 0;
-}
-
-#endif /* HAVE_LIBCAP_SUPPORT */
 
 /* For older systems */
 #ifndef CAP_SYSLOG
@@ -33,4 +13,7 @@ static inline bool perf_cap__capable(int cap __maybe_unused)
 #define CAP_PERFMON	38
 #endif
 
+/* Query if a capability is supported, used_root is set if the fallback root check was used. */
+bool perf_cap__capable(int cap, bool *used_root);
+
 #endif /* __PERF_CAP_H */
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 19eb623e0826..a18927d792af 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2425,14 +2425,14 @@ static bool symbol__read_kptr_restrict(void)
 {
 	bool value = false;
 	FILE *fp = fopen("/proc/sys/kernel/kptr_restrict", "r");
+	bool used_root;
+	bool cap_syslog = perf_cap__capable(CAP_SYSLOG, &used_root);
 
 	if (fp != NULL) {
 		char line[8];
 
 		if (fgets(line, sizeof(line), fp) != NULL)
-			value = perf_cap__capable(CAP_SYSLOG) ?
-					(atoi(line) >= 2) :
-					(atoi(line) != 0);
+			value = cap_syslog ? (atoi(line) >= 2) : (atoi(line) != 0);
 
 		fclose(fp);
 	}
@@ -2440,7 +2440,7 @@ static bool symbol__read_kptr_restrict(void)
 	/* Per kernel/kallsyms.c:
 	 * we also restrict when perf_event_paranoid > 1 w/o CAP_SYSLOG
 	 */
-	if (perf_event_paranoid() > 1 && !perf_cap__capable(CAP_SYSLOG))
+	if (perf_event_paranoid() > 1 && !cap_syslog)
 		value = true;
 
 	return value;
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 4f561e5e4162..9d55a13787ce 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -325,9 +325,15 @@ int perf_event_paranoid(void)
 
 bool perf_event_paranoid_check(int max_level)
 {
-	return perf_cap__capable(CAP_SYS_ADMIN) ||
-			perf_cap__capable(CAP_PERFMON) ||
-			perf_event_paranoid() <= max_level;
+	bool used_root;
+
+	if (perf_cap__capable(CAP_SYS_ADMIN, &used_root))
+		return true;
+
+	if (!used_root && perf_cap__capable(CAP_PERFMON, &used_root))
+		return true;
+
+	return perf_event_paranoid() <= max_level;
 }
 
 static int
-- 
2.46.0.rc1.232.g9752f9e123-goog

Re: [PATCH v2] perf cap: Tidy up and improve capability testing

[Namhyung Kim]

On Mon, Jul 29, 2024 at 11:19:31AM -0700, Ian Rogers wrote:
> Remove dependence on libcap. libcap is only used to query whether a
> capability is supported, which is just 1 capget system call.
> 
> If the capget system call fails, fall back on root permission
> checking. Previously if libcap fails then the permission is assumed
> not present which may be pessimistic/wrong.
> 
> Add a used_root out argument to perf_cap__capable to say whether the
> fall back root check was used. This allows the correct error message,
> "root" vs "users with the CAP_PERFMON or CAP_SYS_ADMIN capability", to
> be selected.
> 
> Tidy uses of perf_cap__capable so that tests aren't repeated if capget
> isn't supported, to reduce calls or refactor similar to:
> https://lore.kernel.org/lkml/20240729004127.238611-3-namhyung@kernel.org/

I'm not familiar with the capability so it's hard to review the code but
it'd be better to split the code for perf_cap__capable() and its usage.

> ---
> v2: fix syscall number and '>' should have been '>='
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/Makefile.config  | 11 -------
>  tools/perf/builtin-ftrace.c | 44 ++++++++++++--------------
>  tools/perf/util/Build       |  2 +-
>  tools/perf/util/cap.c       | 63 ++++++++++++++++++++++++++-----------
>  tools/perf/util/cap.h       | 23 ++------------
>  tools/perf/util/symbol.c    |  8 ++---
>  tools/perf/util/util.c      | 12 +++++--
>  7 files changed, 81 insertions(+), 82 deletions(-)
> 
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index a4829b6532d8..a9517272f80c 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -1018,17 +1018,6 @@ ifndef NO_LIBZSTD
>    endif
>  endif
>  
> -ifndef NO_LIBCAP
> -  ifeq ($(feature-libcap), 1)
> -    CFLAGS += -DHAVE_LIBCAP_SUPPORT
> -    EXTLIBS += -lcap
> -    $(call detected,CONFIG_LIBCAP)
> -  else
> -    $(warning No libcap found, disables capability support, please install libcap-devel/libcap-dev)
> -    NO_LIBCAP := 1
> -  endif
> -endif
> -
>  ifndef NO_BACKTRACE
>    ifeq ($(feature-backtrace), 1)
>      CFLAGS += -DHAVE_BACKTRACE_SUPPORT
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index eb30c8eca488..435208288d24 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -560,6 +560,23 @@ static void select_tracer(struct perf_ftrace *ftrace)
>  	pr_debug("%s tracer is used\n", ftrace->tracer);
>  }
>  
> +static bool check_ftrace_capable(void)
> +{
> +	bool used_root;
> +
> +	if (perf_cap__capable(CAP_PERFMON, &used_root))
> +		return true;
> +
> +	if (!used_root && perf_cap__capable(CAP_SYS_ADMIN, &used_root))
> +		return true;
> +
> +	pr_err("ftrace only works for %s!\n",
> +		used_root ? "root"
> +			  : "users with the CAP_PERFMON or CAP_SYS_ADMIN capability"
> +		);
> +	return -1;
> +}
> +
>  static int __cmd_ftrace(struct perf_ftrace *ftrace)
>  {
>  	char *trace_file;
> @@ -569,18 +586,6 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>  		.events = POLLIN,
>  	};
>  
> -	if (!(perf_cap__capable(CAP_PERFMON) ||
> -	      perf_cap__capable(CAP_SYS_ADMIN))) {
> -		pr_err("ftrace only works for %s!\n",
> -#ifdef HAVE_LIBCAP_SUPPORT
> -		"users with the CAP_PERFMON or CAP_SYS_ADMIN capability"
> -#else
> -		"root"
> -#endif
> -		);
> -		return -1;
> -	}
> -
>  	select_tracer(ftrace);
>  
>  	if (reset_tracing_files(ftrace) < 0) {
> @@ -885,18 +890,6 @@ static int __cmd_latency(struct perf_ftrace *ftrace)
>  	};
>  	int buckets[NUM_BUCKET] = { };
>  
> -	if (!(perf_cap__capable(CAP_PERFMON) ||
> -	      perf_cap__capable(CAP_SYS_ADMIN))) {
> -		pr_err("ftrace only works for %s!\n",
> -#ifdef HAVE_LIBCAP_SUPPORT
> -		"users with the CAP_PERFMON or CAP_SYS_ADMIN capability"
> -#else
> -		"root"
> -#endif
> -		);
> -		return -1;
> -	}
> -
>  	trace_fd = prepare_func_latency(ftrace);
>  	if (trace_fd < 0)
>  		goto out;
> @@ -1197,6 +1190,9 @@ int cmd_ftrace(int argc, const char **argv)
>  	INIT_LIST_HEAD(&ftrace.graph_funcs);
>  	INIT_LIST_HEAD(&ftrace.nograph_funcs);
>  
> +	if (!check_ftrace_capable())
> +		return -1;
> +
>  	signal(SIGINT, sig_handler);
>  	signal(SIGUSR1, sig_handler);
>  	signal(SIGCHLD, sig_handler);
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index 0f18fe81ef0b..91ce0ab4defc 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -220,7 +220,7 @@ perf-util-$(CONFIG_ZLIB) += zlib.o
>  perf-util-$(CONFIG_LZMA) += lzma.o
>  perf-util-$(CONFIG_ZSTD) += zstd.o
>  
> -perf-util-$(CONFIG_LIBCAP) += cap.o
> +perf-util-y += cap.o
>  
>  perf-util-$(CONFIG_CXX_DEMANGLE) += demangle-cxx.o
>  perf-util-y += demangle-ocaml.o
> diff --git a/tools/perf/util/cap.c b/tools/perf/util/cap.c
> index c3ba841bbf37..7574a67651bc 100644
> --- a/tools/perf/util/cap.c
> +++ b/tools/perf/util/cap.c
> @@ -3,27 +3,52 @@
>   * Capability utilities
>   */
>  
> -#ifdef HAVE_LIBCAP_SUPPORT
> -
>  #include "cap.h"
> -#include <stdbool.h>
> -#include <sys/capability.h>
> -
> -bool perf_cap__capable(cap_value_t cap)
> -{
> -	cap_flag_value_t val;
> -	cap_t caps = cap_get_proc();
> +#include "debug.h"
> +#include <errno.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <linux/capability.h>
> +#include <sys/syscall.h>
>  
> -	if (!caps)
> -		return false;
> +#ifndef SYS_capget
> +#define SYS_capget 90
> +#endif
>  
> -	if (cap_get_flag(caps, cap, CAP_EFFECTIVE, &val) != 0)
> -		val = CAP_CLEAR;
> +#define MAX_LINUX_CAPABILITY_U32S _LINUX_CAPABILITY_U32S_3
>  
> -	if (cap_free(caps) != 0)
> -		return false;
> -
> -	return val == CAP_SET;
> +bool perf_cap__capable(int cap, bool *used_root)
> +{
> +	struct __user_cap_header_struct header = {
> +		.version = _LINUX_CAPABILITY_VERSION_3,
> +		.pid = getpid(),
> +	};
> +	struct __user_cap_data_struct data[MAX_LINUX_CAPABILITY_U32S];
> +	__u32 cap_val;
> +
> +	*used_root = false;
> +	while (syscall(SYS_capget, &header, &data[0]) == -1) {
> +		/* Retry, first attempt has set the header.version correctly. */
> +		if (errno == EINVAL && header.version != _LINUX_CAPABILITY_VERSION_3 &&
> +		    header.version == _LINUX_CAPABILITY_VERSION_1)

It seems you can just check it with _VERSION1.

But I'm not sure about what this code does.  Who set the version
correctly?  Is there any chance for an infinite loop?

Thanks,
Namhyung

> +			continue;
> +
> +		pr_debug2("capget syscall failed (%s - %d) fall back on root check\n",
> +			  strerror(errno), errno);
> +		*used_root = true;
> +		return geteuid() == 0;
> +	}
> +
> +	/* Extract the relevant capability bit. */
> +	if (cap >= 32) {
> +		if (header.version == _LINUX_CAPABILITY_VERSION_3) {
> +			cap_val = data[1].effective;
> +		} else {
> +			/* Capability beyond 32 is requested but only 32 are supported. */
> +			return false;
> +		}
> +	} else {
> +		cap_val = data[0].effective;
> +	}
> +	return (cap_val & (1 << (cap & 0x1f))) != 0;
>  }
> -
> -#endif  /* HAVE_LIBCAP_SUPPORT */
> diff --git a/tools/perf/util/cap.h b/tools/perf/util/cap.h
> index ae52878c0b2e..0c6a1ff55f07 100644
> --- a/tools/perf/util/cap.h
> +++ b/tools/perf/util/cap.h
> @@ -3,26 +3,6 @@
>  #define __PERF_CAP_H
>  
>  #include <stdbool.h>
> -#include <linux/capability.h>
> -#include <linux/compiler.h>
> -
> -#ifdef HAVE_LIBCAP_SUPPORT
> -
> -#include <sys/capability.h>
> -
> -bool perf_cap__capable(cap_value_t cap);
> -
> -#else
> -
> -#include <unistd.h>
> -#include <sys/types.h>
> -
> -static inline bool perf_cap__capable(int cap __maybe_unused)
> -{
> -	return geteuid() == 0;
> -}
> -
> -#endif /* HAVE_LIBCAP_SUPPORT */
>  
>  /* For older systems */
>  #ifndef CAP_SYSLOG
> @@ -33,4 +13,7 @@ static inline bool perf_cap__capable(int cap __maybe_unused)
>  #define CAP_PERFMON	38
>  #endif
>  
> +/* Query if a capability is supported, used_root is set if the fallback root check was used. */
> +bool perf_cap__capable(int cap, bool *used_root);
> +
>  #endif /* __PERF_CAP_H */
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 19eb623e0826..a18927d792af 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -2425,14 +2425,14 @@ static bool symbol__read_kptr_restrict(void)
>  {
>  	bool value = false;
>  	FILE *fp = fopen("/proc/sys/kernel/kptr_restrict", "r");
> +	bool used_root;
> +	bool cap_syslog = perf_cap__capable(CAP_SYSLOG, &used_root);
>  
>  	if (fp != NULL) {
>  		char line[8];
>  
>  		if (fgets(line, sizeof(line), fp) != NULL)
> -			value = perf_cap__capable(CAP_SYSLOG) ?
> -					(atoi(line) >= 2) :
> -					(atoi(line) != 0);
> +			value = cap_syslog ? (atoi(line) >= 2) : (atoi(line) != 0);
>  
>  		fclose(fp);
>  	}
> @@ -2440,7 +2440,7 @@ static bool symbol__read_kptr_restrict(void)
>  	/* Per kernel/kallsyms.c:
>  	 * we also restrict when perf_event_paranoid > 1 w/o CAP_SYSLOG
>  	 */
> -	if (perf_event_paranoid() > 1 && !perf_cap__capable(CAP_SYSLOG))
> +	if (perf_event_paranoid() > 1 && !cap_syslog)
>  		value = true;
>  
>  	return value;
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 4f561e5e4162..9d55a13787ce 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -325,9 +325,15 @@ int perf_event_paranoid(void)
>  
>  bool perf_event_paranoid_check(int max_level)
>  {
> -	return perf_cap__capable(CAP_SYS_ADMIN) ||
> -			perf_cap__capable(CAP_PERFMON) ||
> -			perf_event_paranoid() <= max_level;
> +	bool used_root;
> +
> +	if (perf_cap__capable(CAP_SYS_ADMIN, &used_root))
> +		return true;
> +
> +	if (!used_root && perf_cap__capable(CAP_PERFMON, &used_root))
> +		return true;
> +
> +	return perf_event_paranoid() <= max_level;
>  }
>  
>  static int
> -- 
> 2.46.0.rc1.232.g9752f9e123-goog
> 

Re: [PATCH v2] perf cap: Tidy up and improve capability testing

[Ian Rogers]

On Tue, Jul 30, 2024 at 9:36 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Jul 29, 2024 at 11:19:31AM -0700, Ian Rogers wrote:
> > Remove dependence on libcap. libcap is only used to query whether a
> > capability is supported, which is just 1 capget system call.
> >
> > If the capget system call fails, fall back on root permission
> > checking. Previously if libcap fails then the permission is assumed
> > not present which may be pessimistic/wrong.
> >
> > Add a used_root out argument to perf_cap__capable to say whether the
> > fall back root check was used. This allows the correct error message,
> > "root" vs "users with the CAP_PERFMON or CAP_SYS_ADMIN capability", to
> > be selected.
> >
> > Tidy uses of perf_cap__capable so that tests aren't repeated if capget
> > isn't supported, to reduce calls or refactor similar to:
> > https://lore.kernel.org/lkml/20240729004127.238611-3-namhyung@kernel.org/
>
> I'm not familiar with the capability so it's hard to review the code but
> it'd be better to split the code for perf_cap__capable() and its usage.

There's an API change, passing the out arg if root checking was used,
so this would turn into a mess.

> > ---
> > v2: fix syscall number and '>' should have been '>='
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/Makefile.config  | 11 -------
> >  tools/perf/builtin-ftrace.c | 44 ++++++++++++--------------
> >  tools/perf/util/Build       |  2 +-
> >  tools/perf/util/cap.c       | 63 ++++++++++++++++++++++++++-----------
> >  tools/perf/util/cap.h       | 23 ++------------
> >  tools/perf/util/symbol.c    |  8 ++---
> >  tools/perf/util/util.c      | 12 +++++--
> >  7 files changed, 81 insertions(+), 82 deletions(-)
[snip]
> > --- a/tools/perf/util/cap.c
> > +++ b/tools/perf/util/cap.c
[snip]
> > +bool perf_cap__capable(int cap, bool *used_root)
> > +{
> > +     struct __user_cap_header_struct header = {
> > +             .version = _LINUX_CAPABILITY_VERSION_3,
> > +             .pid = getpid(),
> > +     };
> > +     struct __user_cap_data_struct data[MAX_LINUX_CAPABILITY_U32S];
> > +     __u32 cap_val;
> > +
> > +     *used_root = false;
> > +     while (syscall(SYS_capget, &header, &data[0]) == -1) {
> > +             /* Retry, first attempt has set the header.version correctly. */
> > +             if (errno == EINVAL && header.version != _LINUX_CAPABILITY_VERSION_3 &&
> > +                 header.version == _LINUX_CAPABILITY_VERSION_1)
>
> It seems you can just check it with _VERSION1.

Similarly not familiar. Basically there used to be a data struct of 3
u32s in v1, v2 shouldn't exist, v3 turned the data from an array of
structs of size [1] to [2]. v3 has been present for 16 years as shown
by its magic number 0x20080522 (v2.6.26 iirc - can't find the comment
again). It seems a pretty safe bet that v3 is the version being used.
In old kernels if the versions don't match they just return EINVAL and
you have to try again like this loop:
https://fossd.anu.edu.au/linux/v2.6.14/source/kernel/capability.c#L43
```
if (get_user(version, &header->version))
  return -EFAULT;

if (version != _LINUX_CAPABILITY_VERSION) {
  if (put_user(_LINUX_CAPABILITY_VERSION, &header->version))
    return -EFAULT;
   return -EINVAL;
}
```
So testing v3 is almost certainly what we want, what's going to work
and avoids a loop. In newer kernels the EINVAL isn't returned for a
mismatch and the code just does the right thing for whatever version
is requested.

> But I'm not sure about what this code does.  Who set the version
> correctly?  Is there any chance for an infinite loop?

The system call could return EINVAL with _LINUX_CAPABILITY_VERSION_1
and set the header.version to _LINUX_CAPABILITY_VERSION_1 again and
return EINVAL again, the kernel would be broken but it would lead to
an infinite loop.

Thanks,
Ian