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, just handle this case in do_test_single().
After including bpf/libbpf_internal.h, there exist some build errors:
error: attempt to use poisoned "u32"
error: attempt to use poisoned "u64"
replace u32 and u64 with __u32 and __u64 to fix them.
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>
Acked-by: Hou Tao <houtao1@huawei.com>
---
tools/testing/selftests/bpf/test_verifier.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 1a09fc34d093..c7f57b5b04a7 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -41,6 +41,7 @@
#include "test_btf.h"
#include "../../../include/linux/filter.h"
#include "testing_helpers.h"
+#include "bpf/libbpf_internal.h"
#ifndef ENOTSUPP
#define ENOTSUPP 524
@@ -74,6 +75,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;
@@ -1143,8 +1145,8 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
} while (*fixup_map_xskmap);
}
if (*fixup_map_stacktrace) {
- map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(u32),
- sizeof(u64), 1);
+ map_fds[12] = create_map(BPF_MAP_TYPE_STACK_TRACE, sizeof(__u32),
+ sizeof(__u64), 1);
do {
prog[*fixup_map_stacktrace].imm = map_fds[12];
fixup_map_stacktrace++;
@@ -1203,7 +1205,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
}
if (*fixup_map_reuseport_array) {
map_fds[19] = __create_map(BPF_MAP_TYPE_REUSEPORT_SOCKARRAY,
- sizeof(u32), sizeof(u64), 1, 0);
+ sizeof(__u32), sizeof(__u64), 1, 0);
do {
prog[*fixup_map_reuseport_array].imm = map_fds[19];
fixup_map_reuseport_array++;
@@ -1622,6 +1624,16 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
alignment_prevented_execution = 0;
if (expected_ret == ACCEPT || expected_ret == VERBOSE_ACCEPT) {
+ if (fd_prog < 0 && saved_errno == EINVAL && jit_disabled) {
+ for (i = 0; i < prog_len; i++, prog++) {
+ if (!insn_is_pseudo_func(prog))
+ continue;
+ printf("SKIP (callbacks are not allowed in non-JITed programs)\n");
+ skips++;
+ goto close_fds;
+ }
+ }
+
if (fd_prog < 0) {
printf("FAIL\nFailed to load prog '%s'!\n",
strerror(saved_errno));
@@ -1844,6 +1856,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);
--
2.42.0On Wed, Jan 17, 2024 at 3:10 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
[...]
> @@ -1622,6 +1624,16 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> alignment_prevented_execution = 0;
>
> if (expected_ret == ACCEPT || expected_ret == VERBOSE_ACCEPT) {
> + if (fd_prog < 0 && saved_errno == EINVAL && jit_disabled) {
> + for (i = 0; i < prog_len; i++, prog++) {
> + if (!insn_is_pseudo_func(prog))
> + continue;
> + printf("SKIP (callbacks are not allowed in non-JITed programs)\n");
> + skips++;
> + goto close_fds;
> + }
> + }
> +
I would put this chunk above "alignment_prevented_execution = 0;".
@@ -1619,6 +1621,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))
+ continue;
+ 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) {
Other than this,
Acked-by: Song Liu <song@kernel.org>
Thanks,
Song
Hi Song,
On 1/18/2024 1:20 AM, Song Liu wrote:
> On Wed, Jan 17, 2024 at 3:10 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
> [...]
>> @@ -1622,6 +1624,16 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>> alignment_prevented_execution = 0;
>>
>> if (expected_ret == ACCEPT || expected_ret == VERBOSE_ACCEPT) {
>> + if (fd_prog < 0 && saved_errno == EINVAL && jit_disabled) {
>> + for (i = 0; i < prog_len; i++, prog++) {
>> + if (!insn_is_pseudo_func(prog))
>> + continue;
>> + printf("SKIP (callbacks are not allowed in non-JITed programs)\n");
>> + skips++;
>> + goto close_fds;
>> + }
>> + }
>> +
> I would put this chunk above "alignment_prevented_execution = 0;".
>
> @@ -1619,6 +1621,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))
> + continue;
> + 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) {
>
> Other than this,
The check was placed before the checking of expected_ret in v3. However
I suggested Tiezhu to move it after the checking of expected_ret due to
the following two reasons:
1) when the expected result is REJECT, the return value in about one
third of these test cases is -EINVAL. And I think we should not waste
the cpu to check the pseudo func and exit prematurely, instead we should
let test_verifier check expected_err.
2) As for now all expected_ret of these failed cases are ACCEPT when jit
is disabled, so I think it will be enough for current situation and we
can revise it later if the checking of pseudo func is too later.
So wdyt ?
>
> Acked-by: Song Liu <song@kernel.org>
>
> Thanks,
> Song
>
> .
Hi,
On Wed, Jan 17, 2024 at 5:11 PM Hou Tao <houtao@huaweicloud.com> wrote:
>
> Hi Song,
>
> On 1/18/2024 1:20 AM, Song Liu wrote:
> > On Wed, Jan 17, 2024 at 3:10 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
> > [...]
> >> @@ -1622,6 +1624,16 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >> alignment_prevented_execution = 0;
> >>
> >> if (expected_ret == ACCEPT || expected_ret == VERBOSE_ACCEPT) {
> >> + if (fd_prog < 0 && saved_errno == EINVAL && jit_disabled) {
> >> + for (i = 0; i < prog_len; i++, prog++) {
> >> + if (!insn_is_pseudo_func(prog))
> >> + continue;
> >> + printf("SKIP (callbacks are not allowed in non-JITed programs)\n");
> >> + skips++;
> >> + goto close_fds;
> >> + }
> >> + }
> >> +
> > I would put this chunk above "alignment_prevented_execution = 0;".
> >
> > @@ -1619,6 +1621,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))
> > + continue;
> > + 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) {
> >
> > Other than this,
>
> The check was placed before the checking of expected_ret in v3. However
> I suggested Tiezhu to move it after the checking of expected_ret due to
I missed this part while reading the history of the set.
> the following two reasons:
> 1) when the expected result is REJECT, the return value in about one
> third of these test cases is -EINVAL. And I think we should not waste
> the cpu to check the pseudo func and exit prematurely, instead we should
> let test_verifier check expected_err.
I was thinking jit_disabled is not a common use case so that it is OK for
this path to be a little expensive.
> 2) As for now all expected_ret of these failed cases are ACCEPT when jit
> is disabled, so I think it will be enough for current situation and we
> can revise it later if the checking of pseudo func is too later.
That said, I won't object if we ship this version as-is.
Thanks,
Song
Hi,
On 1/18/2024 9:27 AM, Song Liu wrote:
> Hi,
>
> On Wed, Jan 17, 2024 at 5:11 PM Hou Tao <houtao@huaweicloud.com> wrote:
>> Hi Song,
>>
>> On 1/18/2024 1:20 AM, Song Liu wrote:
>>> On Wed, Jan 17, 2024 at 3:10 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>>> [...]
>>>> @@ -1622,6 +1624,16 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>>>> alignment_prevented_execution = 0;
>>>>
>>>> if (expected_ret == ACCEPT || expected_ret == VERBOSE_ACCEPT) {
>>>> + if (fd_prog < 0 && saved_errno == EINVAL && jit_disabled) {
>>>> + for (i = 0; i < prog_len; i++, prog++) {
>>>> + if (!insn_is_pseudo_func(prog))
>>>> + continue;
>>>> + printf("SKIP (callbacks are not allowed in non-JITed programs)\n");
>>>> + skips++;
>>>> + goto close_fds;
>>>> + }
>>>> + }
>>>> +
>>> I would put this chunk above "alignment_prevented_execution = 0;".
>>>
>>> @@ -1619,6 +1621,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))
>>> + continue;
>>> + 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) {
>>>
>>> Other than this,
>> The check was placed before the checking of expected_ret in v3. However
>> I suggested Tiezhu to move it after the checking of expected_ret due to
> I missed this part while reading the history of the set.
>
>> the following two reasons:
>> 1) when the expected result is REJECT, the return value in about one
>> third of these test cases is -EINVAL. And I think we should not waste
>> the cpu to check the pseudo func and exit prematurely, instead we should
>> let test_verifier check expected_err.
> I was thinking jit_disabled is not a common use case so that it is OK for
> this path to be a little expensive.
>
>> 2) As for now all expected_ret of these failed cases are ACCEPT when jit
>> is disabled, so I think it will be enough for current situation and we
>> can revise it later if the checking of pseudo func is too later.
> That said, I won't object if we ship this version as-is.
I see and thanks for the explanation.
> Thanks,
> Song
© 2016 - 2025 Red Hat, Inc.