On 8/15/19 5:25 AM, Ilya Leoshkevich 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_constant_ntohl,
> __bpf_constant_be64_to_cpu and __bpf_le64_to_cpu.
>
> 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@xxxxxxxxxxxxx>
> ---
> tools/testing/selftests/bpf/bpf_endian.h | 4 +
> tools/testing/selftests/bpf/test_sysctl.c | 122 +++++++++++++++-------
> 2 files changed, 86 insertions(+), 40 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/bpf_endian.h b/tools/testing/selftests/bpf/bpf_endian.h
> index 05f036df8a4c..94175c993806 100644
> --- a/tools/testing/selftests/bpf/bpf_endian.h
> +++ b/tools/testing/selftests/bpf/bpf_endian.h
> @@ -29,6 +29,8 @@
> # 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_le64_to_cpu(x)
Maybe we can remove this? See comments below.
(x)
> +# define __bpf_constant_be64_to_cpu(x) ___constant_swab64(x)
bpf_endian.h is used for both bpf program and native applications.
Could you make sure it works for bpf programs? It should be, but want to
double check.
The __constant_swab64 looks like a little bit expensive
for bpf programs compared to __builtin_bswap64. But
__builtin_bswap64 may not be available for all architectures, esp.
32bit system. So macro __bpf__ is required to use it.
In any case, bpf programs can directly use __builtin_bswap64,
so I am fine with __bpf_constant_be64_to_cpu().
> #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> # define __bpf_ntohs(x) (x)
> # define __bpf_htons(x) (x)
> @@ -38,6 +40,8 @@
> # define __bpf_htonl(x) (x)
> # define __bpf_constant_ntohl(x) (x)
> # define __bpf_constant_htonl(x) (x)
> +# define __bpf_le64_to_cpu(x) __swab64(x)
> +# define __bpf_constant_be64_to_cpu(x) (x)
> #else
> # error "Fix your compiler's __BYTE_ORDER__?!"
> #endif
> diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
> index a3bebd7c68dd..9e4986b03e95 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_constant_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),
>
[....]
>
> BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
> @@ -1344,20 +1379,26 @@ 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;
> + uint64_t value_num = 0;
> uint8_t c, i;
>
> if (buf_len > sizeof(value_num)) {
> log_err("Value is too big (%zd) to use in fixup", buf_len);
> return -1;
> }
> + if (prog[insn_num].code != (BPF_LD | BPF_DW | BPF_IMM)) {
> + log_err("Can fixup only BPF_LD_IMM64 insns");
> + return -1;
> + }
>
> for (i = 0; i < buf_len; ++i) {
> c = buf[i];
> value_num |= (c << i * 8);
> }
> + value_num = __bpf_le64_to_cpu(value_num);
Can we avoid to use __bpf_le64_to_cpu?
Look like we already having the value in buf, can we just cast it
to get value_num. Note that bpf program and host always have
the same endianness. This way, no endianness conversion
is needed.
>
> - prog[insn_num].imm = value_num;
> + prog[insn_num].imm = (__u32)value_num;
> + prog[insn_num + 1].imm = (__u32)(value_num >> 32);
>
> return 0;
> }
> @@ -1499,6 +1540,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 +1549,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;
> }
>
>
