Re: [PATCH v3 bpf-next] selftests/bpf: Fix selftests broken by mitigations=off

[Yonghong Song <yonghong.song@linux.dev>]

On 10/24/23 8:11 PM, Yafang Shao wrote:
> When we configure the kernel command line with 'mitigations=off' and set
> the sysctl knob 'kernel.unprivileged_bpf_disabled' to 0, the commit
> bc5bc309db45 ("bpf: Inherit system settings for CPU security mitigations")
> causes issues in the execution of `test_progs -t verifier`. This is because
> 'mitigations=off' bypasses Spectre v1 and Spectre v4 protections.
>
> Currently, when a program requests to run in unprivileged mode
> (kernel.unprivileged_bpf_disabled = 0), the BPF verifier may prevent it
> from running due to the following conditions not being enabled:
>
>    - bypass_spec_v1
>    - bypass_spec_v4
>    - allow_ptr_leaks
>    - allow_uninit_stack
>
> While 'mitigations=off' enables the first two conditions, it does not
> enable the latter two. As a result, some test cases in
> 'test_progs -t verifier' that were expected to fail to run may run
> successfully, while others still fail but with different error messages.
> This makes it challenging to address them comprehensively.
>
> Moreover, in the future, we may introduce more fine-grained control over
> CPU mitigations, such as enabling only bypass_spec_v1 or bypass_spec_v4.
>
> Given the complexity of the situation, rather than fixing each broken test
> case individually, it's preferable to skip them when 'mitigations=off' is
> in effect and introduce specific test cases for the new 'mitigations=off'
> scenario. For instance, we can introduce new BTF declaration tags like
> '__failure__nospec', '__failure_nospecv1' and '__failure_nospecv4'.
>
> In this patch, the approach is to simply skip the broken test cases when
> 'mitigations=off' is enabled. The result of `test_progs -t verifier` as
> follows after this commit,
>
> Before this commit
> ==================
> - without 'mitigations=off'
>    - kernel.unprivileged_bpf_disabled = 2
>      Summary: 74/948 PASSED, 388 SKIPPED, 0 FAILED
>    - kernel.unprivileged_bpf_disabled = 0
>      Summary: 74/1336 PASSED, 0 SKIPPED, 0 FAILED   <<<<
> - with 'mitigations=off'
>    - kernel.unprivileged_bpf_disabled = 2
>      Summary: 74/948 PASSED, 388 SKIPPED, 0 FAILED
>    - kernel.unprivileged_bpf_disabled = 0
>      Summary: 63/1276 PASSED, 0 SKIPPED, 11 FAILED   <<<< 11 FAILED
>
> After this commit
> =================
> - without 'mitigations=off'
>    - kernel.unprivileged_bpf_disabled = 2
>      Summary: 74/948 PASSED, 388 SKIPPED, 0 FAILED
>    - kernel.unprivileged_bpf_disabled = 0
>      Summary: 74/1336 PASSED, 0 SKIPPED, 0 FAILED    <<<<
> - with this patch, with 'mitigations=off'
>    - kernel.unprivileged_bpf_disabled = 2
>      Summary: 74/948 PASSED, 388 SKIPPED, 0 FAILED
>    - kernel.unprivileged_bpf_disabled = 0
>      Summary: 74/948 PASSED, 388 SKIPPED, 0 FAILED   <<<< SKIPPED
>
> Fixes: bc5bc309db45 ("bpf: Inherit system settings for CPU security mitigations")
> Reported-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Closes: https://lore.kernel.org/bpf/CAADnVQKUBJqg+hHtbLeeC2jhoJAWqnmRAzXW3hmUCNSV9kx4sQ@mail.gmail.com
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Ack with a nit below.
Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   tools/testing/selftests/bpf/unpriv_helpers.c | 35 +++++++++++++++++++-
>   1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/unpriv_helpers.c b/tools/testing/selftests/bpf/unpriv_helpers.c
> index 2a6efbd0401e..7101e72ef4a3 100644
> --- a/tools/testing/selftests/bpf/unpriv_helpers.c
> +++ b/tools/testing/selftests/bpf/unpriv_helpers.c
> @@ -4,9 +4,42 @@
>   #include <stdlib.h>
>   #include <error.h>
>   #include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <fcntl.h>
>   
>   #include "unpriv_helpers.h"
>   
> [...]
>   bool get_unpriv_disabled(void)
>   {
>   	bool disabled;
> @@ -22,5 +55,5 @@ bool get_unpriv_disabled(void)
>   		disabled = true;
>   	}
>   
> -	return disabled;
> +	return disabled ? true : get_mitigations_off();

Above code is correct. But you could slightly simplify it with
	return disabled ? : get_mitigations_off();

I guess maintainer can decide whether simplification is needed
or not.

>   }