On 21/07/2023 08:19, Kevin Brodsky wrote:
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()?
Yeah - in hindsight I should have taken more time to create a proper test aligned with the other LTP tests. This was originally some experimental code I had/was using to test the basic functionality and just quickly made it work with LTP.
I've fixed it up to be more consistent with normal LTP tests i.e. using bpf_common.c and that also makes the test significantly shorter and clearer.
- 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...
Ack
- 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.
Yeah - essentially a nop, CHECK_ATTR is the only novel part. The only other thing this test is doing is checking if it can load the simple program as above once we fix the malformed bpf_attr. This is checked implicitly by all other tests, but it's nice here just to confirm it was the non-null data that caused BPF_PROG_LOAD to fail with EINVAL.
- 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?
Nope - just copied from one of the other bpf_* tests.
Now I think about it, the v2 of this test now relies on log_true_size being the last member of the BPF_PROG_LOAD struct - and this field was added in 47a71c1f9af0 ("bpf: Add log_true_size output field to return necessary log buffer size") which didn't appear until 6.4.
$ git describe --contains 47a71c1f9af0a334c9dfa97633c41de4feda4287 v6.4-rc1~77^2~118^2~19^2~7
So I've changed to 6.4 in v2.
Kevin
- .bufs = (struct tst_buffers []) {
{&bpf_log_buf, .size = LOG_BUF_SIZE},{&attr, .size = sizeof(*attr)},{},- }
+};