On 08/06/2023 10:46, Zachary Leaf wrote:
Add a simple and minimal example calling only BPF_PROG_LOAD via a bpf syscall.
In addition check kernel/bpf/syscall.c:CHECK_ATTR macro implementation. The syscall should fail when we have non-zero memory in the union beyond the last element of the active sub-command struct.
Signed-off-by: Zachary Leaf zachary.leaf@arm.com
testcases/kernel/syscalls/bpf/bpf_prog_load.c | 108 ++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 testcases/kernel/syscalls/bpf/bpf_prog_load.c
diff --git a/testcases/kernel/syscalls/bpf/bpf_prog_load.c b/testcases/kernel/syscalls/bpf/bpf_prog_load.c new file mode 100644 index 000000000..33a9ef70e --- /dev/null +++ b/testcases/kernel/syscalls/bpf/bpf_prog_load.c @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*
- Copyright (c) Arm Ltd. 2022. All rights reserved.
- Author: Zachary Leaf zachary.leaf@arm.com
- */
+/*\
- minimal example, testing only BPF_PROG_LOAD option of bpf syscall
- */
+#include <limits.h> +#include <string.h> +#include <stdio.h> +#include <stddef.h>
+#include "config.h" +#include "tst_test.h" +#include "lapi/bpf.h"
+#define LOG_BUF_SIZE 4096 +static char *bpf_log_buf; +static union bpf_attr *attr;
+// as per BPF_PROG_LOAD section of man 2 bpf +int +bpf_prog_load(enum bpf_prog_type type,
const struct bpf_insn *insns, int insn_cnt)+{
- memset(attr, 0, sizeof(*attr));
- attr->prog_type = type;
- attr->insns = (uintptr_t)insns;
- attr->insn_cnt = insn_cnt;
- attr->license = (uintptr_t)"GPL";
- attr->log_buf = (uintptr_t)bpf_log_buf;
- attr->log_size = LOG_BUF_SIZE;
- attr->log_level = 1;
- char name[16] = "minimal_example";
- strncpy(attr->prog_name, name, 16);
Couldn't this be done using bpf_init_prog_attr()?
- printf("sizeof(bpf_attr): %lu\n", sizeof(union bpf_attr));
- printf("insns: %#p offset:%#lx\n", (void *)attr->insns, offsetof(union bpf_attr, insns));
- printf("license: %#p offset:%#lx\n", (void *)attr->license, offsetof(union bpf_attr, license));
- printf("log_buf: %#p offset:%#lx\n", (void *)attr->log_buf, offsetof(union bpf_attr, log_buf));
- printf("prog_name: %s offset:%#lx\n", (char *)attr->prog_name, offsetof(union bpf_attr, prog_name));
- /* test CHECK_ATTR
* check syscall fails if there is non-null data somewhere beyond* the last struct member for the BPF_PROG_LOAD option*/- printf("offset of core_relo_rec_size: %#lx\n", offsetof(union bpf_attr, core_relo_rec_size));
- size_t offset = offsetof(union bpf_attr, core_relo_rec_size)
+ sizeof(uint32_t) + 1;
It seems to me that LTP uses C89-style declarations like the kernel, i.e. all declarations must be at the beginning of their block (without any statement in between). However unlike the kernel -Wdeclaration-after-statement is not used, so hard to tell whether it's a hard rule or not...
- char *ptr = (char *)attr;
- *(ptr+offset) = 'x';
- TST_EXP_FAIL(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)), EINVAL);
- *(ptr+offset) = '\0';
- const int ret = bpf(BPF_PROG_LOAD, attr, sizeof(*attr));
- if (ret >= 0) {
tst_res(TPASS, "Loaded program");return ret;- }
- if (ret != -1)
tst_brk(TBROK, "Invalid bpf() return value: %d", ret);- else
tst_brk(TBROK, "Invalid bpf() return value: %d", ret);
Hmm, looks like that else should be removed. I'm not sure the if is that useful either, it's essentially testing that the bpf() wrapper is implemented correctly, which is not really the point of LTP tests.
- if (bpf_log_buf[0] != 0) {
tst_printf("%s\n", bpf_log_buf);tst_brk(TBROK | TERRNO, "Failed verification");- }
- tst_brk(TBROK | TERRNO, "Failed to load program");
- return ret;
+}
+void run(void) +{
- unsigned char raw_bpf[] = {
- 0xb7, 0x01, 0x00, 0x00, 0x0a, 0x00, 0x00, 0x00, 0x6b, 0x1a, 0xfc, 0xff,
- 0x00, 0x00, 0x00, 0x00, 0xb7, 0x01, 0x00, 0x00, 0x7a, 0x6f, 0x72, 0x67,
- 0x63, 0x1a, 0xf8, 0xff, 0x00, 0x00, 0x00, 0x00, 0xbf, 0xa1, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x07, 0x01, 0x00, 0x00, 0xf8, 0xff, 0xff, 0xff,
- 0xb7, 0x02, 0x00, 0x00, 0x06, 0x00, 0x00, 0x00, 0x85, 0x00, 0x00, 0x00,
- 0x06, 0x00, 0x00, 0x00, 0xb7, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
- 0x95, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
- };
Is this just a long no-op? I'm trying to figure out what this test is adding compared to e.g. bpf_prog02, besides the CHECK_ATTR test.
- unsigned int raw_bpf_len = 80;
Is that sizeof(raw_bpf)?
- struct bpf_insn *insn;
- insn = (struct bpf_insn*)raw_bpf;
Should be just one line (declaration after statement).
- int prog_fd = bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, insn,
raw_bpf_len/sizeof(struct bpf_insn));- SAFE_CLOSE(prog_fd);
+}
+static struct tst_test test = {
- .test_all = run,
- .min_kver = "3.19",
Just curious, did you arrive at that version in a particular way?
Kevin
- .bufs = (struct tst_buffers []) {
{&bpf_log_buf, .size = LOG_BUF_SIZE},{&attr, .size = sizeof(*attr)},{},- }
+};