* [PATCH bpf-next 0/4] fix __retval() being always ignored
@ 2023-04-20 23:23 Eduard Zingerman
2023-04-20 23:23 ` [PATCH bpf-next 1/4] selftests/bpf: disable program test run for progs/refcounted_kptr.c Eduard Zingerman
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Eduard Zingerman @ 2023-04-20 23:23 UTC (permalink / raw)
To: bpf, ast; +Cc: andrii, daniel, martin.lau, kernel-team, yhs, Eduard Zingerman
Florian Westphal found a bug in test_loader.c processing of __retval tag.
Because of this bug the function test_loader.c:do_prog_test_run()
never executed and all __retval test tags were ignored. See [1].
Fix for this bug uncovers two additional bugs:
- During test_verifier tests migration to inline assembly (see [2])
I missed the fact that some tests require maps to contain mock values;
- Some issue with a new refcounted_kptr test, which causes kernel to
produce dead lock and refcount saturation warnings when subject to
libbpf's bpf_test_run_opts().
This series fixes the bug in __retval() processing, and address the
issue with test maps not being populated. The issue in refcounted_kptr
is not addressed, __retval tags in those tests are commented out.
I found that the following tests depend on test maps being populated:
- progs/verifier_array_access.c
- verifier/value_ptr_arith.c (planned for migration to inline assembly)
Given the small amount of these tests I decided to opt for simple
non-generic solution (see patch #4).
[1] https://lore.kernel.org/bpf/f4c4aee644425842ee6aa8edf1da68f0a8260e7c.camel@gmail.com/T/
[2] https://lore.kernel.org/bpf/20230325025524.144043-1-eddyz87@gmail.com/
Eduard Zingerman (4):
selftests/bpf: disable program test run for progs/refcounted_kptr.c
selftests/bpf: fix __retval() being always ignored
selftests/bpf: add pre bpf_prog_test_run_opts() callback for
test_loader
selftests/bpf: populate map_array_ro map for verifier_array_access
test
.../selftests/bpf/prog_tests/verifier.c | 42 +++++++++++++++++--
.../selftests/bpf/progs/refcounted_kptr.c | 8 ++--
tools/testing/selftests/bpf/test_loader.c | 10 ++++-
tools/testing/selftests/bpf/test_progs.h | 9 ++++
4 files changed, 61 insertions(+), 8 deletions(-)
--
2.40.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 1/4] selftests/bpf: disable program test run for progs/refcounted_kptr.c
2023-04-20 23:23 [PATCH bpf-next 0/4] fix __retval() being always ignored Eduard Zingerman
@ 2023-04-20 23:23 ` Eduard Zingerman
2023-04-20 23:23 ` [PATCH bpf-next 2/4] selftests/bpf: fix __retval() being always ignored Eduard Zingerman
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Eduard Zingerman @ 2023-04-20 23:23 UTC (permalink / raw)
To: bpf, ast
Cc: andrii, daniel, martin.lau, kernel-team, yhs, Eduard Zingerman,
Florian Westphal
Florian Westphal found a bug in test_loader.c processing of __retval
tag. Because of this bug the function test_loader.c:do_prog_test_run()
never executed and all __retval test tags were ignored. This hid an
issue with progs/refcounted_kptr.c tests.
When __retval tag bug is fixed and refcounted_kptr.c tests are run
kernel reports various issues and eventually hangs. Shortest reproducer
is the following command run a few times:
$ for i in $(seq 1 4); do (./test_progs --allow=refcounted_kptr &); done
Commenting out __retval tags for these tests until this issue is resolved.
Reported-by: Florian Westphal <fw@strlen.de>
Link: https://lore.kernel.org/bpf/f4c4aee644425842ee6aa8edf1da68f0a8260e7c.camel@gmail.com/T/
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
tools/testing/selftests/bpf/progs/refcounted_kptr.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr.c b/tools/testing/selftests/bpf/progs/refcounted_kptr.c
index 1d348a225140..b6b2d4f97b19 100644
--- a/tools/testing/selftests/bpf/progs/refcounted_kptr.c
+++ b/tools/testing/selftests/bpf/progs/refcounted_kptr.c
@@ -219,7 +219,7 @@ static long __read_from_unstash(int idx)
#define INSERT_READ_BOTH(rem_tree, rem_list, desc) \
SEC("tc") \
__description(desc) \
-__success __retval(579) \
+__success /* __retval(579) temporarily disabled */ \
long insert_and_remove_tree_##rem_tree##_list_##rem_list(void *ctx) \
{ \
long err, tree_data, list_data; \
@@ -258,7 +258,7 @@ INSERT_READ_BOTH(false, true, "insert_read_both: remove from list");
#define INSERT_READ_BOTH(rem_tree, rem_list, desc) \
SEC("tc") \
__description(desc) \
-__success __retval(579) \
+__success /* __retval(579) temporarily disabled */ \
long insert_and_remove_lf_tree_##rem_tree##_list_##rem_list(void *ctx) \
{ \
long err, tree_data, list_data; \
@@ -296,7 +296,7 @@ INSERT_READ_BOTH(false, true, "insert_read_both_list_first: remove from list");
#define INSERT_DOUBLE_READ_AND_DEL(read_fn, read_root, desc) \
SEC("tc") \
__description(desc) \
-__success __retval(-1) \
+__success /* temporarily __retval(-1) disabled */ \
long insert_double_##read_fn##_and_del_##read_root(void *ctx) \
{ \
long err, list_data; \
@@ -329,7 +329,7 @@ INSERT_DOUBLE_READ_AND_DEL(__read_from_list, head, "insert_double_del: 2x read-a
#define INSERT_STASH_READ(rem_tree, desc) \
SEC("tc") \
__description(desc) \
-__success __retval(84) \
+__success /* __retval(84) temporarily disabled */ \
long insert_rbtree_and_stash__del_tree_##rem_tree(void *ctx) \
{ \
long err, tree_data, map_data; \
--
2.40.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/4] selftests/bpf: fix __retval() being always ignored
2023-04-20 23:23 [PATCH bpf-next 0/4] fix __retval() being always ignored Eduard Zingerman
2023-04-20 23:23 ` [PATCH bpf-next 1/4] selftests/bpf: disable program test run for progs/refcounted_kptr.c Eduard Zingerman
@ 2023-04-20 23:23 ` Eduard Zingerman
2023-04-20 23:23 ` [PATCH bpf-next 3/4] selftests/bpf: add pre bpf_prog_test_run_opts() callback for test_loader Eduard Zingerman
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Eduard Zingerman @ 2023-04-20 23:23 UTC (permalink / raw)
To: bpf, ast
Cc: andrii, daniel, martin.lau, kernel-team, yhs, Eduard Zingerman,
Florian Westphal
Florian Westphal found a bug in and suggested a fix for test_loader.c
processing of __retval tag. Because of this bug the function
test_loader.c:do_prog_test_run() never executed and all __retval test
tags were ignored.
If this bug is fixed a number of test cases from
progs/verifier_array_access.c fail with retval not matching the
expected value. This test was recently converted to use test_loader.c
and inline assembly in [1]. When doing the conversion I missed the
important detail of test_verifier.c operation: when it creates
fixup_map_array_ro, fixup_map_array_wo and fixup_map_array_small it
populates these maps with a dummy record.
Disabling the __retval checks for the affected verifier_array_access
in this commit to avoid false-postivies in any potential bisects.
The issue is addressed in the next patch.
I verified that the __retval tags are now respected by changing
expected return values for all tests annotated with __retval, and
checking that these tests started to fail.
[1] https://lore.kernel.org/bpf/20230325025524.144043-1-eddyz87@gmail.com/
Fixes: 19a8e06f5f91 ("selftests/bpf: Tests execution support for test_loader.c")
Reported-by: Florian Westphal <fw@strlen.de>
Link: https://lore.kernel.org/bpf/f4c4aee644425842ee6aa8edf1da68f0a8260e7c.camel@gmail.com/T/
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
tools/testing/selftests/bpf/progs/verifier_array_access.c | 4 ++--
tools/testing/selftests/bpf/test_loader.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/verifier_array_access.c b/tools/testing/selftests/bpf/progs/verifier_array_access.c
index 95d7ecc12963..fceeeef78721 100644
--- a/tools/testing/selftests/bpf/progs/verifier_array_access.c
+++ b/tools/testing/selftests/bpf/progs/verifier_array_access.c
@@ -330,7 +330,7 @@ l0_%=: exit; \
SEC("socket")
__description("valid read map access into a read-only array 1")
-__success __success_unpriv __retval(28)
+__success __success_unpriv /* __retval(28) temporarily disable */
__naked void a_read_only_array_1_1(void)
{
asm volatile (" \
@@ -351,7 +351,7 @@ l0_%=: exit; \
SEC("tc")
__description("valid read map access into a read-only array 2")
-__success __retval(65507)
+__success /* __retval(65507) temporarily disable */
__naked void a_read_only_array_2_1(void)
{
asm volatile (" \
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 47e9e076bc8f..e2a1bdc5a570 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -587,7 +587,7 @@ void run_subtest(struct test_loader *tester,
/* For some reason test_verifier executes programs
* with all capabilities restored. Do the same here.
*/
- if (!restore_capabilities(&caps))
+ if (restore_capabilities(&caps))
goto tobj_cleanup;
do_prog_test_run(bpf_program__fd(tprog), &retval);
--
2.40.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next 3/4] selftests/bpf: add pre bpf_prog_test_run_opts() callback for test_loader
2023-04-20 23:23 [PATCH bpf-next 0/4] fix __retval() being always ignored Eduard Zingerman
2023-04-20 23:23 ` [PATCH bpf-next 1/4] selftests/bpf: disable program test run for progs/refcounted_kptr.c Eduard Zingerman
2023-04-20 23:23 ` [PATCH bpf-next 2/4] selftests/bpf: fix __retval() being always ignored Eduard Zingerman
@ 2023-04-20 23:23 ` Eduard Zingerman
2023-04-20 23:23 ` [PATCH bpf-next 4/4] selftests/bpf: populate map_array_ro map for verifier_array_access test Eduard Zingerman
2023-04-21 2:00 ` [PATCH bpf-next 0/4] fix __retval() being always ignored patchwork-bot+netdevbpf
4 siblings, 0 replies; 6+ messages in thread
From: Eduard Zingerman @ 2023-04-20 23:23 UTC (permalink / raw)
To: bpf, ast; +Cc: andrii, daniel, martin.lau, kernel-team, yhs, Eduard Zingerman
When a test case is annotated with __retval tag the test_loader engine
would use libbpf's bpf_prog_test_run_opts() to do a test run of the
program and compare retvals.
This commit allows to perform arbitrary actions on bpf object right
before test loader invokes bpf_prog_test_run_opts(). This could be
used to setup some state for program execution, e.g. fill some maps.
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
tools/testing/selftests/bpf/test_loader.c | 8 ++++++++
tools/testing/selftests/bpf/test_progs.h | 9 +++++++++
2 files changed, 17 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index e2a1bdc5a570..40c9b7d532c4 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -590,6 +590,14 @@ void run_subtest(struct test_loader *tester,
if (restore_capabilities(&caps))
goto tobj_cleanup;
+ if (tester->pre_execution_cb) {
+ err = tester->pre_execution_cb(tobj);
+ if (err) {
+ PRINT_FAIL("pre_execution_cb failed: %d\n", err);
+ goto tobj_cleanup;
+ }
+ }
+
do_prog_test_run(bpf_program__fd(tprog), &retval);
if (retval != subspec->retval && subspec->retval != POINTER_VALUE) {
PRINT_FAIL("Unexpected retval: %d != %d\n", retval, subspec->retval);
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 10ba43250668..0ed3134333d4 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -424,14 +424,23 @@ int get_bpf_max_tramp_links(void);
#define BPF_TESTMOD_TEST_FILE "/sys/kernel/bpf_testmod"
+typedef int (*pre_execution_cb)(struct bpf_object *obj);
+
struct test_loader {
char *log_buf;
size_t log_buf_sz;
size_t next_match_pos;
+ pre_execution_cb pre_execution_cb;
struct bpf_object *obj;
};
+static inline void test_loader__set_pre_execution_cb(struct test_loader *tester,
+ pre_execution_cb cb)
+{
+ tester->pre_execution_cb = cb;
+}
+
typedef const void *(*skel_elf_bytes_fn)(size_t *sz);
extern void test_loader__run_subtests(struct test_loader *tester,
--
2.40.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next 4/4] selftests/bpf: populate map_array_ro map for verifier_array_access test
2023-04-20 23:23 [PATCH bpf-next 0/4] fix __retval() being always ignored Eduard Zingerman
` (2 preceding siblings ...)
2023-04-20 23:23 ` [PATCH bpf-next 3/4] selftests/bpf: add pre bpf_prog_test_run_opts() callback for test_loader Eduard Zingerman
@ 2023-04-20 23:23 ` Eduard Zingerman
2023-04-21 2:00 ` [PATCH bpf-next 0/4] fix __retval() being always ignored patchwork-bot+netdevbpf
4 siblings, 0 replies; 6+ messages in thread
From: Eduard Zingerman @ 2023-04-20 23:23 UTC (permalink / raw)
To: bpf, ast; +Cc: andrii, daniel, martin.lau, kernel-team, yhs, Eduard Zingerman
Two test cases:
- "valid read map access into a read-only array 1" and
- "valid read map access into a read-only array 2"
Expect that map_array_ro map is filled with mock data. This logic was
not taken into acount during initial test conversion.
This commit modifies prog_tests/verifier.c entry point for this test
to fill the map.
Fixes: a3c830ae0209 ("selftests/bpf: verifier/array_access.c converted to inline assembly")
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
.../selftests/bpf/prog_tests/verifier.c | 42 +++++++++++++++++--
.../bpf/progs/verifier_array_access.c | 4 +-
2 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index 25bc8958dbfe..7c68d78da9ea 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -44,8 +44,17 @@
#include "verifier_xdp.skel.h"
#include "verifier_xdp_direct_packet_access.skel.h"
+#define MAX_ENTRIES 11
+
+struct test_val {
+ unsigned int index;
+ int foo[MAX_ENTRIES];
+};
+
__maybe_unused
-static void run_tests_aux(const char *skel_name, skel_elf_bytes_fn elf_bytes_factory)
+static void run_tests_aux(const char *skel_name,
+ skel_elf_bytes_fn elf_bytes_factory,
+ pre_execution_cb pre_execution_cb)
{
struct test_loader tester = {};
__u64 old_caps;
@@ -58,6 +67,7 @@ static void run_tests_aux(const char *skel_name, skel_elf_bytes_fn elf_bytes_fac
return;
}
+ test_loader__set_pre_execution_cb(&tester, pre_execution_cb);
test_loader__run_subtests(&tester, skel_name, elf_bytes_factory);
test_loader_fini(&tester);
@@ -66,10 +76,9 @@ static void run_tests_aux(const char *skel_name, skel_elf_bytes_fn elf_bytes_fac
PRINT_FAIL("failed to restore CAP_SYS_ADMIN: %i, %s\n", err, strerror(err));
}
-#define RUN(skel) run_tests_aux(#skel, skel##__elf_bytes)
+#define RUN(skel) run_tests_aux(#skel, skel##__elf_bytes, NULL)
void test_verifier_and(void) { RUN(verifier_and); }
-void test_verifier_array_access(void) { RUN(verifier_array_access); }
void test_verifier_basic_stack(void) { RUN(verifier_basic_stack); }
void test_verifier_bounds_deduction(void) { RUN(verifier_bounds_deduction); }
void test_verifier_bounds_deduction_non_const(void) { RUN(verifier_bounds_deduction_non_const); }
@@ -108,3 +117,30 @@ void test_verifier_var_off(void) { RUN(verifier_var_off); }
void test_verifier_xadd(void) { RUN(verifier_xadd); }
void test_verifier_xdp(void) { RUN(verifier_xdp); }
void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_packet_access); }
+
+static int init_array_access_maps(struct bpf_object *obj)
+{
+ struct bpf_map *array_ro;
+ struct test_val value = {
+ .index = (6 + 1) * sizeof(int),
+ .foo[6] = 0xabcdef12,
+ };
+ int err, key = 0;
+
+ array_ro = bpf_object__find_map_by_name(obj, "map_array_ro");
+ if (!ASSERT_OK_PTR(array_ro, "lookup map_array_ro"))
+ return -EINVAL;
+
+ err = bpf_map_update_elem(bpf_map__fd(array_ro), &key, &value, 0);
+ if (!ASSERT_OK(err, "map_array_ro update"))
+ return err;
+
+ return 0;
+}
+
+void test_verifier_array_access(void)
+{
+ run_tests_aux("verifier_array_access",
+ verifier_array_access__elf_bytes,
+ init_array_access_maps);
+}
diff --git a/tools/testing/selftests/bpf/progs/verifier_array_access.c b/tools/testing/selftests/bpf/progs/verifier_array_access.c
index fceeeef78721..95d7ecc12963 100644
--- a/tools/testing/selftests/bpf/progs/verifier_array_access.c
+++ b/tools/testing/selftests/bpf/progs/verifier_array_access.c
@@ -330,7 +330,7 @@ l0_%=: exit; \
SEC("socket")
__description("valid read map access into a read-only array 1")
-__success __success_unpriv /* __retval(28) temporarily disable */
+__success __success_unpriv __retval(28)
__naked void a_read_only_array_1_1(void)
{
asm volatile (" \
@@ -351,7 +351,7 @@ l0_%=: exit; \
SEC("tc")
__description("valid read map access into a read-only array 2")
-__success /* __retval(65507) temporarily disable */
+__success __retval(65507)
__naked void a_read_only_array_2_1(void)
{
asm volatile (" \
--
2.40.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 0/4] fix __retval() being always ignored
2023-04-20 23:23 [PATCH bpf-next 0/4] fix __retval() being always ignored Eduard Zingerman
` (3 preceding siblings ...)
2023-04-20 23:23 ` [PATCH bpf-next 4/4] selftests/bpf: populate map_array_ro map for verifier_array_access test Eduard Zingerman
@ 2023-04-21 2:00 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-04-21 2:00 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yhs
Hello:
This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Fri, 21 Apr 2023 02:23:13 +0300 you wrote:
> Florian Westphal found a bug in test_loader.c processing of __retval tag.
> Because of this bug the function test_loader.c:do_prog_test_run()
> never executed and all __retval test tags were ignored. See [1].
>
> Fix for this bug uncovers two additional bugs:
> - During test_verifier tests migration to inline assembly (see [2])
> I missed the fact that some tests require maps to contain mock values;
> - Some issue with a new refcounted_kptr test, which causes kernel to
> produce dead lock and refcount saturation warnings when subject to
> libbpf's bpf_test_run_opts().
>
> [...]
Here is the summary with links:
- [bpf-next,1/4] selftests/bpf: disable program test run for progs/refcounted_kptr.c
https://git.kernel.org/bpf/bpf-next/c/7c4b96c00043
- [bpf-next,2/4] selftests/bpf: fix __retval() being always ignored
https://git.kernel.org/bpf/bpf-next/c/7cdddb99e4a6
- [bpf-next,3/4] selftests/bpf: add pre bpf_prog_test_run_opts() callback for test_loader
https://git.kernel.org/bpf/bpf-next/c/5b22f4d1436b
- [bpf-next,4/4] selftests/bpf: populate map_array_ro map for verifier_array_access test
https://git.kernel.org/bpf/bpf-next/c/cbb110bc6672
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-21 2:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-20 23:23 [PATCH bpf-next 0/4] fix __retval() being always ignored Eduard Zingerman
2023-04-20 23:23 ` [PATCH bpf-next 1/4] selftests/bpf: disable program test run for progs/refcounted_kptr.c Eduard Zingerman
2023-04-20 23:23 ` [PATCH bpf-next 2/4] selftests/bpf: fix __retval() being always ignored Eduard Zingerman
2023-04-20 23:23 ` [PATCH bpf-next 3/4] selftests/bpf: add pre bpf_prog_test_run_opts() callback for test_loader Eduard Zingerman
2023-04-20 23:23 ` [PATCH bpf-next 4/4] selftests/bpf: populate map_array_ro map for verifier_array_access test Eduard Zingerman
2023-04-21 2:00 ` [PATCH bpf-next 0/4] fix __retval() being always ignored patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox