| Message ID | 20210508034837.64585-8-alexei.starovoitov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: syscall program, FD array, loader program, light skeleton. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 8 maintainers not CCed: linux-kselftest@vger.kernel.org netdev@vger.kernel.org yhs@fb.com kpsingh@kernel.org kafai@fb.com ast@kernel.org songliubraving@fb.com shuah@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | fail | ERROR: Macros with complex values should be enclosed in parentheses WARNING: line length of 81 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Fri, May 7, 2021 at 8:48 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > From: Alexei Starovoitov <ast@kernel.org> > > Improve selftest to check that btf_load is working from bpf program. > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > --- > tools/testing/selftests/bpf/progs/syscall.c | 48 +++++++++++++++++++++ > 1 file changed, 48 insertions(+) > [...] > SEC("syscall") > int bpf_prog(struct args *ctx) > { > @@ -33,6 +73,8 @@ int bpf_prog(struct args *ctx) > .map_type = BPF_MAP_TYPE_HASH, > .key_size = 8, > .value_size = 8, > + .btf_key_type_id = 1, > + .btf_value_type_id = 2, > }; > static union bpf_attr map_update_attr = { .map_fd = 1, }; > static __u64 key = 12; > @@ -43,7 +85,13 @@ int bpf_prog(struct args *ctx) > }; > int ret; > > + ret = btf_load(); Maybe let's move patch #11 (bpf_sys_close() helper) in front of these selftests and call bpf_sys_close() appropriately on error and (if success) after map is created? > + if (ret < 0) > + return ret; > + > map_create_attr.max_entries = ctx->max_entries; > + map_create_attr.btf_fd = ret; > + > prog_load_attr.license = (long) license; > prog_load_attr.insns = (long) insns; > prog_load_attr.log_buf = ctx->log_buf; > -- > 2.30.2 >
On 5/11/21 3:45 PM, Andrii Nakryiko wrote: > On Fri, May 7, 2021 at 8:48 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: >> >> From: Alexei Starovoitov <ast@kernel.org> >> >> Improve selftest to check that btf_load is working from bpf program. >> >> Signed-off-by: Alexei Starovoitov <ast@kernel.org> >> --- >> tools/testing/selftests/bpf/progs/syscall.c | 48 +++++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> > > [...] > >> SEC("syscall") >> int bpf_prog(struct args *ctx) >> { >> @@ -33,6 +73,8 @@ int bpf_prog(struct args *ctx) >> .map_type = BPF_MAP_TYPE_HASH, >> .key_size = 8, >> .value_size = 8, >> + .btf_key_type_id = 1, >> + .btf_value_type_id = 2, >> }; >> static union bpf_attr map_update_attr = { .map_fd = 1, }; >> static __u64 key = 12; >> @@ -43,7 +85,13 @@ int bpf_prog(struct args *ctx) >> }; >> int ret; >> >> + ret = btf_load(); > > Maybe let's move patch #11 (bpf_sys_close() helper) in front of these > selftests and call bpf_sys_close() appropriately on error and (if > success) after map is created? Interesting idea. I took a stab at it, but it's not unit-test like. That bpf_sys_close is going to be used assuming it's working. I'd rather add explicit test for bpf_sys_close eventually instead of mixing the two. Since your concern is fd leak I've added btf_fd to context instead and added explicit close() in user space.
On Tue, May 11, 2021 at 9:05 PM Alexei Starovoitov <ast@fb.com> wrote: > > On 5/11/21 3:45 PM, Andrii Nakryiko wrote: > > On Fri, May 7, 2021 at 8:48 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > >> > >> From: Alexei Starovoitov <ast@kernel.org> > >> > >> Improve selftest to check that btf_load is working from bpf program. > >> > >> Signed-off-by: Alexei Starovoitov <ast@kernel.org> > >> --- > >> tools/testing/selftests/bpf/progs/syscall.c | 48 +++++++++++++++++++++ > >> 1 file changed, 48 insertions(+) > >> > > > > [...] > > > >> SEC("syscall") > >> int bpf_prog(struct args *ctx) > >> { > >> @@ -33,6 +73,8 @@ int bpf_prog(struct args *ctx) > >> .map_type = BPF_MAP_TYPE_HASH, > >> .key_size = 8, > >> .value_size = 8, > >> + .btf_key_type_id = 1, > >> + .btf_value_type_id = 2, > >> }; > >> static union bpf_attr map_update_attr = { .map_fd = 1, }; > >> static __u64 key = 12; > >> @@ -43,7 +85,13 @@ int bpf_prog(struct args *ctx) > >> }; > >> int ret; > >> > >> + ret = btf_load(); > > > > Maybe let's move patch #11 (bpf_sys_close() helper) in front of these > > selftests and call bpf_sys_close() appropriately on error and (if > > success) after map is created? > > Interesting idea. I took a stab at it, but it's not unit-test like. > That bpf_sys_close is going to be used assuming it's working. > I'd rather add explicit test for bpf_sys_close eventually > instead of mixing the two. > Since your concern is fd leak I've added btf_fd to context instead > and added explicit close() in user space. Ok, that's fine. And yes, my concern was FD leak. But also having BPF selftests that demonstrates how, when you create FD in a "syscall" BPF program, such FDs can be closed inside "syscall" program as well.
diff --git a/tools/testing/selftests/bpf/progs/syscall.c b/tools/testing/selftests/bpf/progs/syscall.c index 865b5269ecbb..4353b8d8fb7f 100644 --- a/tools/testing/selftests/bpf/progs/syscall.c +++ b/tools/testing/selftests/bpf/progs/syscall.c @@ -5,6 +5,7 @@ #include <bpf/bpf_helpers.h> #include <bpf/bpf_tracing.h> #include <../../../tools/include/linux/filter.h> +#include <linux/btf.h> char _license[] SEC("license") = "GPL"; @@ -16,6 +17,45 @@ struct args { int prog_fd; }; +#define BTF_INFO_ENC(kind, kind_flag, vlen) \ + ((!!(kind_flag) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN)) +#define BTF_TYPE_ENC(name, info, size_or_type) (name), (info), (size_or_type) +#define BTF_INT_ENC(encoding, bits_offset, nr_bits) \ + ((encoding) << 24 | (bits_offset) << 16 | (nr_bits)) +#define BTF_TYPE_INT_ENC(name, encoding, bits_offset, bits, sz) \ + BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_INT, 0, 0), sz), \ + BTF_INT_ENC(encoding, bits_offset, bits) + +static int btf_load(void) +{ + struct btf_blob { + struct btf_header btf_hdr; + __u32 types[8]; + __u32 str; + } raw_btf = { + .btf_hdr = { + .magic = BTF_MAGIC, + .version = BTF_VERSION, + .hdr_len = sizeof(struct btf_header), + .type_len = sizeof(__u32) * 8, + .str_off = sizeof(__u32) * 8, + .str_len = sizeof(__u32), + }, + .types = { + /* long */ + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 64, 8), /* [1] */ + /* unsigned long */ + BTF_TYPE_INT_ENC(0, 0, 0, 64, 8), /* [2] */ + }, + }; + static union bpf_attr btf_load_attr = { + .btf_size = sizeof(raw_btf), + }; + + btf_load_attr.btf = (long)&raw_btf; + return bpf_sys_bpf(BPF_BTF_LOAD, &btf_load_attr, sizeof(btf_load_attr)); +} + SEC("syscall") int bpf_prog(struct args *ctx) { @@ -33,6 +73,8 @@ int bpf_prog(struct args *ctx) .map_type = BPF_MAP_TYPE_HASH, .key_size = 8, .value_size = 8, + .btf_key_type_id = 1, + .btf_value_type_id = 2, }; static union bpf_attr map_update_attr = { .map_fd = 1, }; static __u64 key = 12; @@ -43,7 +85,13 @@ int bpf_prog(struct args *ctx) }; int ret; + ret = btf_load(); + if (ret < 0) + return ret; + map_create_attr.max_entries = ctx->max_entries; + map_create_attr.btf_fd = ret; + prog_load_attr.license = (long) license; prog_load_attr.insns = (long) insns; prog_load_attr.log_buf = ctx->log_buf;