linux-kernel - Re: [PATCH bpf-next v2] selftests/bpf: Skip callback tests if jit is disabled in test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 12 Jan 2024 12:21:28 +0800
From: Hou Tao <houtao@...weicloud.com>
To: Tiezhu Yang <yangtiezhu@...ngson.cn>, Alexei Starovoitov
 <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Andrii Nakryiko <andrii@...nel.org>
Cc: Eduard Zingerman <eddyz87@...il.com>,
 John Fastabend <john.fastabend@...il.com>, Jiri Olsa <jolsa@...nel.org>,
 bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2] selftests/bpf: Skip callback tests if jit is
 disabled in test_verifier

Hi,

On 1/12/2024 9:57 AM, Tiezhu Yang wrote:
> If CONFIG_BPF_JIT_ALWAYS_ON is not set and bpf_jit_enable is 0, there
> exist 6 failed tests.
>
>   [root@...ux bpf]# echo 0 > /proc/sys/net/core/bpf_jit_enable
>   [root@...ux bpf]# echo 0 > /proc/sys/kernel/unprivileged_bpf_disabled
>   [root@...ux bpf]# ./test_verifier | grep FAIL
>   #106/p inline simple bpf_loop call FAIL
>   #107/p don't inline bpf_loop call, flags non-zero FAIL
>   #108/p don't inline bpf_loop call, callback non-constant FAIL
>   #109/p bpf_loop_inline and a dead func FAIL
>   #110/p bpf_loop_inline stack locations for loop vars FAIL
>   #111/p inline bpf_loop call in a big program FAIL
>   Summary: 768 PASSED, 15 SKIPPED, 6 FAILED
>
> The test log shows that callbacks are not allowed in non-JITed programs,
> interpreter doesn't support them yet, thus these tests should be skipped
> if jit is disabled, copy some check functions from the other places under
> tools directory, and then handle this case in do_test_single().
>
> With this patch:
>
>   [root@...ux bpf]# echo 0 > /proc/sys/net/core/bpf_jit_enable
>   [root@...ux bpf]# echo 0 > /proc/sys/kernel/unprivileged_bpf_disabled
>   [root@...ux bpf]# ./test_verifier | grep FAIL
>   Summary: 768 PASSED, 21 SKIPPED, 0 FAILED
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
> ---
> v2: Remove inline keyword in C files, sorry for that.
>
> Thanks very much for the feedbacks from Eduard, John, Jiri and Daniel.
> I do not move loop inlining tests to test_progs, just copy some check
> functions and do the minimal changes in test_verifier.
>
>  tools/testing/selftests/bpf/test_verifier.c | 39 +++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index f36e41435be7..d4e600e3caec 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -21,6 +21,7 @@
>  #include <sched.h>
>  #include <limits.h>
>  #include <assert.h>
> +#include <fcntl.h>
>  
>  #include <linux/unistd.h>
>  #include <linux/filter.h>
> @@ -1397,6 +1398,34 @@ static bool is_skip_insn(struct bpf_insn *insn)
>  	return memcmp(insn, &skip_insn, sizeof(skip_insn)) == 0;
>  }
>  
> +static bool is_ldimm64_insn(struct bpf_insn *insn)
> +{
> +	return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
> +}
> +
> +static bool insn_is_pseudo_func(struct bpf_insn *insn)
> +{
> +	return is_ldimm64_insn(insn) && insn->src_reg == BPF_PSEUDO_FUNC;
> +}
> +
> +static bool is_jit_enabled(void)
> +{
> +	const char *jit_sysctl = "/proc/sys/net/core/bpf_jit_enable";
> +	bool enabled = false;
> +	int sysctl_fd;
> +
> +	sysctl_fd = open(jit_sysctl, 0, O_RDONLY);

It should be open(jit_sysctl, O_RDONLY).
> +	if (sysctl_fd != -1) {
> +		char tmpc;
> +
> +		if (read(sysctl_fd, &tmpc, sizeof(tmpc)) == 1)
> +			enabled = (tmpc != '0');
> +		close(sysctl_fd);
> +	}
> +
> +	return enabled;
> +}
> +
>  static int null_terminated_insn_len(struct bpf_insn *seq, int max_len)
>  {
>  	int i;
> @@ -1662,6 +1691,16 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>  		goto close_fds;
>  	}
>  
> +	if (!is_jit_enabled()) {

Is it necessary to check whether jit is enabled or not each time ? Could
we just check it only once just like unpriv_disabled does ?
> +		for (i = 0; i < prog_len; i++, prog++) {

Is it better to only check pseudo_func only when both fd_prog < 0 and
saved_errno == EINVAL are true, so unnecessary check can be skipped ?
> +			if (insn_is_pseudo_func(prog)) {
> +				printf("SKIP (callbacks are not allowed in non-JITed programs)\n");
> +				skips++;
> +				goto close_fds;
> +			}
> +		}
> +	}
> +
>  	alignment_prevented_execution = 0;
>  
>  	if (expected_ret == ACCEPT || expected_ret == VERBOSE_ACCEPT) {