* Re: [PATCH bpf-next v2 1/1] selftests/bpf: more precise cpu_mitigations state detection
2025-06-14 5:06 ` [PATCH bpf-next v2 1/1] " Eduard Zingerman
@ 2025-06-15 2:18 ` Yafang Shao
2025-06-16 21:13 ` Andrii Nakryiko
1 sibling, 0 replies; 6+ messages in thread
From: Yafang Shao @ 2025-06-15 2:18 UTC (permalink / raw)
To: Eduard Zingerman
Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yonghong.song,
mykyta.yatsenko5
On Sat, Jun 14, 2025 at 1:06 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> test_progs and test_verifier binaries execute unpriv tests under the
> following conditions:
> - unpriv BPF is enabled;
> - CPU mitigations are enabled (see [1] for details).
>
> The detection of the "mitigations enabled" state is performed by
> unpriv_helpers.c:get_mitigations_off() via inspecting kernel boot
> command line, looking for a parameter "mitigations=off".
>
> Such detection scheme won't work for certain configurations,
> e.g. when CONFIG_CPU_MITIGATIONS is disabled and boot parameter is
> not supplied.
>
> Miss-detection leads to test_progs executing tests meant to be run
> only with mitigations enabled, e.g.
> verifier_and.c:known_subreg_with_unknown_reg(), and reporting false
> failures.
>
> Internally, verifier sets bpf_verifier_env->bypass_spec_{v1,v4}
> basing on the value returned by kernel/cpu.c:cpu_mitigations_off().
> This function is backed by a variable kernel/cpu.c:cpu_mitigations.
>
> This state is not fully introspect-able via sysfs. The closest proxy
> is /sys/devices/system/cpu/vulnerabilities/spectre_v1, but it reports
> "vulnerable" state only if mitigations are disabled *and* current cpu
> is vulnerable, while verifier does not check cpu state.
>
> There are only two ways the kernel/cpu.c:cpu_mitigations can be set:
> - via boot parameter;
> - via CONFIG_CPU_MITIGATIONS option.
>
> This commit updates unpriv_helpers.c:get_mitigations_off() to scan
> /boot/config-$(uname -r) and /proc/config.gz for
> CONFIG_CPU_MITIGATIONS value in addition to boot command line check.
>
> Tested using the following configurations:
> - mitigations enabled (unpriv tests are enabled)
> - mitigations disabled via boot cmdline (unpriv tests skipped)
> - mitigations disabled via CONFIG_CPU_MITIGATIONS
> (unpriv tests skipped)
>
> [1] https://lore.kernel.org/bpf/20231025031144.5508-1-laoar.shao@gmail.com/
>
> Reported-by: Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
Acked-by: Yafang Shao <laoar.shao@gmail.com>
> ---
> tools/testing/selftests/bpf/unpriv_helpers.c | 94 +++++++++++++++++++-
> 1 file changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/unpriv_helpers.c b/tools/testing/selftests/bpf/unpriv_helpers.c
> index 220f6a963813..625556a0e7f1 100644
> --- a/tools/testing/selftests/bpf/unpriv_helpers.c
> +++ b/tools/testing/selftests/bpf/unpriv_helpers.c
> @@ -1,15 +1,76 @@
> // SPDX-License-Identifier: GPL-2.0-only
>
> +#include <errno.h>
> #include <stdbool.h>
> #include <stdlib.h>
> #include <stdio.h>
> #include <string.h>
> +#include <sys/utsname.h>
> #include <unistd.h>
> #include <fcntl.h>
> +#include <zlib.h>
>
> #include "unpriv_helpers.h"
>
> -static bool get_mitigations_off(void)
> +static gzFile open_config(void)
> +{
> + struct utsname uts;
> + char buf[PATH_MAX];
> + gzFile config;
> +
> + if (uname(&uts)) {
> + perror("uname");
> + goto config_gz;
> + }
> +
> + snprintf(buf, sizeof(buf), "/boot/config-%s", uts.release);
> + config = gzopen(buf, "rb");
> + if (config)
> + return config;
> + fprintf(stderr, "gzopen %s: %s\n", buf, strerror(errno));
> +
> +config_gz:
> + config = gzopen("/proc/config.gz", "rb");
> + if (!config)
> + perror("gzopen /proc/config.gz");
> + return config;
> +}
> +
> +static int config_contains(const char *pat)
> +{
> + int n, err, ret = -1;
> + const char *msg;
> + char buf[1024];
> + gzFile config;
> +
> + config = open_config();
> + if (!config)
> + goto out;
> +
> + for (;;) {
> + if (!gzgets(config, buf, sizeof(buf))) {
> + msg = gzerror(config, &err);
> + if (err == Z_ERRNO)
> + perror("gzgets /proc/config.gz");
> + else if (err != Z_OK)
> + fprintf(stderr, "gzgets /proc/config.gz: %s", msg);
> + goto out;
> + }
> + n = strlen(buf);
> + if (buf[n - 1] == '\n')
> + buf[n - 1] = 0;
> + if (strcmp(buf, pat) == 0) {
> + ret = true;
> + goto out;
> + }
> + }
> + ret = false;
> +out:
> + gzclose(config);
> + return ret;
> +}
> +
> +static bool cmdline_contains(const char *pat)
> {
> char cmdline[4096], *c;
> int fd, ret = false;
> @@ -27,7 +88,7 @@ static bool get_mitigations_off(void)
>
> cmdline[sizeof(cmdline) - 1] = '\0';
> for (c = strtok(cmdline, " \n"); c; c = strtok(NULL, " \n")) {
> - if (strncmp(c, "mitigations=off", strlen(c)))
> + if (strncmp(c, pat, strlen(c)))
> continue;
> ret = true;
> break;
> @@ -37,8 +98,21 @@ static bool get_mitigations_off(void)
> return ret;
> }
>
> +static int get_mitigations_off(void)
> +{
> + int enabled_in_config;
> +
> + if (cmdline_contains("mitigations=off"))
> + return true;
> + enabled_in_config = config_contains("CONFIG_CPU_MITIGATIONS=y");
> + if (enabled_in_config < 0)
> + return -1;
> + return !enabled_in_config;
> +}
> +
> bool get_unpriv_disabled(void)
> {
> + int mitigations_off;
> bool disabled;
> char buf[2];
> FILE *fd;
> @@ -52,5 +126,19 @@ bool get_unpriv_disabled(void)
> disabled = true;
> }
>
> - return disabled ? true : get_mitigations_off();
> + if (disabled)
> + return true;
> +
> + /*
> + * Some unpriv tests rely on spectre mitigations being on.
> + * If mitigations are off or status can't be determined
> + * assume that unpriv tests are disabled.
> + */
> + mitigations_off = get_mitigations_off();
> + if (mitigations_off < 0) {
> + fprintf(stderr,
> + "Can't determine if mitigations are enabled, disabling unpriv tests.");
> + return true;
> + }
> + return mitigations_off;
> }
> --
> 2.47.1
>
--
Regards
Yafang
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf-next v2 1/1] selftests/bpf: more precise cpu_mitigations state detection
2025-06-14 5:06 ` [PATCH bpf-next v2 1/1] " Eduard Zingerman
2025-06-15 2:18 ` Yafang Shao
@ 2025-06-16 21:13 ` Andrii Nakryiko
2025-06-16 21:27 ` Eduard Zingerman
1 sibling, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2025-06-16 21:13 UTC (permalink / raw)
To: Eduard Zingerman
Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yonghong.song,
laoar.shao, mykyta.yatsenko5
On Fri, Jun 13, 2025 at 10:06 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> test_progs and test_verifier binaries execute unpriv tests under the
> following conditions:
> - unpriv BPF is enabled;
> - CPU mitigations are enabled (see [1] for details).
>
> The detection of the "mitigations enabled" state is performed by
> unpriv_helpers.c:get_mitigations_off() via inspecting kernel boot
> command line, looking for a parameter "mitigations=off".
>
> Such detection scheme won't work for certain configurations,
> e.g. when CONFIG_CPU_MITIGATIONS is disabled and boot parameter is
> not supplied.
>
> Miss-detection leads to test_progs executing tests meant to be run
> only with mitigations enabled, e.g.
> verifier_and.c:known_subreg_with_unknown_reg(), and reporting false
> failures.
>
> Internally, verifier sets bpf_verifier_env->bypass_spec_{v1,v4}
> basing on the value returned by kernel/cpu.c:cpu_mitigations_off().
> This function is backed by a variable kernel/cpu.c:cpu_mitigations.
>
> This state is not fully introspect-able via sysfs. The closest proxy
> is /sys/devices/system/cpu/vulnerabilities/spectre_v1, but it reports
> "vulnerable" state only if mitigations are disabled *and* current cpu
> is vulnerable, while verifier does not check cpu state.
>
> There are only two ways the kernel/cpu.c:cpu_mitigations can be set:
> - via boot parameter;
> - via CONFIG_CPU_MITIGATIONS option.
>
> This commit updates unpriv_helpers.c:get_mitigations_off() to scan
> /boot/config-$(uname -r) and /proc/config.gz for
> CONFIG_CPU_MITIGATIONS value in addition to boot command line check.
>
> Tested using the following configurations:
> - mitigations enabled (unpriv tests are enabled)
> - mitigations disabled via boot cmdline (unpriv tests skipped)
> - mitigations disabled via CONFIG_CPU_MITIGATIONS
> (unpriv tests skipped)
>
> [1] https://lore.kernel.org/bpf/20231025031144.5508-1-laoar.shao@gmail.com/
>
> Reported-by: Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
> tools/testing/selftests/bpf/unpriv_helpers.c | 94 +++++++++++++++++++-
> 1 file changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/unpriv_helpers.c b/tools/testing/selftests/bpf/unpriv_helpers.c
> index 220f6a963813..625556a0e7f1 100644
> --- a/tools/testing/selftests/bpf/unpriv_helpers.c
> +++ b/tools/testing/selftests/bpf/unpriv_helpers.c
> @@ -1,15 +1,76 @@
> // SPDX-License-Identifier: GPL-2.0-only
>
> +#include <errno.h>
> #include <stdbool.h>
> #include <stdlib.h>
> #include <stdio.h>
> #include <string.h>
> +#include <sys/utsname.h>
> #include <unistd.h>
> #include <fcntl.h>
> +#include <zlib.h>
>
> #include "unpriv_helpers.h"
>
> -static bool get_mitigations_off(void)
> +static gzFile open_config(void)
> +{
> + struct utsname uts;
> + char buf[PATH_MAX];
> + gzFile config;
> +
> + if (uname(&uts)) {
> + perror("uname");
> + goto config_gz;
> + }
> +
> + snprintf(buf, sizeof(buf), "/boot/config-%s", uts.release);
> + config = gzopen(buf, "rb");
> + if (config)
> + return config;
> + fprintf(stderr, "gzopen %s: %s\n", buf, strerror(errno));
> +
> +config_gz:
> + config = gzopen("/proc/config.gz", "rb");
> + if (!config)
> + perror("gzopen /proc/config.gz");
> + return config;
> +}
> +
> +static int config_contains(const char *pat)
int-returning function... but we return do `ret = true;`, that looks
accidental and sloppy. Let's add a comment that it returns <0 on
error, 0 for no match, 1 for match and stick to numbers everywhere?
> +{
> + int n, err, ret = -1;
> + const char *msg;
> + char buf[1024];
> + gzFile config;
> +
> + config = open_config();
> + if (!config)
> + goto out;
nothing to gzclose if open_config() returns NULL, just return
pw-bot: cr
> +
> + for (;;) {
> + if (!gzgets(config, buf, sizeof(buf))) {
> + msg = gzerror(config, &err);
> + if (err == Z_ERRNO)
> + perror("gzgets /proc/config.gz");
> + else if (err != Z_OK)
> + fprintf(stderr, "gzgets /proc/config.gz: %s", msg);
> + goto out;
nit: I'd probably just do
gzclose(config);
return -EINVAL; (or whatever the error code might be)
> + }
> + n = strlen(buf);
> + if (buf[n - 1] == '\n')
> + buf[n - 1] = 0;
> + if (strcmp(buf, pat) == 0) {
> + ret = true;
> + goto out;
same here, gzclose() + return;
it will be easier to follow in this rather straightforward function
> + }
> + }
> + ret = false;
> +out:
> + gzclose(config);
> + return ret;
> +}
> +
> +static bool cmdline_contains(const char *pat)
> {
> char cmdline[4096], *c;
> int fd, ret = false;
> @@ -27,7 +88,7 @@ static bool get_mitigations_off(void)
>
> cmdline[sizeof(cmdline) - 1] = '\0';
> for (c = strtok(cmdline, " \n"); c; c = strtok(NULL, " \n")) {
> - if (strncmp(c, "mitigations=off", strlen(c)))
> + if (strncmp(c, pat, strlen(c)))
> continue;
> ret = true;
> break;
> @@ -37,8 +98,21 @@ static bool get_mitigations_off(void)
> return ret;
> }
>
> +static int get_mitigations_off(void)
> +{
> + int enabled_in_config;
> +
> + if (cmdline_contains("mitigations=off"))
> + return true;
> + enabled_in_config = config_contains("CONFIG_CPU_MITIGATIONS=y");
> + if (enabled_in_config < 0)
> + return -1;
> + return !enabled_in_config;
same mix of bool and integers
> +}
> +
> bool get_unpriv_disabled(void)
> {
> + int mitigations_off;
> bool disabled;
> char buf[2];
> FILE *fd;
> @@ -52,5 +126,19 @@ bool get_unpriv_disabled(void)
> disabled = true;
> }
>
> - return disabled ? true : get_mitigations_off();
> + if (disabled)
> + return true;
> +
> + /*
> + * Some unpriv tests rely on spectre mitigations being on.
> + * If mitigations are off or status can't be determined
> + * assume that unpriv tests are disabled.
> + */
> + mitigations_off = get_mitigations_off();
> + if (mitigations_off < 0) {
> + fprintf(stderr,
> + "Can't determine if mitigations are enabled, disabling unpriv tests.");
> + return true;
> + }
> + return mitigations_off;
> }
> --
> 2.47.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread