v5:
-- Reuse is_ldimm64_insn() and insn_is_pseudo_func(),
thanks Song Liu.
v4:
-- Move the not-allowed-checking into "if (expected_ret ...)"
block, thanks Hou Tao.
-- Do some small changes to avoid checkpatch warning
about "line length exceeds 100 columns".
v3:
-- Rebase on the latest bpf-next tree.
-- Address the review comments by Hou Tao,
remove the second argument "0" of open(),
check only once whether jit is disabled,
check fd_prog, saved_errno and jit_disabled to skip.
Tiezhu Yang (3):
selftests/bpf: Move is_jit_enabled() to testing_helpers
libbpf: Move insn_is_pseudo_func() to libbpf_internal.h
selftests/bpf: Skip callback tests if jit is disabled in
test_verifier
tools/lib/bpf/libbpf.c | 5 -----
tools/lib/bpf/libbpf_internal.h | 5 +++++
tools/testing/selftests/bpf/test_progs.c | 18 -----------------
tools/testing/selftests/bpf/test_verifier.c | 20 ++++++++++++++++---
tools/testing/selftests/bpf/testing_helpers.c | 18 +++++++++++++++++
tools/testing/selftests/bpf/testing_helpers.h | 1 +
6 files changed, 41 insertions(+), 26 deletions(-)
--
2.42.0
Currently, insn_is_pseudo_func() is only used in libbpf.c, move it
to libbpf_internal.h so that it can be used in test_verifier, this
is preparation for later patch.
Suggested-by: Song Liu <[email protected]>
Signed-off-by: Tiezhu Yang <[email protected]>
---
tools/lib/bpf/libbpf.c | 5 -----
tools/lib/bpf/libbpf_internal.h | 5 +++++
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index c5a42ac309fd..259d585d6ff5 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -748,11 +748,6 @@ static bool is_call_insn(const struct bpf_insn *insn)
return insn->code == (BPF_JMP | BPF_CALL);
}
-static bool insn_is_pseudo_func(struct bpf_insn *insn)
-{
- return is_ldimm64_insn(insn) && insn->src_reg == BPF_PSEUDO_FUNC;
-}
-
static int
bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog,
const char *name, size_t sec_idx, const char *sec_name,
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 27e4e320e1a6..a9c337345aff 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -532,6 +532,11 @@ static inline bool is_ldimm64_insn(struct bpf_insn *insn)
return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
}
+static inline bool insn_is_pseudo_func(struct bpf_insn *insn)
+{
+ return is_ldimm64_insn(insn) && insn->src_reg == BPF_PSEUDO_FUNC;
+}
+
/* if fd is stdin, stdout, or stderr, dup to a fd greater than 2
* Takes ownership of the fd passed in, and closes it if calling
* fcntl(fd, F_DUPFD_CLOEXEC, 3).
--
2.42.0
Currently, is_jit_enabled() is only used in test_progs, move it to
testing_helpers so that it can be used in test_verifier. While at
it, remove the second argument "0" of open() as Hou Tao suggested.
Signed-off-by: Tiezhu Yang <[email protected]>
Acked-by: Hou Tao <[email protected]>
Acked-by: Song Liu <[email protected]>
---
tools/testing/selftests/bpf/test_progs.c | 18 ------------------
tools/testing/selftests/bpf/testing_helpers.c | 18 ++++++++++++++++++
tools/testing/selftests/bpf/testing_helpers.h | 1 +
3 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 1b9387890148..808550986f30 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -547,24 +547,6 @@ int bpf_find_map(const char *test, struct bpf_object *obj, const char *name)
return bpf_map__fd(map);
}
-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);
- if (sysctl_fd != -1) {
- char tmpc;
-
- if (read(sysctl_fd, &tmpc, sizeof(tmpc)) == 1)
- enabled = (tmpc != '0');
- close(sysctl_fd);
- }
-
- return enabled;
-}
-
int compare_map_keys(int map1_fd, int map2_fd)
{
__u32 key, next_key;
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 106ef05586b8..a59e56d804ee 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -457,3 +457,21 @@ int get_xlated_program(int fd_prog, struct bpf_insn **buf, __u32 *cnt)
*buf = NULL;
return -1;
}
+
+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, O_RDONLY);
+ if (sysctl_fd != -1) {
+ char tmpc;
+
+ if (read(sysctl_fd, &tmpc, sizeof(tmpc)) == 1)
+ enabled = (tmpc != '0');
+ close(sysctl_fd);
+ }
+
+ return enabled;
+}
diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
index e099aa4da611..d14de81727e6 100644
--- a/tools/testing/selftests/bpf/testing_helpers.h
+++ b/tools/testing/selftests/bpf/testing_helpers.h
@@ -52,5 +52,6 @@ struct bpf_insn;
*/
int get_xlated_program(int fd_prog, struct bpf_insn **buf, __u32 *cnt);
int testing_prog_flags(void);
+bool is_jit_enabled(void);
#endif /* __TESTING_HELPERS_H */
--
2.42.0
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 <[email protected]>
Acked-by: Hou Tao <[email protected]>
---
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.0
On Wed, Jan 17, 2024 at 3:10 AM Tiezhu Yang <[email protected]> wrote:
>
> Currently, insn_is_pseudo_func() is only used in libbpf.c, move it
> to libbpf_internal.h so that it can be used in test_verifier, this
> is preparation for later patch.
>
> Suggested-by: Song Liu <[email protected]>
> Signed-off-by: Tiezhu Yang <[email protected]>
Acked-by: Song Liu <[email protected]>
On Wed, Jan 17, 2024 at 3:10 AM Tiezhu Yang <[email protected]> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>
> Thanks,
> Song
>
> .
Hi,
On Wed, Jan 17, 2024 at 5:11 PM Hou Tao <[email protected]> wrote:
>
> Hi Song,
>
> On 1/18/2024 1:20 AM, Song Liu wrote:
> > On Wed, Jan 17, 2024 at 3:10 AM Tiezhu Yang <[email protected]> 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 <[email protected]> wrote:
>> Hi Song,
>>
>> On 1/18/2024 1:20 AM, Song Liu wrote:
>>> On Wed, Jan 17, 2024 at 3:10 AM Tiezhu Yang <[email protected]> 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
On 1/17/2024 7:09 PM, Tiezhu Yang wrote:
> Currently, insn_is_pseudo_func() is only used in libbpf.c, move it
> to libbpf_internal.h so that it can be used in test_verifier, this
> is preparation for later patch.
>
> Suggested-by: Song Liu <[email protected]>
> Signed-off-by: Tiezhu Yang <[email protected]>
Acked-by: Hou Tao <[email protected]>
On Wed, Jan 17, 2024 at 3:10 AM Tiezhu Yang <[email protected]> wrote:
>
> Currently, insn_is_pseudo_func() is only used in libbpf.c, move it
> to libbpf_internal.h so that it can be used in test_verifier, this
> is preparation for later patch.
>
> Suggested-by: Song Liu <[email protected]>
> Signed-off-by: Tiezhu Yang <[email protected]>
> ---
> tools/lib/bpf/libbpf.c | 5 -----
> tools/lib/bpf/libbpf_internal.h | 5 +++++
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index c5a42ac309fd..259d585d6ff5 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -748,11 +748,6 @@ static bool is_call_insn(const struct bpf_insn *insn)
> return insn->code == (BPF_JMP | BPF_CALL);
> }
>
> -static bool insn_is_pseudo_func(struct bpf_insn *insn)
> -{
> - return is_ldimm64_insn(insn) && insn->src_reg == BPF_PSEUDO_FUNC;
> -}
> -
> static int
> bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog,
> const char *name, size_t sec_idx, const char *sec_name,
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 27e4e320e1a6..a9c337345aff 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -532,6 +532,11 @@ static inline bool is_ldimm64_insn(struct bpf_insn *insn)
> return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
> }
>
> +static inline bool insn_is_pseudo_func(struct bpf_insn *insn)
> +{
> + return is_ldimm64_insn(insn) && insn->src_reg == BPF_PSEUDO_FUNC;
> +}
> +
This just adds more internal code of libbpf used by selftests. While
we've allowed it in some cases to avoid duplication of more complex
logic, I don't feel like it's justified in this case. These helpers
are trivial enough to copy/paste somewhere into selftests helpers
header, so please do that instead.
> /* if fd is stdin, stdout, or stderr, dup to a fd greater than 2
> * Takes ownership of the fd passed in, and closes it if calling
> * fcntl(fd, F_DUPFD_CLOEXEC, 3).
> --
> 2.42.0
>