* [LTP] [PATCH v3] BPF: Regression test for 64bit arithmetic
@ 2019-09-11 9:54 Richard Palethorpe
2019-09-12 14:56 ` Cyril Hrubis
2019-09-13 10:55 ` Clemens Famulla-Conrad
0 siblings, 2 replies; 7+ messages in thread
From: Richard Palethorpe @ 2019-09-11 9:54 UTC (permalink / raw)
To: ltp
Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Reviewed-by: Jan Stancek <jstancek@redhat.com>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
---
V3: Rebased on master and included line numbers in instructions
Capability patch has not been applied to master at time of rebase.
include/lapi/bpf.h | 27 +++
runtest/syscalls | 1 +
testcases/kernel/syscalls/bpf/.gitignore | 1 +
testcases/kernel/syscalls/bpf/bpf_prog02.c | 182 +++++++++++++++++++++
4 files changed, 211 insertions(+)
create mode 100644 testcases/kernel/syscalls/bpf/bpf_prog02.c
diff --git a/include/lapi/bpf.h b/include/lapi/bpf.h
index 122eb5469..03073b45e 100644
--- a/include/lapi/bpf.h
+++ b/include/lapi/bpf.h
@@ -18,7 +18,9 @@
/* Start copy from linux/bpf_(common).h */
#define BPF_CLASS(code) ((code) & 0x07)
#define BPF_LD 0x00
+#define BPF_LDX 0x01
#define BPF_ST 0x02
+#define BPF_STX 0x03
#define BPF_JMP 0x05
#define BPF_SIZE(code) ((code) & 0x18)
@@ -30,6 +32,7 @@
#define BPF_OP(code) ((code) & 0xf0)
#define BPF_ADD 0x00
+#define BPF_SUB 0x10
#define BPF_JEQ 0x10
@@ -432,6 +435,14 @@ enum bpf_func_id {
/* Start copy from tools/include/filter.h */
+#define BPF_ALU64_REG(OP, DST, SRC) \
+ ((struct bpf_insn) { \
+ .code = BPF_ALU64 | BPF_OP(OP) | BPF_X, \
+ .dst_reg = DST, \
+ .src_reg = SRC, \
+ .off = 0, \
+ .imm = 0 })
+
#define BPF_ALU64_IMM(OP, DST, IMM) \
((struct bpf_insn) { \
.code = BPF_ALU64 | BPF_OP(OP) | BPF_K, \
@@ -477,6 +488,22 @@ enum bpf_func_id {
.off = OFF, \
.imm = IMM })
+#define BPF_LDX_MEM(SIZE, DST, SRC, OFF) \
+ ((struct bpf_insn) { \
+ .code = BPF_LDX | BPF_SIZE(SIZE) | BPF_MEM, \
+ .dst_reg = DST, \
+ .src_reg = SRC, \
+ .off = OFF, \
+ .imm = 0 })
+
+#define BPF_STX_MEM(SIZE, DST, SRC, OFF) \
+ ((struct bpf_insn) { \
+ .code = BPF_STX | BPF_SIZE(SIZE) | BPF_MEM, \
+ .dst_reg = DST, \
+ .src_reg = SRC, \
+ .off = OFF, \
+ .imm = 0 })
+
#define BPF_JMP_IMM(OP, DST, IMM, OFF) \
((struct bpf_insn) { \
.code = BPF_JMP | BPF_OP(OP) | BPF_K, \
diff --git a/runtest/syscalls b/runtest/syscalls
index 874ae4d4f..4e6310193 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -34,6 +34,7 @@ bind03 bind03
bpf_map01 bpf_map01
bpf_prog01 bpf_prog01
+bpf_prog02 bpf_prog02
brk01 brk01
diff --git a/testcases/kernel/syscalls/bpf/.gitignore b/testcases/kernel/syscalls/bpf/.gitignore
index 7eb5f7c92..1704f9841 100644
--- a/testcases/kernel/syscalls/bpf/.gitignore
+++ b/testcases/kernel/syscalls/bpf/.gitignore
@@ -1,2 +1,3 @@
bpf_map01
bpf_prog01
+bpf_prog02
diff --git a/testcases/kernel/syscalls/bpf/bpf_prog02.c b/testcases/kernel/syscalls/bpf/bpf_prog02.c
new file mode 100644
index 000000000..dc8b92f00
--- /dev/null
+++ b/testcases/kernel/syscalls/bpf/bpf_prog02.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2019 Richard Palethorpe <rpalethorpe@suse.com>
+ *
+ * Check if eBPF can do arithmetic with 64bits. This targets a specific
+ * regression which only effects unprivileged users who are subject to extra
+ * pointer arithmetic checks during verification.
+ *
+ * Fixed by commit 3612af783cf52c74a031a2f11b82247b2599d3cd.
+ * https://new.blog.cloudflare.com/ebpf-cant-count/
+ *
+ * This test is very similar in structure to bpf_prog01 which is better
+ * annotated.
+ */
+
+#include <limits.h>
+#include <string.h>
+#include <stdio.h>
+
+#include "config.h"
+#include "tst_test.h"
+#include "tst_capability.h"
+#include "lapi/socket.h"
+#include "lapi/bpf.h"
+
+#define A64INT (((uint64_t)1) << 60)
+
+const char MSG[] = "Ahoj!";
+static char *msg;
+
+static char *log;
+static uint32_t *key;
+static uint64_t *val;
+static union bpf_attr *attr;
+
+static int load_prog(int fd)
+{
+ struct bpf_insn *prog;
+ struct bpf_insn insn[] = {
+ BPF_MOV64_IMM(BPF_REG_6, 1), /* 0: r6 = 1 */
+
+ BPF_LD_MAP_FD(BPF_REG_1, fd), /* 1: r1 = &fd */
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), /* 3: r2 = fp */
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), /* 4: r2 = r2 - 8 */
+ BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0), /* 5: *r2 = 0 */
+ BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 6: map_lookup_elem */
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 17), /* 7: if(!r0) goto 25 */
+ BPF_MOV64_REG(BPF_REG_3, BPF_REG_0), /* 8: r3 = r0 */
+ BPF_LD_IMM64(BPF_REG_4, A64INT), /* 9: r4 = 2^61 */
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_4, BPF_REG_6), /* 11: r4 += r6 */
+ BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 12: *r3 = r4 */
+
+ BPF_LD_MAP_FD(BPF_REG_1, fd), /* 13: r1 = &fd */
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), /* 15: r2 = fp */
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), /* 16: r2 = r2 - 8 */
+ BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 1), /* 17: *r2 = 1 */
+ BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 18: map_lookup_elem */
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5), /* 19: if(!r0) goto 25 */
+ BPF_MOV64_REG(BPF_REG_3, BPF_REG_0), /* 20: r3 = r0 */
+ BPF_LD_IMM64(BPF_REG_4, A64INT), /* 21: r4 = 2^61 */
+ BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23: r4 -= r6 */
+ BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24: *r3 = r4 */
+
+ BPF_MOV64_IMM(BPF_REG_0, 0), /* 25: r0 = 0 */
+ BPF_EXIT_INSN(), /* 26: return r0 */
+ };
+
+ /* Leaks memory when -i is specified */
+ prog = tst_alloc(sizeof(insn));
+ memcpy(prog, insn, sizeof(insn));
+
+ memset(attr, 0, sizeof(*attr));
+ attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
+ attr->insns = ptr_to_u64(prog);
+ attr->insn_cnt = ARRAY_SIZE(insn);
+ attr->license = ptr_to_u64("GPL");
+ attr->log_buf = ptr_to_u64(log);
+ attr->log_size = BUFSIZ;
+ attr->log_level = 1;
+
+ TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
+ if (TST_RET == -1) {
+ if (log[0] != 0) {
+ tst_res(TINFO, "Verification log:");
+ fputs(log, stderr);
+ tst_brk(TBROK | TTERRNO, "Failed verification");
+ } else {
+ tst_brk(TBROK | TTERRNO, "Failed to load program");
+ }
+ }
+
+ return TST_RET;
+}
+
+static void setup(void)
+{
+ memcpy(msg, MSG, sizeof(MSG));
+}
+
+static void run(void)
+{
+ int map_fd, prog_fd;
+ int sk[2];
+
+ memset(attr, 0, sizeof(*attr));
+ attr->map_type = BPF_MAP_TYPE_ARRAY;
+ attr->key_size = 4;
+ attr->value_size = 8;
+ attr->max_entries = 2;
+
+ TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
+ if (TST_RET == -1) {
+ if (TST_ERR == EPERM) {
+ tst_brk(TCONF | TTERRNO,
+ "bpf() requires CAP_SYS_ADMIN on this system");
+ } else {
+ tst_brk(TBROK | TTERRNO, "Failed to create array map");
+ }
+ }
+ map_fd = TST_RET;
+
+ prog_fd = load_prog(map_fd);
+
+ SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
+ SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
+ &prog_fd, sizeof(prog_fd));
+
+ SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
+
+ memset(attr, 0, sizeof(*attr));
+ attr->map_fd = map_fd;
+ attr->key = ptr_to_u64(key);
+ attr->value = ptr_to_u64(val);
+ *key = 0;
+
+ TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
+ if (TST_RET == -1) {
+ tst_res(TFAIL | TTERRNO, "array map lookup");
+ } else if (*val != A64INT + 1) {
+ tst_res(TFAIL,
+ "val = %lu, but should be val = %lu + 1",
+ *val, A64INT);
+ } else {
+ tst_res(TPASS, "val = %lu + 1", A64INT);
+ }
+
+ *key = 1;
+
+ TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
+ if (TST_RET == -1) {
+ tst_res(TFAIL | TTERRNO, "array map lookup");
+ } else if (*val != A64INT - 1) {
+ tst_res(TFAIL,
+ "val = %lu, but should be val = %lu - 1",
+ *val, A64INT);
+ } else {
+ tst_res(TPASS, "val = %lu - 1", A64INT);
+ }
+
+ SAFE_CLOSE(prog_fd);
+ SAFE_CLOSE(map_fd);
+ SAFE_CLOSE(sk[0]);
+ SAFE_CLOSE(sk[1]);
+}
+
+static struct tst_test test = {
+ .setup = setup,
+ .test_all = run,
+ .min_kver = "3.18",
+ .caps = (struct tst_cap []) {
+ TST_CAP(TST_CAP_DROP, CAP_SYS_ADMIN),
+ {}
+ },
+ .bufs = (struct tst_buffers []) {
+ {&key, .size = sizeof(*key)},
+ {&val, .size = sizeof(*val)},
+ {&log, .size = BUFSIZ},
+ {&attr, .size = sizeof(*attr)},
+ {&msg, .size = sizeof(MSG)},
+ {},
+ }
+};
--
2.22.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [LTP] [PATCH v3] BPF: Regression test for 64bit arithmetic
2019-09-11 9:54 [LTP] [PATCH v3] BPF: Regression test for 64bit arithmetic Richard Palethorpe
@ 2019-09-12 14:56 ` Cyril Hrubis
2019-09-17 7:26 ` Richard Palethorpe
2019-09-13 10:55 ` Clemens Famulla-Conrad
1 sibling, 1 reply; 7+ messages in thread
From: Cyril Hrubis @ 2019-09-12 14:56 UTC (permalink / raw)
To: ltp
Hi!
A few minor points below, I guess I can fix these before pushing as
well.
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Reviewed-by: Jan Stancek <jstancek@redhat.com>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>
> V3: Rebased on master and included line numbers in instructions
>
> Capability patch has not been applied to master at time of rebase.
>
> include/lapi/bpf.h | 27 +++
> runtest/syscalls | 1 +
> testcases/kernel/syscalls/bpf/.gitignore | 1 +
> testcases/kernel/syscalls/bpf/bpf_prog02.c | 182 +++++++++++++++++++++
> 4 files changed, 211 insertions(+)
> create mode 100644 testcases/kernel/syscalls/bpf/bpf_prog02.c
>
> diff --git a/include/lapi/bpf.h b/include/lapi/bpf.h
> index 122eb5469..03073b45e 100644
> --- a/include/lapi/bpf.h
> +++ b/include/lapi/bpf.h
> @@ -18,7 +18,9 @@
> /* Start copy from linux/bpf_(common).h */
> #define BPF_CLASS(code) ((code) & 0x07)
> #define BPF_LD 0x00
> +#define BPF_LDX 0x01
> #define BPF_ST 0x02
> +#define BPF_STX 0x03
> #define BPF_JMP 0x05
>
> #define BPF_SIZE(code) ((code) & 0x18)
> @@ -30,6 +32,7 @@
>
> #define BPF_OP(code) ((code) & 0xf0)
> #define BPF_ADD 0x00
> +#define BPF_SUB 0x10
>
> #define BPF_JEQ 0x10
>
> @@ -432,6 +435,14 @@ enum bpf_func_id {
>
> /* Start copy from tools/include/filter.h */
>
> +#define BPF_ALU64_REG(OP, DST, SRC) \
> + ((struct bpf_insn) { \
> + .code = BPF_ALU64 | BPF_OP(OP) | BPF_X, \
> + .dst_reg = DST, \
> + .src_reg = SRC, \
> + .off = 0, \
> + .imm = 0 })
> +
> #define BPF_ALU64_IMM(OP, DST, IMM) \
> ((struct bpf_insn) { \
> .code = BPF_ALU64 | BPF_OP(OP) | BPF_K, \
> @@ -477,6 +488,22 @@ enum bpf_func_id {
> .off = OFF, \
> .imm = IMM })
>
> +#define BPF_LDX_MEM(SIZE, DST, SRC, OFF) \
> + ((struct bpf_insn) { \
> + .code = BPF_LDX | BPF_SIZE(SIZE) | BPF_MEM, \
> + .dst_reg = DST, \
> + .src_reg = SRC, \
> + .off = OFF, \
> + .imm = 0 })
> +
> +#define BPF_STX_MEM(SIZE, DST, SRC, OFF) \
> + ((struct bpf_insn) { \
> + .code = BPF_STX | BPF_SIZE(SIZE) | BPF_MEM, \
> + .dst_reg = DST, \
> + .src_reg = SRC, \
> + .off = OFF, \
> + .imm = 0 })
> +
> #define BPF_JMP_IMM(OP, DST, IMM, OFF) \
> ((struct bpf_insn) { \
> .code = BPF_JMP | BPF_OP(OP) | BPF_K, \
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 874ae4d4f..4e6310193 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -34,6 +34,7 @@ bind03 bind03
>
> bpf_map01 bpf_map01
> bpf_prog01 bpf_prog01
> +bpf_prog02 bpf_prog02
>
> brk01 brk01
>
> diff --git a/testcases/kernel/syscalls/bpf/.gitignore b/testcases/kernel/syscalls/bpf/.gitignore
> index 7eb5f7c92..1704f9841 100644
> --- a/testcases/kernel/syscalls/bpf/.gitignore
> +++ b/testcases/kernel/syscalls/bpf/.gitignore
> @@ -1,2 +1,3 @@
> bpf_map01
> bpf_prog01
> +bpf_prog02
> diff --git a/testcases/kernel/syscalls/bpf/bpf_prog02.c b/testcases/kernel/syscalls/bpf/bpf_prog02.c
> new file mode 100644
> index 000000000..dc8b92f00
> --- /dev/null
> +++ b/testcases/kernel/syscalls/bpf/bpf_prog02.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2019 Richard Palethorpe <rpalethorpe@suse.com>
> + *
> + * Check if eBPF can do arithmetic with 64bits. This targets a specific
> + * regression which only effects unprivileged users who are subject to extra
> + * pointer arithmetic checks during verification.
> + *
> + * Fixed by commit 3612af783cf52c74a031a2f11b82247b2599d3cd.
> + * https://new.blog.cloudflare.com/ebpf-cant-count/
> + *
> + * This test is very similar in structure to bpf_prog01 which is better
> + * annotated.
> + */
> +
> +#include <limits.h>
> +#include <string.h>
> +#include <stdio.h>
> +
> +#include "config.h"
> +#include "tst_test.h"
> +#include "tst_capability.h"
> +#include "lapi/socket.h"
> +#include "lapi/bpf.h"
> +
> +#define A64INT (((uint64_t)1) << 60)
> +
> +const char MSG[] = "Ahoj!";
> +static char *msg;
> +
> +static char *log;
> +static uint32_t *key;
> +static uint64_t *val;
> +static union bpf_attr *attr;
> +
> +static int load_prog(int fd)
> +{
> + struct bpf_insn *prog;
> + struct bpf_insn insn[] = {
> + BPF_MOV64_IMM(BPF_REG_6, 1), /* 0: r6 = 1 */
> +
> + BPF_LD_MAP_FD(BPF_REG_1, fd), /* 1: r1 = &fd */
> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), /* 3: r2 = fp */
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), /* 4: r2 = r2 - 8 */
> + BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0), /* 5: *r2 = 0 */
> + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 6: map_lookup_elem */
> + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 17), /* 7: if(!r0) goto 25 */
> + BPF_MOV64_REG(BPF_REG_3, BPF_REG_0), /* 8: r3 = r0 */
> + BPF_LD_IMM64(BPF_REG_4, A64INT), /* 9: r4 = 2^61 */
> + BPF_ALU64_REG(BPF_ADD, BPF_REG_4, BPF_REG_6), /* 11: r4 += r6 */
> + BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 12: *r3 = r4 */
> +
> + BPF_LD_MAP_FD(BPF_REG_1, fd), /* 13: r1 = &fd */
> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), /* 15: r2 = fp */
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), /* 16: r2 = r2 - 8 */
> + BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 1), /* 17: *r2 = 1 */
> + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 18: map_lookup_elem */
> + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5), /* 19: if(!r0) goto 25 */
> + BPF_MOV64_REG(BPF_REG_3, BPF_REG_0), /* 20: r3 = r0 */
> + BPF_LD_IMM64(BPF_REG_4, A64INT), /* 21: r4 = 2^61 */
> + BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23: r4 -= r6 */
> + BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24: *r3 = r4 */
> +
> + BPF_MOV64_IMM(BPF_REG_0, 0), /* 25: r0 = 0 */
> + BPF_EXIT_INSN(), /* 26: return r0 */
> + };
> +
> + /* Leaks memory when -i is specified */
> + prog = tst_alloc(sizeof(insn));
Why not just declare the prog static? Then we could do:
if (!prog)
prog = tst_alloc(sizeof(insn));
> + memcpy(prog, insn, sizeof(insn));
> +
> + memset(attr, 0, sizeof(*attr));
> + attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> + attr->insns = ptr_to_u64(prog);
> + attr->insn_cnt = ARRAY_SIZE(insn);
> + attr->license = ptr_to_u64("GPL");
> + attr->log_buf = ptr_to_u64(log);
> + attr->log_size = BUFSIZ;
> + attr->log_level = 1;
> +
> + TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
> + if (TST_RET == -1) {
> + if (log[0] != 0) {
> + tst_res(TINFO, "Verification log:");
> + fputs(log, stderr);
I guess that we can do tst_res(TINFO, "Verification log:\n%s", log); instead.
> + tst_brk(TBROK | TTERRNO, "Failed verification");
> + } else {
> + tst_brk(TBROK | TTERRNO, "Failed to load program");
> + }
> + }
> +
> + return TST_RET;
> +}
> +
> +static void setup(void)
> +{
> + memcpy(msg, MSG, sizeof(MSG));
> +}
> +
> +static void run(void)
> +{
> + int map_fd, prog_fd;
> + int sk[2];
> +
> + memset(attr, 0, sizeof(*attr));
> + attr->map_type = BPF_MAP_TYPE_ARRAY;
> + attr->key_size = 4;
> + attr->value_size = 8;
> + attr->max_entries = 2;
> +
> + TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
> + if (TST_RET == -1) {
> + if (TST_ERR == EPERM) {
> + tst_brk(TCONF | TTERRNO,
> + "bpf() requires CAP_SYS_ADMIN on this system");
> + } else {
> + tst_brk(TBROK | TTERRNO, "Failed to create array map");
> + }
> + }
> + map_fd = TST_RET;
> +
> + prog_fd = load_prog(map_fd);
> +
> + SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
> + SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
> + &prog_fd, sizeof(prog_fd));
> +
> + SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
> +
> + memset(attr, 0, sizeof(*attr));
> + attr->map_fd = map_fd;
> + attr->key = ptr_to_u64(key);
> + attr->value = ptr_to_u64(val);
> + *key = 0;
> +
> + TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
> + if (TST_RET == -1) {
> + tst_res(TFAIL | TTERRNO, "array map lookup");
> + } else if (*val != A64INT + 1) {
> + tst_res(TFAIL,
> + "val = %lu, but should be val = %lu + 1",
> + *val, A64INT);
> + } else {
> + tst_res(TPASS, "val = %lu + 1", A64INT);
> + }
Wrong indentation, also I do not fancy this if else maze. I guess that
we can can safely do goto exit; if map lookup fails that would point
right before the SAFE_CLOSE() block.
> + *key = 1;
> +
> + TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
> + if (TST_RET == -1) {
> + tst_res(TFAIL | TTERRNO, "array map lookup");
> + } else if (*val != A64INT - 1) {
> + tst_res(TFAIL,
> + "val = %lu, but should be val = %lu - 1",
> + *val, A64INT);
> + } else {
> + tst_res(TPASS, "val = %lu - 1", A64INT);
> + }
Here as well.
> + SAFE_CLOSE(prog_fd);
> + SAFE_CLOSE(map_fd);
> + SAFE_CLOSE(sk[0]);
> + SAFE_CLOSE(sk[1]);
> +}
> +
> +static struct tst_test test = {
> + .setup = setup,
> + .test_all = run,
> + .min_kver = "3.18",
> + .caps = (struct tst_cap []) {
> + TST_CAP(TST_CAP_DROP, CAP_SYS_ADMIN),
> + {}
> + },
> + .bufs = (struct tst_buffers []) {
> + {&key, .size = sizeof(*key)},
> + {&val, .size = sizeof(*val)},
> + {&log, .size = BUFSIZ},
> + {&attr, .size = sizeof(*attr)},
> + {&msg, .size = sizeof(MSG)},
> + {},
> + }
> +};
> --
> 2.22.1
>
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 7+ messages in thread* [LTP] [PATCH v3] BPF: Regression test for 64bit arithmetic
2019-09-12 14:56 ` Cyril Hrubis
@ 2019-09-17 7:26 ` Richard Palethorpe
0 siblings, 0 replies; 7+ messages in thread
From: Richard Palethorpe @ 2019-09-17 7:26 UTC (permalink / raw)
To: ltp
Hello,
Cyril Hrubis <chrubis@suse.cz> writes:
> Hi!
> A few minor points below, I guess I can fix these before pushing as
> well.
>
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> Reviewed-by: Jan Stancek <jstancek@redhat.com>
>> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
>> ---
>>
>> V3: Rebased on master and included line numbers in instructions
>>
>> Capability patch has not been applied to master at time of rebase.
>>
>> include/lapi/bpf.h | 27 +++
>> runtest/syscalls | 1 +
>> testcases/kernel/syscalls/bpf/.gitignore | 1 +
>> testcases/kernel/syscalls/bpf/bpf_prog02.c | 182 +++++++++++++++++++++
>> 4 files changed, 211 insertions(+)
>> create mode 100644 testcases/kernel/syscalls/bpf/bpf_prog02.c
>>
>> diff --git a/include/lapi/bpf.h b/include/lapi/bpf.h
>> index 122eb5469..03073b45e 100644
>> --- a/include/lapi/bpf.h
>> +++ b/include/lapi/bpf.h
>> @@ -18,7 +18,9 @@
>> /* Start copy from linux/bpf_(common).h */
>> #define BPF_CLASS(code) ((code) & 0x07)
>> #define BPF_LD 0x00
>> +#define BPF_LDX 0x01
>> #define BPF_ST 0x02
>> +#define BPF_STX 0x03
>> #define BPF_JMP 0x05
>>
>> #define BPF_SIZE(code) ((code) & 0x18)
>> @@ -30,6 +32,7 @@
>>
>> #define BPF_OP(code) ((code) & 0xf0)
>> #define BPF_ADD 0x00
>> +#define BPF_SUB 0x10
>>
>> #define BPF_JEQ 0x10
>>
>> @@ -432,6 +435,14 @@ enum bpf_func_id {
>>
>> /* Start copy from tools/include/filter.h */
>>
>> +#define BPF_ALU64_REG(OP, DST, SRC) \
>> + ((struct bpf_insn) { \
>> + .code = BPF_ALU64 | BPF_OP(OP) | BPF_X, \
>> + .dst_reg = DST, \
>> + .src_reg = SRC, \
>> + .off = 0, \
>> + .imm = 0 })
>> +
>> #define BPF_ALU64_IMM(OP, DST, IMM) \
>> ((struct bpf_insn) { \
>> .code = BPF_ALU64 | BPF_OP(OP) | BPF_K, \
>> @@ -477,6 +488,22 @@ enum bpf_func_id {
>> .off = OFF, \
>> .imm = IMM })
>>
>> +#define BPF_LDX_MEM(SIZE, DST, SRC, OFF) \
>> + ((struct bpf_insn) { \
>> + .code = BPF_LDX | BPF_SIZE(SIZE) | BPF_MEM, \
>> + .dst_reg = DST, \
>> + .src_reg = SRC, \
>> + .off = OFF, \
>> + .imm = 0 })
>> +
>> +#define BPF_STX_MEM(SIZE, DST, SRC, OFF) \
>> + ((struct bpf_insn) { \
>> + .code = BPF_STX | BPF_SIZE(SIZE) | BPF_MEM, \
>> + .dst_reg = DST, \
>> + .src_reg = SRC, \
>> + .off = OFF, \
>> + .imm = 0 })
>> +
>> #define BPF_JMP_IMM(OP, DST, IMM, OFF) \
>> ((struct bpf_insn) { \
>> .code = BPF_JMP | BPF_OP(OP) | BPF_K, \
>> diff --git a/runtest/syscalls b/runtest/syscalls
>> index 874ae4d4f..4e6310193 100644
>> --- a/runtest/syscalls
>> +++ b/runtest/syscalls
>> @@ -34,6 +34,7 @@ bind03 bind03
>>
>> bpf_map01 bpf_map01
>> bpf_prog01 bpf_prog01
>> +bpf_prog02 bpf_prog02
>>
>> brk01 brk01
>>
>> diff --git a/testcases/kernel/syscalls/bpf/.gitignore b/testcases/kernel/syscalls/bpf/.gitignore
>> index 7eb5f7c92..1704f9841 100644
>> --- a/testcases/kernel/syscalls/bpf/.gitignore
>> +++ b/testcases/kernel/syscalls/bpf/.gitignore
>> @@ -1,2 +1,3 @@
>> bpf_map01
>> bpf_prog01
>> +bpf_prog02
>> diff --git a/testcases/kernel/syscalls/bpf/bpf_prog02.c b/testcases/kernel/syscalls/bpf/bpf_prog02.c
>> new file mode 100644
>> index 000000000..dc8b92f00
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/bpf/bpf_prog02.c
>> @@ -0,0 +1,182 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2019 Richard Palethorpe <rpalethorpe@suse.com>
>> + *
>> + * Check if eBPF can do arithmetic with 64bits. This targets a specific
>> + * regression which only effects unprivileged users who are subject to extra
>> + * pointer arithmetic checks during verification.
>> + *
>> + * Fixed by commit 3612af783cf52c74a031a2f11b82247b2599d3cd.
>> + * https://new.blog.cloudflare.com/ebpf-cant-count/
>> + *
>> + * This test is very similar in structure to bpf_prog01 which is better
>> + * annotated.
>> + */
>> +
>> +#include <limits.h>
>> +#include <string.h>
>> +#include <stdio.h>
>> +
>> +#include "config.h"
>> +#include "tst_test.h"
>> +#include "tst_capability.h"
>> +#include "lapi/socket.h"
>> +#include "lapi/bpf.h"
>> +
>> +#define A64INT (((uint64_t)1) << 60)
>> +
>> +const char MSG[] = "Ahoj!";
>> +static char *msg;
>> +
>> +static char *log;
>> +static uint32_t *key;
>> +static uint64_t *val;
>> +static union bpf_attr *attr;
>> +
>> +static int load_prog(int fd)
>> +{
>> + struct bpf_insn *prog;
>> + struct bpf_insn insn[] = {
>> + BPF_MOV64_IMM(BPF_REG_6, 1), /* 0: r6 = 1 */
>> +
>> + BPF_LD_MAP_FD(BPF_REG_1, fd), /* 1: r1 = &fd */
>> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), /* 3: r2 = fp */
>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), /* 4: r2 = r2 - 8 */
>> + BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0), /* 5: *r2 = 0 */
>> + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 6: map_lookup_elem */
>> + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 17), /* 7: if(!r0) goto 25 */
>> + BPF_MOV64_REG(BPF_REG_3, BPF_REG_0), /* 8: r3 = r0 */
>> + BPF_LD_IMM64(BPF_REG_4, A64INT), /* 9: r4 = 2^61 */
>> + BPF_ALU64_REG(BPF_ADD, BPF_REG_4, BPF_REG_6), /* 11: r4 += r6 */
>> + BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 12: *r3 = r4 */
>> +
>> + BPF_LD_MAP_FD(BPF_REG_1, fd), /* 13: r1 = &fd */
>> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), /* 15: r2 = fp */
>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), /* 16: r2 = r2 - 8 */
>> + BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 1), /* 17: *r2 = 1 */
>> + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 18: map_lookup_elem */
>> + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5), /* 19: if(!r0) goto 25 */
>> + BPF_MOV64_REG(BPF_REG_3, BPF_REG_0), /* 20: r3 = r0 */
>> + BPF_LD_IMM64(BPF_REG_4, A64INT), /* 21: r4 = 2^61 */
>> + BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23: r4 -= r6 */
>> + BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24: *r3 = r4 */
>> +
>> + BPF_MOV64_IMM(BPF_REG_0, 0), /* 25: r0 = 0 */
>> + BPF_EXIT_INSN(), /* 26: return r0 */
>> + };
>> +
>> + /* Leaks memory when -i is specified */
>> + prog = tst_alloc(sizeof(insn));
>
> Why not just declare the prog static? Then we could do:
>
> if (!prog)
> prog = tst_alloc(sizeof(insn));
OK!
>
>> + memcpy(prog, insn, sizeof(insn));
>> +
>> + memset(attr, 0, sizeof(*attr));
>> + attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
>> + attr->insns = ptr_to_u64(prog);
>> + attr->insn_cnt = ARRAY_SIZE(insn);
>> + attr->license = ptr_to_u64("GPL");
>> + attr->log_buf = ptr_to_u64(log);
>> + attr->log_size = BUFSIZ;
>> + attr->log_level = 1;
>> +
>> + TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
>> + if (TST_RET == -1) {
>> + if (log[0] != 0) {
>> + tst_res(TINFO, "Verification log:");
>> + fputs(log, stderr);
>
> I guess that we can do tst_res(TINFO, "Verification log:\n%s", log);
> instead.
Nope, the log is often too long for tst_res and is truncated.
>
>> + tst_brk(TBROK | TTERRNO, "Failed verification");
>> + } else {
>> + tst_brk(TBROK | TTERRNO, "Failed to load program");
>> + }
>> + }
>> +
>> + return TST_RET;
>> +}
>> +
>> +static void setup(void)
>> +{
>> + memcpy(msg, MSG, sizeof(MSG));
>> +}
>> +
>> +static void run(void)
>> +{
>> + int map_fd, prog_fd;
>> + int sk[2];
>> +
>> + memset(attr, 0, sizeof(*attr));
>> + attr->map_type = BPF_MAP_TYPE_ARRAY;
>> + attr->key_size = 4;
>> + attr->value_size = 8;
>> + attr->max_entries = 2;
>> +
>> + TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
>> + if (TST_RET == -1) {
>> + if (TST_ERR == EPERM) {
>> + tst_brk(TCONF | TTERRNO,
>> + "bpf() requires CAP_SYS_ADMIN on this system");
>> + } else {
>> + tst_brk(TBROK | TTERRNO, "Failed to create array map");
>> + }
>> + }
>> + map_fd = TST_RET;
>> +
>> + prog_fd = load_prog(map_fd);
>> +
>> + SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
>> + SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
>> + &prog_fd, sizeof(prog_fd));
>> +
>> + SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
>> +
>> + memset(attr, 0, sizeof(*attr));
>> + attr->map_fd = map_fd;
>> + attr->key = ptr_to_u64(key);
>> + attr->value = ptr_to_u64(val);
>> + *key = 0;
>> +
>> + TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
>> + if (TST_RET == -1) {
>> + tst_res(TFAIL | TTERRNO, "array map lookup");
>> + } else if (*val != A64INT + 1) {
>> + tst_res(TFAIL,
>> + "val = %lu, but should be val = %lu + 1",
>> + *val, A64INT);
>> + } else {
>> + tst_res(TPASS, "val = %lu + 1", A64INT);
>> + }
>
> Wrong indentation, also I do not fancy this if else maze. I guess that
> we can can safely do goto exit; if map lookup fails that would point
> right before the SAFE_CLOSE() block.
OK!
>
>> + *key = 1;
>> +
>> + TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
>> + if (TST_RET == -1) {
>> + tst_res(TFAIL | TTERRNO, "array map lookup");
>> + } else if (*val != A64INT - 1) {
>> + tst_res(TFAIL,
>> + "val = %lu, but should be val = %lu - 1",
>> + *val, A64INT);
>> + } else {
>> + tst_res(TPASS, "val = %lu - 1", A64INT);
>> + }
>
>
> Here as well.
OK!
>
>> + SAFE_CLOSE(prog_fd);
>> + SAFE_CLOSE(map_fd);
>> + SAFE_CLOSE(sk[0]);
>> + SAFE_CLOSE(sk[1]);
>> +}
>> +
>> +static struct tst_test test = {
>> + .setup = setup,
>> + .test_all = run,
>> + .min_kver = "3.18",
>> + .caps = (struct tst_cap []) {
>> + TST_CAP(TST_CAP_DROP, CAP_SYS_ADMIN),
>> + {}
>> + },
>> + .bufs = (struct tst_buffers []) {
>> + {&key, .size = sizeof(*key)},
>> + {&val, .size = sizeof(*val)},
>> + {&log, .size = BUFSIZ},
>> + {&attr, .size = sizeof(*attr)},
>> + {&msg, .size = sizeof(MSG)},
>> + {},
>> + }
>> +};
>> --
>> 2.22.1
>>
--
Thank you,
Richard.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [LTP] [PATCH v3] BPF: Regression test for 64bit arithmetic
2019-09-11 9:54 [LTP] [PATCH v3] BPF: Regression test for 64bit arithmetic Richard Palethorpe
2019-09-12 14:56 ` Cyril Hrubis
@ 2019-09-13 10:55 ` Clemens Famulla-Conrad
2019-09-13 11:46 ` Cyril Hrubis
1 sibling, 1 reply; 7+ messages in thread
From: Clemens Famulla-Conrad @ 2019-09-13 10:55 UTC (permalink / raw)
To: ltp
Hi,
agree with comments from Cyril. And add two tiny one.
Reviewed-by: Clemens Famulla-Conrad<cfamullaconrad@suse.de>
On Wed, 2019-09-11 at 11:54 +0200, Richard Palethorpe wrote:
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Reviewed-by: Jan Stancek <jstancek@redhat.com>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>
> V3: Rebased on master and included line numbers in instructions
>
> Capability patch has not been applied to master at time of rebase.
>
> include/lapi/bpf.h | 27 +++
> runtest/syscalls | 1 +
> testcases/kernel/syscalls/bpf/.gitignore | 1 +
> testcases/kernel/syscalls/bpf/bpf_prog02.c | 182
> +++++++++++++++++++++
> 4 files changed, 211 insertions(+)
> create mode 100644 testcases/kernel/syscalls/bpf/bpf_prog02.c
>
> diff --git a/include/lapi/bpf.h b/include/lapi/bpf.h
> index 122eb5469..03073b45e 100644
> --- a/include/lapi/bpf.h
> +++ b/include/lapi/bpf.h
> @@ -18,7 +18,9 @@
> /* Start copy from linux/bpf_(common).h */
> #define BPF_CLASS(code) ((code) & 0x07)
> #define BPF_LD 0x00
> +#define BPF_LDX 0x01
> #define BPF_ST 0x02
> +#define BPF_STX 0x03
> #define BPF_JMP 0x05
>
> #define BPF_SIZE(code) ((code) & 0x18)
> @@ -30,6 +32,7 @@
>
> #define BPF_OP(code) ((code) & 0xf0)
> #define BPF_ADD 0x00
> +#define BPF_SUB 0x10
>
> #define BPF_JEQ 0x10
>
> @@ -432,6 +435,14 @@ enum bpf_func_id {
>
> /* Start copy from tools/include/filter.h */
>
> +#define BPF_ALU64_REG(OP, DST, SRC) \
> + ((struct bpf_insn) {
> \
> + .code = BPF_ALU64 | BPF_OP(OP) | BPF_X, \
> + .dst_reg = DST,
> \
> + .src_reg = SRC,
> \
> + .off = 0, \
> + .imm = 0 })
> +
> #define BPF_ALU64_IMM(OP, DST, IMM) \
> ((struct bpf_insn) {
> \
> .code = BPF_ALU64 | BPF_OP(OP) | BPF_K, \
> @@ -477,6 +488,22 @@ enum bpf_func_id {
> .off = OFF,
> \
> .imm = IMM })
>
> +#define BPF_LDX_MEM(SIZE, DST, SRC, OFF) \
> + ((struct bpf_insn) {
> \
> + .code = BPF_LDX | BPF_SIZE(SIZE) | BPF_MEM,
> \
> + .dst_reg = DST,
> \
> + .src_reg = SRC,
> \
> + .off = OFF,
> \
> + .imm = 0 })
> +
> +#define BPF_STX_MEM(SIZE, DST, SRC, OFF) \
> + ((struct bpf_insn) {
> \
> + .code = BPF_STX | BPF_SIZE(SIZE) | BPF_MEM,
> \
> + .dst_reg = DST,
> \
> + .src_reg = SRC,
> \
> + .off = OFF,
> \
> + .imm = 0 })
> +
> #define BPF_JMP_IMM(OP, DST, IMM, OFF)
> \
> ((struct bpf_insn) {
> \
> .code = BPF_JMP | BPF_OP(OP) | BPF_K,
> \
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 874ae4d4f..4e6310193 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -34,6 +34,7 @@ bind03 bind03
>
> bpf_map01 bpf_map01
> bpf_prog01 bpf_prog01
> +bpf_prog02 bpf_prog02
>
> brk01 brk01
>
> diff --git a/testcases/kernel/syscalls/bpf/.gitignore
> b/testcases/kernel/syscalls/bpf/.gitignore
> index 7eb5f7c92..1704f9841 100644
> --- a/testcases/kernel/syscalls/bpf/.gitignore
> +++ b/testcases/kernel/syscalls/bpf/.gitignore
> @@ -1,2 +1,3 @@
> bpf_map01
> bpf_prog01
> +bpf_prog02
> diff --git a/testcases/kernel/syscalls/bpf/bpf_prog02.c
> b/testcases/kernel/syscalls/bpf/bpf_prog02.c
> new file mode 100644
> index 000000000..dc8b92f00
> --- /dev/null
> +++ b/testcases/kernel/syscalls/bpf/bpf_prog02.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2019 Richard Palethorpe <rpalethorpe@suse.com>
> + *
> + * Check if eBPF can do arithmetic with 64bits. This targets a
> specific
> + * regression which only effects unprivileged users who are subject
> to extra
> + * pointer arithmetic checks during verification.
> + *
> + * Fixed by commit 3612af783cf52c74a031a2f11b82247b2599d3cd.
> + * https://new.blog.cloudflare.com/ebpf-cant-count/
> + *
> + * This test is very similar in structure to bpf_prog01 which is
> better
> + * annotated.
> + */
> +
> +#include <limits.h>
> +#include <string.h>
> +#include <stdio.h>
> +
> +#include "config.h"
> +#include "tst_test.h"
> +#include "tst_capability.h"
> +#include "lapi/socket.h"
> +#include "lapi/bpf.h"
> +
> +#define A64INT (((uint64_t)1) << 60)
> +
> +const char MSG[] = "Ahoj!";
> +static char *msg;
> +
> +static char *log;
> +static uint32_t *key;
> +static uint64_t *val;
> +static union bpf_attr *attr;
> +
> +static int load_prog(int fd)
> +{
> + struct bpf_insn *prog;
> + struct bpf_insn insn[] = {
> + BPF_MOV64_IMM(BPF_REG_6, 1), /* 0: r6 = 1
> */
> +
> + BPF_LD_MAP_FD(BPF_REG_1, fd), /* 1:
> r1 = &fd */
> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), /* 3: r2 =
> fp */
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), /* 4: r2 =
> r2 - 8 */
> + BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0), /* 5: *r2 =
> 0 */
> + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 6:
> map_lookup_elem */
> + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 17), /* 7:
> if(!r0) goto 25 */
> + BPF_MOV64_REG(BPF_REG_3, BPF_REG_0), /* 8: r3 =
> r0 */
> + BPF_LD_IMM64(BPF_REG_4, A64INT), /* 9: r4 =
> 2^61 */
> + BPF_ALU64_REG(BPF_ADD, BPF_REG_4, BPF_REG_6), /* 11:
> r4 += r6 */
> + BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 12:
> *r3 = r4 */
> +
> + BPF_LD_MAP_FD(BPF_REG_1, fd), /* 13:
> r1 = &fd */
> + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), /* 15: r2 =
> fp */
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), /* 16: r2 =
> r2 - 8 */
> + BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 1), /* 17: *r2 =
> 1 */
> + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 18:
> map_lookup_elem */
> + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5), /* 19:
> if(!r0) goto 25 */
> + BPF_MOV64_REG(BPF_REG_3, BPF_REG_0), /* 20: r3 =
> r0 */
> + BPF_LD_IMM64(BPF_REG_4, A64INT), /* 21: r4 =
> 2^61 */
^
I think 2^61 is different to 1<<60
> + BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23:
> r4 -= r6 */
> + BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24:
> *r3 = r4 */
> +
> + BPF_MOV64_IMM(BPF_REG_0, 0), /* 25: r0 =
> 0 */
> + BPF_EXIT_INSN(), /* 26:
> return r0 */
> + };
> +
> + /* Leaks memory when -i is specified */
> + prog = tst_alloc(sizeof(insn));
> + memcpy(prog, insn, sizeof(insn));
> +
> + memset(attr, 0, sizeof(*attr));
> + attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> + attr->insns = ptr_to_u64(prog);
> + attr->insn_cnt = ARRAY_SIZE(insn);
> + attr->license = ptr_to_u64("GPL");
> + attr->log_buf = ptr_to_u64(log);
> + attr->log_size = BUFSIZ;
> + attr->log_level = 1;
> +
> + TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
> + if (TST_RET == -1) {
> + if (log[0] != 0) {
> + tst_res(TINFO, "Verification log:");
> + fputs(log, stderr);
> + tst_brk(TBROK | TTERRNO, "Failed
> verification");
> + } else {
> + tst_brk(TBROK | TTERRNO, "Failed to load
> program");
> + }
> + }
> +
> + return TST_RET;
> +}
> +
> +static void setup(void)
> +{
> + memcpy(msg, MSG, sizeof(MSG));
> +}
> +
> +static void run(void)
> +{
> + int map_fd, prog_fd;
> + int sk[2];
> +
> + memset(attr, 0, sizeof(*attr));
> + attr->map_type = BPF_MAP_TYPE_ARRAY;
> + attr->key_size = 4;
> + attr->value_size = 8;
> + attr->max_entries = 2;
> +
> + TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
> + if (TST_RET == -1) {
> + if (TST_ERR == EPERM) {
> + tst_brk(TCONF | TTERRNO,
> + "bpf() requires CAP_SYS_ADMIN on
> this system");
> + } else {
> + tst_brk(TBROK | TTERRNO, "Failed to create
> array map");
> + }
> + }
> + map_fd = TST_RET;
> +
> + prog_fd = load_prog(map_fd);
> +
> + SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
> + SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
> + &prog_fd, sizeof(prog_fd));
> +
> + SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
> +
> + memset(attr, 0, sizeof(*attr));
> + attr->map_fd = map_fd;
> + attr->key = ptr_to_u64(key);
> + attr->value = ptr_to_u64(val);
> + *key = 0;
> +
> + TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
> + if (TST_RET == -1) {
> + tst_res(TFAIL | TTERRNO, "array map lookup");
> + } else if (*val != A64INT + 1) {
> + tst_res(TFAIL,
> + "val = %lu, but should be val = %lu + 1",
^
Not sure if it is really needed but I would use %llu here. If so, other
places as well.
> + *val, A64INT);
> + } else {
> + tst_res(TPASS, "val = %lu + 1", A64INT);
> + }
> +
> + *key = 1;
> +
> + TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
> + if (TST_RET == -1) {
> + tst_res(TFAIL | TTERRNO, "array map lookup");
> + } else if (*val != A64INT - 1) {
> + tst_res(TFAIL,
> + "val = %lu, but should be val = %lu - 1",
> + *val, A64INT);
> + } else {
> + tst_res(TPASS, "val = %lu - 1", A64INT);
> + }
> +
> + SAFE_CLOSE(prog_fd);
> + SAFE_CLOSE(map_fd);
> + SAFE_CLOSE(sk[0]);
> + SAFE_CLOSE(sk[1]);
> +}
> +
> +static struct tst_test test = {
> + .setup = setup,
> + .test_all = run,
> + .min_kver = "3.18",
> + .caps = (struct tst_cap []) {
> + TST_CAP(TST_CAP_DROP, CAP_SYS_ADMIN),
> + {}
> + },
> + .bufs = (struct tst_buffers []) {
> + {&key, .size = sizeof(*key)},
> + {&val, .size = sizeof(*val)},
> + {&log, .size = BUFSIZ},
> + {&attr, .size = sizeof(*attr)},
> + {&msg, .size = sizeof(MSG)},
> + {},
> + }
> +};
> --
> 2.22.1
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* [LTP] [PATCH v3] BPF: Regression test for 64bit arithmetic
2019-09-13 10:55 ` Clemens Famulla-Conrad
@ 2019-09-13 11:46 ` Cyril Hrubis
2019-09-17 7:27 ` Richard Palethorpe
0 siblings, 1 reply; 7+ messages in thread
From: Cyril Hrubis @ 2019-09-13 11:46 UTC (permalink / raw)
To: ltp
Hi!
> > + BPF_LD_IMM64(BPF_REG_4, A64INT), /* 21: r4 =
> > 2^61 */
> ^
> I think 2^61 is different to 1<<60
Agreed, 1<<60 is 2^60
> > + BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23:
> > r4 -= r6 */
> > + BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24:
> > *r3 = r4 */
> > +
> > + BPF_MOV64_IMM(BPF_REG_0, 0), /* 25: r0 =
> > 0 */
> > + BPF_EXIT_INSN(), /* 26:
> > return r0 */
> > + };
> > +
> > + /* Leaks memory when -i is specified */
> > + prog = tst_alloc(sizeof(insn));
> > + memcpy(prog, insn, sizeof(insn));
> > +
> > + memset(attr, 0, sizeof(*attr));
> > + attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> > + attr->insns = ptr_to_u64(prog);
> > + attr->insn_cnt = ARRAY_SIZE(insn);
> > + attr->license = ptr_to_u64("GPL");
> > + attr->log_buf = ptr_to_u64(log);
> > + attr->log_size = BUFSIZ;
> > + attr->log_level = 1;
> > +
> > + TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
> > + if (TST_RET == -1) {
> > + if (log[0] != 0) {
> > + tst_res(TINFO, "Verification log:");
> > + fputs(log, stderr);
> > + tst_brk(TBROK | TTERRNO, "Failed
> > verification");
> > + } else {
> > + tst_brk(TBROK | TTERRNO, "Failed to load
> > program");
> > + }
> > + }
> > +
> > + return TST_RET;
> > +}
> > +
> > +static void setup(void)
> > +{
> > + memcpy(msg, MSG, sizeof(MSG));
> > +}
> > +
> > +static void run(void)
> > +{
> > + int map_fd, prog_fd;
> > + int sk[2];
> > +
> > + memset(attr, 0, sizeof(*attr));
> > + attr->map_type = BPF_MAP_TYPE_ARRAY;
> > + attr->key_size = 4;
> > + attr->value_size = 8;
> > + attr->max_entries = 2;
> > +
> > + TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
> > + if (TST_RET == -1) {
> > + if (TST_ERR == EPERM) {
> > + tst_brk(TCONF | TTERRNO,
> > + "bpf() requires CAP_SYS_ADMIN on
> > this system");
> > + } else {
> > + tst_brk(TBROK | TTERRNO, "Failed to create
> > array map");
> > + }
> > + }
> > + map_fd = TST_RET;
> > +
> > + prog_fd = load_prog(map_fd);
> > +
> > + SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
> > + SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
> > + &prog_fd, sizeof(prog_fd));
> > +
> > + SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
> > +
> > + memset(attr, 0, sizeof(*attr));
> > + attr->map_fd = map_fd;
> > + attr->key = ptr_to_u64(key);
> > + attr->value = ptr_to_u64(val);
> > + *key = 0;
> > +
> > + TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
> > + if (TST_RET == -1) {
> > + tst_res(TFAIL | TTERRNO, "array map lookup");
> > + } else if (*val != A64INT + 1) {
> > + tst_res(TFAIL,
> > + "val = %lu, but should be val = %lu + 1",
> ^
> Not sure if it is really needed but I would use %llu here. If so, other
> places as well.
Technically we should be using PRIu64 which expands to %lu on 64 bit and
to %llu on 32 bit. But given that sizeof(long long) == sizeof(long) on
64 bit on Linux %llu should work as well.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 7+ messages in thread* [LTP] [PATCH v3] BPF: Regression test for 64bit arithmetic
2019-09-13 11:46 ` Cyril Hrubis
@ 2019-09-17 7:27 ` Richard Palethorpe
2019-09-27 11:08 ` Cyril Hrubis
0 siblings, 1 reply; 7+ messages in thread
From: Richard Palethorpe @ 2019-09-17 7:27 UTC (permalink / raw)
To: ltp
Hello,
Cyril Hrubis <chrubis@suse.cz> writes:
> Hi!
>> > + BPF_LD_IMM64(BPF_REG_4, A64INT), /* 21: r4 =
>> > 2^61 */
>> ^
>> I think 2^61 is different to 1<<60
>
> Agreed, 1<<60 is 2^60
>
>> > + BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23:
>> > r4 -= r6 */
>> > + BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24:
>> > *r3 = r4 */
>> > +
>> > + BPF_MOV64_IMM(BPF_REG_0, 0), /* 25: r0 =
>> > 0 */
>> > + BPF_EXIT_INSN(), /* 26:
>> > return r0 */
>> > + };
>> > +
>> > + /* Leaks memory when -i is specified */
>> > + prog = tst_alloc(sizeof(insn));
>> > + memcpy(prog, insn, sizeof(insn));
>> > +
>> > + memset(attr, 0, sizeof(*attr));
>> > + attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
>> > + attr->insns = ptr_to_u64(prog);
>> > + attr->insn_cnt = ARRAY_SIZE(insn);
>> > + attr->license = ptr_to_u64("GPL");
>> > + attr->log_buf = ptr_to_u64(log);
>> > + attr->log_size = BUFSIZ;
>> > + attr->log_level = 1;
>> > +
>> > + TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
>> > + if (TST_RET == -1) {
>> > + if (log[0] != 0) {
>> > + tst_res(TINFO, "Verification log:");
>> > + fputs(log, stderr);
>> > + tst_brk(TBROK | TTERRNO, "Failed
>> > verification");
>> > + } else {
>> > + tst_brk(TBROK | TTERRNO, "Failed to load
>> > program");
>> > + }
>> > + }
>> > +
>> > + return TST_RET;
>> > +}
>> > +
>> > +static void setup(void)
>> > +{
>> > + memcpy(msg, MSG, sizeof(MSG));
>> > +}
>> > +
>> > +static void run(void)
>> > +{
>> > + int map_fd, prog_fd;
>> > + int sk[2];
>> > +
>> > + memset(attr, 0, sizeof(*attr));
>> > + attr->map_type = BPF_MAP_TYPE_ARRAY;
>> > + attr->key_size = 4;
>> > + attr->value_size = 8;
>> > + attr->max_entries = 2;
>> > +
>> > + TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
>> > + if (TST_RET == -1) {
>> > + if (TST_ERR == EPERM) {
>> > + tst_brk(TCONF | TTERRNO,
>> > + "bpf() requires CAP_SYS_ADMIN on
>> > this system");
>> > + } else {
>> > + tst_brk(TBROK | TTERRNO, "Failed to create
>> > array map");
>> > + }
>> > + }
>> > + map_fd = TST_RET;
>> > +
>> > + prog_fd = load_prog(map_fd);
>> > +
>> > + SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
>> > + SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
>> > + &prog_fd, sizeof(prog_fd));
>> > +
>> > + SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
>> > +
>> > + memset(attr, 0, sizeof(*attr));
>> > + attr->map_fd = map_fd;
>> > + attr->key = ptr_to_u64(key);
>> > + attr->value = ptr_to_u64(val);
>> > + *key = 0;
>> > +
>> > + TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
>> > + if (TST_RET == -1) {
>> > + tst_res(TFAIL | TTERRNO, "array map lookup");
>> > + } else if (*val != A64INT + 1) {
>> > + tst_res(TFAIL,
>> > + "val = %lu, but should be val = %lu + 1",
>> ^
>> Not sure if it is really needed but I would use %llu here. If so, other
>> places as well.
>
> Technically we should be using PRIu64 which expands to %lu on 64 bit and
> to %llu on 32 bit. But given that sizeof(long long) == sizeof(long) on
> 64 bit on Linux %llu should work as well.
Ack.
--
Thank you,
Richard.
^ permalink raw reply [flat|nested] 7+ messages in thread* [LTP] [PATCH v3] BPF: Regression test for 64bit arithmetic
2019-09-17 7:27 ` Richard Palethorpe
@ 2019-09-27 11:08 ` Cyril Hrubis
0 siblings, 0 replies; 7+ messages in thread
From: Cyril Hrubis @ 2019-09-27 11:08 UTC (permalink / raw)
To: ltp
Hi!
> > Technically we should be using PRIu64 which expands to %lu on 64 bit and
> > to %llu on 32 bit. But given that sizeof(long long) == sizeof(long) on
> > 64 bit on Linux %llu should work as well.
>
> Ack.
In the end I've pushed this with the PRIu64 since the compiler was
throwing warnings for %llu on 64 bit.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-27 11:08 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-09-11 9:54 [LTP] [PATCH v3] BPF: Regression test for 64bit arithmetic Richard Palethorpe
2019-09-12 14:56 ` Cyril Hrubis
2019-09-17 7:26 ` Richard Palethorpe
2019-09-13 10:55 ` Clemens Famulla-Conrad
2019-09-13 11:46 ` Cyril Hrubis
2019-09-17 7:27 ` Richard Palethorpe
2019-09-27 11:08 ` Cyril Hrubis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox