Skip to contentSkip to conversation

selftests/bpf: fix endianness issues in test_sysctl

PATCH Superseded Ilya Leoshkevich 2 participants 6 messages Aug 28, 2019 – Aug 30, 2019

Subsystems (3)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v3 cover selftests/bpf: fix endianness issues in test_sysctl

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(-)
Signature · 1 line
2.21.0
#1 lore raw

PATCH v3 1/2 selftests/bpf: introduce bpf_cpu_to_be64 and bpf_be64_to_cpu

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.

---
 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(-)
tools/testing/selftests/bpf/bpf_endian.h +14 -0
index 05f036df8a4c..f92f29c00d50 100644
@@ -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__ */
tools/testing/selftests/bpf/progs/test_lwt_seg6local.c +6 -10
index a334a0e882e4..5e86afb8fb31 100644
@@ -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)
tools/testing/selftests/bpf/progs/test_seg6_loop.c +2 -6
index 1dbe1d4d467e..c4d104428643 100644
@@ -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)
Signature · 1 line
2.21.0
Acked-by on v3 1/2
#2 lore raw

Re: [PATCH bpf v3 1/2] selftests/bpf: introduce bpf_cpu_to_be64 and bpf_be64_to_cpu

Quoted 7 lines · 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.

#3 lore raw

PATCH v3 2/2 selftests/bpf: fix endianness issues in test_sysctl

In reply to 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")
---
 tools/testing/selftests/bpf/test_sysctl.c | 130 ++++++++++++++--------
 1 file changed, 85 insertions(+), 45 deletions(-)
tools/testing/selftests/bpf/test_sysctl.c +85 -45
index a3bebd7c68dd..b8c6e31645cb 100644
@@ -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;
 	}
 
Signature · 1 line
2.21.0
#4 lore raw
Quoted 35 lines · 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")
---
 tools/testing/selftests/bpf/test_sysctl.c | 130 ++++++++++++++--------
 1 file changed, 85 insertions(+), 45 deletions(-)
tools/testing/selftests/bpf/test_sysctl.c +1 -0
index a3bebd7c68dd..b8c6e31645cb 100644
@@ -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"

tools/testing/selftests/bpf/test_sysctl.c static struct sysctl_test tests[] = { · line 104

Quoted 5 lines
tools/testing/selftests/bpf/test_sysctl.c +1 -1
@@ -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?

Quoted 298 lines
tools/testing/selftests/bpf/test_sysctl.c +70 -36
                                    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),

tools/testing/selftests/bpf/test_sysctl.c static size_t probe_prog_length(const struct bpf_insn *fp) · line 1385

Quoted 8 lines
tools/testing/selftests/bpf/test_sysctl.c +4 -1
@@ -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.

Quoted 43 lines
tools/testing/selftests/bpf/test_sysctl.c +9 -8
        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

#5 lore raw
Quoted 13 lines

Am 29.08.2019 um 23:53 schrieb Song Liu <[email protected]>:

Quoted 10 lines · 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".

...

Quoted 4 lines
Quoted 4 lines
tools/testing/selftests/bpf/test_sysctl.c +0 -0
@@ -13,6 +13,7 @@
 #include <bpf/bpf.h>
 #include <bpf/libbpf.h>
 
Quoted 13 lines

tools/testing/selftests/bpf/test_sysctl.c static struct sysctl_test tests[] = { · line 104

Quoted 10 lines
tools/testing/selftests/bpf/test_sysctl.c +2 -1
+#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.

...

Quoted 11 lines

tools/testing/selftests/bpf/test_sysctl.c static size_t probe_prog_length(const struct bpf_insn *fp) · line 1385

Quoted 9 lines
tools/testing/selftests/bpf/test_sysctl.c +4 -1
@@ -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.