On Thu, Jun 6, 2024 at 3:50 AM Cupertino Miranda
<cupertino.miranda@xxxxxxxxxx> wrote:
>
>
> Andrii Nakryiko writes:
>
> > On Mon, Jun 3, 2024 at 8:53 AM Cupertino Miranda
> > <cupertino.miranda@xxxxxxxxxx> wrote:
> >>
> >> This patch changes a few tests to make use of regular expressions such
> >> that the test validation would allow to properly verify the tests when
> >> compiled with GCC.
> >>
> >> signed-off-by: Cupertino Miranda <cupertino.miranda@xxxxxxxxxx>
> >> Cc: jose.marchesi@xxxxxxxxxx
> >> Cc: david.faust@xxxxxxxxxx
> >> Cc: Yonghong Song <yonghong.song@xxxxxxxxx>
> >> Cc: Eduard Zingerman <eddyz87@xxxxxxxxx>
> >> Cc: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> >> ---
> >> tools/testing/selftests/bpf/progs/dynptr_fail.c | 6 +++---
> >> tools/testing/selftests/bpf/progs/exceptions_assert.c | 8 ++++----
> >> tools/testing/selftests/bpf/progs/rbtree_fail.c | 8 ++++----
> >> tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c | 4 ++--
> >> tools/testing/selftests/bpf/progs/verifier_sock.c | 4 ++--
> >> 5 files changed, 15 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> >> index 66a60bfb5867..64cc9d936a13 100644
> >> --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
> >> +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> >> @@ -964,7 +964,7 @@ int dynptr_invalidate_slice_reinit(void *ctx)
> >> * mem_or_null pointers.
> >> */
> >> SEC("?raw_tp")
> >> -__failure __msg("R1 type=scalar expected=percpu_ptr_")
> >> +__failure __regex("R[0-9]+ type=scalar expected=percpu_ptr_")
> >> int dynptr_invalidate_slice_or_null(void *ctx)
> >> {
> >> struct bpf_dynptr ptr;
> >> @@ -982,7 +982,7 @@ int dynptr_invalidate_slice_or_null(void *ctx)
> >>
> >> /* Destruction of dynptr should also any slices obtained from it */
> >> SEC("?raw_tp")
> >> -__failure __msg("R7 invalid mem access 'scalar'")
> >> +__failure __regex("R[0-9]+ invalid mem access 'scalar'")
> >> int dynptr_invalidate_slice_failure(void *ctx)
> >> {
> >> struct bpf_dynptr ptr1;
> >> @@ -1069,7 +1069,7 @@ int dynptr_read_into_slot(void *ctx)
> >>
> >> /* bpf_dynptr_slice()s are read-only and cannot be written to */
> >> SEC("?tc")
> >> -__failure __msg("R0 cannot write into rdonly_mem")
> >> +__failure __regex("R[0-9]+ cannot write into rdonly_mem")
> >> int skb_invalid_slice_write(struct __sk_buff *skb)
> >> {
> >> struct bpf_dynptr ptr;
> >> diff --git a/tools/testing/selftests/bpf/progs/exceptions_assert.c b/tools/testing/selftests/bpf/progs/exceptions_assert.c
> >> index 5e0a1ca96d4e..deb67d198caf 100644
> >> --- a/tools/testing/selftests/bpf/progs/exceptions_assert.c
> >> +++ b/tools/testing/selftests/bpf/progs/exceptions_assert.c
> >> @@ -59,7 +59,7 @@ check_assert(s64, >=, ge_neg, INT_MIN);
> >>
> >> SEC("?tc")
> >> __log_level(2) __failure
> >> -__msg(": R0=0 R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
> >> +__regex(": R0=[^ ]+ R1=ctx() R2=scalar(smin=0xffffffff80000002,smax=smax32=0x7ffffffd,smin32=0x80000002) R10=fp0")
> >
> > curious, what R0 value do we end up with with GCC generated code?
> Oups, this file should have not been committed. Those changes were just
> for experimentation, nothing else. :(
>
> >
> >> int check_assert_range_s64(struct __sk_buff *ctx)
> >> {
> >> struct bpf_sock *sk = ctx->sk;
> >> @@ -75,7 +75,7 @@ int check_assert_range_s64(struct __sk_buff *ctx)
> >>
> >> SEC("?tc")
> >> __log_level(2) __failure
> >> -__msg(": R1=ctx() R2=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
> >> +__regex("R[0-9]=scalar(smin=umin=smin32=umin32=4096,smax=umax=smax32=umax32=8192,var_off=(0x0; 0x3fff))")
> >> int check_assert_range_u64(struct __sk_buff *ctx)
> >> {
> >> u64 num = ctx->len;
> >> @@ -86,7 +86,7 @@ int check_assert_range_u64(struct __sk_buff *ctx)
> >>
> >> SEC("?tc")
> >> __log_level(2) __failure
> >> -__msg(": R0=0 R1=ctx() R2=4096 R10=fp0")
> >> +__regex(": R0=[^ ]+ R1=ctx() R2=4096 R10=fp0")
> >> int check_assert_single_range_s64(struct __sk_buff *ctx)
> >> {
> >> struct bpf_sock *sk = ctx->sk;
> >> @@ -114,7 +114,7 @@ int check_assert_single_range_u64(struct __sk_buff *ctx)
> >>
> >> SEC("?tc")
> >> __log_level(2) __failure
> >> -__msg(": R1=pkt(off=64,r=64) R2=pkt_end() R6=pkt(r=64) R10=fp0")
> >> +__msg("R1=pkt(off=64,r=64)")
> >> int check_assert_generic(struct __sk_buff *ctx)
> >> {
> >> u8 *data_end = (void *)(long)ctx->data_end;
> >> diff --git a/tools/testing/selftests/bpf/progs/rbtree_fail.c b/tools/testing/selftests/bpf/progs/rbtree_fail.c
> >> index 3fecf1c6dfe5..8399304eca72 100644
> >> --- a/tools/testing/selftests/bpf/progs/rbtree_fail.c
> >> +++ b/tools/testing/selftests/bpf/progs/rbtree_fail.c
> >> @@ -29,7 +29,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
> >> long rbtree_api_nolock_add(void *ctx)
> >> {
> >> struct node_data *n;
> >> @@ -43,7 +43,7 @@ long rbtree_api_nolock_add(void *ctx)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
> >> long rbtree_api_nolock_remove(void *ctx)
> >> {
> >> struct node_data *n;
> >> @@ -61,7 +61,7 @@ long rbtree_api_nolock_remove(void *ctx)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("bpf_spin_lock at off=16 must be held for bpf_rb_root")
> >> +__failure __regex("bpf_spin_lock at off=[0-9]+ must be held for bpf_rb_root")
> >> long rbtree_api_nolock_first(void *ctx)
> >> {
> >> bpf_rbtree_first(&groot);
> >> @@ -105,7 +105,7 @@ long rbtree_api_remove_unadded_node(void *ctx)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("Unreleased reference id=3 alloc_insn=10")
> >> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
> >
> > this test definitely should have been written in BPF assembly if we
> > care to check alloc_insn... Otherwise we just care that there is
> > "Unreleased reference" message, we should match on that without
> > hard-coding id and alloc_insn?
> I agree. Unfortunately I see a lot of tests that fall in this category.
> I must admit, most of the time I do not know what is the proper approach
> to correct it.
>
> Also found some tests that made expectations on .bss section data
> layout, expeting a particular variable order.
> For example in prog_tests/core_reloc.c, when it maps .bss and assigns it
> to data.
I haven't checked every single one, but I think most (if not all) of
these progs/test_core_reloc_*.c tests (which are what is being tested
in prog_tests/core_reloc.c) are structured with a singular variable in
.bss. And then the variable type is some well-defined struct type. As
Alexei pointed out, compiler is not allowed to just arbitrarily
reorder fields within a struct, unless randomization is enabled with
an extra attribute (which we do not use).
So if you have specific cases where something isn't correct, let's go
over them, but I think prog_tests/core_reloc.c should be fine.
> GCC will allocate variables in a different order then clang and when
> comparing content is not where comparisson is expecting.
>
> Some other test, would expect that struct fields would be in some
> particular order, while GCC decides it would benefit from reordering
> struct fields. For passing those tests I need to disable GCC
> optimization that would make this reordering.
> However reordering of the struct fields is a perfectly valid
Nope, it's not.
As mentioned, struct layout is effectively an ABI, so the compiler
cannot just reorder it. Lots and lots of things would be broken if
this was true for C programs.
> optimization. Maybe disabling for this tests is acceptable, but in any
> case the test itself is prune for any future optimizations that can be
> added to GCC or CLANG.
> This happened in progs/test_core_autosize.c for example.
We probably should rewrite such tests that have to deal with
.bss/.data to BPF skeletons, I think they were written before BPF
skeletons were available.
>
> Anyway, just a couple of examples of tests that were made very tight to
> compiler.
>
> >
> >> long rbtree_api_remove_no_drop(void *ctx)
> >> {
> >> struct bpf_rb_node *res;
> >> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
> >> index 1553b9c16aa7..f8d4b7cfcd68 100644
> >> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
> >> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c
> >> @@ -32,7 +32,7 @@ static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("Unreleased reference id=4 alloc_insn=21")
> >> +__failure __regex("Unreleased reference id=4 alloc_insn=[0-9]+")
> >
> > same, relying on ID and alloc_insns in tests written in C is super fragile.
> >
> >> long rbtree_refcounted_node_ref_escapes(void *ctx)
> >> {
> >> struct node_acquire *n, *m;
> >> @@ -73,7 +73,7 @@ long refcount_acquire_maybe_null(void *ctx)
> >> }
> >>
> >> SEC("?tc")
> >> -__failure __msg("Unreleased reference id=3 alloc_insn=9")
> >> +__failure __regex("Unreleased reference id=3 alloc_insn=[0-9]+")
> >> long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx)
> >
> > ditto
> >
> >> {
> >> struct node_acquire *n, *m;
> >> diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c
> >> index ee76b51005ab..450b57933c79 100644
> >> --- a/tools/testing/selftests/bpf/progs/verifier_sock.c
> >> +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c
> >> @@ -799,7 +799,7 @@ l0_%=: r0 = *(u32*)(r0 + %[bpf_xdp_sock_queue_id]); \
> >>
> >> SEC("sk_skb")
> >> __description("bpf_map_lookup_elem(sockmap, &key)")
> >> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
> >> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
> >
> > same here and below
> >
> >
> >> __naked void map_lookup_elem_sockmap_key(void)
> >> {
> >> asm volatile (" \
> >> @@ -819,7 +819,7 @@ __naked void map_lookup_elem_sockmap_key(void)
> >>
> >> SEC("sk_skb")
> >> __description("bpf_map_lookup_elem(sockhash, &key)")
> >> -__failure __msg("Unreleased reference id=2 alloc_insn=6")
> >> +__failure __regex("Unreleased reference id=2 alloc_insn=[0-9]+")
> >> __naked void map_lookup_elem_sockhash_key(void)
> >> {
> >> asm volatile (" \
> >> --
> >> 2.39.2
> >>
