Received-SPF: pass (google.com: domain of linux-kernel+bounces-26068-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Subject: Re: [PATCH bpf-next v3 2/2] selftests/bpf: Skip callback tests if jit
 is disabled in test_verifier
To: Tiezhu Yang <yangtiezhu@loongson.cn>, Alexei Starovoitov
 <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>,
 Andrii Nakryiko <andrii@kernel.org>
Cc: Eduard Zingerman <eddyz87@gmail.com>,
 John Fastabend <john.fastabend@gmail.com>, Jiri Olsa <jolsa@kernel.org>,
 bpf@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240115070010.12338-1-yangtiezhu@loongson.cn>
 <20240115070010.12338-3-yangtiezhu@loongson.cn>
From: Hou Tao <houtao@huaweicloud.com>
Message-ID: <84e15d1c-c2a6-9af4-c123-beea01893a8f@huaweicloud.com>
Date: Mon, 15 Jan 2024 22:00:17 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240115070010.12338-3-yangtiezhu@loongson.cn>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US

Hi,

On 1/15/2024 3:00 PM, Tiezhu Yang wrote:
> If CONFIG_BPF_JIT_ALWAYS_ON is not set and bpf_jit_enable is 0, there
> exist 6 failed tests.
>
>   [root@linux bpf]# echo 0 > /proc/sys/net/core/bpf_jit_enable
>   [root@linux bpf]# echo 0 > /proc/sys/kernel/unprivileged_bpf_disabled
>   [root@linux 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@linux bpf]# echo 0 > /proc/sys/net/core/bpf_jit_enable
>   [root@linux bpf]# echo 0 > /proc/sys/kernel/unprivileged_bpf_disabled
>   [root@linux bpf]# ./test_verifier | grep FAIL
>   Summary: 768 PASSED, 21 SKIPPED, 0 FAILED
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  tools/testing/selftests/bpf/test_verifier.c | 23 +++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 1a09fc34d093..70f903e869b7 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -74,6 +74,7 @@
>  		    1ULL << CAP_BPF)
>  #define UNPRIV_SYSCTL "kernel/unprivileged_bpf_disabled"
>  static bool unpriv_disabled = false;
> +static bool jit_disabled;
>  static int skips;
>  static bool verbose = false;
>  static int verif_log_level = 0;
> @@ -1355,6 +1356,16 @@ 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 int null_terminated_insn_len(struct bpf_insn *seq, int max_len)
>  {
>  	int i;
> @@ -1619,6 +1630,16 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>  		goto close_fds;
>  	}
>  
> +	if (fd_prog < 0 && saved_errno == EINVAL && jit_disabled) {
> +		for (i = 0; i < prog_len; i++, prog++) {
> +			if (insn_is_pseudo_func(prog)) {
> +				printf("SKIP (callbacks are not allowed in non-JITed programs)\n");
> +				skips++;
> +				goto close_fds;
> +			}
> +		}
> +	}

I ran test_verifier before applying the patch set, it seems all
expected_ret for these failed programs are ACCEPT, so I think it would
be better to move the not-allowed-checking into "if (expected_ret ==
ACCEPT || expected_ret == VERBOSE_ACCEPT)" block. I should suggest such
modification in v2, sorry about that.
> +
>  	alignment_prevented_execution = 0;
>  
>  	if (expected_ret == ACCEPT || expected_ret == VERBOSE_ACCEPT) {
> @@ -1844,6 +1865,8 @@ int main(int argc, char **argv)
>  		return EXIT_FAILURE;
>  	}
>  
> +	jit_disabled = !is_jit_enabled();
> +
>  	/* Use libbpf 1.0 API mode */
>  	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
>