| Message ID | e4d14fb5f07145ff4a367cc01d8dcf6c82581c88.1701462010.git.dxu@dxuuu.xyz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add bpf_xdp_get_xfrm_state() kfunc | expand |
On Fri, Dec 1, 2023 at 12:24 PM Daniel Xu <dxu@dxuuu.xyz> wrote: > > Add some tests that exercise BPF_CORE_WRITE_BITFIELD() macro. Since some > non-trivial bit fiddling is going on, make sure various edge cases (such > as adjacent bitfields and bitfields at the edge of structs) are > exercised. > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > --- > .../selftests/bpf/prog_tests/verifier.c | 2 + > .../bpf/progs/verifier_bitfield_write.c | 100 ++++++++++++++++++ > 2 files changed, 102 insertions(+) > create mode 100644 tools/testing/selftests/bpf/progs/verifier_bitfield_write.c > LGTM, but I'm not sure why we need all those __failure_unpriv, see below. Regardless: Acked-by: Andrii Nakryiko <andrii@kernel.org> > diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c > index 5cfa7a6316b6..67b4948865a3 100644 > --- a/tools/testing/selftests/bpf/prog_tests/verifier.c > +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c > @@ -6,6 +6,7 @@ > #include "verifier_and.skel.h" > #include "verifier_array_access.skel.h" > #include "verifier_basic_stack.skel.h" > +#include "verifier_bitfield_write.skel.h" > #include "verifier_bounds.skel.h" > #include "verifier_bounds_deduction.skel.h" > #include "verifier_bounds_deduction_non_const.skel.h" > @@ -115,6 +116,7 @@ static void run_tests_aux(const char *skel_name, > > void test_verifier_and(void) { RUN(verifier_and); } > void test_verifier_basic_stack(void) { RUN(verifier_basic_stack); } > +void test_verifier_bitfield_write(void) { RUN(verifier_bitfield_write); } > void test_verifier_bounds(void) { RUN(verifier_bounds); } > void test_verifier_bounds_deduction(void) { RUN(verifier_bounds_deduction); } > void test_verifier_bounds_deduction_non_const(void) { RUN(verifier_bounds_deduction_non_const); } > diff --git a/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c b/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c > new file mode 100644 > index 000000000000..8fe355a19ba6 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c > @@ -0,0 +1,100 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include <linux/bpf.h> > +#include <stdint.h> > + > +#include <bpf/bpf_helpers.h> > +#include <bpf/bpf_core_read.h> > + > +#include "bpf_misc.h" > + > +struct core_reloc_bitfields { > + /* unsigned bitfields */ > + uint8_t ub1: 1; > + uint8_t ub2: 2; > + uint32_t ub7: 7; > + /* signed bitfields */ > + int8_t sb4: 4; > + int32_t sb20: 20; > + /* non-bitfields */ > + uint32_t u32; > + int32_t s32; > +} __attribute__((preserve_access_index)); > + > +SEC("tc") > +__description("single CO-RE bitfield roundtrip") > +__btf_path("btf__core_reloc_bitfields.bpf.o") > +__success __failure_unpriv do we want __failure_unpriv at all? Is this failure related to *bitfield* logic at all? > +__retval(3) > +int single_field_roundtrip(struct __sk_buff *ctx) > +{ > + struct core_reloc_bitfields bitfields; > + > + __builtin_memset(&bitfields, 0, sizeof(bitfields)); > + BPF_CORE_WRITE_BITFIELD(&bitfields, ub2, 3); > + return BPF_CORE_READ_BITFIELD(&bitfields, ub2); > +} > + [...]
Hi Andrii, On Fri, Dec 01, 2023 at 03:52:25PM -0800, Andrii Nakryiko wrote: > On Fri, Dec 1, 2023 at 12:24 PM Daniel Xu <dxu@dxuuu.xyz> wrote: > > > > Add some tests that exercise BPF_CORE_WRITE_BITFIELD() macro. Since some > > non-trivial bit fiddling is going on, make sure various edge cases (such > > as adjacent bitfields and bitfields at the edge of structs) are > > exercised. > > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> > > --- > > .../selftests/bpf/prog_tests/verifier.c | 2 + > > .../bpf/progs/verifier_bitfield_write.c | 100 ++++++++++++++++++ > > 2 files changed, 102 insertions(+) > > create mode 100644 tools/testing/selftests/bpf/progs/verifier_bitfield_write.c > > > > LGTM, but I'm not sure why we need all those __failure_unpriv, see > below. Regardless: > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c > > index 5cfa7a6316b6..67b4948865a3 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/verifier.c > > +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c > > @@ -6,6 +6,7 @@ > > #include "verifier_and.skel.h" > > #include "verifier_array_access.skel.h" > > #include "verifier_basic_stack.skel.h" > > +#include "verifier_bitfield_write.skel.h" > > #include "verifier_bounds.skel.h" > > #include "verifier_bounds_deduction.skel.h" > > #include "verifier_bounds_deduction_non_const.skel.h" > > @@ -115,6 +116,7 @@ static void run_tests_aux(const char *skel_name, > > > > void test_verifier_and(void) { RUN(verifier_and); } > > void test_verifier_basic_stack(void) { RUN(verifier_basic_stack); } > > +void test_verifier_bitfield_write(void) { RUN(verifier_bitfield_write); } > > void test_verifier_bounds(void) { RUN(verifier_bounds); } > > void test_verifier_bounds_deduction(void) { RUN(verifier_bounds_deduction); } > > void test_verifier_bounds_deduction_non_const(void) { RUN(verifier_bounds_deduction_non_const); } > > diff --git a/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c b/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c > > new file mode 100644 > > index 000000000000..8fe355a19ba6 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c > > @@ -0,0 +1,100 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +#include <linux/bpf.h> > > +#include <stdint.h> > > + > > +#include <bpf/bpf_helpers.h> > > +#include <bpf/bpf_core_read.h> > > + > > +#include "bpf_misc.h" > > + > > +struct core_reloc_bitfields { > > + /* unsigned bitfields */ > > + uint8_t ub1: 1; > > + uint8_t ub2: 2; > > + uint32_t ub7: 7; > > + /* signed bitfields */ > > + int8_t sb4: 4; > > + int32_t sb20: 20; > > + /* non-bitfields */ > > + uint32_t u32; > > + int32_t s32; > > +} __attribute__((preserve_access_index)); > > + > > +SEC("tc") > > +__description("single CO-RE bitfield roundtrip") > > +__btf_path("btf__core_reloc_bitfields.bpf.o") > > +__success __failure_unpriv > > do we want __failure_unpriv at all? Is this failure related to > *bitfield* logic at all? Oh, I pre-emptively added it. From the docs, I thought __failure_unpriv meant "don't try to load this as an unprivileged used cuz it'll fail". And since I used the tc hook, I figured it'd fail. Removing the annotation doesn't seem to do anything bad so I'll drop it for v4. [...] Thanks, Daniel
On Fri, 2023-12-01 at 17:10 -0700, Daniel Xu wrote: [...] > > > +SEC("tc") > > > +__description("single CO-RE bitfield roundtrip") > > > +__btf_path("btf__core_reloc_bitfields.bpf.o") > > > +__success __failure_unpriv > > > > do we want __failure_unpriv at all? Is this failure related to > > *bitfield* logic at all? > > Oh, I pre-emptively added it. From the docs, I thought __failure_unpriv > meant "don't try to load this as an unprivileged used cuz it'll fail". > And since I used the tc hook, I figured it'd fail. Actually it means: "try to load as unprivileged user and expect failure, report error on successful load". In general, the meaning of "___xxx" and "___xxx_unpriv" annotations is identical, except first instructs to run the test in privileged mode, while second instructs to run test in unprivileged mode: - if only annotations w/o "*_unpriv" suffix are present the test would be executed as privileged; - if only annotations with "*_unpriv" suffix are present the test would be executed as unprivileged; - if both kinds of annotations are present the test would be executed in both modes. [...]
diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c index 5cfa7a6316b6..67b4948865a3 100644 --- a/tools/testing/selftests/bpf/prog_tests/verifier.c +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c @@ -6,6 +6,7 @@ #include "verifier_and.skel.h" #include "verifier_array_access.skel.h" #include "verifier_basic_stack.skel.h" +#include "verifier_bitfield_write.skel.h" #include "verifier_bounds.skel.h" #include "verifier_bounds_deduction.skel.h" #include "verifier_bounds_deduction_non_const.skel.h" @@ -115,6 +116,7 @@ static void run_tests_aux(const char *skel_name, void test_verifier_and(void) { RUN(verifier_and); } void test_verifier_basic_stack(void) { RUN(verifier_basic_stack); } +void test_verifier_bitfield_write(void) { RUN(verifier_bitfield_write); } void test_verifier_bounds(void) { RUN(verifier_bounds); } void test_verifier_bounds_deduction(void) { RUN(verifier_bounds_deduction); } void test_verifier_bounds_deduction_non_const(void) { RUN(verifier_bounds_deduction_non_const); } diff --git a/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c b/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c new file mode 100644 index 000000000000..8fe355a19ba6 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/bpf.h> +#include <stdint.h> + +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_core_read.h> + +#include "bpf_misc.h" + +struct core_reloc_bitfields { + /* unsigned bitfields */ + uint8_t ub1: 1; + uint8_t ub2: 2; + uint32_t ub7: 7; + /* signed bitfields */ + int8_t sb4: 4; + int32_t sb20: 20; + /* non-bitfields */ + uint32_t u32; + int32_t s32; +} __attribute__((preserve_access_index)); + +SEC("tc") +__description("single CO-RE bitfield roundtrip") +__btf_path("btf__core_reloc_bitfields.bpf.o") +__success __failure_unpriv +__retval(3) +int single_field_roundtrip(struct __sk_buff *ctx) +{ + struct core_reloc_bitfields bitfields; + + __builtin_memset(&bitfields, 0, sizeof(bitfields)); + BPF_CORE_WRITE_BITFIELD(&bitfields, ub2, 3); + return BPF_CORE_READ_BITFIELD(&bitfields, ub2); +} + +SEC("tc") +__description("multiple CO-RE bitfield roundtrip") +__btf_path("btf__core_reloc_bitfields.bpf.o") +__success __failure_unpriv +__retval(0x3FD) +int multiple_field_roundtrip(struct __sk_buff *ctx) +{ + struct core_reloc_bitfields bitfields; + uint8_t ub2; + int8_t sb4; + + __builtin_memset(&bitfields, 0, sizeof(bitfields)); + BPF_CORE_WRITE_BITFIELD(&bitfields, ub2, 1); + BPF_CORE_WRITE_BITFIELD(&bitfields, sb4, -1); + + ub2 = BPF_CORE_READ_BITFIELD(&bitfields, ub2); + sb4 = BPF_CORE_READ_BITFIELD(&bitfields, sb4); + + return (((uint8_t)sb4) << 2) | ub2; +} + +SEC("tc") +__description("adjacent CO-RE bitfield roundtrip") +__btf_path("btf__core_reloc_bitfields.bpf.o") +__success __failure_unpriv +__retval(7) +int adjacent_field_roundtrip(struct __sk_buff *ctx) +{ + struct core_reloc_bitfields bitfields; + uint8_t ub1, ub2; + + __builtin_memset(&bitfields, 0, sizeof(bitfields)); + BPF_CORE_WRITE_BITFIELD(&bitfields, ub1, 1); + BPF_CORE_WRITE_BITFIELD(&bitfields, ub2, 3); + + ub1 = BPF_CORE_READ_BITFIELD(&bitfields, ub1); + ub2 = BPF_CORE_READ_BITFIELD(&bitfields, ub2); + + return (ub2 << 1) | ub1; +} + +SEC("tc") +__description("multibyte CO-RE bitfield roundtrip") +__btf_path("btf__core_reloc_bitfields.bpf.o") +__success __failure_unpriv +__retval(0x21) +int multibyte_field_roundtrip(struct __sk_buff *ctx) +{ + struct core_reloc_bitfields bitfields; + uint32_t ub7; + uint8_t ub1; + + __builtin_memset(&bitfields, 0, sizeof(bitfields)); + BPF_CORE_WRITE_BITFIELD(&bitfields, ub1, 1); + BPF_CORE_WRITE_BITFIELD(&bitfields, ub7, 16); + + ub1 = BPF_CORE_READ_BITFIELD(&bitfields, ub1); + ub7 = BPF_CORE_READ_BITFIELD(&bitfields, ub7); + + return (ub7 << 1) | ub1; +} + +char _license[] SEC("license") = "GPL";
Add some tests that exercise BPF_CORE_WRITE_BITFIELD() macro. Since some non-trivial bit fiddling is going on, make sure various edge cases (such as adjacent bitfields and bitfields at the edge of structs) are exercised. Signed-off-by: Daniel Xu <dxu@dxuuu.xyz> --- .../selftests/bpf/prog_tests/verifier.c | 2 + .../bpf/progs/verifier_bitfield_write.c | 100 ++++++++++++++++++ 2 files changed, 102 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/verifier_bitfield_write.c