* [PATCH bpf v3 0/2] selftests/bpf: fix endianness issues in test_sysctl
@ 2019-08-28 13:22 Ilya Leoshkevich
2019-08-28 13:22 ` [PATCH bpf v3 1/2] selftests/bpf: introduce bpf_cpu_to_be64 and bpf_be64_to_cpu Ilya Leoshkevich
2019-08-28 13:22 ` [PATCH bpf v3 2/2] selftests/bpf: fix endianness issues in test_sysctl Ilya Leoshkevich
0 siblings, 2 replies; 6+ messages in thread
From: Ilya Leoshkevich @ 2019-08-28 13:22 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov
Cc: bpf, Yonghong Song, Andrey Ignatov, Heiko Carstens, Vasily Gorbik,
Ilya Leoshkevich
The first patch is a preparatory commit, which introduces 64-bit
endianness conversion functions.
The second patch is an actual fix.
v1->v2: Use bpf_ntohl and bpf_be64_to_cpu, drop __bpf_le64_to_cpu.
v2->v3: Split bpf_be64_to_cpu introduction into a separate patch.
Use the new functions in test_lwt_seg6local.c and
test_seg6_loop.c.
Ilya Leoshkevich (2):
selftests/bpf: introduce bpf_cpu_to_be64 and bpf_be64_to_cpu
selftests/bpf: fix endianness issues in test_sysctl
tools/testing/selftests/bpf/bpf_endian.h | 14 ++
.../selftests/bpf/progs/test_lwt_seg6local.c | 16 +--
.../selftests/bpf/progs/test_seg6_loop.c | 8 +-
tools/testing/selftests/bpf/test_sysctl.c | 130 ++++++++++++------
4 files changed, 107 insertions(+), 61 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH bpf v3 1/2] selftests/bpf: introduce bpf_cpu_to_be64 and bpf_be64_to_cpu 2019-08-28 13:22 [PATCH bpf v3 0/2] selftests/bpf: fix endianness issues in test_sysctl Ilya Leoshkevich @ 2019-08-28 13:22 ` Ilya Leoshkevich 2019-08-29 21:46 ` Song Liu 2019-08-28 13:22 ` [PATCH bpf v3 2/2] selftests/bpf: fix endianness issues in test_sysctl Ilya Leoshkevich 1 sibling, 1 reply; 6+ messages in thread From: Ilya Leoshkevich @ 2019-08-28 13:22 UTC (permalink / raw) To: Daniel Borkmann, Alexei Starovoitov Cc: bpf, Yonghong Song, Andrey Ignatov, Heiko Carstens, Vasily Gorbik, Ilya Leoshkevich test_lwt_seg6local and test_seg6_loop use custom 64-bit endianness conversion macros. Centralize their definitions in bpf_endian.h in order to reduce code duplication. This will also be useful when bpf_endian.h is promoted to an offical libbpf header. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- tools/testing/selftests/bpf/bpf_endian.h | 14 ++++++++++++++ .../selftests/bpf/progs/test_lwt_seg6local.c | 16 ++++++---------- .../testing/selftests/bpf/progs/test_seg6_loop.c | 8 ++------ 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h index 05f036df8a4c..f92f29c00d50 100644 --- a/tools/testing/selftests/bpf/bpf_endian.h +++ b/tools/testing/selftests/bpf/bpf_endian.h @@ -29,6 +29,10 @@ # define __bpf_htonl(x) __builtin_bswap32(x) # define __bpf_constant_ntohl(x) ___constant_swab32(x) # define __bpf_constant_htonl(x) ___constant_swab32(x) +# define __bpf_be64_to_cpu(x) __builtin_bswap64(x) +# define __bpf_cpu_to_be64(x) __builtin_bswap64(x) +# define __bpf_constant_be64_to_cpu(x) ___constant_swab64(x) +# define __bpf_constant_cpu_to_be64(x) ___constant_swab64(x) #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ # define __bpf_ntohs(x) (x) # define __bpf_htons(x) (x) @@ -38,6 +42,10 @@ # define __bpf_htonl(x) (x) # define __bpf_constant_ntohl(x) (x) # define __bpf_constant_htonl(x) (x) +# define __bpf_be64_to_cpu(x) (x) +# define __bpf_cpu_to_be64(x) (x) +# define __bpf_constant_be64_to_cpu(x) (x) +# define __bpf_constant_cpu_to_be64(x) (x) #else # error "Fix your compiler's __BYTE_ORDER__?!" #endif @@ -54,5 +62,11 @@ #define bpf_ntohl(x) \ (__builtin_constant_p(x) ? \ __bpf_constant_ntohl(x) : __bpf_ntohl(x)) +#define bpf_cpu_to_be64(x) \ + (__builtin_constant_p(x) ? \ + __bpf_constant_cpu_to_be64(x) : __bpf_cpu_to_be64(x)) +#define bpf_be64_to_cpu(x) \ + (__builtin_constant_p(x) ? \ + __bpf_constant_be64_to_cpu(x) : __bpf_be64_to_cpu(x)) #endif /* __BPF_ENDIAN__ */ diff --git a/tools/testing/selftests/bpf/progs/test_lwt_seg6local.c b/tools/testing/selftests/bpf/progs/test_lwt_seg6local.c index a334a0e882e4..5e86afb8fb31 100644 --- a/tools/testing/selftests/bpf/progs/test_lwt_seg6local.c +++ b/tools/testing/selftests/bpf/progs/test_lwt_seg6local.c @@ -12,10 +12,6 @@ #define SR6_FLAG_ALERT (1 << 4) -#define htonll(x) ((bpf_htonl(1)) == 1 ? (x) : ((uint64_t)bpf_htonl((x) & \ - 0xFFFFFFFF) << 32) | bpf_htonl((x) >> 32)) -#define ntohll(x) ((bpf_ntohl(1)) == 1 ? (x) : ((uint64_t)bpf_ntohl((x) & \ - 0xFFFFFFFF) << 32) | bpf_ntohl((x) >> 32)) #define BPF_PACKET_HEADER __attribute__((packed)) struct ip6_t { @@ -276,8 +272,8 @@ int has_egr_tlv(struct __sk_buff *skb, struct ip6_srh_t *srh) return 0; // check if egress TLV value is correct - if (ntohll(egr_addr.hi) == 0xfd00000000000000 && - ntohll(egr_addr.lo) == 0x4) + if (bpf_be64_to_cpu(egr_addr.hi) == 0xfd00000000000000 && + bpf_be64_to_cpu(egr_addr.lo) == 0x4) return 1; } @@ -308,8 +304,8 @@ int __encap_srh(struct __sk_buff *skb) #pragma clang loop unroll(full) for (unsigned long long lo = 0; lo < 4; lo++) { - seg->lo = htonll(4 - lo); - seg->hi = htonll(hi); + seg->lo = bpf_cpu_to_be64(4 - lo); + seg->hi = bpf_cpu_to_be64(hi); seg = (struct ip6_addr_t *)((char *)seg + sizeof(*seg)); } @@ -349,8 +345,8 @@ int __add_egr_x(struct __sk_buff *skb) if (err) return BPF_DROP; - addr.lo = htonll(lo); - addr.hi = htonll(hi); + addr.lo = bpf_cpu_to_be64(lo); + addr.hi = bpf_cpu_to_be64(hi); err = bpf_lwt_seg6_action(skb, SEG6_LOCAL_ACTION_END_X, (void *)&addr, sizeof(addr)); if (err) diff --git a/tools/testing/selftests/bpf/progs/test_seg6_loop.c b/tools/testing/selftests/bpf/progs/test_seg6_loop.c index 1dbe1d4d467e..c4d104428643 100644 --- a/tools/testing/selftests/bpf/progs/test_seg6_loop.c +++ b/tools/testing/selftests/bpf/progs/test_seg6_loop.c @@ -12,10 +12,6 @@ #define SR6_FLAG_ALERT (1 << 4) -#define htonll(x) ((bpf_htonl(1)) == 1 ? (x) : ((uint64_t)bpf_htonl((x) & \ - 0xFFFFFFFF) << 32) | bpf_htonl((x) >> 32)) -#define ntohll(x) ((bpf_ntohl(1)) == 1 ? (x) : ((uint64_t)bpf_ntohl((x) & \ - 0xFFFFFFFF) << 32) | bpf_ntohl((x) >> 32)) #define BPF_PACKET_HEADER __attribute__((packed)) struct ip6_t { @@ -251,8 +247,8 @@ int __add_egr_x(struct __sk_buff *skb) if (err) return BPF_DROP; - addr.lo = htonll(lo); - addr.hi = htonll(hi); + addr.lo = bpf_cpu_to_be64(lo); + addr.hi = bpf_cpu_to_be64(hi); err = bpf_lwt_seg6_action(skb, SEG6_LOCAL_ACTION_END_X, (void *)&addr, sizeof(addr)); if (err) -- 2.21.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v3 1/2] selftests/bpf: introduce bpf_cpu_to_be64 and bpf_be64_to_cpu 2019-08-28 13:22 ` [PATCH bpf v3 1/2] selftests/bpf: introduce bpf_cpu_to_be64 and bpf_be64_to_cpu Ilya Leoshkevich @ 2019-08-29 21:46 ` Song Liu 0 siblings, 0 replies; 6+ messages in thread From: Song Liu @ 2019-08-29 21:46 UTC (permalink / raw) To: Ilya Leoshkevich Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Yonghong Song, Andrey Ignatov, Heiko Carstens, Vasily Gorbik On Wed, Aug 28, 2019 at 6:23 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > test_lwt_seg6local and test_seg6_loop use custom 64-bit endianness > conversion macros. Centralize their definitions in bpf_endian.h in order > to reduce code duplication. This will also be useful when bpf_endian.h > is promoted to an offical libbpf header. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> Acked-by: Song Liu <songliubraving@fb.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf v3 2/2] selftests/bpf: fix endianness issues in test_sysctl 2019-08-28 13:22 [PATCH bpf v3 0/2] selftests/bpf: fix endianness issues in test_sysctl Ilya Leoshkevich 2019-08-28 13:22 ` [PATCH bpf v3 1/2] selftests/bpf: introduce bpf_cpu_to_be64 and bpf_be64_to_cpu Ilya Leoshkevich @ 2019-08-28 13:22 ` Ilya Leoshkevich 2019-08-29 21:53 ` Song Liu 1 sibling, 1 reply; 6+ messages in thread From: Ilya Leoshkevich @ 2019-08-28 13:22 UTC (permalink / raw) To: Daniel Borkmann, Alexei Starovoitov Cc: bpf, Yonghong Song, Andrey Ignatov, Heiko Carstens, Vasily Gorbik, Ilya Leoshkevich A lot of test_sysctl sub-tests fail due to handling strings as a bunch of immediate values in a little-endian-specific manner. Fix by wrapping all immediates in bpf_ntohl and the new bpf_be64_to_cpu. Also, sometimes tests fail because sysctl() unexpectedly succeeds with an inappropriate "Unexpected failure" message and a random errno. Zero out errno before calling sysctl() and replace the message with "Unexpected success". Fixes: 1f5fa9ab6e2e ("selftests/bpf: Test BPF_CGROUP_SYSCTL") Fixes: 9a1027e52535 ("selftests/bpf: Test file_pos field in bpf_sysctl ctx") Fixes: 6041c67f28d8 ("selftests/bpf: Test bpf_sysctl_get_name helper") Fixes: 11ff34f74e32 ("selftests/bpf: Test sysctl_get_current_value helper") Fixes: 786047dd08de ("selftests/bpf: Test bpf_sysctl_{get,set}_new_value helpers") Fixes: 8549ddc832d6 ("selftests/bpf: Test bpf_strtol and bpf_strtoul helpers") Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- tools/testing/selftests/bpf/test_sysctl.c | 130 ++++++++++++++-------- 1 file changed, 85 insertions(+), 45 deletions(-) diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c index a3bebd7c68dd..b8c6e31645cb 100644 --- a/tools/testing/selftests/bpf/test_sysctl.c +++ b/tools/testing/selftests/bpf/test_sysctl.c @@ -13,6 +13,7 @@ #include <bpf/bpf.h> #include <bpf/libbpf.h> +#include "bpf_endian.h" #include "bpf_rlimit.h" #include "bpf_util.h" #include "cgroup_helpers.h" @@ -100,7 +101,7 @@ static struct sysctl_test tests[] = { .descr = "ctx:write sysctl:write read ok", .insns = { /* If (write) */ - BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_1, + BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_1, offsetof(struct bpf_sysctl, write)), BPF_JMP_IMM(BPF_JNE, BPF_REG_7, 1, 2), @@ -214,7 +215,8 @@ static struct sysctl_test tests[] = { /* if (ret == expected && */ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, sizeof("tcp_mem") - 1, 6), /* buf == "tcp_mem\0") */ - BPF_LD_IMM64(BPF_REG_8, 0x006d656d5f706374ULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x7463705f6d656d00ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), @@ -255,7 +257,8 @@ static struct sysctl_test tests[] = { BPF_JMP_IMM(BPF_JNE, BPF_REG_0, -E2BIG, 6), /* buf[0:7] == "tcp_me\0") */ - BPF_LD_IMM64(BPF_REG_8, 0x00656d5f706374ULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x7463705f6d650000ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), @@ -298,12 +301,14 @@ static struct sysctl_test tests[] = { BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 16, 14), /* buf[0:8] == "net/ipv4" && */ - BPF_LD_IMM64(BPF_REG_8, 0x347670692f74656eULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x6e65742f69707634ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 10), /* buf[8:16] == "/tcp_mem" && */ - BPF_LD_IMM64(BPF_REG_8, 0x6d656d5f7063742fULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x2f7463705f6d656dULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 8), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 6), @@ -350,12 +355,14 @@ static struct sysctl_test tests[] = { BPF_JMP_IMM(BPF_JNE, BPF_REG_0, -E2BIG, 10), /* buf[0:8] == "net/ipv4" && */ - BPF_LD_IMM64(BPF_REG_8, 0x347670692f74656eULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x6e65742f69707634ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 6), /* buf[8:16] == "/tcp_me\0") */ - BPF_LD_IMM64(BPF_REG_8, 0x00656d5f7063742fULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x2f7463705f6d6500ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 8), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), @@ -396,7 +403,8 @@ static struct sysctl_test tests[] = { BPF_JMP_IMM(BPF_JNE, BPF_REG_0, -E2BIG, 6), /* buf[0:8] == "net/ip\0") */ - BPF_LD_IMM64(BPF_REG_8, 0x000070692f74656eULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x6e65742f69700000ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), @@ -431,7 +439,8 @@ static struct sysctl_test tests[] = { BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 6, 6), /* buf[0:6] == "Linux\n\0") */ - BPF_LD_IMM64(BPF_REG_8, 0x000a78756e694cULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x4c696e75780a0000ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), @@ -469,7 +478,8 @@ static struct sysctl_test tests[] = { BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 6, 6), /* buf[0:6] == "Linux\n\0") */ - BPF_LD_IMM64(BPF_REG_8, 0x000a78756e694cULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x4c696e75780a0000ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), @@ -507,7 +517,8 @@ static struct sysctl_test tests[] = { BPF_JMP_IMM(BPF_JNE, BPF_REG_0, -E2BIG, 6), /* buf[0:6] == "Linux\0") */ - BPF_LD_IMM64(BPF_REG_8, 0x000078756e694cULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x4c696e7578000000ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), @@ -650,7 +661,8 @@ static struct sysctl_test tests[] = { /* buf[0:4] == "606\0") */ BPF_LDX_MEM(BPF_W, BPF_REG_9, BPF_REG_7, 0), - BPF_JMP_IMM(BPF_JNE, BPF_REG_9, 0x00363036, 2), + BPF_JMP_IMM(BPF_JNE, BPF_REG_9, + bpf_ntohl(0x36303600), 2), /* return DENY; */ BPF_MOV64_IMM(BPF_REG_0, 0), @@ -685,17 +697,20 @@ static struct sysctl_test tests[] = { BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 23, 14), /* buf[0:8] == "3000000 " && */ - BPF_LD_IMM64(BPF_REG_8, 0x2030303030303033ULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x3330303030303020ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 10), /* buf[8:16] == "4000000 " && */ - BPF_LD_IMM64(BPF_REG_8, 0x2030303030303034ULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x3430303030303020ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 8), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 6), /* buf[16:24] == "6000000\0") */ - BPF_LD_IMM64(BPF_REG_8, 0x0030303030303036ULL), + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( + 0x3630303030303000ULL)), BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 16), BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), @@ -735,7 +750,8 @@ static struct sysctl_test tests[] = { /* buf[0:3] == "60\0") */ BPF_LDX_MEM(BPF_W, BPF_REG_9, BPF_REG_7, 0), - BPF_JMP_IMM(BPF_JNE, BPF_REG_9, 0x003036, 2), + BPF_JMP_IMM(BPF_JNE, BPF_REG_9, + bpf_ntohl(0x36300000), 2), /* return DENY; */ BPF_MOV64_IMM(BPF_REG_0, 0), @@ -757,7 +773,8 @@ static struct sysctl_test tests[] = { /* sysctl_set_new_value arg2 (buf) */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), - BPF_MOV64_IMM(BPF_REG_0, 0x00303036), + BPF_MOV64_IMM(BPF_REG_0, + bpf_ntohl(0x36303000)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), BPF_MOV64_REG(BPF_REG_2, BPF_REG_7), @@ -791,7 +808,7 @@ static struct sysctl_test tests[] = { /* sysctl_set_new_value arg2 (buf) */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), - BPF_MOV64_IMM(BPF_REG_0, FIXUP_SYSCTL_VALUE), + BPF_LD_IMM64(BPF_REG_0, FIXUP_SYSCTL_VALUE), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), BPF_MOV64_REG(BPF_REG_2, BPF_REG_7), @@ -825,8 +842,9 @@ static struct sysctl_test tests[] = { /* arg1 (buf) */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), - BPF_MOV64_IMM(BPF_REG_0, 0x00303036), - BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), + BPF_MOV64_IMM(BPF_REG_0, + bpf_ntohl(0x36303000)), + BPF_STX_MEM(BPF_W, BPF_REG_7, BPF_REG_0, 0), BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), @@ -869,7 +887,8 @@ static struct sysctl_test tests[] = { BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), /* "600 602\0" */ - BPF_LD_IMM64(BPF_REG_0, 0x0032303620303036ULL), + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( + 0x3630302036303200ULL)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), @@ -937,7 +956,8 @@ static struct sysctl_test tests[] = { /* arg1 (buf) */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), - BPF_MOV64_IMM(BPF_REG_0, 0x00303036), + BPF_MOV64_IMM(BPF_REG_0, + bpf_ntohl(0x36303000)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), @@ -969,8 +989,9 @@ static struct sysctl_test tests[] = { /* arg1 (buf) */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), - BPF_MOV64_IMM(BPF_REG_0, 0x00373730), - BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), + BPF_MOV64_IMM(BPF_REG_0, + bpf_ntohl(0x30373700)), + BPF_STX_MEM(BPF_W, BPF_REG_7, BPF_REG_0, 0), BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), @@ -1012,7 +1033,8 @@ static struct sysctl_test tests[] = { /* arg1 (buf) */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), - BPF_MOV64_IMM(BPF_REG_0, 0x00303036), + BPF_MOV64_IMM(BPF_REG_0, + bpf_ntohl(0x36303000)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), @@ -1052,7 +1074,8 @@ static struct sysctl_test tests[] = { /* arg1 (buf) */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), - BPF_MOV64_IMM(BPF_REG_0, 0x090a0c0d), + BPF_MOV64_IMM(BPF_REG_0, + bpf_ntohl(0x0d0c0a09)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), @@ -1092,7 +1115,9 @@ static struct sysctl_test tests[] = { /* arg1 (buf) */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), - BPF_MOV64_IMM(BPF_REG_0, 0x00362d0a), /* " -6\0" */ + /* " -6\0" */ + BPF_MOV64_IMM(BPF_REG_0, + bpf_ntohl(0x0a2d3600)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), @@ -1132,8 +1157,10 @@ static struct sysctl_test tests[] = { /* arg1 (buf) */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), - BPF_MOV64_IMM(BPF_REG_0, 0x00362d0a), /* " -6\0" */ - BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), + /* " -6\0" */ + BPF_MOV64_IMM(BPF_REG_0, + bpf_ntohl(0x0a2d3600)), + BPF_STX_MEM(BPF_W, BPF_REG_7, BPF_REG_0, 0), BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), @@ -1175,8 +1202,10 @@ static struct sysctl_test tests[] = { /* arg1 (buf) */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), - BPF_MOV64_IMM(BPF_REG_0, 0x65667830), /* "0xfe" */ - BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), + /* "0xfe" */ + BPF_MOV64_IMM(BPF_REG_0, + bpf_ntohl(0x30786665)), + BPF_STX_MEM(BPF_W, BPF_REG_7, BPF_REG_0, 0), BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), @@ -1218,11 +1247,14 @@ static struct sysctl_test tests[] = { /* arg1 (buf) 9223372036854775807 */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -24), - BPF_LD_IMM64(BPF_REG_0, 0x3032373333323239ULL), + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( + 0x3932323333373230ULL)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), - BPF_LD_IMM64(BPF_REG_0, 0x3537373435383633ULL), + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( + 0x3336383534373735ULL)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 8), - BPF_LD_IMM64(BPF_REG_0, 0x0000000000373038ULL), + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( + 0x3830370000000000ULL)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 16), BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), @@ -1266,11 +1298,14 @@ static struct sysctl_test tests[] = { /* arg1 (buf) 9223372036854775808 */ BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -24), - BPF_LD_IMM64(BPF_REG_0, 0x3032373333323239ULL), + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( + 0x3932323333373230ULL)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), - BPF_LD_IMM64(BPF_REG_0, 0x3537373435383633ULL), + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( + 0x3336383534373735ULL)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 8), - BPF_LD_IMM64(BPF_REG_0, 0x0000000000383038ULL), + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( + 0x3830380000000000ULL)), BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 16), BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), @@ -1344,20 +1379,24 @@ static size_t probe_prog_length(const struct bpf_insn *fp) static int fixup_sysctl_value(const char *buf, size_t buf_len, struct bpf_insn *prog, size_t insn_num) { - uint32_t value_num = 0; + union { + uint8_t raw[sizeof(uint64_t)]; + uint64_t num; + } value = {}; uint8_t c, i; - if (buf_len > sizeof(value_num)) { + if (buf_len > sizeof(value)) { log_err("Value is too big (%zd) to use in fixup", buf_len); return -1; } - - for (i = 0; i < buf_len; ++i) { - c = buf[i]; - value_num |= (c << i * 8); + if (prog[insn_num].code != (BPF_LD | BPF_DW | BPF_IMM)) { + log_err("Can fixup only BPF_LD_IMM64 insns"); + return -1; } - prog[insn_num].imm = value_num; + memcpy(value.raw, buf, buf_len); + prog[insn_num].imm = (uint32_t)value.num; + prog[insn_num + 1].imm = (uint32_t)(value.num >> 32); return 0; } @@ -1499,6 +1538,7 @@ static int run_test_case(int cgfd, struct sysctl_test *test) goto err; } + errno = 0; if (access_sysctl(sysctl_path, test) == -1) { if (test->result == OP_EPERM && errno == EPERM) goto out; @@ -1507,7 +1547,7 @@ static int run_test_case(int cgfd, struct sysctl_test *test) } if (test->result != SUCCESS) { - log_err("Unexpected failure"); + log_err("Unexpected success"); goto err; } -- 2.21.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v3 2/2] selftests/bpf: fix endianness issues in test_sysctl 2019-08-28 13:22 ` [PATCH bpf v3 2/2] selftests/bpf: fix endianness issues in test_sysctl Ilya Leoshkevich @ 2019-08-29 21:53 ` Song Liu 2019-08-30 10:22 ` Ilya Leoshkevich 0 siblings, 1 reply; 6+ messages in thread From: Song Liu @ 2019-08-29 21:53 UTC (permalink / raw) To: Ilya Leoshkevich Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Yonghong Song, Andrey Ignatov, Heiko Carstens, Vasily Gorbik On Wed, Aug 28, 2019 at 6:22 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > A lot of test_sysctl sub-tests fail due to handling strings as a bunch > of immediate values in a little-endian-specific manner. > > Fix by wrapping all immediates in bpf_ntohl and the new bpf_be64_to_cpu. > > Also, sometimes tests fail because sysctl() unexpectedly succeeds with > an inappropriate "Unexpected failure" message and a random errno. Zero > out errno before calling sysctl() and replace the message with > "Unexpected success". > > Fixes: 1f5fa9ab6e2e ("selftests/bpf: Test BPF_CGROUP_SYSCTL") > Fixes: 9a1027e52535 ("selftests/bpf: Test file_pos field in bpf_sysctl ctx") > Fixes: 6041c67f28d8 ("selftests/bpf: Test bpf_sysctl_get_name helper") > Fixes: 11ff34f74e32 ("selftests/bpf: Test sysctl_get_current_value helper") > Fixes: 786047dd08de ("selftests/bpf: Test bpf_sysctl_{get,set}_new_value helpers") > Fixes: 8549ddc832d6 ("selftests/bpf: Test bpf_strtol and bpf_strtoul helpers") > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > tools/testing/selftests/bpf/test_sysctl.c | 130 ++++++++++++++-------- > 1 file changed, 85 insertions(+), 45 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c > index a3bebd7c68dd..b8c6e31645cb 100644 > --- a/tools/testing/selftests/bpf/test_sysctl.c > +++ b/tools/testing/selftests/bpf/test_sysctl.c > @@ -13,6 +13,7 @@ > #include <bpf/bpf.h> > #include <bpf/libbpf.h> > > +#include "bpf_endian.h" > #include "bpf_rlimit.h" > #include "bpf_util.h" > #include "cgroup_helpers.h" > @@ -100,7 +101,7 @@ static struct sysctl_test tests[] = { > .descr = "ctx:write sysctl:write read ok", > .insns = { > /* If (write) */ > - BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_1, > + BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_1, I didn't find explanation for this change in the commit log. Is this a typo, or a real fix? > offsetof(struct bpf_sysctl, write)), > BPF_JMP_IMM(BPF_JNE, BPF_REG_7, 1, 2), > > @@ -214,7 +215,8 @@ static struct sysctl_test tests[] = { > /* if (ret == expected && */ > BPF_JMP_IMM(BPF_JNE, BPF_REG_0, sizeof("tcp_mem") - 1, 6), > /* buf == "tcp_mem\0") */ > - BPF_LD_IMM64(BPF_REG_8, 0x006d656d5f706374ULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x7463705f6d656d00ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), > > @@ -255,7 +257,8 @@ static struct sysctl_test tests[] = { > BPF_JMP_IMM(BPF_JNE, BPF_REG_0, -E2BIG, 6), > > /* buf[0:7] == "tcp_me\0") */ > - BPF_LD_IMM64(BPF_REG_8, 0x00656d5f706374ULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x7463705f6d650000ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), > > @@ -298,12 +301,14 @@ static struct sysctl_test tests[] = { > BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 16, 14), > > /* buf[0:8] == "net/ipv4" && */ > - BPF_LD_IMM64(BPF_REG_8, 0x347670692f74656eULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x6e65742f69707634ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 10), > > /* buf[8:16] == "/tcp_mem" && */ > - BPF_LD_IMM64(BPF_REG_8, 0x6d656d5f7063742fULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x2f7463705f6d656dULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 8), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 6), > > @@ -350,12 +355,14 @@ static struct sysctl_test tests[] = { > BPF_JMP_IMM(BPF_JNE, BPF_REG_0, -E2BIG, 10), > > /* buf[0:8] == "net/ipv4" && */ > - BPF_LD_IMM64(BPF_REG_8, 0x347670692f74656eULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x6e65742f69707634ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 6), > > /* buf[8:16] == "/tcp_me\0") */ > - BPF_LD_IMM64(BPF_REG_8, 0x00656d5f7063742fULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x2f7463705f6d6500ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 8), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), > > @@ -396,7 +403,8 @@ static struct sysctl_test tests[] = { > BPF_JMP_IMM(BPF_JNE, BPF_REG_0, -E2BIG, 6), > > /* buf[0:8] == "net/ip\0") */ > - BPF_LD_IMM64(BPF_REG_8, 0x000070692f74656eULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x6e65742f69700000ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), > > @@ -431,7 +439,8 @@ static struct sysctl_test tests[] = { > BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 6, 6), > > /* buf[0:6] == "Linux\n\0") */ > - BPF_LD_IMM64(BPF_REG_8, 0x000a78756e694cULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x4c696e75780a0000ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), > > @@ -469,7 +478,8 @@ static struct sysctl_test tests[] = { > BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 6, 6), > > /* buf[0:6] == "Linux\n\0") */ > - BPF_LD_IMM64(BPF_REG_8, 0x000a78756e694cULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x4c696e75780a0000ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), > > @@ -507,7 +517,8 @@ static struct sysctl_test tests[] = { > BPF_JMP_IMM(BPF_JNE, BPF_REG_0, -E2BIG, 6), > > /* buf[0:6] == "Linux\0") */ > - BPF_LD_IMM64(BPF_REG_8, 0x000078756e694cULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x4c696e7578000000ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), > > @@ -650,7 +661,8 @@ static struct sysctl_test tests[] = { > > /* buf[0:4] == "606\0") */ > BPF_LDX_MEM(BPF_W, BPF_REG_9, BPF_REG_7, 0), > - BPF_JMP_IMM(BPF_JNE, BPF_REG_9, 0x00363036, 2), > + BPF_JMP_IMM(BPF_JNE, BPF_REG_9, > + bpf_ntohl(0x36303600), 2), > > /* return DENY; */ > BPF_MOV64_IMM(BPF_REG_0, 0), > @@ -685,17 +697,20 @@ static struct sysctl_test tests[] = { > BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 23, 14), > > /* buf[0:8] == "3000000 " && */ > - BPF_LD_IMM64(BPF_REG_8, 0x2030303030303033ULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x3330303030303020ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 10), > > /* buf[8:16] == "4000000 " && */ > - BPF_LD_IMM64(BPF_REG_8, 0x2030303030303034ULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x3430303030303020ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 8), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 6), > > /* buf[16:24] == "6000000\0") */ > - BPF_LD_IMM64(BPF_REG_8, 0x0030303030303036ULL), > + BPF_LD_IMM64(BPF_REG_8, bpf_be64_to_cpu( > + 0x3630303030303000ULL)), > BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 16), > BPF_JMP_REG(BPF_JNE, BPF_REG_8, BPF_REG_9, 2), > > @@ -735,7 +750,8 @@ static struct sysctl_test tests[] = { > > /* buf[0:3] == "60\0") */ > BPF_LDX_MEM(BPF_W, BPF_REG_9, BPF_REG_7, 0), > - BPF_JMP_IMM(BPF_JNE, BPF_REG_9, 0x003036, 2), > + BPF_JMP_IMM(BPF_JNE, BPF_REG_9, > + bpf_ntohl(0x36300000), 2), > > /* return DENY; */ > BPF_MOV64_IMM(BPF_REG_0, 0), > @@ -757,7 +773,8 @@ static struct sysctl_test tests[] = { > /* sysctl_set_new_value arg2 (buf) */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), > - BPF_MOV64_IMM(BPF_REG_0, 0x00303036), > + BPF_MOV64_IMM(BPF_REG_0, > + bpf_ntohl(0x36303000)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > > BPF_MOV64_REG(BPF_REG_2, BPF_REG_7), > @@ -791,7 +808,7 @@ static struct sysctl_test tests[] = { > /* sysctl_set_new_value arg2 (buf) */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), > - BPF_MOV64_IMM(BPF_REG_0, FIXUP_SYSCTL_VALUE), > + BPF_LD_IMM64(BPF_REG_0, FIXUP_SYSCTL_VALUE), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > > BPF_MOV64_REG(BPF_REG_2, BPF_REG_7), > @@ -825,8 +842,9 @@ static struct sysctl_test tests[] = { > /* arg1 (buf) */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), > - BPF_MOV64_IMM(BPF_REG_0, 0x00303036), > - BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > + BPF_MOV64_IMM(BPF_REG_0, > + bpf_ntohl(0x36303000)), > + BPF_STX_MEM(BPF_W, BPF_REG_7, BPF_REG_0, 0), > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > > @@ -869,7 +887,8 @@ static struct sysctl_test tests[] = { > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), > /* "600 602\0" */ > - BPF_LD_IMM64(BPF_REG_0, 0x0032303620303036ULL), > + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( > + 0x3630302036303200ULL)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > > @@ -937,7 +956,8 @@ static struct sysctl_test tests[] = { > /* arg1 (buf) */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), > - BPF_MOV64_IMM(BPF_REG_0, 0x00303036), > + BPF_MOV64_IMM(BPF_REG_0, > + bpf_ntohl(0x36303000)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > @@ -969,8 +989,9 @@ static struct sysctl_test tests[] = { > /* arg1 (buf) */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), > - BPF_MOV64_IMM(BPF_REG_0, 0x00373730), > - BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > + BPF_MOV64_IMM(BPF_REG_0, > + bpf_ntohl(0x30373700)), > + BPF_STX_MEM(BPF_W, BPF_REG_7, BPF_REG_0, 0), > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > > @@ -1012,7 +1033,8 @@ static struct sysctl_test tests[] = { > /* arg1 (buf) */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), > - BPF_MOV64_IMM(BPF_REG_0, 0x00303036), > + BPF_MOV64_IMM(BPF_REG_0, > + bpf_ntohl(0x36303000)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > @@ -1052,7 +1074,8 @@ static struct sysctl_test tests[] = { > /* arg1 (buf) */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), > - BPF_MOV64_IMM(BPF_REG_0, 0x090a0c0d), > + BPF_MOV64_IMM(BPF_REG_0, > + bpf_ntohl(0x0d0c0a09)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > @@ -1092,7 +1115,9 @@ static struct sysctl_test tests[] = { > /* arg1 (buf) */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), > - BPF_MOV64_IMM(BPF_REG_0, 0x00362d0a), /* " -6\0" */ > + /* " -6\0" */ > + BPF_MOV64_IMM(BPF_REG_0, > + bpf_ntohl(0x0a2d3600)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > @@ -1132,8 +1157,10 @@ static struct sysctl_test tests[] = { > /* arg1 (buf) */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), > - BPF_MOV64_IMM(BPF_REG_0, 0x00362d0a), /* " -6\0" */ > - BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > + /* " -6\0" */ > + BPF_MOV64_IMM(BPF_REG_0, > + bpf_ntohl(0x0a2d3600)), > + BPF_STX_MEM(BPF_W, BPF_REG_7, BPF_REG_0, 0), > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > > @@ -1175,8 +1202,10 @@ static struct sysctl_test tests[] = { > /* arg1 (buf) */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -8), > - BPF_MOV64_IMM(BPF_REG_0, 0x65667830), /* "0xfe" */ > - BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > + /* "0xfe" */ > + BPF_MOV64_IMM(BPF_REG_0, > + bpf_ntohl(0x30786665)), > + BPF_STX_MEM(BPF_W, BPF_REG_7, BPF_REG_0, 0), > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > > @@ -1218,11 +1247,14 @@ static struct sysctl_test tests[] = { > /* arg1 (buf) 9223372036854775807 */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -24), > - BPF_LD_IMM64(BPF_REG_0, 0x3032373333323239ULL), > + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( > + 0x3932323333373230ULL)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > - BPF_LD_IMM64(BPF_REG_0, 0x3537373435383633ULL), > + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( > + 0x3336383534373735ULL)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 8), > - BPF_LD_IMM64(BPF_REG_0, 0x0000000000373038ULL), > + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( > + 0x3830370000000000ULL)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 16), > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > @@ -1266,11 +1298,14 @@ static struct sysctl_test tests[] = { > /* arg1 (buf) 9223372036854775808 */ > BPF_MOV64_REG(BPF_REG_7, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, -24), > - BPF_LD_IMM64(BPF_REG_0, 0x3032373333323239ULL), > + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( > + 0x3932323333373230ULL)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 0), > - BPF_LD_IMM64(BPF_REG_0, 0x3537373435383633ULL), > + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( > + 0x3336383534373735ULL)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 8), > - BPF_LD_IMM64(BPF_REG_0, 0x0000000000383038ULL), > + BPF_LD_IMM64(BPF_REG_0, bpf_be64_to_cpu( > + 0x3830380000000000ULL)), > BPF_STX_MEM(BPF_DW, BPF_REG_7, BPF_REG_0, 16), > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > @@ -1344,20 +1379,24 @@ static size_t probe_prog_length(const struct bpf_insn *fp) > static int fixup_sysctl_value(const char *buf, size_t buf_len, > struct bpf_insn *prog, size_t insn_num) > { > - uint32_t value_num = 0; > + union { > + uint8_t raw[sizeof(uint64_t)]; > + uint64_t num; > + } value = {}; This change doesn't match the description in the changelog. > uint8_t c, i; > > - if (buf_len > sizeof(value_num)) { > + if (buf_len > sizeof(value)) { > log_err("Value is too big (%zd) to use in fixup", buf_len); > return -1; > } > - > - for (i = 0; i < buf_len; ++i) { > - c = buf[i]; > - value_num |= (c << i * 8); > + if (prog[insn_num].code != (BPF_LD | BPF_DW | BPF_IMM)) { > + log_err("Can fixup only BPF_LD_IMM64 insns"); > + return -1; > } > > - prog[insn_num].imm = value_num; > + memcpy(value.raw, buf, buf_len); > + prog[insn_num].imm = (uint32_t)value.num; > + prog[insn_num + 1].imm = (uint32_t)(value.num >> 32); > > return 0; > } > @@ -1499,6 +1538,7 @@ static int run_test_case(int cgfd, struct sysctl_test *test) > goto err; > } > > + errno = 0; > if (access_sysctl(sysctl_path, test) == -1) { > if (test->result == OP_EPERM && errno == EPERM) > goto out; > @@ -1507,7 +1547,7 @@ static int run_test_case(int cgfd, struct sysctl_test *test) > } > > if (test->result != SUCCESS) { > - log_err("Unexpected failure"); > + log_err("Unexpected success"); > goto err; > } > > -- > 2.21.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v3 2/2] selftests/bpf: fix endianness issues in test_sysctl 2019-08-29 21:53 ` Song Liu @ 2019-08-30 10:22 ` Ilya Leoshkevich 0 siblings, 0 replies; 6+ messages in thread From: Ilya Leoshkevich @ 2019-08-30 10:22 UTC (permalink / raw) To: Song Liu Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Yonghong Song, Andrey Ignatov, Heiko Carstens, Vasily Gorbik > Am 29.08.2019 um 23:53 schrieb Song Liu <liu.song.a23@gmail.com>: > > On Wed, Aug 28, 2019 at 6:22 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: >> >> A lot of test_sysctl sub-tests fail due to handling strings as a bunch >> of immediate values in a little-endian-specific manner. >> >> Fix by wrapping all immediates in bpf_ntohl and the new bpf_be64_to_cpu. >> >> Also, sometimes tests fail because sysctl() unexpectedly succeeds with >> an inappropriate "Unexpected failure" message and a random errno. Zero >> out errno before calling sysctl() and replace the message with >> "Unexpected success". ... >> @@ -13,6 +13,7 @@ >> #include <bpf/bpf.h> >> #include <bpf/libbpf.h> >> >> +#include "bpf_endian.h" >> #include "bpf_rlimit.h" >> #include "bpf_util.h" >> #include "cgroup_helpers.h" >> @@ -100,7 +101,7 @@ static struct sysctl_test tests[] = { >> .descr = "ctx:write sysctl:write read ok", >> .insns = { >> /* If (write) */ >> - BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_1, >> + BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_1, > > I didn't find explanation for this change in the commit log. Is this a typo, or > a real fix? This is a real fix, but I forgot to explain it. We read the first byte of an int assuming it's the least-significant one, which is not the case on BE arches. Two fixes are possible: use a different offset on BE arches or just read the whole int. Since we are not testing narrow accesses here (there is e.g. "ctx:file_pos sysctl:read read ok narrow" for that), I went with the simplest solution. ... >> @@ -1344,20 +1379,24 @@ static size_t probe_prog_length(const struct bpf_insn *fp) >> static int fixup_sysctl_value(const char *buf, size_t buf_len, >> struct bpf_insn *prog, size_t insn_num) >> { >> - uint32_t value_num = 0; >> + union { >> + uint8_t raw[sizeof(uint64_t)]; >> + uint64_t num; >> + } value = {}; > > This change doesn't match the description in the changelog. This one I also forgot to explain - writing an immediate into things like BPF_LD_IMM64(BPF_REG_0, FIXUP_SYSCTL_VALUE) should be endianness-aware. We can also do bpf_be64_to_cpu or similar here, but a better trick (suggested by Yonghong) is to simply memcpy the raw user-provided value, since testcase endianness and bpf program endianness match. Let me send a v4 with an improved commit message. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-30 10:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-28 13:22 [PATCH bpf v3 0/2] selftests/bpf: fix endianness issues in test_sysctl Ilya Leoshkevich 2019-08-28 13:22 ` [PATCH bpf v3 1/2] selftests/bpf: introduce bpf_cpu_to_be64 and bpf_be64_to_cpu Ilya Leoshkevich 2019-08-29 21:46 ` Song Liu 2019-08-28 13:22 ` [PATCH bpf v3 2/2] selftests/bpf: fix endianness issues in test_sysctl Ilya Leoshkevich 2019-08-29 21:53 ` Song Liu 2019-08-30 10:22 ` Ilya Leoshkevich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox