* [RFC bpf-next 0/5] test_verifier tests migration to inline assembly
@ 2023-01-23 14:51 Eduard Zingerman
2023-01-23 14:51 ` [RFC bpf-next 1/5] selftests/bpf: support custom per-test flags and multiple expected messages Eduard Zingerman
` (5 more replies)
0 siblings, 6 replies; 18+ messages in thread
From: Eduard Zingerman @ 2023-01-23 14:51 UTC (permalink / raw)
To: bpf, ast; +Cc: andrii, daniel, kernel-team, yhs, Eduard Zingerman
As a part of the discussion started in [2] I developed a script [1]
that can convert tests specified in test_verifier.c format to inline
BPF assembly compatible for use with test_loader.c.
For example, test definition like below:
{
"invalid and of negative number",
.insns = {
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
BPF_LD_MAP_FD(BPF_REG_1, 0),
BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5),
BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
BPF_EXIT_INSN(),
},
.fixup_map_hash_48b = { 3 },
.errstr = "R0 max value is outside of the allowed memory range",
.result = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},
Is converted to:
struct test_val { ... };
struct { ...} map_hash_48b SEC(".maps");
__description("invalid and of negative number")
__failure __msg("R0 max value is outside of the allowed memory range")
__failure_unpriv
__flag(BPF_F_ANY_ALIGNMENT)
SEC("socket")
__naked void invalid_and_of_negative_number(void)
{
asm volatile (
" r1 = 0; \n\
*(u64*)(r10 - 8) = r1; \n\
r2 = r10; \n\
r2 += -8; \n\
r1 = %[map_hash_48b] ll; \n\
call %[bpf_map_lookup_elem]; \n\
if r0 == 0 goto l0_%=; \n\
r1 = *(u8*)(r0 + 0); \n\
r1 &= -4; \n\
r1 <<= 2; \n\
r0 += r1; \n\
r1 = %[test_val_foo_offset]; \n\
*(u64*)(r0 + 0) = r1; \n\
l0_%=: \n\
exit; \n\
" :
: [test_val_foo_offset]"i"(offsetof(struct test_val, foo)),
__imm(bpf_map_lookup_elem),
__imm_addr(map_hash_48b)
: __clobber_all);
}
The script introduces labels for gotos and calls, immediate values for
complex expressions like `offsetof(...)', takes care of map
instructions patching, inserts map declarations used in the test,
transfers comments from test, test fields and instructions. There are
some issues with BPF_ST_MEM instruction support as described in [4],
thus the script replaces such instructions with pairs of MOV/STX_MEM
instructions.
This patch-set introduces changes necessary for test_loader.c and
includes migration of a single test from test_verifier to test_progs
format, here are descriptions for individual patches:
1. "Support custom per-test flags and multiple expected messages"
This patch was shared by Andrii Nakryiko [3], it adds capability
to specify flags required by the BPF program.
2. "Unprivileged tests for test_loader.c"
Extends test_loader.c by capability to load test programs in
unprivileged mode and specify separate test outcomes for
privileged and unprivileged modes.
Note: for a reason I do not understand I had to use different set
of capabilities compared to test_verifier:
- test_verifier: CAP_NET_ADMIN, CAP_PERFMON, CAP_BPF;
- test_loader : CAP_SYS_ADMIN, CAP_PERFMON, CAP_BPF.
3. "Generate boilerplate code for test_loader-based tests"
Extends selftests/bpf Makefile to automatically generate
prog_tests/tests.h entry that uses test_loader for progs/*.c
BPF programs if special marker is present.
4. "__imm_insn macro to embed raw insns in inline asm"
This macro can be generated by migration script for instructions
that cannot be expressed in inline assembly, e.g. invalid instructions.
5. "convert jeq_infer_not_null tests to inline assembly"
Shows an example of the test migration.
The test was updated slightly after automatic translation:
- unnecessary comments removed;
- functions renamed;
- some labels renamed.
The migration script has some limitations:
- Technical, test features that are not yet supported:
- few instructions like BPF_ENDIAN;
- macro like BPF_SK_LOOKUP or BPF_LD_MAP_VALUE;
- program types like BPF_PROG_TYPE_CGROUP_SOCK_ADDR and
BPF_PROG_TYPE_TRACING;
- tests that specify fields 'expected_attach_type' or 'insn_processed';
- a few tests expose complex structure that could not be
automatically migrated, e.g.: 'atomic_fetch', 'lwt',
'bpf_loop_inline'.
- Tests that use .run field cannot be migrated as test_loader.c tests.
- Tests with generated bodies, e.g. test_verifier.c:bpf_fill_scale()
cannot be migrated as test_loader.c tests.
- LLVM related:
- BPF_ST instruction is not supported by LLVM BPF assembly yet
(I'll take care of it);
- Issues with parsing of some assembly instructions like
"r0 = cmpxchg_64(r10 - 8, r0, r5)"
(I'll take care of it);
- libbpf related:
- libbpf does not support call instructions within a single
function, e.g.:
0: r1 = 1
1: r2 = 2
2: call 1
3: exit
4: r0 = r1
5: r0 += r2
6: exit
This pattern is common in verifier tests, I see two possible
mitigation:
(a) use an additional libbpf flag to allow such code;
(b) extend migration script to split such code in several functions.
I like (a) more.
- libbpf does not allow empty programs.
All in all the current script stats are as follows:
- 62 out of 93 files from progs/*.c can be converted w/o warnings;
- 55 converted files could be compiled;
- 40 pass testing, 15 fail.
By submitting this RFC I seek the following feedback:
- is community interested in such migration?
- if yes, should I pursue partial or complete tests migration?
- in case of partial migration which tests should be prioritized?
- should I offer migrated tests one by one or in big butches?
Thanks,
Eduard
[1] https://github.com/eddyz87/verifier-tests-migrator
[2] https://lore.kernel.org/bpf/CAEf4BzYPsDWdRgx+ND1wiKAB62P=WwoLhr2uWkbVpQfbHqi1oA@mail.gmail.com/
[3] https://lore.kernel.org/bpf/CAEf4BzZH0ZxorCi7nPDbRqSK9f+410RooNwNJGwfw8=0a5i1nw@mail.gmail.com/
[4] https://lore.kernel.org/bpf/20221231163122.1360813-1-eddyz87@gmail.com/
Andrii Nakryiko (1):
selftests/bpf: support custom per-test flags and multiple expected
messages
Eduard Zingerman (4):
selftests/bpf: unprivileged tests for test_loader.c
selftests/bpf: generate boilerplate code for test_loader-based tests
selftests/bpf: __imm_insn macro to embed raw insns in inline asm
selftests/bpf: convert jeq_infer_not_null tests to inline assembly
tools/testing/selftests/bpf/Makefile | 41 +-
tools/testing/selftests/bpf/autoconf_helper.h | 9 +
.../selftests/bpf/prog_tests/.gitignore | 1 +
.../bpf/prog_tests/jeq_infer_not_null.c | 9 -
tools/testing/selftests/bpf/progs/bpf_misc.h | 49 +++
.../selftests/bpf/progs/jeq_infer_not_null.c | 186 +++++++++
.../bpf/progs/jeq_infer_not_null_fail.c | 1 +
tools/testing/selftests/bpf/test_loader.c | 370 +++++++++++++++---
tools/testing/selftests/bpf/test_progs.h | 1 +
tools/testing/selftests/bpf/test_verifier.c | 25 +-
tools/testing/selftests/bpf/unpriv_helpers.c | 26 ++
tools/testing/selftests/bpf/unpriv_helpers.h | 7 +
.../bpf/verifier/jeq_infer_not_null.c | 174 --------
13 files changed, 625 insertions(+), 274 deletions(-)
create mode 100644 tools/testing/selftests/bpf/autoconf_helper.h
delete mode 100644 tools/testing/selftests/bpf/prog_tests/jeq_infer_not_null.c
create mode 100644 tools/testing/selftests/bpf/progs/jeq_infer_not_null.c
create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.c
create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.h
delete mode 100644 tools/testing/selftests/bpf/verifier/jeq_infer_not_null.c
--
2.39.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC bpf-next 1/5] selftests/bpf: support custom per-test flags and multiple expected messages
2023-01-23 14:51 [RFC bpf-next 0/5] test_verifier tests migration to inline assembly Eduard Zingerman
@ 2023-01-23 14:51 ` Eduard Zingerman
2023-02-28 18:53 ` Andrii Nakryiko
2023-01-23 14:51 ` [RFC bpf-next 2/5] selftests/bpf: unprivileged tests for test_loader.c Eduard Zingerman
` (4 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Eduard Zingerman @ 2023-01-23 14:51 UTC (permalink / raw)
To: bpf, ast; +Cc: andrii, daniel, kernel-team, yhs, Eduard Zingerman
From: Andrii Nakryiko <andrii@kernel.org>
Extend __flag attribute by allowing to specify one of the following:
* BPF_F_STRICT_ALIGNMENT
* BPF_F_ANY_ALIGNMENT
* BPF_F_TEST_RND_HI32
* BPF_F_TEST_STATE_FREQ
* BPF_F_SLEEPABLE
* BPF_F_XDP_HAS_FRAGS
* Some numeric value
Extend __msg attribute by allowing to specify multiple exepcted messages.
All messages are expected to be present in the verifier log in the
order of application.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
[ Eduard: added commit message, formatting ]
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
tools/testing/selftests/bpf/test_loader.c | 69 ++++++++++++++++++++---
tools/testing/selftests/bpf/test_progs.h | 1 +
2 files changed, 61 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 679efb3aa785..bf41390157bf 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -13,12 +13,15 @@
#define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
#define TEST_TAG_EXPECT_MSG_PFX "comment:test_expect_msg="
#define TEST_TAG_LOG_LEVEL_PFX "comment:test_log_level="
+#define TEST_TAG_PROG_FLAGS_PFX "comment:test_prog_flags="
struct test_spec {
const char *name;
bool expect_failure;
- const char *expect_msg;
+ const char **expect_msgs;
+ size_t expect_msg_cnt;
int log_level;
+ int prog_flags;
};
static int tester_init(struct test_loader *tester)
@@ -67,7 +70,8 @@ static int parse_test_spec(struct test_loader *tester,
for (i = 1; i < btf__type_cnt(btf); i++) {
const struct btf_type *t;
- const char *s;
+ const char *s, *val;
+ char *e;
t = btf__type_by_id(btf, i);
if (!btf_is_decl_tag(t))
@@ -82,14 +86,48 @@ static int parse_test_spec(struct test_loader *tester,
} else if (strcmp(s, TEST_TAG_EXPECT_SUCCESS) == 0) {
spec->expect_failure = false;
} else if (str_has_pfx(s, TEST_TAG_EXPECT_MSG_PFX)) {
- spec->expect_msg = s + sizeof(TEST_TAG_EXPECT_MSG_PFX) - 1;
+ void *tmp;
+ const char **msg;
+
+ tmp = realloc(spec->expect_msgs,
+ (1 + spec->expect_msg_cnt) * sizeof(void *));
+ if (!tmp) {
+ ASSERT_FAIL("failed to realloc memory for messages\n");
+ return -ENOMEM;
+ }
+ spec->expect_msgs = tmp;
+ msg = &spec->expect_msgs[spec->expect_msg_cnt++];
+ *msg = s + sizeof(TEST_TAG_EXPECT_MSG_PFX) - 1;
} else if (str_has_pfx(s, TEST_TAG_LOG_LEVEL_PFX)) {
+ val = s + sizeof(TEST_TAG_LOG_LEVEL_PFX) - 1;
errno = 0;
- spec->log_level = strtol(s + sizeof(TEST_TAG_LOG_LEVEL_PFX) - 1, NULL, 0);
- if (errno) {
+ spec->log_level = strtol(val, &e, 0);
+ if (errno || e[0] != '\0') {
ASSERT_FAIL("failed to parse test log level from '%s'", s);
return -EINVAL;
}
+ } else if (str_has_pfx(s, TEST_TAG_PROG_FLAGS_PFX)) {
+ val = s + sizeof(TEST_TAG_PROG_FLAGS_PFX) - 1;
+ if (strcmp(val, "BPF_F_STRICT_ALIGNMENT") == 0) {
+ spec->prog_flags |= BPF_F_STRICT_ALIGNMENT;
+ } else if (strcmp(val, "BPF_F_ANY_ALIGNMENT") == 0) {
+ spec->prog_flags |= BPF_F_ANY_ALIGNMENT;
+ } else if (strcmp(val, "BPF_F_TEST_RND_HI32") == 0) {
+ spec->prog_flags |= BPF_F_TEST_RND_HI32;
+ } else if (strcmp(val, "BPF_F_TEST_STATE_FREQ") == 0) {
+ spec->prog_flags |= BPF_F_TEST_STATE_FREQ;
+ } else if (strcmp(val, "BPF_F_SLEEPABLE") == 0) {
+ spec->prog_flags |= BPF_F_SLEEPABLE;
+ } else if (strcmp(val, "BPF_F_XDP_HAS_FRAGS") == 0) {
+ spec->prog_flags |= BPF_F_XDP_HAS_FRAGS;
+ } else /* assume numeric value */ {
+ errno = 0;
+ spec->prog_flags |= strtol(val, &e, 0);
+ if (errno || e[0] != '\0') {
+ ASSERT_FAIL("failed to parse test prog flags from '%s'", s);
+ return -EINVAL;
+ }
+ }
}
}
@@ -101,7 +139,7 @@ static void prepare_case(struct test_loader *tester,
struct bpf_object *obj,
struct bpf_program *prog)
{
- int min_log_level = 0;
+ int min_log_level = 0, prog_flags;
if (env.verbosity > VERBOSE_NONE)
min_log_level = 1;
@@ -119,7 +157,11 @@ static void prepare_case(struct test_loader *tester,
else
bpf_program__set_log_level(prog, spec->log_level);
+ prog_flags = bpf_program__flags(prog);
+ bpf_program__set_flags(prog, prog_flags | spec->prog_flags);
+
tester->log_buf[0] = '\0';
+ tester->next_match_pos = 0;
}
static void emit_verifier_log(const char *log_buf, bool force)
@@ -135,17 +177,26 @@ static void validate_case(struct test_loader *tester,
struct bpf_program *prog,
int load_err)
{
- if (spec->expect_msg) {
+ int i, j;
+
+ for (i = 0; i < spec->expect_msg_cnt; i++) {
char *match;
+ const char *expect_msg;
+
+ expect_msg = spec->expect_msgs[i];
- match = strstr(tester->log_buf, spec->expect_msg);
+ match = strstr(tester->log_buf + tester->next_match_pos, expect_msg);
if (!ASSERT_OK_PTR(match, "expect_msg")) {
/* if we are in verbose mode, we've already emitted log */
if (env.verbosity == VERBOSE_NONE)
emit_verifier_log(tester->log_buf, true /*force*/);
- fprintf(stderr, "EXPECTED MSG: '%s'\n", spec->expect_msg);
+ for (j = 0; j < i; j++)
+ fprintf(stderr, "MATCHED MSG: '%s'\n", spec->expect_msgs[j]);
+ fprintf(stderr, "EXPECTED MSG: '%s'\n", expect_msg);
return;
}
+
+ tester->next_match_pos = match - tester->log_buf + strlen(expect_msg);
}
}
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 3f058dfadbaf..9af80704f20a 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -410,6 +410,7 @@ int write_sysctl(const char *sysctl, const char *value);
struct test_loader {
char *log_buf;
size_t log_buf_sz;
+ size_t next_match_pos;
struct bpf_object *obj;
};
--
2.39.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [RFC bpf-next 2/5] selftests/bpf: unprivileged tests for test_loader.c
2023-01-23 14:51 [RFC bpf-next 0/5] test_verifier tests migration to inline assembly Eduard Zingerman
2023-01-23 14:51 ` [RFC bpf-next 1/5] selftests/bpf: support custom per-test flags and multiple expected messages Eduard Zingerman
@ 2023-01-23 14:51 ` Eduard Zingerman
2023-01-23 14:51 ` [RFC bpf-next 3/5] selftests/bpf: generate boilerplate code for test_loader-based tests Eduard Zingerman
` (3 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Eduard Zingerman @ 2023-01-23 14:51 UTC (permalink / raw)
To: bpf, ast; +Cc: andrii, daniel, kernel-team, yhs, Eduard Zingerman
Extends test_loader.c:test_loader__run_subtests() by allowing to
execute tests in unprivileged mode, similar to test_verifier.c.
Adds the following new attributes controlling test_loader behavior:
__msg_unpriv
__success_unpriv
__failure_unpriv
* If any of these attributes is present the test would be loaded in
unprivileged mode.
* If only "privileged" attributes are present the test would be loaded
only in privileged mode.
* If both "privileged" and "unprivileged" attributes are present the
test would be loaded in both modes.
* If test has to be executed in both modes, __msg(text) is specified
and __msg_unpriv is not specified the behavior is the same as if
__msg_unpriv(text) is specified.
* For test filtering purposes the name of the program loaded in
unprivileged mode is derived from the usual program name by adding
`@unpriv' suffix.
Also adds attribute '__description'. This attribute specifies text to
be used instead of a program name for display and filtering purposes.
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
tools/testing/selftests/bpf/Makefile | 7 +-
tools/testing/selftests/bpf/autoconf_helper.h | 9 +
tools/testing/selftests/bpf/progs/bpf_misc.h | 48 +++
tools/testing/selftests/bpf/test_loader.c | 337 ++++++++++++++----
tools/testing/selftests/bpf/test_verifier.c | 25 +-
tools/testing/selftests/bpf/unpriv_helpers.c | 26 ++
tools/testing/selftests/bpf/unpriv_helpers.h | 7 +
7 files changed, 359 insertions(+), 100 deletions(-)
create mode 100644 tools/testing/selftests/bpf/autoconf_helper.h
create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.c
create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.h
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 22533a18705e..26e66f9a0977 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -218,8 +218,9 @@ TEST_GEN_PROGS_EXTENDED += $(DEFAULT_BPFTOOL)
$(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED): $(BPFOBJ)
-CGROUP_HELPERS := $(OUTPUT)/cgroup_helpers.o
TESTING_HELPERS := $(OUTPUT)/testing_helpers.o
+CGROUP_HELPERS := $(OUTPUT)/cgroup_helpers.o
+UNPRIV_HELPERS := $(OUTPUT)/unpriv_helpers.o
TRACE_HELPERS := $(OUTPUT)/trace_helpers.o
CAP_HELPERS := $(OUTPUT)/cap_helpers.o
@@ -238,7 +239,7 @@ $(OUTPUT)/test_lirc_mode2_user: $(TESTING_HELPERS)
$(OUTPUT)/xdping: $(TESTING_HELPERS)
$(OUTPUT)/flow_dissector_load: $(TESTING_HELPERS)
$(OUTPUT)/test_maps: $(TESTING_HELPERS)
-$(OUTPUT)/test_verifier: $(TESTING_HELPERS) $(CAP_HELPERS)
+$(OUTPUT)/test_verifier: $(TESTING_HELPERS) $(CAP_HELPERS) $(UNPRIV_HELPERS)
$(OUTPUT)/xsk.o: $(BPFOBJ)
BPFTOOL ?= $(DEFAULT_BPFTOOL)
@@ -527,7 +528,7 @@ TRUNNER_BPF_PROGS_DIR := progs
TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c \
network_helpers.c testing_helpers.c \
btf_helpers.c flow_dissector_load.h \
- cap_helpers.c test_loader.c
+ cap_helpers.c test_loader.c unpriv_helpers.c
TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \
$(OUTPUT)/liburandom_read.so \
$(OUTPUT)/xdp_synproxy \
diff --git a/tools/testing/selftests/bpf/autoconf_helper.h b/tools/testing/selftests/bpf/autoconf_helper.h
new file mode 100644
index 000000000000..5b243b9cdf8c
--- /dev/null
+++ b/tools/testing/selftests/bpf/autoconf_helper.h
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#ifdef HAVE_GENHDR
+# include "autoconf.h"
+#else
+# if defined(__i386) || defined(__x86_64) || defined(__s390x__) || defined(__aarch64__)
+# define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS 1
+# endif
+#endif
diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index 2d7b89b447b2..e742a935de98 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -2,10 +2,58 @@
#ifndef __BPF_MISC_H__
#define __BPF_MISC_H__
+/* This set of attributes controls behavior of the
+ * test_loader.c:test_loader__run_subtests().
+ *
+ * The test_loader sequentially loads each program in a skeleton.
+ * Programs could be loaded in privileged and unprivileged modes.
+ * - __success, __failure, __msg imply privileged mode;
+ * - __success_unpriv, __failure_unpriv, __msg_unpriv imply
+ * unprivileged mode.
+ * If combination of privileged and unprivileged attributes is present
+ * both modes are used. If none are present privileged mode is implied.
+ *
+ * See test_loader.c:set_admin() for exact set of capabilities that
+ * differ between privileged and unprivileged modes.
+ *
+ * For test filtering purposes the name of the program loaded in
+ * unprivileged mode is derived from the usual program name by adding
+ * `@unpriv' suffix.
+ *
+ * __msg Message expected to be found in the verifier log.
+ * Multiple __msg attributes could be specified.
+ * __msg_unpriv Same as __msg but for unprivileged mode.
+ *
+ * __success Expect program load success in privileged mode.
+ * __success_unpriv Expect program load success in unprivileged mode.
+ *
+ * __failure Expect program load failure in privileged mode.
+ * __failure_unpriv Expect program load failure in unprivileged mode.
+ *
+ * __description Text to be used instead of a program name for display
+ * and filtering purposes.
+ *
+ * __log_level Log level to use for the program, numeric value expected.
+ *
+ * __flag Adds one flag use for the program, the following values are valid:
+ * - BPF_F_STRICT_ALIGNMENT;
+ * - BPF_F_TEST_RND_HI32;
+ * - BPF_F_TEST_STATE_FREQ;
+ * - BPF_F_SLEEPABLE;
+ * - BPF_F_XDP_HAS_FRAGS;
+ * - A numeric value.
+ * Multiple __flag attributes could be specified, the final flags
+ * value is derived by applying binary "or" to all specified values.
+ */
#define __msg(msg) __attribute__((btf_decl_tag("comment:test_expect_msg=" msg)))
#define __failure __attribute__((btf_decl_tag("comment:test_expect_failure")))
#define __success __attribute__((btf_decl_tag("comment:test_expect_success")))
+#define __description(desc) __attribute__((btf_decl_tag("comment:test_description=" desc)))
+#define __msg_unpriv(msg) __attribute__((btf_decl_tag("comment:test_expect_msg_unpriv=" msg)))
+#define __failure_unpriv __attribute__((btf_decl_tag("comment:test_expect_failure_unpriv")))
+#define __success_unpriv __attribute__((btf_decl_tag("comment:test_expect_success_unpriv")))
#define __log_level(lvl) __attribute__((btf_decl_tag("comment:test_log_level="#lvl)))
+#define __flag(flag) __attribute__((btf_decl_tag("comment:test_prog_flags="#flag)))
/* Convenience macro for use with 'asm volatile' blocks */
#define __naked __attribute__((naked))
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index bf41390157bf..f035f08c413c 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -1,9 +1,14 @@
// SPDX-License-Identifier: GPL-2.0-only
/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
+#include <linux/capability.h>
#include <stdlib.h>
#include <test_progs.h>
#include <bpf/btf.h>
+#include "autoconf_helper.h"
+#include "unpriv_helpers.h"
+#include "cap_helpers.h"
+
#define str_has_pfx(str, pfx) \
(strncmp(str, pfx, __builtin_constant_p(pfx) ? sizeof(pfx) - 1 : strlen(pfx)) == 0)
@@ -12,16 +17,38 @@
#define TEST_TAG_EXPECT_FAILURE "comment:test_expect_failure"
#define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
#define TEST_TAG_EXPECT_MSG_PFX "comment:test_expect_msg="
+#define TEST_TAG_EXPECT_FAILURE_UNPRIV "comment:test_expect_failure_unpriv"
+#define TEST_TAG_EXPECT_SUCCESS_UNPRIV "comment:test_expect_success_unpriv"
+#define TEST_TAG_EXPECT_MSG_PFX_UNPRIV "comment:test_expect_msg_unpriv="
#define TEST_TAG_LOG_LEVEL_PFX "comment:test_log_level="
#define TEST_TAG_PROG_FLAGS_PFX "comment:test_prog_flags="
+#define TEST_TAG_DESCRIPTION_PFX "comment:test_description="
-struct test_spec {
- const char *name;
+#define ADMIN_CAPS (1ULL << CAP_SYS_ADMIN | \
+ 1ULL << CAP_PERFMON | \
+ 1ULL << CAP_BPF)
+
+static int sysctl_unpriv_disabled = -1;
+
+enum mode {
+ PRIV = 1,
+ UNPRIV = 2
+};
+
+struct test_subspec {
+ char *name;
bool expect_failure;
const char **expect_msgs;
size_t expect_msg_cnt;
+};
+
+struct test_spec {
+ const char *prog_name;
+ struct test_subspec priv;
+ struct test_subspec unpriv;
int log_level;
int prog_flags;
+ int mode_mask;
};
static int tester_init(struct test_loader *tester)
@@ -44,17 +71,46 @@ void test_loader_fini(struct test_loader *tester)
free(tester->log_buf);
}
+static void free_test_spec(struct test_spec *spec)
+{
+ free(spec->priv.name);
+ free(spec->unpriv.name);
+ free(spec->priv.expect_msgs);
+ free(spec->unpriv.expect_msgs);
+}
+
+static int push_msg(const char *msg, struct test_subspec *subspec)
+{
+ void *tmp;
+
+ tmp = realloc(subspec->expect_msgs, (1 + subspec->expect_msg_cnt) * sizeof(void *));
+ if (!tmp) {
+ ASSERT_FAIL("failed to realloc memory for messages\n");
+ return -ENOMEM;
+ }
+ subspec->expect_msgs = tmp;
+ subspec->expect_msgs[subspec->expect_msg_cnt++] = msg;
+
+ return 0;
+}
+
+/* Uses btf_decl_tag attributes to describe the expected test
+ * behavior, see bpf_misc.h for detailed description of each attribute
+ * and attribute combinations.
+ */
static int parse_test_spec(struct test_loader *tester,
struct bpf_object *obj,
struct bpf_program *prog,
struct test_spec *spec)
{
+ const char *description = NULL;
+ bool has_unpriv_result = false;
+ int func_id, i, err = 0;
struct btf *btf;
- int func_id, i;
memset(spec, 0, sizeof(*spec));
- spec->name = bpf_program__name(prog);
+ spec->prog_name = bpf_program__name(prog);
btf = bpf_object__btf(obj);
if (!btf) {
@@ -62,15 +118,15 @@ static int parse_test_spec(struct test_loader *tester,
return -EINVAL;
}
- func_id = btf__find_by_name_kind(btf, spec->name, BTF_KIND_FUNC);
+ func_id = btf__find_by_name_kind(btf, spec->prog_name, BTF_KIND_FUNC);
if (func_id < 0) {
- ASSERT_FAIL("failed to find FUNC BTF type for '%s'", spec->name);
+ ASSERT_FAIL("failed to find FUNC BTF type for '%s'", spec->prog_name);
return -EINVAL;
}
for (i = 1; i < btf__type_cnt(btf); i++) {
+ const char *s, *val, *msg;
const struct btf_type *t;
- const char *s, *val;
char *e;
t = btf__type_by_id(btf, i);
@@ -81,30 +137,42 @@ static int parse_test_spec(struct test_loader *tester,
continue;
s = btf__str_by_offset(btf, t->name_off);
- if (strcmp(s, TEST_TAG_EXPECT_FAILURE) == 0) {
- spec->expect_failure = true;
+ if (str_has_pfx(s, TEST_TAG_DESCRIPTION_PFX)) {
+ description = s + sizeof(TEST_TAG_DESCRIPTION_PFX) - 1;
+ } else if (strcmp(s, TEST_TAG_EXPECT_FAILURE) == 0) {
+ spec->priv.expect_failure = true;
+ spec->mode_mask |= PRIV;
} else if (strcmp(s, TEST_TAG_EXPECT_SUCCESS) == 0) {
- spec->expect_failure = false;
+ spec->priv.expect_failure = false;
+ spec->mode_mask |= PRIV;
+ } else if (strcmp(s, TEST_TAG_EXPECT_FAILURE_UNPRIV) == 0) {
+ spec->unpriv.expect_failure = true;
+ spec->mode_mask |= UNPRIV;
+ has_unpriv_result = true;
+ } else if (strcmp(s, TEST_TAG_EXPECT_SUCCESS_UNPRIV) == 0) {
+ spec->unpriv.expect_failure = false;
+ spec->mode_mask |= UNPRIV;
+ has_unpriv_result = true;
} else if (str_has_pfx(s, TEST_TAG_EXPECT_MSG_PFX)) {
- void *tmp;
- const char **msg;
-
- tmp = realloc(spec->expect_msgs,
- (1 + spec->expect_msg_cnt) * sizeof(void *));
- if (!tmp) {
- ASSERT_FAIL("failed to realloc memory for messages\n");
- return -ENOMEM;
- }
- spec->expect_msgs = tmp;
- msg = &spec->expect_msgs[spec->expect_msg_cnt++];
- *msg = s + sizeof(TEST_TAG_EXPECT_MSG_PFX) - 1;
+ msg = s + sizeof(TEST_TAG_EXPECT_MSG_PFX) - 1;
+ err = push_msg(msg, &spec->priv);
+ if (err)
+ goto cleanup;
+ spec->mode_mask |= PRIV;
+ } else if (str_has_pfx(s, TEST_TAG_EXPECT_MSG_PFX_UNPRIV)) {
+ msg = s + sizeof(TEST_TAG_EXPECT_MSG_PFX_UNPRIV) - 1;
+ err = push_msg(msg, &spec->unpriv);
+ if (err)
+ goto cleanup;
+ spec->mode_mask |= UNPRIV;
} else if (str_has_pfx(s, TEST_TAG_LOG_LEVEL_PFX)) {
val = s + sizeof(TEST_TAG_LOG_LEVEL_PFX) - 1;
errno = 0;
spec->log_level = strtol(val, &e, 0);
if (errno || e[0] != '\0') {
- ASSERT_FAIL("failed to parse test log level from '%s'", s);
- return -EINVAL;
+ PRINT_FAIL("failed to parse test log level from '%s'\n", s);
+ err = -EINVAL;
+ goto cleanup;
}
} else if (str_has_pfx(s, TEST_TAG_PROG_FLAGS_PFX)) {
val = s + sizeof(TEST_TAG_PROG_FLAGS_PFX) - 1;
@@ -124,14 +192,70 @@ static int parse_test_spec(struct test_loader *tester,
errno = 0;
spec->prog_flags |= strtol(val, &e, 0);
if (errno || e[0] != '\0') {
- ASSERT_FAIL("failed to parse test prog flags from '%s'", s);
- return -EINVAL;
+ PRINT_FAIL("failed to parse test prog flags from '%s'\n",
+ val);
+ err = -EINVAL;
+ goto cleanup;
}
}
}
}
+ if (spec->mode_mask == 0)
+ spec->mode_mask = PRIV;
+
+ if (!description)
+ description = spec->prog_name;
+
+ if (spec->mode_mask & PRIV) {
+ spec->priv.name = strdup(description);
+ if (!spec->priv.name) {
+ PRINT_FAIL("failed to allocate memory for priv.name\n");
+ err = -ENOMEM;
+ goto cleanup;
+ }
+ }
+
+ if (spec->mode_mask & UNPRIV) {
+ int descr_len = strlen(description);
+ const char *suffix = " @unpriv";
+ char *name;
+
+ name = malloc(descr_len + strlen(suffix) + 1);
+ if (!name) {
+ PRINT_FAIL("failed to allocate memory for unpriv.name\n");
+ err = -ENOMEM;
+ goto cleanup;
+ }
+
+ strcpy(name, description);
+ strcpy(&name[descr_len], suffix);
+ spec->unpriv.name = name;
+ }
+
+ if (spec->mode_mask & (PRIV | UNPRIV)) {
+ if (!has_unpriv_result)
+ spec->unpriv.expect_failure = spec->priv.expect_failure;
+
+ if (!spec->unpriv.expect_msgs) {
+ size_t sz = spec->priv.expect_msg_cnt * sizeof(void *);
+
+ spec->unpriv.expect_msgs = malloc(sz);
+ if (!spec->unpriv.expect_msgs) {
+ PRINT_FAIL("failed to allocate memory for unpriv.expect_msgs\n");
+ err = -ENOMEM;
+ goto cleanup;
+ }
+ memcpy(spec->unpriv.expect_msgs, spec->priv.expect_msgs, sz);
+ spec->unpriv.expect_msg_cnt = spec->priv.expect_msg_cnt;
+ }
+ }
+
return 0;
+
+cleanup:
+ free_test_spec(spec);
+ return err;
}
static void prepare_case(struct test_loader *tester,
@@ -148,7 +272,7 @@ static void prepare_case(struct test_loader *tester,
bpf_program__set_log_buf(prog, tester->log_buf, tester->log_buf_sz);
- /* Make sure we set at least minimal log level, unless test requirest
+ /* Make sure we set at least minimal log level, unless test requires
* even higher level already. Make sure to preserve independent log
* level 4 (verifier stats), though.
*/
@@ -172,18 +296,18 @@ static void emit_verifier_log(const char *log_buf, bool force)
}
static void validate_case(struct test_loader *tester,
- struct test_spec *spec,
+ struct test_subspec *subspec,
struct bpf_object *obj,
struct bpf_program *prog,
int load_err)
{
int i, j;
- for (i = 0; i < spec->expect_msg_cnt; i++) {
+ for (i = 0; i < subspec->expect_msg_cnt; i++) {
char *match;
const char *expect_msg;
- expect_msg = spec->expect_msgs[i];
+ expect_msg = subspec->expect_msgs[i];
match = strstr(tester->log_buf + tester->next_match_pos, expect_msg);
if (!ASSERT_OK_PTR(match, "expect_msg")) {
@@ -191,7 +315,8 @@ static void validate_case(struct test_loader *tester,
if (env.verbosity == VERBOSE_NONE)
emit_verifier_log(tester->log_buf, true /*force*/);
for (j = 0; j < i; j++)
- fprintf(stderr, "MATCHED MSG: '%s'\n", spec->expect_msgs[j]);
+ fprintf(stderr,
+ "MATCHED MSG: '%s'\n", subspec->expect_msgs[j]);
fprintf(stderr, "EXPECTED MSG: '%s'\n", expect_msg);
return;
}
@@ -200,17 +325,114 @@ static void validate_case(struct test_loader *tester,
}
}
+static int set_admin(bool admin)
+{
+ int err;
+
+ if (admin)
+ err = cap_enable_effective(ADMIN_CAPS, NULL);
+ else
+ err = cap_disable_effective(ADMIN_CAPS, NULL);
+
+ if (err)
+ PRINT_FAIL("%s admin privs failed: %i, %s\n",
+ admin ? "gain" : "drop", err, strerror(err));
+
+ return err;
+}
+
+static bool can_execute_unpriv(struct test_loader *tester, struct test_spec *spec)
+{
+#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+ bool efficient_unaligned_access = true;
+#else
+ bool efficient_unaligned_access = false;
+#endif
+ if (sysctl_unpriv_disabled < 0)
+ sysctl_unpriv_disabled = get_unpriv_disabled() ? 1 : 0;
+ if (sysctl_unpriv_disabled)
+ return false;
+ if ((spec->prog_flags & BPF_F_ANY_ALIGNMENT) && !efficient_unaligned_access)
+ return false;
+ return true;
+}
+
/* this function is forced noinline and has short generic name to look better
* in test_progs output (in case of a failure)
*/
static noinline
void run_subtest(struct test_loader *tester,
- const char *skel_name,
- skel_elf_bytes_fn elf_bytes_factory)
+ struct bpf_object_open_opts *open_opts,
+ const void *obj_bytes,
+ size_t obj_byte_cnt,
+ struct test_spec *spec,
+ bool unpriv)
+{
+ struct test_subspec *subspec = unpriv ? &spec->unpriv : &spec->priv;
+ struct bpf_program *tprog;
+ struct bpf_object *tobj;
+ int err;
+
+ if (!test__start_subtest(subspec->name))
+ return;
+
+ if (unpriv && !can_execute_unpriv(tester, spec)) {
+ test__skip();
+ test__end_subtest();
+ return;
+ }
+
+ if (unpriv)
+ set_admin(false);
+
+ tobj = bpf_object__open_mem(obj_bytes, obj_byte_cnt, open_opts);
+ if (!ASSERT_OK_PTR(tobj, "obj_open_mem")) /* shouldn't happen */
+ goto subtest_cleanup;
+
+ bpf_object__for_each_program(tprog, tobj)
+ bpf_program__set_autoload(tprog, false);
+
+ bpf_object__for_each_program(tprog, tobj) {
+ /* only load specified program */
+ if (strcmp(bpf_program__name(tprog), spec->prog_name) == 0) {
+ bpf_program__set_autoload(tprog, true);
+ break;
+ }
+ }
+
+ prepare_case(tester, spec, tobj, tprog);
+
+ err = bpf_object__load(tobj);
+ if (subspec->expect_failure) {
+ if (!ASSERT_ERR(err, "unexpected_load_success")) {
+ emit_verifier_log(tester->log_buf, false /*force*/);
+ goto tobj_cleanup;
+ }
+ } else {
+ if (!ASSERT_OK(err, "unexpected_load_failure")) {
+ emit_verifier_log(tester->log_buf, true /*force*/);
+ goto tobj_cleanup;
+ }
+ }
+
+ emit_verifier_log(tester->log_buf, false /*force*/);
+ validate_case(tester, subspec, tobj, tprog, err);
+
+tobj_cleanup:
+ bpf_object__close(tobj);
+subtest_cleanup:
+ test__end_subtest();
+ if (unpriv)
+ set_admin(true);
+}
+
+static void process_subtest(struct test_loader *tester,
+ const char *skel_name,
+ skel_elf_bytes_fn elf_bytes_factory)
{
LIBBPF_OPTS(bpf_object_open_opts, open_opts, .object_name = skel_name);
- struct bpf_object *obj = NULL, *tobj;
- struct bpf_program *prog, *tprog;
+ struct bpf_object *obj = NULL;
+ struct bpf_program *prog;
const void *obj_bytes;
size_t obj_byte_cnt;
int err;
@@ -224,52 +446,19 @@ void run_subtest(struct test_loader *tester,
return;
bpf_object__for_each_program(prog, obj) {
- const char *prog_name = bpf_program__name(prog);
struct test_spec spec;
- if (!test__start_subtest(prog_name))
- continue;
-
/* if we can't derive test specification, go to the next test */
err = parse_test_spec(tester, obj, prog, &spec);
if (!ASSERT_OK(err, "parse_test_spec"))
continue;
- tobj = bpf_object__open_mem(obj_bytes, obj_byte_cnt, &open_opts);
- if (!ASSERT_OK_PTR(tobj, "obj_open_mem")) /* shouldn't happen */
- continue;
+ if (spec.mode_mask & PRIV)
+ run_subtest(tester, &open_opts, obj_bytes, obj_byte_cnt, &spec, false);
+ if (spec.mode_mask & UNPRIV)
+ run_subtest(tester, &open_opts, obj_bytes, obj_byte_cnt, &spec, true);
- bpf_object__for_each_program(tprog, tobj)
- bpf_program__set_autoload(tprog, false);
-
- bpf_object__for_each_program(tprog, tobj) {
- /* only load specified program */
- if (strcmp(bpf_program__name(tprog), prog_name) == 0) {
- bpf_program__set_autoload(tprog, true);
- break;
- }
- }
-
- prepare_case(tester, &spec, tobj, tprog);
-
- err = bpf_object__load(tobj);
- if (spec.expect_failure) {
- if (!ASSERT_ERR(err, "unexpected_load_success")) {
- emit_verifier_log(tester->log_buf, false /*force*/);
- goto tobj_cleanup;
- }
- } else {
- if (!ASSERT_OK(err, "unexpected_load_failure")) {
- emit_verifier_log(tester->log_buf, true /*force*/);
- goto tobj_cleanup;
- }
- }
-
- emit_verifier_log(tester->log_buf, false /*force*/);
- validate_case(tester, &spec, tobj, tprog, err);
-
-tobj_cleanup:
- bpf_object__close(tobj);
+ free_test_spec(&spec);
}
bpf_object__close(obj);
@@ -280,5 +469,5 @@ void test_loader__run_subtests(struct test_loader *tester,
skel_elf_bytes_fn elf_bytes_factory)
{
/* see comment in run_subtest() for why we do this function nesting */
- run_subtest(tester, skel_name, elf_bytes_factory);
+ process_subtest(tester, skel_name, elf_bytes_factory);
}
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 8c808551dfd7..6071dff9c8a1 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -33,13 +33,8 @@
#include <bpf/bpf.h>
#include <bpf/libbpf.h>
-#ifdef HAVE_GENHDR
-# include "autoconf.h"
-#else
-# if defined(__i386) || defined(__x86_64) || defined(__s390x__) || defined(__aarch64__)
-# define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS 1
-# endif
-#endif
+#include "autoconf_helper.h"
+#include "unpriv_helpers.h"
#include "cap_helpers.h"
#include "bpf_rand.h"
#include "bpf_util.h"
@@ -1665,22 +1660,6 @@ static bool is_admin(void)
return (caps & ADMIN_CAPS) == ADMIN_CAPS;
}
-static void get_unpriv_disabled()
-{
- char buf[2];
- FILE *fd;
-
- fd = fopen("/proc/sys/"UNPRIV_SYSCTL, "r");
- if (!fd) {
- perror("fopen /proc/sys/"UNPRIV_SYSCTL);
- unpriv_disabled = true;
- return;
- }
- if (fgets(buf, 2, fd) == buf && atoi(buf))
- unpriv_disabled = true;
- fclose(fd);
-}
-
static bool test_as_unpriv(struct bpf_test *test)
{
#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
diff --git a/tools/testing/selftests/bpf/unpriv_helpers.c b/tools/testing/selftests/bpf/unpriv_helpers.c
new file mode 100644
index 000000000000..488371edb3e4
--- /dev/null
+++ b/tools/testing/selftests/bpf/unpriv_helpers.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <stdbool.h>
+#include <stdlib.h>
+#include <error.h>
+#include <stdio.h>
+
+#include "unpriv_helpers.h"
+
+bool get_unpriv_disabled(void)
+{
+ bool disabled;
+ char buf[2];
+ FILE *fd;
+
+ fd = fopen("/proc/sys/"UNPRIV_SYSCTL, "r");
+ if (fd) {
+ disabled = (fgets(buf, 2, fd) == buf && atoi(buf));
+ fclose(fd);
+ } else {
+ perror("fopen /proc/sys/"UNPRIV_SYSCTL);
+ disabled = true;
+ }
+
+ return disabled;
+}
diff --git a/tools/testing/selftests/bpf/unpriv_helpers.h b/tools/testing/selftests/bpf/unpriv_helpers.h
new file mode 100644
index 000000000000..151f67329665
--- /dev/null
+++ b/tools/testing/selftests/bpf/unpriv_helpers.h
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <stdbool.h>
+
+#define UNPRIV_SYSCTL "kernel/unprivileged_bpf_disabled"
+
+bool get_unpriv_disabled(void);
--
2.39.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [RFC bpf-next 3/5] selftests/bpf: generate boilerplate code for test_loader-based tests
2023-01-23 14:51 [RFC bpf-next 0/5] test_verifier tests migration to inline assembly Eduard Zingerman
2023-01-23 14:51 ` [RFC bpf-next 1/5] selftests/bpf: support custom per-test flags and multiple expected messages Eduard Zingerman
2023-01-23 14:51 ` [RFC bpf-next 2/5] selftests/bpf: unprivileged tests for test_loader.c Eduard Zingerman
@ 2023-01-23 14:51 ` Eduard Zingerman
2023-01-26 1:43 ` Andrii Nakryiko
2023-01-23 14:51 ` [RFC bpf-next 4/5] selftests/bpf: __imm_insn macro to embed raw insns in inline asm Eduard Zingerman
` (2 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Eduard Zingerman @ 2023-01-23 14:51 UTC (permalink / raw)
To: bpf, ast; +Cc: andrii, daniel, kernel-team, yhs, Eduard Zingerman
Automatically generate boilerplate code necessary to run tests that
use test_loader.c.
Adds a target 'prog_tests/test_loader_auto_wrappers.c' as part of
rulesets for 'test_progs' and 'test_progs-no_alu32'. The content of
this C file is generated by make and has the following structure:
#include <test_progs.h>
#include "some_test_1.skel.h"
#include "some_test_2.skel.h"
...
void test_some_test_1(void) { RUN_TESTS(some_test_1); }
void test_some_test_2(void) { RUN_TESTS(some_test_2); }
...
Here RUN_TESTS is a macro defined in test_progs.h, it expands to a
code that uses test_loader.c:test_loader__run_subtests() function to
load tests specified by appropriate skel.h.
In order to get the list of tests included in
'test_loader_auto_wrappers.c' the generation script looks for
'progs/*.c' files that contain a special comment:
/* Use test_loader marker */
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
tools/testing/selftests/bpf/Makefile | 34 +++++++++++++++++++
.../selftests/bpf/prog_tests/.gitignore | 1 +
2 files changed, 35 insertions(+)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 26e66f9a0977..66ba2941677c 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -398,6 +398,11 @@ TRUNNER_OUTPUT := $(OUTPUT)$(if $2,/)$2
TRUNNER_BINARY := $1$(if $2,-)$2
TRUNNER_TEST_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.test.o, \
$$(notdir $$(wildcard $(TRUNNER_TESTS_DIR)/*.c)))
+ifneq ($(TRUNNER_TEST_LOADER_AUTO_WRAPPERS),)
+TRUNNER_TEST_OBJS := $$(filter-out $$(TRUNNER_OUTPUT)/test_loader_auto_wrappers.test.o, \
+ $$(TRUNNER_TEST_OBJS))
+TRUNNER_TEST_OBJS += $$(TRUNNER_OUTPUT)/test_loader_auto_wrappers.test.o
+endif
TRUNNER_EXTRA_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o, \
$$(filter %.c,$(TRUNNER_EXTRA_SOURCES)))
TRUNNER_EXTRA_HDRS := $$(filter %.h,$(TRUNNER_EXTRA_SOURCES))
@@ -482,6 +487,32 @@ $(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c
) > $$@)
endif
+ifneq ($(TRUNNER_TEST_LOADER_AUTO_WRAPPERS),)
+ifeq ($($(TRUNNER_TESTS_DIR)-test-loader-auto-wrappers-c),)
+$(TRUNNER_TESTS_DIR)-test-loader-auto-wrappers-c := y
+$(TRUNNER_TESTS_DIR)/test_loader_auto_wrappers.c: $(TRUNNER_BPF_PROGS_DIR)/*.c
+ $$(call msg,GEN-TEST,$(TRUNNER_BINARY),$$@)
+ $$(shell (echo '/* Generated source, do not edit */'; \
+ tests=$$$$(grep --null -lF '/* Use test_loader marker */' \
+ $$(TRUNNER_BPF_PROGS_DIR)/*.c \
+ | xargs -0 -I {} basename {} .c); \
+ echo "#include <test_progs.h>"; \
+ echo ""; \
+ for case in $$$$tests; \
+ do \
+ echo "#include \"$$$$case.skel.h\""; \
+ done; \
+ echo ""; \
+ for case in $$$$tests; \
+ do \
+ printf "void %-50s { %-50s }\n" \
+ "test_$$$$case(void)" \
+ "RUN_TESTS($$$$case);"; \
+ done) > $$@)
+$(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/test_loader_auto_wrappers.c
+endif
+endif # TRUNNER_TEST_LOADER_AUTO_WRAPPERS
+
# compile individual test files
# Note: we cd into output directory to ensure embedded BPF object is found
$(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o: \
@@ -537,6 +568,7 @@ TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \
verify_sig_setup.sh \
$(wildcard progs/btf_dump_test_case_*.c) \
$(wildcard progs/*.bpf.o)
+TRUNNER_TEST_LOADER_AUTO_WRAPPERS := t
TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
$(eval $(call DEFINE_TEST_RUNNER,test_progs))
@@ -560,6 +592,7 @@ TRUNNER_EXTRA_SOURCES := test_maps.c
TRUNNER_EXTRA_FILES :=
TRUNNER_BPF_BUILD_RULE := $$(error no BPF objects should be built)
TRUNNER_BPF_CFLAGS :=
+TRUNNER_TEST_LOADER_AUTO_WRAPPERS :=
$(eval $(call DEFINE_TEST_RUNNER,test_maps))
# Define test_verifier test runner.
@@ -625,6 +658,7 @@ $(OUTPUT)/veristat: $(OUTPUT)/veristat.o
EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR) \
prog_tests/tests.h map_tests/tests.h verifier/tests.h \
+ prog_tests/test_loader_auto_wrappers.c \
feature bpftool \
$(addprefix $(OUTPUT)/,*.o *.skel.h *.lskel.h *.subskel.h \
no_alu32 bpf_gcc bpf_testmod.ko \
diff --git a/tools/testing/selftests/bpf/prog_tests/.gitignore b/tools/testing/selftests/bpf/prog_tests/.gitignore
index 89c4a3d37544..6a937c1cd78d 100644
--- a/tools/testing/selftests/bpf/prog_tests/.gitignore
+++ b/tools/testing/selftests/bpf/prog_tests/.gitignore
@@ -1,2 +1,3 @@
# SPDX-License-Identifier: GPL-2.0-only
tests.h
+test_loader_auto_wrappers.c
--
2.39.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [RFC bpf-next 4/5] selftests/bpf: __imm_insn macro to embed raw insns in inline asm
2023-01-23 14:51 [RFC bpf-next 0/5] test_verifier tests migration to inline assembly Eduard Zingerman
` (2 preceding siblings ...)
2023-01-23 14:51 ` [RFC bpf-next 3/5] selftests/bpf: generate boilerplate code for test_loader-based tests Eduard Zingerman
@ 2023-01-23 14:51 ` Eduard Zingerman
2023-01-26 2:48 ` Andrii Nakryiko
2023-01-23 14:51 ` [RFC bpf-next 5/5] selftests/bpf: convert jeq_infer_not_null tests to inline assembly Eduard Zingerman
2023-01-26 1:33 ` [RFC bpf-next 0/5] test_verifier tests migration " Andrii Nakryiko
5 siblings, 1 reply; 18+ messages in thread
From: Eduard Zingerman @ 2023-01-23 14:51 UTC (permalink / raw)
To: bpf, ast; +Cc: andrii, daniel, kernel-team, yhs, Eduard Zingerman
A convenience macro to allow the following usage:
#include <linux/filter.h>
...
asm volatile (
...
".8byte %[raw_insn];"
...
:
: __imm_insn(raw_insn, BPF_RAW_INSN(...))
: __clobber_all);
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
tools/testing/selftests/bpf/progs/bpf_misc.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index e742a935de98..832bec4818d9 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -61,6 +61,7 @@
#define __clobber_common "r0", "r1", "r2", "r3", "r4", "r5", "memory"
#define __imm(name) [name]"i"(name)
#define __imm_addr(name) [name]"i"(&name)
+#define __imm_insn(name, expr) [name]"i"(*(long *)&(expr))
#if defined(__TARGET_ARCH_x86)
#define SYSCALL_WRAPPER 1
--
2.39.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [RFC bpf-next 5/5] selftests/bpf: convert jeq_infer_not_null tests to inline assembly
2023-01-23 14:51 [RFC bpf-next 0/5] test_verifier tests migration to inline assembly Eduard Zingerman
` (3 preceding siblings ...)
2023-01-23 14:51 ` [RFC bpf-next 4/5] selftests/bpf: __imm_insn macro to embed raw insns in inline asm Eduard Zingerman
@ 2023-01-23 14:51 ` Eduard Zingerman
2023-01-26 1:33 ` [RFC bpf-next 0/5] test_verifier tests migration " Andrii Nakryiko
5 siblings, 0 replies; 18+ messages in thread
From: Eduard Zingerman @ 2023-01-23 14:51 UTC (permalink / raw)
To: bpf, ast; +Cc: andrii, daniel, kernel-team, yhs, Eduard Zingerman
Use updated test_loader interface to convert progs/jeq_infer_not_null
verifier test to inline assembly. Some redundant comments are removed
in the process.
Existing test progs/jeq_infer_not_null_fail.c is updated to use
"Use test_loader marker" to remove trivial
progs_tests/jeq_infer_not_null.c boilerplate code.
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
.../bpf/prog_tests/jeq_infer_not_null.c | 9 -
.../selftests/bpf/progs/jeq_infer_not_null.c | 186 ++++++++++++++++++
.../bpf/progs/jeq_infer_not_null_fail.c | 1 +
.../bpf/verifier/jeq_infer_not_null.c | 174 ----------------
4 files changed, 187 insertions(+), 183 deletions(-)
delete mode 100644 tools/testing/selftests/bpf/prog_tests/jeq_infer_not_null.c
create mode 100644 tools/testing/selftests/bpf/progs/jeq_infer_not_null.c
delete mode 100644 tools/testing/selftests/bpf/verifier/jeq_infer_not_null.c
diff --git a/tools/testing/selftests/bpf/prog_tests/jeq_infer_not_null.c b/tools/testing/selftests/bpf/prog_tests/jeq_infer_not_null.c
deleted file mode 100644
index 3add34df5767..000000000000
--- a/tools/testing/selftests/bpf/prog_tests/jeq_infer_not_null.c
+++ /dev/null
@@ -1,9 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-#include <test_progs.h>
-#include "jeq_infer_not_null_fail.skel.h"
-
-void test_jeq_infer_not_null(void)
-{
- RUN_TESTS(jeq_infer_not_null_fail);
-}
diff --git a/tools/testing/selftests/bpf/progs/jeq_infer_not_null.c b/tools/testing/selftests/bpf/progs/jeq_infer_not_null.c
new file mode 100644
index 000000000000..7c506eccacaf
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/jeq_infer_not_null.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Converted from tools/testing/selftests/bpf/verifier/jeq_infer_not_null.c */
+/* Use test_loader marker */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
+
+struct {
+ __uint(type, BPF_MAP_TYPE_XSKMAP);
+ __uint(max_entries, 1);
+ __type(key, int);
+ __type(value, int);
+} map_xskmap SEC(".maps");
+
+/* This is equivalent to the following program:
+ *
+ * r6 = skb->sk;
+ * r7 = sk_fullsock(r6);
+ * r0 = sk_fullsock(r6);
+ * if (r0 == 0) return 0; (a)
+ * if (r0 != r7) return 0; (b)
+ * *r7->type; (c)
+ * return 0;
+ *
+ * It is safe to dereference r7 at point (c), because of (a) and (b).
+ * The test verifies that relation r0 == r7 is propagated from (b) to (c).
+ */
+__description("jne/jeq infer not null, PTR_TO_SOCKET_OR_NULL -> PTR_TO_SOCKET for JNE false branch")
+__success __failure_unpriv __msg_unpriv("R7 pointer comparison")
+SEC("cgroup/skb")
+__naked void sock_or_null_jne_false_branch(void)
+{
+ asm volatile (
+" r6 = *(u64*)(r1 + %[__sk_buff_sk_offset]); \n\
+ if r6 == 0 goto exit_%=; \n\
+ r1 = r6; \n\
+ call %[bpf_sk_fullsock]; \n\
+ r7 = r0; \n\
+ r1 = r6; \n\
+ call %[bpf_sk_fullsock]; \n\
+ if r0 == 0 goto exit_%=; \n\
+ if r0 != r7 goto exit_%=; \n\
+ r0 = *(u32*)(r7 + %[bpf_sock_type_offset]); \n\
+exit_%=: \n\
+ r0 = 0; \n\
+ exit; \n\
+" :
+ : [__sk_buff_sk_offset]"i"(offsetof(struct __sk_buff, sk)),
+ [bpf_sock_type_offset]"i"(offsetof(struct bpf_sock, type)),
+ __imm(bpf_sk_fullsock)
+ : __clobber_all);
+}
+
+/* Same as above, but verify that another branch of JNE still
+ * prohibits access to PTR_MAYBE_NULL.
+ */
+__description("jne/jeq infer not null, PTR_TO_SOCKET_OR_NULL unchanged for JNE true branch")
+__failure __msg("R7 invalid mem access 'sock_or_null'")
+__failure_unpriv __msg_unpriv("R7 pointer comparison")
+SEC("cgroup/skb")
+__naked void sock_or_null_jne_true_branch(void)
+{
+ asm volatile (
+" r6 = *(u64*)(r1 + %[__sk_buff_sk_offset]); \n\
+ if r6 == 0 goto exit_%=; \n\
+ r1 = r6; \n\
+ call %[bpf_sk_fullsock]; \n\
+ r7 = r0; \n\
+ r1 = r6; \n\
+ call %[bpf_sk_fullsock]; \n\
+ if r0 != 0 goto exit_%=; \n\
+ if r0 != r7 goto l1_%=; \n\
+ goto exit_%=; \n\
+l1_%=: \n\
+ r0 = *(u32*)(r7 + %[bpf_sock_type_offset]); \n\
+exit_%=: \n\
+ r0 = 0; \n\
+ exit; \n\
+" :
+ : [__sk_buff_sk_offset]"i"(offsetof(struct __sk_buff, sk)),
+ [bpf_sock_type_offset]"i"(offsetof(struct bpf_sock, type)),
+ __imm(bpf_sk_fullsock)
+ : __clobber_all);
+}
+
+/* Same as a first test, but not null should be inferred for JEQ branch */
+__description("jne/jeq infer not null, PTR_TO_SOCKET_OR_NULL -> PTR_TO_SOCKET for JEQ true branch")
+__success __failure_unpriv __msg_unpriv("R7 pointer comparison")
+SEC("cgroup/skb")
+__naked void sock_or_null_jeq_true_branch(void)
+{
+ asm volatile (
+" r6 = *(u64*)(r1 + %[__sk_buff_sk_offset]); \n\
+ if r6 == 0 goto exit_%=; \n\
+ r1 = r6; \n\
+ call %[bpf_sk_fullsock]; \n\
+ r7 = r0; \n\
+ r1 = r6; \n\
+ call %[bpf_sk_fullsock]; \n\
+ if r0 == 0 goto exit_%=; \n\
+ if r0 == r7 goto l1_%=; \n\
+ goto exit_%=; \n\
+l1_%=: \n\
+ r0 = *(u32*)(r7 + %[bpf_sock_type_offset]); \n\
+exit_%=: \n\
+ r0 = 0; \n\
+ exit; \n\
+" :
+ : [__sk_buff_sk_offset]"i"(offsetof(struct __sk_buff, sk)),
+ [bpf_sock_type_offset]"i"(offsetof(struct bpf_sock, type)),
+ __imm(bpf_sk_fullsock)
+ : __clobber_all);
+}
+
+/* Same as above, but verify that another branch of JNE still
+ * prohibits access to PTR_MAYBE_NULL.
+ */
+__description("jne/jeq infer not null, PTR_TO_SOCKET_OR_NULL unchanged for JEQ false branch")
+__failure __msg("R7 invalid mem access 'sock_or_null'")
+__failure_unpriv __msg_unpriv("R7 pointer comparison")
+SEC("cgroup/skb")
+__naked void sock_or_null_jeq_false_branch(void)
+{
+ asm volatile (
+" r6 = *(u64*)(r1 + %[__sk_buff_sk_offset]); \n\
+ if r6 == 0 goto exit_%=; \n\
+ r1 = r6; \n\
+ call %[bpf_sk_fullsock]; \n\
+ r7 = r0; \n\
+ r1 = r6; \n\
+ call %[bpf_sk_fullsock]; \n\
+ if r0 == 0 goto exit_%=; \n\
+ if r0 == r7 goto exit_%=; \n\
+ r0 = *(u32*)(r7 + %[bpf_sock_type_offset]); \n\
+exit_%=: \n\
+ r0 = 0; \n\
+ exit; \n\
+" :
+ : [__sk_buff_sk_offset]"i"(offsetof(struct __sk_buff, sk)),
+ [bpf_sock_type_offset]"i"(offsetof(struct bpf_sock, type)),
+ __imm(bpf_sk_fullsock)
+ : __clobber_all);
+}
+
+/* Maps are treated in a different branch of `mark_ptr_not_null_reg`,
+ * so separate test for maps case.
+ */
+__description("jne/jeq infer not null, PTR_TO_MAP_VALUE_OR_NULL -> PTR_TO_MAP_VALUE")
+__success
+SEC("xdp")
+__naked void ptr_to_map(void)
+{
+ asm volatile (
+" r1 = 0; \n\
+ *(u32*)(r10 - 8) = r1; \n\
+ r9 = r10; \n\
+ r9 += -8; \n\
+ /* r8 = process local map */ \n\
+ r8 = %[map_xskmap] ll; \n\
+ /* r6 = map_lookup_elem(r8, r9); */ \n\
+ r1 = r8; \n\
+ r2 = r9; \n\
+ call %[bpf_map_lookup_elem]; \n\
+ r6 = r0; \n\
+ /* r7 = map_lookup_elem(r8, r9); */ \n\
+ r1 = r8; \n\
+ r2 = r9; \n\
+ call %[bpf_map_lookup_elem]; \n\
+ r7 = r0; \n\
+ if r6 == 0 goto exit_%=; \n\
+ if r6 != r7 goto exit_%=; \n\
+ /* read *r7; */ \n\
+ r0 = *(u32*)(r7 + %[bpf_xdp_sock_queue_id_offset]);\n\
+exit_%=: \n\
+ r0 = 0; \n\
+ exit; \n\
+" :
+ : [bpf_xdp_sock_queue_id_offset]"i"(offsetof(struct bpf_xdp_sock, queue_id)),
+ __imm(bpf_map_lookup_elem),
+ __imm_addr(map_xskmap)
+ : __clobber_all);
+}
+
+char _license[] SEC("license") = "GPL";
+
diff --git a/tools/testing/selftests/bpf/progs/jeq_infer_not_null_fail.c b/tools/testing/selftests/bpf/progs/jeq_infer_not_null_fail.c
index f46965053acb..8048d76f60ca 100644
--- a/tools/testing/selftests/bpf/progs/jeq_infer_not_null_fail.c
+++ b/tools/testing/selftests/bpf/progs/jeq_infer_not_null_fail.c
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: GPL-2.0
+/* Use test_loader marker */
#include "vmlinux.h"
#include <bpf/bpf_helpers.h>
diff --git a/tools/testing/selftests/bpf/verifier/jeq_infer_not_null.c b/tools/testing/selftests/bpf/verifier/jeq_infer_not_null.c
deleted file mode 100644
index 67a1c07ead34..000000000000
--- a/tools/testing/selftests/bpf/verifier/jeq_infer_not_null.c
+++ /dev/null
@@ -1,174 +0,0 @@
-{
- /* This is equivalent to the following program:
- *
- * r6 = skb->sk;
- * r7 = sk_fullsock(r6);
- * r0 = sk_fullsock(r6);
- * if (r0 == 0) return 0; (a)
- * if (r0 != r7) return 0; (b)
- * *r7->type; (c)
- * return 0;
- *
- * It is safe to dereference r7 at point (c), because of (a) and (b).
- * The test verifies that relation r0 == r7 is propagated from (b) to (c).
- */
- "jne/jeq infer not null, PTR_TO_SOCKET_OR_NULL -> PTR_TO_SOCKET for JNE false branch",
- .insns = {
- /* r6 = skb->sk; */
- BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, offsetof(struct __sk_buff, sk)),
- /* if (r6 == 0) return 0; */
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_6, 0, 8),
- /* r7 = sk_fullsock(skb); */
- BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
- BPF_EMIT_CALL(BPF_FUNC_sk_fullsock),
- BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
- /* r0 = sk_fullsock(skb); */
- BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
- BPF_EMIT_CALL(BPF_FUNC_sk_fullsock),
- /* if (r0 == null) return 0; */
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
- /* if (r0 == r7) r0 = *(r7->type); */
- BPF_JMP_REG(BPF_JNE, BPF_REG_0, BPF_REG_7, 1), /* Use ! JNE ! */
- BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct bpf_sock, type)),
- /* return 0 */
- BPF_MOV64_IMM(BPF_REG_0, 0),
- BPF_EXIT_INSN(),
- },
- .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
- .result = ACCEPT,
- .result_unpriv = REJECT,
- .errstr_unpriv = "R7 pointer comparison",
-},
-{
- /* Same as above, but verify that another branch of JNE still
- * prohibits access to PTR_MAYBE_NULL.
- */
- "jne/jeq infer not null, PTR_TO_SOCKET_OR_NULL unchanged for JNE true branch",
- .insns = {
- /* r6 = skb->sk */
- BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, offsetof(struct __sk_buff, sk)),
- /* if (r6 == 0) return 0; */
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_6, 0, 9),
- /* r7 = sk_fullsock(skb); */
- BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
- BPF_EMIT_CALL(BPF_FUNC_sk_fullsock),
- BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
- /* r0 = sk_fullsock(skb); */
- BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
- BPF_EMIT_CALL(BPF_FUNC_sk_fullsock),
- /* if (r0 == null) return 0; */
- BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 3),
- /* if (r0 == r7) return 0; */
- BPF_JMP_REG(BPF_JNE, BPF_REG_0, BPF_REG_7, 1), /* Use ! JNE ! */
- BPF_JMP_IMM(BPF_JA, 0, 0, 1),
- /* r0 = *(r7->type); */
- BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct bpf_sock, type)),
- /* return 0 */
- BPF_MOV64_IMM(BPF_REG_0, 0),
- BPF_EXIT_INSN(),
- },
- .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
- .result = REJECT,
- .errstr = "R7 invalid mem access 'sock_or_null'",
- .result_unpriv = REJECT,
- .errstr_unpriv = "R7 pointer comparison",
-},
-{
- /* Same as a first test, but not null should be inferred for JEQ branch */
- "jne/jeq infer not null, PTR_TO_SOCKET_OR_NULL -> PTR_TO_SOCKET for JEQ true branch",
- .insns = {
- /* r6 = skb->sk; */
- BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, offsetof(struct __sk_buff, sk)),
- /* if (r6 == null) return 0; */
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_6, 0, 9),
- /* r7 = sk_fullsock(skb); */
- BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
- BPF_EMIT_CALL(BPF_FUNC_sk_fullsock),
- BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
- /* r0 = sk_fullsock(skb); */
- BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
- BPF_EMIT_CALL(BPF_FUNC_sk_fullsock),
- /* if (r0 == null) return 0; */
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3),
- /* if (r0 != r7) return 0; */
- BPF_JMP_REG(BPF_JEQ, BPF_REG_0, BPF_REG_7, 1), /* Use ! JEQ ! */
- BPF_JMP_IMM(BPF_JA, 0, 0, 1),
- /* r0 = *(r7->type); */
- BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct bpf_sock, type)),
- /* return 0; */
- BPF_MOV64_IMM(BPF_REG_0, 0),
- BPF_EXIT_INSN(),
- },
- .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
- .result = ACCEPT,
- .result_unpriv = REJECT,
- .errstr_unpriv = "R7 pointer comparison",
-},
-{
- /* Same as above, but verify that another branch of JNE still
- * prohibits access to PTR_MAYBE_NULL.
- */
- "jne/jeq infer not null, PTR_TO_SOCKET_OR_NULL unchanged for JEQ false branch",
- .insns = {
- /* r6 = skb->sk; */
- BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, offsetof(struct __sk_buff, sk)),
- /* if (r6 == null) return 0; */
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_6, 0, 8),
- /* r7 = sk_fullsock(skb); */
- BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
- BPF_EMIT_CALL(BPF_FUNC_sk_fullsock),
- BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
- /* r0 = sk_fullsock(skb); */
- BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
- BPF_EMIT_CALL(BPF_FUNC_sk_fullsock),
- /* if (r0 == null) return 0; */
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
- /* if (r0 != r7) r0 = *(r7->type); */
- BPF_JMP_REG(BPF_JEQ, BPF_REG_0, BPF_REG_7, 1), /* Use ! JEQ ! */
- BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct bpf_sock, type)),
- /* return 0; */
- BPF_MOV64_IMM(BPF_REG_0, 0),
- BPF_EXIT_INSN(),
- },
- .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
- .result = REJECT,
- .errstr = "R7 invalid mem access 'sock_or_null'",
- .result_unpriv = REJECT,
- .errstr_unpriv = "R7 pointer comparison",
-},
-{
- /* Maps are treated in a different branch of `mark_ptr_not_null_reg`,
- * so separate test for maps case.
- */
- "jne/jeq infer not null, PTR_TO_MAP_VALUE_OR_NULL -> PTR_TO_MAP_VALUE",
- .insns = {
- /* r9 = &some stack to use as key */
- BPF_ST_MEM(BPF_W, BPF_REG_10, -8, 0),
- BPF_MOV64_REG(BPF_REG_9, BPF_REG_10),
- BPF_ALU64_IMM(BPF_ADD, BPF_REG_9, -8),
- /* r8 = process local map */
- BPF_LD_MAP_FD(BPF_REG_8, 0),
- /* r6 = map_lookup_elem(r8, r9); */
- BPF_MOV64_REG(BPF_REG_1, BPF_REG_8),
- BPF_MOV64_REG(BPF_REG_2, BPF_REG_9),
- BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
- BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
- /* r7 = map_lookup_elem(r8, r9); */
- BPF_MOV64_REG(BPF_REG_1, BPF_REG_8),
- BPF_MOV64_REG(BPF_REG_2, BPF_REG_9),
- BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
- BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
- /* if (r6 == 0) return 0; */
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_6, 0, 2),
- /* if (r6 != r7) return 0; */
- BPF_JMP_REG(BPF_JNE, BPF_REG_6, BPF_REG_7, 1),
- /* read *r7; */
- BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct bpf_xdp_sock, queue_id)),
- /* return 0; */
- BPF_MOV64_IMM(BPF_REG_0, 0),
- BPF_EXIT_INSN(),
- },
- .fixup_map_xskmap = { 3 },
- .prog_type = BPF_PROG_TYPE_XDP,
- .result = ACCEPT,
-},
--
2.39.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 0/5] test_verifier tests migration to inline assembly
2023-01-23 14:51 [RFC bpf-next 0/5] test_verifier tests migration to inline assembly Eduard Zingerman
` (4 preceding siblings ...)
2023-01-23 14:51 ` [RFC bpf-next 5/5] selftests/bpf: convert jeq_infer_not_null tests to inline assembly Eduard Zingerman
@ 2023-01-26 1:33 ` Andrii Nakryiko
2023-01-26 3:25 ` Alexei Starovoitov
2023-01-27 0:30 ` Eduard Zingerman
5 siblings, 2 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2023-01-26 1:33 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, ast, andrii, daniel, kernel-team, yhs
On Mon, Jan 23, 2023 at 6:52 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> As a part of the discussion started in [2] I developed a script [1]
> that can convert tests specified in test_verifier.c format to inline
> BPF assembly compatible for use with test_loader.c.
>
> For example, test definition like below:
>
> {
> "invalid and of negative number",
> .insns = {
> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> BPF_LD_MAP_FD(BPF_REG_1, 0),
> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5),
> BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
> BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
> BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
> BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
> BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
> BPF_EXIT_INSN(),
> },
> .fixup_map_hash_48b = { 3 },
> .errstr = "R0 max value is outside of the allowed memory range",
> .result = REJECT,
> .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
> },
>
> Is converted to:
>
> struct test_val { ... };
>
> struct { ...} map_hash_48b SEC(".maps");
>
> __description("invalid and of negative number")
> __failure __msg("R0 max value is outside of the allowed memory range")
> __failure_unpriv
> __flag(BPF_F_ANY_ALIGNMENT)
> SEC("socket")
nit: let's make sure that SEC() is the first annotation, makes
grepping easier and is consistent with how we use SEC() with BPF
programs
> __naked void invalid_and_of_negative_number(void)
>
> {
> asm volatile (
> " r1 = 0; \n\
Kumar recently landed similarly formatted inline asm-based test, let's
make sure we stick to common style. \n at the end are pretty
distracting, IMO (though helpful to debug syntax errors in asm, of
course). I'd also move starting " into the same line as asm volatile:
asm volatile (" \
this will make adding/removing asm lines at the beginning simpler (and
you already put closing quote on separate line, so that side is taken
care of)
> *(u64*)(r10 - 8) = r1; \n\
> r2 = r10; \n\
> r2 += -8; \n\
> r1 = %[map_hash_48b] ll; \n\
> call %[bpf_map_lookup_elem]; \n\
> if r0 == 0 goto l0_%=; \n\
> r1 = *(u8*)(r0 + 0); \n\
> r1 &= -4; \n\
> r1 <<= 2; \n\
> r0 += r1; \n\
> r1 = %[test_val_foo_offset]; \n\
> *(u64*)(r0 + 0) = r1; \n\
> l0_%=: \n\
> exit; \n\
> " :
> : [test_val_foo_offset]"i"(offsetof(struct test_val, foo)),
should we use some __imm_const(name, value) macro (or __imm_named, or
__imm_alias, or something like that) for cases like this?
> __imm(bpf_map_lookup_elem),
> __imm_addr(map_hash_48b)
> : __clobber_all);
> }
>
> The script introduces labels for gotos and calls, immediate values for
> complex expressions like `offsetof(...)', takes care of map
> instructions patching, inserts map declarations used in the test,
> transfers comments from test, test fields and instructions. There are
> some issues with BPF_ST_MEM instruction support as described in [4],
> thus the script replaces such instructions with pairs of MOV/STX_MEM
> instructions.
>
> This patch-set introduces changes necessary for test_loader.c and
> includes migration of a single test from test_verifier to test_progs
> format, here are descriptions for individual patches:
>
> 1. "Support custom per-test flags and multiple expected messages"
> This patch was shared by Andrii Nakryiko [3], it adds capability
> to specify flags required by the BPF program.
>
> 2. "Unprivileged tests for test_loader.c"
> Extends test_loader.c by capability to load test programs in
> unprivileged mode and specify separate test outcomes for
> privileged and unprivileged modes.
>
> Note: for a reason I do not understand I had to use different set
> of capabilities compared to test_verifier:
> - test_verifier: CAP_NET_ADMIN, CAP_PERFMON, CAP_BPF;
> - test_loader : CAP_SYS_ADMIN, CAP_PERFMON, CAP_BPF.
CAP_SYS_ADMIN is a superset of PERFMON and BPF, so it should be
NET_ADMIN... Setting CAP_SYS_ADMIN makes PERFMON and BPF meaningless,
so we should investigate.
>
> 3. "Generate boilerplate code for test_loader-based tests"
> Extends selftests/bpf Makefile to automatically generate
> prog_tests/tests.h entry that uses test_loader for progs/*.c
> BPF programs if special marker is present.
>
> 4. "__imm_insn macro to embed raw insns in inline asm"
nit: __raw_insns perhaps, otherwise confusing given __imm and __imm_addr macros
> This macro can be generated by migration script for instructions
> that cannot be expressed in inline assembly, e.g. invalid instructions.
>
> 5. "convert jeq_infer_not_null tests to inline assembly"
> Shows an example of the test migration.
> The test was updated slightly after automatic translation:
> - unnecessary comments removed;
> - functions renamed;
> - some labels renamed.
>
> The migration script has some limitations:
> - Technical, test features that are not yet supported:
> - few instructions like BPF_ENDIAN;
> - macro like BPF_SK_LOOKUP or BPF_LD_MAP_VALUE;
> - program types like BPF_PROG_TYPE_CGROUP_SOCK_ADDR and
> BPF_PROG_TYPE_TRACING;
> - tests that specify fields 'expected_attach_type' or 'insn_processed';
> - a few tests expose complex structure that could not be
> automatically migrated, e.g.: 'atomic_fetch', 'lwt',
> 'bpf_loop_inline'.
> - Tests that use .run field cannot be migrated as test_loader.c tests.
> - Tests with generated bodies, e.g. test_verifier.c:bpf_fill_scale()
> cannot be migrated as test_loader.c tests.
> - LLVM related:
> - BPF_ST instruction is not supported by LLVM BPF assembly yet
> (I'll take care of it);
> - Issues with parsing of some assembly instructions like
> "r0 = cmpxchg_64(r10 - 8, r0, r5)"
> (I'll take care of it);
> - libbpf related:
> - libbpf does not support call instructions within a single
> function, e.g.:
it's more of an issue of not having valid relocation emitted and also
corresponding .BTF.ext line/func info (that's my guess)
>
> 0: r1 = 1
> 1: r2 = 2
> 2: call 1
> 3: exit
> 4: r0 = r1
> 5: r0 += r2
> 6: exit
so libbpf itself doesn't care, but given we use BTF and send .BTF.ext,
I suspect kernel just doesn't like that it can't find func_info
information for subprog. So we'll need to split into proper functions
or disable .BTF.ext somehow.
>
> This pattern is common in verifier tests, I see two possible
> mitigation:
> (a) use an additional libbpf flag to allow such code;
I hope not :)
> (b) extend migration script to split such code in several functions.
> I like (a) more.
see above, this won't be a libbpf flag, rather some code in test
runner to strip away .BTF.ext information
>
> - libbpf does not allow empty programs.
like:
SEC("kprobe")
int prog()
{
return 0;
}
?
>
> All in all the current script stats are as follows:
> - 62 out of 93 files from progs/*.c can be converted w/o warnings;
> - 55 converted files could be compiled;
> - 40 pass testing, 15 fail.
>
> By submitting this RFC I seek the following feedback:
> - is community interested in such migration?
+1
This is a great work!
> - if yes, should I pursue partial or complete tests migration?
I'd start with partial
> - in case of partial migration which tests should be prioritized?
those that work out of the box?
> - should I offer migrated tests one by one or in big butches?
they come grouped in files, would it be possible to live them in
similar groupings?
>
> Thanks,
> Eduard
>
> [1] https://github.com/eddyz87/verifier-tests-migrator
> [2] https://lore.kernel.org/bpf/CAEf4BzYPsDWdRgx+ND1wiKAB62P=WwoLhr2uWkbVpQfbHqi1oA@mail.gmail.com/
> [3] https://lore.kernel.org/bpf/CAEf4BzZH0ZxorCi7nPDbRqSK9f+410RooNwNJGwfw8=0a5i1nw@mail.gmail.com/
> [4] https://lore.kernel.org/bpf/20221231163122.1360813-1-eddyz87@gmail.com/
>
> Andrii Nakryiko (1):
> selftests/bpf: support custom per-test flags and multiple expected
> messages
>
> Eduard Zingerman (4):
> selftests/bpf: unprivileged tests for test_loader.c
> selftests/bpf: generate boilerplate code for test_loader-based tests
> selftests/bpf: __imm_insn macro to embed raw insns in inline asm
> selftests/bpf: convert jeq_infer_not_null tests to inline assembly
>
> tools/testing/selftests/bpf/Makefile | 41 +-
> tools/testing/selftests/bpf/autoconf_helper.h | 9 +
> .../selftests/bpf/prog_tests/.gitignore | 1 +
> .../bpf/prog_tests/jeq_infer_not_null.c | 9 -
> tools/testing/selftests/bpf/progs/bpf_misc.h | 49 +++
> .../selftests/bpf/progs/jeq_infer_not_null.c | 186 +++++++++
> .../bpf/progs/jeq_infer_not_null_fail.c | 1 +
> tools/testing/selftests/bpf/test_loader.c | 370 +++++++++++++++---
> tools/testing/selftests/bpf/test_progs.h | 1 +
> tools/testing/selftests/bpf/test_verifier.c | 25 +-
> tools/testing/selftests/bpf/unpriv_helpers.c | 26 ++
> tools/testing/selftests/bpf/unpriv_helpers.h | 7 +
> .../bpf/verifier/jeq_infer_not_null.c | 174 --------
> 13 files changed, 625 insertions(+), 274 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/autoconf_helper.h
> delete mode 100644 tools/testing/selftests/bpf/prog_tests/jeq_infer_not_null.c
> create mode 100644 tools/testing/selftests/bpf/progs/jeq_infer_not_null.c
> create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.c
> create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.h
> delete mode 100644 tools/testing/selftests/bpf/verifier/jeq_infer_not_null.c
>
> --
> 2.39.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 3/5] selftests/bpf: generate boilerplate code for test_loader-based tests
2023-01-23 14:51 ` [RFC bpf-next 3/5] selftests/bpf: generate boilerplate code for test_loader-based tests Eduard Zingerman
@ 2023-01-26 1:43 ` Andrii Nakryiko
2023-01-26 23:29 ` Eduard Zingerman
0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2023-01-26 1:43 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, ast, andrii, daniel, kernel-team, yhs
On Mon, Jan 23, 2023 at 6:52 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Automatically generate boilerplate code necessary to run tests that
> use test_loader.c.
>
> Adds a target 'prog_tests/test_loader_auto_wrappers.c' as part of
> rulesets for 'test_progs' and 'test_progs-no_alu32'. The content of
> this C file is generated by make and has the following structure:
>
> #include <test_progs.h>
>
> #include "some_test_1.skel.h"
> #include "some_test_2.skel.h"
> ...
>
> void test_some_test_1(void) { RUN_TESTS(some_test_1); }
> void test_some_test_2(void) { RUN_TESTS(some_test_2); }
> ...
>
> Here RUN_TESTS is a macro defined in test_progs.h, it expands to a
> code that uses test_loader.c:test_loader__run_subtests() function to
> load tests specified by appropriate skel.h.
>
> In order to get the list of tests included in
> 'test_loader_auto_wrappers.c' the generation script looks for
> 'progs/*.c' files that contain a special comment:
>
> /* Use test_loader marker */
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
It feels like this is a bit of an overkill, tbh. There are
$ ls verifier/*.c | wc -l
94
files. We can move each migrated set of tests from verifier/xxx.c to
progs/verifier_xxx.c. And then just have just manually maintained
prog_tests/verifier.c file where for each converted test we have one
#include and one void test_some_test_1(void) { RUN_TESTS(some_test_1);
}.
It sometimes would useful to add some extra debugging printfs in such
a file, so having it auto generated would be actually an
inconvenience. And that on top of further Makefile complication.
For initial conversion we can auto-generate this file, of course. And
then for each migrated file adding 2 lines manually doesn't seem like
a big deal?
> tools/testing/selftests/bpf/Makefile | 34 +++++++++++++++++++
> .../selftests/bpf/prog_tests/.gitignore | 1 +
> 2 files changed, 35 insertions(+)
>
[...]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 4/5] selftests/bpf: __imm_insn macro to embed raw insns in inline asm
2023-01-23 14:51 ` [RFC bpf-next 4/5] selftests/bpf: __imm_insn macro to embed raw insns in inline asm Eduard Zingerman
@ 2023-01-26 2:48 ` Andrii Nakryiko
0 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2023-01-26 2:48 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, ast, andrii, daniel, kernel-team, yhs
On Mon, Jan 23, 2023 at 6:52 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> A convenience macro to allow the following usage:
>
> #include <linux/filter.h>
>
> ...
> asm volatile (
> ...
> ".8byte %[raw_insn];"
> ...
> :
> : __imm_insn(raw_insn, BPF_RAW_INSN(...))
> : __clobber_all);
>
Ah, now I see why it's __imm_insns. Makes sense. Hopefully this won't
be used very frequently anyways.
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
> tools/testing/selftests/bpf/progs/bpf_misc.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
> index e742a935de98..832bec4818d9 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_misc.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
> @@ -61,6 +61,7 @@
> #define __clobber_common "r0", "r1", "r2", "r3", "r4", "r5", "memory"
> #define __imm(name) [name]"i"(name)
> #define __imm_addr(name) [name]"i"(&name)
> +#define __imm_insn(name, expr) [name]"i"(*(long *)&(expr))
>
> #if defined(__TARGET_ARCH_x86)
> #define SYSCALL_WRAPPER 1
> --
> 2.39.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 0/5] test_verifier tests migration to inline assembly
2023-01-26 1:33 ` [RFC bpf-next 0/5] test_verifier tests migration " Andrii Nakryiko
@ 2023-01-26 3:25 ` Alexei Starovoitov
2023-01-27 0:43 ` Eduard Zingerman
2023-01-27 0:30 ` Eduard Zingerman
1 sibling, 1 reply; 18+ messages in thread
From: Alexei Starovoitov @ 2023-01-26 3:25 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Eduard Zingerman, bpf, ast, andrii, daniel, kernel-team, yhs
On Wed, Jan 25, 2023 at 05:33:42PM -0800, Andrii Nakryiko wrote:
>
> > __naked void invalid_and_of_negative_number(void)
> >
> > {
> > asm volatile (
> > " r1 = 0; \n\
>
> Kumar recently landed similarly formatted inline asm-based test, let's
> make sure we stick to common style. \n at the end are pretty
> distracting, IMO (though helpful to debug syntax errors in asm, of
> course). I'd also move starting " into the same line as asm volatile:
+1. Pls drop \n.
You don't have \n anyway in migrator's README on github.
> asm volatile (" \
>
> this will make adding/removing asm lines at the beginning simpler (and
> you already put closing quote on separate line, so that side is taken
> care of)
+1
Also pls indent the asm code with two tabs the way Kumar did.
I think it looks cleaner this way and single tab labels align
with 'asm volatile ('.
> > All in all the current script stats are as follows:
> > - 62 out of 93 files from progs/*.c can be converted w/o warnings;
out of 98 in verifier/*.c ?
> > - 55 converted files could be compiled;
> > - 40 pass testing, 15 fail.
I would land this 40 now and continue step by step.
> >
> > By submitting this RFC I seek the following feedback:
> > - is community interested in such migration?
>
> +1
>
> This is a great work!
+1
> > - if yes, should I pursue partial or complete tests migration?
>
> I'd start with partial
>
> > - in case of partial migration which tests should be prioritized?
>
> those that work out of the box?
>
> > - should I offer migrated tests one by one or in big butches?
Can you do one patch one file in verifier/*.c that would map
to one new file in progs/ ?
> >
> > [1] https://github.com/eddyz87/verifier-tests-migrator
Having this link in patch series is enough.
The 'migrator' itself doesn't need to be in the kernel tree.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 3/5] selftests/bpf: generate boilerplate code for test_loader-based tests
2023-01-26 1:43 ` Andrii Nakryiko
@ 2023-01-26 23:29 ` Eduard Zingerman
0 siblings, 0 replies; 18+ messages in thread
From: Eduard Zingerman @ 2023-01-26 23:29 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, andrii, daniel, kernel-team, yhs
On Wed, 2023-01-25 at 17:43 -0800, Andrii Nakryiko wrote:
> On Mon, Jan 23, 2023 at 6:52 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> >
> > Automatically generate boilerplate code necessary to run tests that
> > use test_loader.c.
> >
> > Adds a target 'prog_tests/test_loader_auto_wrappers.c' as part of
> > rulesets for 'test_progs' and 'test_progs-no_alu32'. The content of
> > this C file is generated by make and has the following structure:
> >
> > #include <test_progs.h>
> >
> > #include "some_test_1.skel.h"
> > #include "some_test_2.skel.h"
> > ...
> >
> > void test_some_test_1(void) { RUN_TESTS(some_test_1); }
> > void test_some_test_2(void) { RUN_TESTS(some_test_2); }
> > ...
> >
> > Here RUN_TESTS is a macro defined in test_progs.h, it expands to a
> > code that uses test_loader.c:test_loader__run_subtests() function to
> > load tests specified by appropriate skel.h.
> >
> > In order to get the list of tests included in
> > 'test_loader_auto_wrappers.c' the generation script looks for
> > 'progs/*.c' files that contain a special comment:
> >
> > /* Use test_loader marker */
> >
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
>
> It feels like this is a bit of an overkill, tbh. There are
>
> $ ls verifier/*.c | wc -l
> 94
>
> files. We can move each migrated set of tests from verifier/xxx.c to
> progs/verifier_xxx.c. And then just have just manually maintained
> prog_tests/verifier.c file where for each converted test we have one
> #include and one void test_some_test_1(void) { RUN_TESTS(some_test_1);
> }.
>
> It sometimes would useful to add some extra debugging printfs in such
> a file, so having it auto generated would be actually an
> inconvenience. And that on top of further Makefile complication.
>
> For initial conversion we can auto-generate this file, of course. And
> then for each migrated file adding 2 lines manually doesn't seem like
> a big deal?
Ok, I'll remove the makefile changes.
>
>
>
> > tools/testing/selftests/bpf/Makefile | 34 +++++++++++++++++++
> > .../selftests/bpf/prog_tests/.gitignore | 1 +
> > 2 files changed, 35 insertions(+)
> >
>
> [...]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 0/5] test_verifier tests migration to inline assembly
2023-01-26 1:33 ` [RFC bpf-next 0/5] test_verifier tests migration " Andrii Nakryiko
2023-01-26 3:25 ` Alexei Starovoitov
@ 2023-01-27 0:30 ` Eduard Zingerman
2023-01-27 17:41 ` Andrii Nakryiko
1 sibling, 1 reply; 18+ messages in thread
From: Eduard Zingerman @ 2023-01-27 0:30 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, andrii, daniel, kernel-team, yhs
On Wed, 2023-01-25 at 17:33 -0800, Andrii Nakryiko wrote:
> On Mon, Jan 23, 2023 at 6:52 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> >
> > As a part of the discussion started in [2] I developed a script [1]
> > that can convert tests specified in test_verifier.c format to inline
> > BPF assembly compatible for use with test_loader.c.
> >
> > For example, test definition like below:
> >
> > {
> > "invalid and of negative number",
> > .insns = {
> > BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> > BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> > BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> > BPF_LD_MAP_FD(BPF_REG_1, 0),
> > BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> > BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5),
> > BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
> > BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
> > BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
> > BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
> > BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
> > BPF_EXIT_INSN(),
> > },
> > .fixup_map_hash_48b = { 3 },
> > .errstr = "R0 max value is outside of the allowed memory range",
> > .result = REJECT,
> > .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
> > },
> >
> > Is converted to:
> >
> > struct test_val { ... };
> >
> > struct { ...} map_hash_48b SEC(".maps");
> >
> > __description("invalid and of negative number")
> > __failure __msg("R0 max value is outside of the allowed memory range")
> > __failure_unpriv
> > __flag(BPF_F_ANY_ALIGNMENT)
> > SEC("socket")
>
> nit: let's make sure that SEC() is the first annotation, makes
> grepping easier and is consistent with how we use SEC() with BPF
> programs
Ok, will move SEC to the top.
>
>
> > __naked void invalid_and_of_negative_number(void)
> >
> > {
> > asm volatile (
> > " r1 = 0; \n\
>
> Kumar recently landed similarly formatted inline asm-based test, let's
> make sure we stick to common style. \n at the end are pretty
Tbh, the '\n\' at the end of the line is much less distracting
compared to the first variant when each instruction had it's own
string ('"r1 = 0;\n"'). And I find it quite useful when compiler
points to a bad asm instruction.
But I'll remove it if you insist.
> distracting, IMO (though helpful to debug syntax errors in asm, of
> course). I'd also move starting " into the same line as asm volatile:
Ok, no problem, will move the '"'.
>
> asm volatile (" \
>
> this will make adding/removing asm lines at the beginning simpler (and
> you already put closing quote on separate line, so that side is taken
> care of)
>
> > *(u64*)(r10 - 8) = r1; \n\
> > r2 = r10; \n\
> > r2 += -8; \n\
> > r1 = %[map_hash_48b] ll; \n\
> > call %[bpf_map_lookup_elem]; \n\
> > if r0 == 0 goto l0_%=; \n\
> > r1 = *(u8*)(r0 + 0); \n\
> > r1 &= -4; \n\
> > r1 <<= 2; \n\
> > r0 += r1; \n\
> > r1 = %[test_val_foo_offset]; \n\
> > *(u64*)(r0 + 0) = r1; \n\
> > l0_%=: \n\
> > exit; \n\
> > " :
> > : [test_val_foo_offset]"i"(offsetof(struct test_val, foo)),
>
> should we use some __imm_const(name, value) macro (or __imm_named, or
> __imm_alias, or something like that) for cases like this?
>
> > __imm(bpf_map_lookup_elem),
> > __imm_addr(map_hash_48b)
> > : __clobber_all);
> > }
> >
> > The script introduces labels for gotos and calls, immediate values for
> > complex expressions like `offsetof(...)', takes care of map
> > instructions patching, inserts map declarations used in the test,
> > transfers comments from test, test fields and instructions. There are
> > some issues with BPF_ST_MEM instruction support as described in [4],
> > thus the script replaces such instructions with pairs of MOV/STX_MEM
> > instructions.
> >
> > This patch-set introduces changes necessary for test_loader.c and
> > includes migration of a single test from test_verifier to test_progs
> > format, here are descriptions for individual patches:
> >
> > 1. "Support custom per-test flags and multiple expected messages"
> > This patch was shared by Andrii Nakryiko [3], it adds capability
> > to specify flags required by the BPF program.
> >
> > 2. "Unprivileged tests for test_loader.c"
> > Extends test_loader.c by capability to load test programs in
> > unprivileged mode and specify separate test outcomes for
> > privileged and unprivileged modes.
> >
> > Note: for a reason I do not understand I had to use different set
> > of capabilities compared to test_verifier:
> > - test_verifier: CAP_NET_ADMIN, CAP_PERFMON, CAP_BPF;
> > - test_loader : CAP_SYS_ADMIN, CAP_PERFMON, CAP_BPF.
>
> CAP_SYS_ADMIN is a superset of PERFMON and BPF, so it should be
> NET_ADMIN... Setting CAP_SYS_ADMIN makes PERFMON and BPF meaningless,
> so we should investigate.
>
Ok, I'll dig some more and ask for help in case if I would get stuck.
> >
> > 3. "Generate boilerplate code for test_loader-based tests"
> > Extends selftests/bpf Makefile to automatically generate
> > prog_tests/tests.h entry that uses test_loader for progs/*.c
> > BPF programs if special marker is present.
> >
> > 4. "__imm_insn macro to embed raw insns in inline asm"
>
> nit: __raw_insns perhaps, otherwise confusing given __imm and __imm_addr macros
Sorry, I'm a bit confused, should I rename it to __raw_insn or
__imm_insn is fine?
>
> > This macro can be generated by migration script for instructions
> > that cannot be expressed in inline assembly, e.g. invalid instructions.
> >
> > 5. "convert jeq_infer_not_null tests to inline assembly"
> > Shows an example of the test migration.
> > The test was updated slightly after automatic translation:
> > - unnecessary comments removed;
> > - functions renamed;
> > - some labels renamed.
> >
> > The migration script has some limitations:
> > - Technical, test features that are not yet supported:
> > - few instructions like BPF_ENDIAN;
> > - macro like BPF_SK_LOOKUP or BPF_LD_MAP_VALUE;
> > - program types like BPF_PROG_TYPE_CGROUP_SOCK_ADDR and
> > BPF_PROG_TYPE_TRACING;
> > - tests that specify fields 'expected_attach_type' or 'insn_processed';
> > - a few tests expose complex structure that could not be
> > automatically migrated, e.g.: 'atomic_fetch', 'lwt',
> > 'bpf_loop_inline'.
> > - Tests that use .run field cannot be migrated as test_loader.c tests.
> > - Tests with generated bodies, e.g. test_verifier.c:bpf_fill_scale()
> > cannot be migrated as test_loader.c tests.
> > - LLVM related:
> > - BPF_ST instruction is not supported by LLVM BPF assembly yet
> > (I'll take care of it);
> > - Issues with parsing of some assembly instructions like
> > "r0 = cmpxchg_64(r10 - 8, r0, r5)"
> > (I'll take care of it);
> > - libbpf related:
> > - libbpf does not support call instructions within a single
> > function, e.g.:
>
> it's more of an issue of not having valid relocation emitted and also
> corresponding .BTF.ext line/func info (that's my guess)
>
> >
> > 0: r1 = 1
> > 1: r2 = 2
> > 2: call 1
> > 3: exit
> > 4: r0 = r1
> > 5: r0 += r2
> > 6: exit
>
> so libbpf itself doesn't care, but given we use BTF and send .BTF.ext,
> I suspect kernel just doesn't like that it can't find func_info
> information for subprog. So we'll need to split into proper functions
> or disable .BTF.ext somehow.
Actual error that I see comes from libbpf itself and looks as follows:
libbpf: prog 'regalloc_in_callee': no .text section found yet sub-program call exists
libbpf: prog 'regalloc_in_callee': failed to relocate calls: -4005
libbpf: failed to load object 'verifier_regalloc'
It is reported from 'libbpf.c:bpf_object__reloc_code()' when processed
instruction is 'insn_is_subprog_call(insn)' and relocation pointer is NULL.
I would like to avoid automatically splitting test cases into
functions as it means that tests with invalid CFG can't be represented
when test_loader.c is used. However if you are completely against
adding a flag to libbpf for this case I'll proceed with "function
inference" approach.
>
>
> >
> > This pattern is common in verifier tests, I see two possible
> > mitigation:
> > (a) use an additional libbpf flag to allow such code;
>
> I hope not :)
>
> > (b) extend migration script to split such code in several functions.
> > I like (a) more.
>
> see above, this won't be a libbpf flag, rather some code in test
> runner to strip away .BTF.ext information
>
> >
> > - libbpf does not allow empty programs.
>
> like:
>
> SEC("kprobe")
> int prog()
> {
> return 0;
> }
>
> ?
Like this:
SEC("kprobe") int prog() {}
No body at all, we have such test. Probably not a big deal if it is
not migrated.
> >
> > All in all the current script stats are as follows:
> > - 62 out of 93 files from progs/*.c can be converted w/o warnings;
> > - 55 converted files could be compiled;
> > - 40 pass testing, 15 fail.
> >
> > By submitting this RFC I seek the following feedback:
> > - is community interested in such migration?
>
> +1
>
> This is a great work!
>
> > - if yes, should I pursue partial or complete tests migration?
>
> I'd start with partial
>
> > - in case of partial migration which tests should be prioritized?
>
> those that work out of the box?
Ok.
>
> > - should I offer migrated tests one by one or in big butches?
>
> they come grouped in files, would it be possible to live them in
> similar groupings?
Yes, file-by-file is how the tool currently works, I will keep the grouping.
After this patch-set I can submit 40 patches one-by-one or as a single
patch-set. The way I understand you and Alexei a single big patch-set
is preferable, is that right?
>
> >
> > Thanks,
> > Eduard
> >
> > [1] https://github.com/eddyz87/verifier-tests-migrator
> > [2] https://lore.kernel.org/bpf/CAEf4BzYPsDWdRgx+ND1wiKAB62P=WwoLhr2uWkbVpQfbHqi1oA@mail.gmail.com/
> > [3] https://lore.kernel.org/bpf/CAEf4BzZH0ZxorCi7nPDbRqSK9f+410RooNwNJGwfw8=0a5i1nw@mail.gmail.com/
> > [4] https://lore.kernel.org/bpf/20221231163122.1360813-1-eddyz87@gmail.com/
> >
> > Andrii Nakryiko (1):
> > selftests/bpf: support custom per-test flags and multiple expected
> > messages
> >
> > Eduard Zingerman (4):
> > selftests/bpf: unprivileged tests for test_loader.c
> > selftests/bpf: generate boilerplate code for test_loader-based tests
> > selftests/bpf: __imm_insn macro to embed raw insns in inline asm
> > selftests/bpf: convert jeq_infer_not_null tests to inline assembly
> >
> > tools/testing/selftests/bpf/Makefile | 41 +-
> > tools/testing/selftests/bpf/autoconf_helper.h | 9 +
> > .../selftests/bpf/prog_tests/.gitignore | 1 +
> > .../bpf/prog_tests/jeq_infer_not_null.c | 9 -
> > tools/testing/selftests/bpf/progs/bpf_misc.h | 49 +++
> > .../selftests/bpf/progs/jeq_infer_not_null.c | 186 +++++++++
> > .../bpf/progs/jeq_infer_not_null_fail.c | 1 +
> > tools/testing/selftests/bpf/test_loader.c | 370 +++++++++++++++---
> > tools/testing/selftests/bpf/test_progs.h | 1 +
> > tools/testing/selftests/bpf/test_verifier.c | 25 +-
> > tools/testing/selftests/bpf/unpriv_helpers.c | 26 ++
> > tools/testing/selftests/bpf/unpriv_helpers.h | 7 +
> > .../bpf/verifier/jeq_infer_not_null.c | 174 --------
> > 13 files changed, 625 insertions(+), 274 deletions(-)
> > create mode 100644 tools/testing/selftests/bpf/autoconf_helper.h
> > delete mode 100644 tools/testing/selftests/bpf/prog_tests/jeq_infer_not_null.c
> > create mode 100644 tools/testing/selftests/bpf/progs/jeq_infer_not_null.c
> > create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.c
> > create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.h
> > delete mode 100644 tools/testing/selftests/bpf/verifier/jeq_infer_not_null.c
> >
> > --
> > 2.39.0
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 0/5] test_verifier tests migration to inline assembly
2023-01-26 3:25 ` Alexei Starovoitov
@ 2023-01-27 0:43 ` Eduard Zingerman
0 siblings, 0 replies; 18+ messages in thread
From: Eduard Zingerman @ 2023-01-27 0:43 UTC (permalink / raw)
To: Alexei Starovoitov, Andrii Nakryiko
Cc: bpf, ast, andrii, daniel, kernel-team, yhs
On Wed, 2023-01-25 at 19:25 -0800, Alexei Starovoitov wrote:
> On Wed, Jan 25, 2023 at 05:33:42PM -0800, Andrii Nakryiko wrote:
> >
> > > __naked void invalid_and_of_negative_number(void)
> > >
> > > {
> > > asm volatile (
> > > " r1 = 0; \n\
> >
> > Kumar recently landed similarly formatted inline asm-based test, let's
> > make sure we stick to common style. \n at the end are pretty
> > distracting, IMO (though helpful to debug syntax errors in asm, of
> > course). I'd also move starting " into the same line as asm volatile:
>
> +1. Pls drop \n.
> You don't have \n anyway in migrator's README on github.
I have --newlines switch there :)
>
> > asm volatile (" \
> >
> > this will make adding/removing asm lines at the beginning simpler (and
> > you already put closing quote on separate line, so that side is taken
> > care of)
>
> +1
>
> Also pls indent the asm code with two tabs the way Kumar did.
> I think it looks cleaner this way and single tab labels align
> with 'asm volatile ('.
Will do.
>
> > > All in all the current script stats are as follows:
> > > - 62 out of 93 files from progs/*.c can be converted w/o warnings;
>
> out of 98 in verifier/*.c ?
Sorry, yes, messed up twice in this statement:
- meant verifier/*.c not progs/*.c;
- counted 93 files after migrating one file picked to be an examle.
I just double checked and there 94 *.c files in that directory on
bpf-next master branch.
>
> > > - 55 converted files could be compiled;
> > > - 40 pass testing, 15 fail.
>
> I would land this 40 now and continue step by step.
>
> > >
> > > By submitting this RFC I seek the following feedback:
> > > - is community interested in such migration?
> >
> > +1
> >
> > This is a great work!
>
> +1
Thanks.
>
> > > - if yes, should I pursue partial or complete tests migration?
> >
> > I'd start with partial
> >
> > > - in case of partial migration which tests should be prioritized?
> >
> > those that work out of the box?
> >
> > > - should I offer migrated tests one by one or in big butches?
>
> Can you do one patch one file in verifier/*.c that would map
> to one new file in progs/ ?
Will do.
>
> > >
> > > [1] https://github.com/eddyz87/verifier-tests-migrator
>
> Having this link in patch series is enough.
> The 'migrator' itself doesn't need to be in the kernel tree.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 0/5] test_verifier tests migration to inline assembly
2023-01-27 0:30 ` Eduard Zingerman
@ 2023-01-27 17:41 ` Andrii Nakryiko
0 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2023-01-27 17:41 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, ast, andrii, daniel, kernel-team, yhs
On Thu, Jan 26, 2023 at 4:30 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2023-01-25 at 17:33 -0800, Andrii Nakryiko wrote:
> > On Mon, Jan 23, 2023 at 6:52 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > As a part of the discussion started in [2] I developed a script [1]
> > > that can convert tests specified in test_verifier.c format to inline
> > > BPF assembly compatible for use with test_loader.c.
> > >
> > > For example, test definition like below:
> > >
> > > {
> > > "invalid and of negative number",
> > > .insns = {
> > > BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> > > BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> > > BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> > > BPF_LD_MAP_FD(BPF_REG_1, 0),
> > > BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> > > BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5),
> > > BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
> > > BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
> > > BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
> > > BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
> > > BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
> > > BPF_EXIT_INSN(),
> > > },
> > > .fixup_map_hash_48b = { 3 },
> > > .errstr = "R0 max value is outside of the allowed memory range",
> > > .result = REJECT,
> > > .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
> > > },
> > >
> > > Is converted to:
> > >
> > > struct test_val { ... };
> > >
> > > struct { ...} map_hash_48b SEC(".maps");
> > >
> > > __description("invalid and of negative number")
> > > __failure __msg("R0 max value is outside of the allowed memory range")
> > > __failure_unpriv
> > > __flag(BPF_F_ANY_ALIGNMENT)
> > > SEC("socket")
> >
> > nit: let's make sure that SEC() is the first annotation, makes
> > grepping easier and is consistent with how we use SEC() with BPF
> > programs
>
> Ok, will move SEC to the top.
>
> >
> >
> > > __naked void invalid_and_of_negative_number(void)
> > >
> > > {
> > > asm volatile (
> > > " r1 = 0; \n\
> >
> > Kumar recently landed similarly formatted inline asm-based test, let's
> > make sure we stick to common style. \n at the end are pretty
>
> Tbh, the '\n\' at the end of the line is much less distracting
> compared to the first variant when each instruction had it's own
> string ('"r1 = 0;\n"'). And I find it quite useful when compiler
> points to a bad asm instruction.
>
> But I'll remove it if you insist.
I'm not strongly opposed, but when writing new tests by hand I
wouldn't want to add all those \n's. So it's mostly style consistency.
>
> > distracting, IMO (though helpful to debug syntax errors in asm, of
> > course). I'd also move starting " into the same line as asm volatile:
>
> Ok, no problem, will move the '"'.
>
> >
> > asm volatile (" \
> >
> > this will make adding/removing asm lines at the beginning simpler (and
> > you already put closing quote on separate line, so that side is taken
> > care of)
> >
> > > *(u64*)(r10 - 8) = r1; \n\
> > > r2 = r10; \n\
> > > r2 += -8; \n\
> > > r1 = %[map_hash_48b] ll; \n\
> > > call %[bpf_map_lookup_elem]; \n\
> > > if r0 == 0 goto l0_%=; \n\
> > > r1 = *(u8*)(r0 + 0); \n\
> > > r1 &= -4; \n\
> > > r1 <<= 2; \n\
> > > r0 += r1; \n\
> > > r1 = %[test_val_foo_offset]; \n\
> > > *(u64*)(r0 + 0) = r1; \n\
> > > l0_%=: \n\
> > > exit; \n\
> > > " :
> > > : [test_val_foo_offset]"i"(offsetof(struct test_val, foo)),
> >
> > should we use some __imm_const(name, value) macro (or __imm_named, or
> > __imm_alias, or something like that) for cases like this?
> >
> > > __imm(bpf_map_lookup_elem),
> > > __imm_addr(map_hash_48b)
> > > : __clobber_all);
> > > }
> > >
> > > The script introduces labels for gotos and calls, immediate values for
> > > complex expressions like `offsetof(...)', takes care of map
> > > instructions patching, inserts map declarations used in the test,
> > > transfers comments from test, test fields and instructions. There are
> > > some issues with BPF_ST_MEM instruction support as described in [4],
> > > thus the script replaces such instructions with pairs of MOV/STX_MEM
> > > instructions.
> > >
> > > This patch-set introduces changes necessary for test_loader.c and
> > > includes migration of a single test from test_verifier to test_progs
> > > format, here are descriptions for individual patches:
> > >
> > > 1. "Support custom per-test flags and multiple expected messages"
> > > This patch was shared by Andrii Nakryiko [3], it adds capability
> > > to specify flags required by the BPF program.
> > >
> > > 2. "Unprivileged tests for test_loader.c"
> > > Extends test_loader.c by capability to load test programs in
> > > unprivileged mode and specify separate test outcomes for
> > > privileged and unprivileged modes.
> > >
> > > Note: for a reason I do not understand I had to use different set
> > > of capabilities compared to test_verifier:
> > > - test_verifier: CAP_NET_ADMIN, CAP_PERFMON, CAP_BPF;
> > > - test_loader : CAP_SYS_ADMIN, CAP_PERFMON, CAP_BPF.
> >
> > CAP_SYS_ADMIN is a superset of PERFMON and BPF, so it should be
> > NET_ADMIN... Setting CAP_SYS_ADMIN makes PERFMON and BPF meaningless,
> > so we should investigate.
> >
>
> Ok, I'll dig some more and ask for help in case if I would get stuck.
>
> > >
> > > 3. "Generate boilerplate code for test_loader-based tests"
> > > Extends selftests/bpf Makefile to automatically generate
> > > prog_tests/tests.h entry that uses test_loader for progs/*.c
> > > BPF programs if special marker is present.
> > >
> > > 4. "__imm_insn macro to embed raw insns in inline asm"
> >
> > nit: __raw_insns perhaps, otherwise confusing given __imm and __imm_addr macros
>
> Sorry, I'm a bit confused, should I rename it to __raw_insn or
> __imm_insn is fine?
nope, keep it as __imm_insn, I haven't seen patch 4 before replying
here, I assumed this raw instruction macro will be used inline in the
body of asm volatile, but it's done as passing immediate 8-byte value,
so it makes sense to name it with __imm prefix
>
> >
> > > This macro can be generated by migration script for instructions
> > > that cannot be expressed in inline assembly, e.g. invalid instructions.
> > >
> > > 5. "convert jeq_infer_not_null tests to inline assembly"
> > > Shows an example of the test migration.
> > > The test was updated slightly after automatic translation:
> > > - unnecessary comments removed;
> > > - functions renamed;
> > > - some labels renamed.
> > >
> > > The migration script has some limitations:
> > > - Technical, test features that are not yet supported:
> > > - few instructions like BPF_ENDIAN;
> > > - macro like BPF_SK_LOOKUP or BPF_LD_MAP_VALUE;
> > > - program types like BPF_PROG_TYPE_CGROUP_SOCK_ADDR and
> > > BPF_PROG_TYPE_TRACING;
> > > - tests that specify fields 'expected_attach_type' or 'insn_processed';
> > > - a few tests expose complex structure that could not be
> > > automatically migrated, e.g.: 'atomic_fetch', 'lwt',
> > > 'bpf_loop_inline'.
> > > - Tests that use .run field cannot be migrated as test_loader.c tests.
> > > - Tests with generated bodies, e.g. test_verifier.c:bpf_fill_scale()
> > > cannot be migrated as test_loader.c tests.
> > > - LLVM related:
> > > - BPF_ST instruction is not supported by LLVM BPF assembly yet
> > > (I'll take care of it);
> > > - Issues with parsing of some assembly instructions like
> > > "r0 = cmpxchg_64(r10 - 8, r0, r5)"
> > > (I'll take care of it);
> > > - libbpf related:
> > > - libbpf does not support call instructions within a single
> > > function, e.g.:
> >
> > it's more of an issue of not having valid relocation emitted and also
> > corresponding .BTF.ext line/func info (that's my guess)
> >
> > >
> > > 0: r1 = 1
> > > 1: r2 = 2
> > > 2: call 1
> > > 3: exit
> > > 4: r0 = r1
> > > 5: r0 += r2
> > > 6: exit
> >
> > so libbpf itself doesn't care, but given we use BTF and send .BTF.ext,
> > I suspect kernel just doesn't like that it can't find func_info
> > information for subprog. So we'll need to split into proper functions
> > or disable .BTF.ext somehow.
>
> Actual error that I see comes from libbpf itself and looks as follows:
>
> libbpf: prog 'regalloc_in_callee': no .text section found yet sub-program call exists
> libbpf: prog 'regalloc_in_callee': failed to relocate calls: -4005
> libbpf: failed to load object 'verifier_regalloc'
>
> It is reported from 'libbpf.c:bpf_object__reloc_code()' when processed
> instruction is 'insn_is_subprog_call(insn)' and relocation pointer is NULL.
> I would like to avoid automatically splitting test cases into
> functions as it means that tests with invalid CFG can't be represented
> when test_loader.c is used. However if you are completely against
> adding a flag to libbpf for this case I'll proceed with "function
> inference" approach.
Ok, I see. Libbpf has restriction that subprograms have to be in
.text, they can't be in SEC("kprobe") and whatnot. The latter is due
to inability to distinguish which function in SEC("kprobe") (as one
specific example of program section) is entry program and which one is
subprog. So any subprogram is just an unannotated function in .text.
Have you tried .section directive to temporarily put subprogram into
.text and then go back to the main program? This probably won't be
enough, but I'm thinking something like
r1 = 1
r2 = 2
call 1
exit
.section .text, "rx"
r0 = r1
r0 += r2
exit
.section kprobe, "rx"
We might need more directives to properly annotate subprog (function)
definition, we can check what compiler generates from C code for
subprogs.
>
> >
> >
> > >
> > > This pattern is common in verifier tests, I see two possible
> > > mitigation:
> > > (a) use an additional libbpf flag to allow such code;
> >
> > I hope not :)
> >
> > > (b) extend migration script to split such code in several functions.
> > > I like (a) more.
> >
> > see above, this won't be a libbpf flag, rather some code in test
> > runner to strip away .BTF.ext information
> >
> > >
> > > - libbpf does not allow empty programs.
> >
> > like:
> >
> > SEC("kprobe")
> > int prog()
> > {
> > return 0;
> > }
> >
> > ?
>
> Like this:
>
> SEC("kprobe") int prog() {}
>
> No body at all, we have such test. Probably not a big deal if it is
> not migrated.
prog returns int, so at the very least it should have `return 0;`. And
in general, any BPF function should at least have `exit` BPF
instruction, right?
>
> > >
> > > All in all the current script stats are as follows:
> > > - 62 out of 93 files from progs/*.c can be converted w/o warnings;
> > > - 55 converted files could be compiled;
> > > - 40 pass testing, 15 fail.
> > >
> > > By submitting this RFC I seek the following feedback:
> > > - is community interested in such migration?
> >
> > +1
> >
> > This is a great work!
> >
> > > - if yes, should I pursue partial or complete tests migration?
> >
> > I'd start with partial
> >
> > > - in case of partial migration which tests should be prioritized?
> >
> > those that work out of the box?
>
> Ok.
>
> >
> > > - should I offer migrated tests one by one or in big butches?
> >
> > they come grouped in files, would it be possible to live them in
> > similar groupings?
>
> Yes, file-by-file is how the tool currently works, I will keep the grouping.
> After this patch-set I can submit 40 patches one-by-one or as a single
> patch-set. The way I understand you and Alexei a single big patch-set
> is preferable, is that right?
yes, one larger patch set would be easier to manager than 40 separate
ones in this particular case
>
> >
> > >
> > > Thanks,
> > > Eduard
> > >
> > > [1] https://github.com/eddyz87/verifier-tests-migrator
> > > [2] https://lore.kernel.org/bpf/CAEf4BzYPsDWdRgx+ND1wiKAB62P=WwoLhr2uWkbVpQfbHqi1oA@mail.gmail.com/
> > > [3] https://lore.kernel.org/bpf/CAEf4BzZH0ZxorCi7nPDbRqSK9f+410RooNwNJGwfw8=0a5i1nw@mail.gmail.com/
> > > [4] https://lore.kernel.org/bpf/20221231163122.1360813-1-eddyz87@gmail.com/
> > >
> > > Andrii Nakryiko (1):
> > > selftests/bpf: support custom per-test flags and multiple expected
> > > messages
> > >
> > > Eduard Zingerman (4):
> > > selftests/bpf: unprivileged tests for test_loader.c
> > > selftests/bpf: generate boilerplate code for test_loader-based tests
> > > selftests/bpf: __imm_insn macro to embed raw insns in inline asm
> > > selftests/bpf: convert jeq_infer_not_null tests to inline assembly
> > >
> > > tools/testing/selftests/bpf/Makefile | 41 +-
> > > tools/testing/selftests/bpf/autoconf_helper.h | 9 +
> > > .../selftests/bpf/prog_tests/.gitignore | 1 +
> > > .../bpf/prog_tests/jeq_infer_not_null.c | 9 -
> > > tools/testing/selftests/bpf/progs/bpf_misc.h | 49 +++
> > > .../selftests/bpf/progs/jeq_infer_not_null.c | 186 +++++++++
> > > .../bpf/progs/jeq_infer_not_null_fail.c | 1 +
> > > tools/testing/selftests/bpf/test_loader.c | 370 +++++++++++++++---
> > > tools/testing/selftests/bpf/test_progs.h | 1 +
> > > tools/testing/selftests/bpf/test_verifier.c | 25 +-
> > > tools/testing/selftests/bpf/unpriv_helpers.c | 26 ++
> > > tools/testing/selftests/bpf/unpriv_helpers.h | 7 +
> > > .../bpf/verifier/jeq_infer_not_null.c | 174 --------
> > > 13 files changed, 625 insertions(+), 274 deletions(-)
> > > create mode 100644 tools/testing/selftests/bpf/autoconf_helper.h
> > > delete mode 100644 tools/testing/selftests/bpf/prog_tests/jeq_infer_not_null.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/jeq_infer_not_null.c
> > > create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.c
> > > create mode 100644 tools/testing/selftests/bpf/unpriv_helpers.h
> > > delete mode 100644 tools/testing/selftests/bpf/verifier/jeq_infer_not_null.c
> > >
> > > --
> > > 2.39.0
> > >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 1/5] selftests/bpf: support custom per-test flags and multiple expected messages
2023-01-23 14:51 ` [RFC bpf-next 1/5] selftests/bpf: support custom per-test flags and multiple expected messages Eduard Zingerman
@ 2023-02-28 18:53 ` Andrii Nakryiko
2023-02-28 22:30 ` Eduard Zingerman
0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2023-02-28 18:53 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, ast, andrii, daniel, kernel-team, yhs
On Mon, Jan 23, 2023 at 6:52 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> From: Andrii Nakryiko <andrii@kernel.org>
>
> Extend __flag attribute by allowing to specify one of the following:
> * BPF_F_STRICT_ALIGNMENT
> * BPF_F_ANY_ALIGNMENT
> * BPF_F_TEST_RND_HI32
> * BPF_F_TEST_STATE_FREQ
> * BPF_F_SLEEPABLE
> * BPF_F_XDP_HAS_FRAGS
> * Some numeric value
>
> Extend __msg attribute by allowing to specify multiple exepcted messages.
> All messages are expected to be present in the verifier log in the
> order of application.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> [ Eduard: added commit message, formatting ]
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
hey Eduard,
When you get a chance, can you please send this patch separately from
the rest of the test_verifier rework patch set (it probably makes
sense to also add #define __flags in this patch as well, given you are
parsing its definition in this patch).
This would great help me with my work that uses all this
assembly-level test facilities. Thanks!
> tools/testing/selftests/bpf/test_loader.c | 69 ++++++++++++++++++++---
> tools/testing/selftests/bpf/test_progs.h | 1 +
> 2 files changed, 61 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> index 679efb3aa785..bf41390157bf 100644
> --- a/tools/testing/selftests/bpf/test_loader.c
> +++ b/tools/testing/selftests/bpf/test_loader.c
> @@ -13,12 +13,15 @@
> #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
> #define TEST_TAG_EXPECT_MSG_PFX "comment:test_expect_msg="
> #define TEST_TAG_LOG_LEVEL_PFX "comment:test_log_level="
> +#define TEST_TAG_PROG_FLAGS_PFX "comment:test_prog_flags="
>
[...]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 1/5] selftests/bpf: support custom per-test flags and multiple expected messages
2023-02-28 18:53 ` Andrii Nakryiko
@ 2023-02-28 22:30 ` Eduard Zingerman
2023-03-01 17:12 ` Andrii Nakryiko
0 siblings, 1 reply; 18+ messages in thread
From: Eduard Zingerman @ 2023-02-28 22:30 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, andrii, daniel, kernel-team, yhs
On Tue, 2023-02-28 at 10:53 -0800, Andrii Nakryiko wrote:
> On Mon, Jan 23, 2023 at 6:52 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> >
> > From: Andrii Nakryiko <andrii@kernel.org>
> >
> > Extend __flag attribute by allowing to specify one of the following:
> > * BPF_F_STRICT_ALIGNMENT
> > * BPF_F_ANY_ALIGNMENT
> > * BPF_F_TEST_RND_HI32
> > * BPF_F_TEST_STATE_FREQ
> > * BPF_F_SLEEPABLE
> > * BPF_F_XDP_HAS_FRAGS
> > * Some numeric value
> >
> > Extend __msg attribute by allowing to specify multiple exepcted messages.
> > All messages are expected to be present in the verifier log in the
> > order of application.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > [ Eduard: added commit message, formatting ]
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
>
> hey Eduard,
>
> When you get a chance, can you please send this patch separately from
> the rest of the test_verifier rework patch set (it probably makes
> sense to also add #define __flags in this patch as well, given you are
> parsing its definition in this patch).
>
> This would great help me with my work that uses all this
> assembly-level test facilities. Thanks!
Hi Andrii,
Rebase didn't change anything in the patch, I added __flags macro,
some some comments, and started the CI job: [1].
Feels weird to post it, tbh, because it's 100% your code w/o added
value from my side.
Thanks,
Eduard
[1] https://github.com/kernel-patches/bpf/pull/4688
>
>
>
> > tools/testing/selftests/bpf/test_loader.c | 69 ++++++++++++++++++++---
> > tools/testing/selftests/bpf/test_progs.h | 1 +
> > 2 files changed, 61 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> > index 679efb3aa785..bf41390157bf 100644
> > --- a/tools/testing/selftests/bpf/test_loader.c
> > +++ b/tools/testing/selftests/bpf/test_loader.c
> > @@ -13,12 +13,15 @@
> > #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
> > #define TEST_TAG_EXPECT_MSG_PFX "comment:test_expect_msg="
> > #define TEST_TAG_LOG_LEVEL_PFX "comment:test_log_level="
> > +#define TEST_TAG_PROG_FLAGS_PFX "comment:test_prog_flags="
> >
>
> [...]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 1/5] selftests/bpf: support custom per-test flags and multiple expected messages
2023-02-28 22:30 ` Eduard Zingerman
@ 2023-03-01 17:12 ` Andrii Nakryiko
2023-03-01 17:58 ` Eduard Zingerman
0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2023-03-01 17:12 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, ast, andrii, daniel, kernel-team, yhs
On Tue, Feb 28, 2023 at 2:30 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Tue, 2023-02-28 at 10:53 -0800, Andrii Nakryiko wrote:
> > On Mon, Jan 23, 2023 at 6:52 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > From: Andrii Nakryiko <andrii@kernel.org>
> > >
> > > Extend __flag attribute by allowing to specify one of the following:
> > > * BPF_F_STRICT_ALIGNMENT
> > > * BPF_F_ANY_ALIGNMENT
> > > * BPF_F_TEST_RND_HI32
> > > * BPF_F_TEST_STATE_FREQ
> > > * BPF_F_SLEEPABLE
> > > * BPF_F_XDP_HAS_FRAGS
> > > * Some numeric value
> > >
> > > Extend __msg attribute by allowing to specify multiple exepcted messages.
> > > All messages are expected to be present in the verifier log in the
> > > order of application.
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > [ Eduard: added commit message, formatting ]
> > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > > ---
> >
> > hey Eduard,
> >
> > When you get a chance, can you please send this patch separately from
> > the rest of the test_verifier rework patch set (it probably makes
> > sense to also add #define __flags in this patch as well, given you are
> > parsing its definition in this patch).
> >
> > This would great help me with my work that uses all this
> > assembly-level test facilities. Thanks!
>
> Hi Andrii,
>
> Rebase didn't change anything in the patch, I added __flags macro,
> some some comments, and started the CI job: [1].
>
> Feels weird to post it, tbh, because it's 100% your code w/o added
> value from my side.
you took the effort to prepare it for submission, testing, and
integrating into your work, so feels well deserved
>
> Thanks,
> Eduard
>
> [1] https://github.com/kernel-patches/bpf/pull/4688
apart from test flakiness, looks good, please send a patch "officially"
> >
> >
> >
> > > tools/testing/selftests/bpf/test_loader.c | 69 ++++++++++++++++++++---
> > > tools/testing/selftests/bpf/test_progs.h | 1 +
> > > 2 files changed, 61 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> > > index 679efb3aa785..bf41390157bf 100644
> > > --- a/tools/testing/selftests/bpf/test_loader.c
> > > +++ b/tools/testing/selftests/bpf/test_loader.c
> > > @@ -13,12 +13,15 @@
> > > #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
> > > #define TEST_TAG_EXPECT_MSG_PFX "comment:test_expect_msg="
> > > #define TEST_TAG_LOG_LEVEL_PFX "comment:test_log_level="
> > > +#define TEST_TAG_PROG_FLAGS_PFX "comment:test_prog_flags="
> > >
> >
> > [...]
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC bpf-next 1/5] selftests/bpf: support custom per-test flags and multiple expected messages
2023-03-01 17:12 ` Andrii Nakryiko
@ 2023-03-01 17:58 ` Eduard Zingerman
0 siblings, 0 replies; 18+ messages in thread
From: Eduard Zingerman @ 2023-03-01 17:58 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, andrii, daniel, kernel-team, yhs
On Wed, 2023-03-01 at 09:12 -0800, Andrii Nakryiko wrote:
[...]
> > Rebase didn't change anything in the patch, I added __flags macro,
> > some some comments, and started the CI job: [1].
> >
> > Feels weird to post it, tbh, because it's 100% your code w/o added
> > value from my side.
>
> you took the effort to prepare it for submission, testing, and
> integrating into your work, so feels well deserved
>
>
> >
> > Thanks,
> > Eduard
> >
> > [1] https://github.com/kernel-patches/bpf/pull/4688
>
> apart from test flakiness, looks good, please send a patch "officially"
Done.
Local test run does not show any issues.
>
> > >
> > >
> > >
> > > > tools/testing/selftests/bpf/test_loader.c | 69 ++++++++++++++++++++---
> > > > tools/testing/selftests/bpf/test_progs.h | 1 +
> > > > 2 files changed, 61 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
> > > > index 679efb3aa785..bf41390157bf 100644
> > > > --- a/tools/testing/selftests/bpf/test_loader.c
> > > > +++ b/tools/testing/selftests/bpf/test_loader.c
> > > > @@ -13,12 +13,15 @@
> > > > #define TEST_TAG_EXPECT_SUCCESS "comment:test_expect_success"
> > > > #define TEST_TAG_EXPECT_MSG_PFX "comment:test_expect_msg="
> > > > #define TEST_TAG_LOG_LEVEL_PFX "comment:test_log_level="
> > > > +#define TEST_TAG_PROG_FLAGS_PFX "comment:test_prog_flags="
> > > >
> > >
> > > [...]
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-03-01 17:58 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-23 14:51 [RFC bpf-next 0/5] test_verifier tests migration to inline assembly Eduard Zingerman
2023-01-23 14:51 ` [RFC bpf-next 1/5] selftests/bpf: support custom per-test flags and multiple expected messages Eduard Zingerman
2023-02-28 18:53 ` Andrii Nakryiko
2023-02-28 22:30 ` Eduard Zingerman
2023-03-01 17:12 ` Andrii Nakryiko
2023-03-01 17:58 ` Eduard Zingerman
2023-01-23 14:51 ` [RFC bpf-next 2/5] selftests/bpf: unprivileged tests for test_loader.c Eduard Zingerman
2023-01-23 14:51 ` [RFC bpf-next 3/5] selftests/bpf: generate boilerplate code for test_loader-based tests Eduard Zingerman
2023-01-26 1:43 ` Andrii Nakryiko
2023-01-26 23:29 ` Eduard Zingerman
2023-01-23 14:51 ` [RFC bpf-next 4/5] selftests/bpf: __imm_insn macro to embed raw insns in inline asm Eduard Zingerman
2023-01-26 2:48 ` Andrii Nakryiko
2023-01-23 14:51 ` [RFC bpf-next 5/5] selftests/bpf: convert jeq_infer_not_null tests to inline assembly Eduard Zingerman
2023-01-26 1:33 ` [RFC bpf-next 0/5] test_verifier tests migration " Andrii Nakryiko
2023-01-26 3:25 ` Alexei Starovoitov
2023-01-27 0:43 ` Eduard Zingerman
2023-01-27 0:30 ` Eduard Zingerman
2023-01-27 17:41 ` Andrii Nakryiko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.