Re: [PATCH bpf] selftests/bpf: fix endianness issues in test_sysctl

[Yonghong Song <yhs@fb.com>]

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@linux.ibm.com>
> ---
>   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;
>   	}
>   
>