| Message ID | 20201117145644.1166255-4-danieltimlee@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: remove bpf_load loader completely | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| 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 | success | total: 0 errors, 0 warnings, 0 checks, 151 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > This commit refactors the existing cgroup program with libbpf bpf > loader. The original test_cgrp2_sock2 has keeped the bpf program > attached to the cgroup hierarchy even after the exit of user program. > To implement the same functionality with libbpf, this commit uses the > BPF_LINK_PINNING to pin the link attachment even after it is closed. > > Since this uses LINK instead of ATTACH, detach of bpf program from > cgroup with 'test_cgrp2_sock' is not used anymore. > > The code to mount the bpf was added to the .sh file in case the bpff > was not mounted on /sys/fs/bpf. Additionally, to fix the problem that > shell script cannot find the binary object from the current path, > relative path './' has been added in front of binary. > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > --- > samples/bpf/Makefile | 2 +- > samples/bpf/test_cgrp2_sock2.c | 63 ++++++++++++++++++++++++--------- > samples/bpf/test_cgrp2_sock2.sh | 21 ++++++++--- > 3 files changed, 64 insertions(+), 22 deletions(-) > [...] > > - return EXIT_SUCCESS; > + err = bpf_link__pin(link, link_pin_path); > + if (err < 0) { > + printf("err : %d\n", err); more meaningful error message would be helpful > + goto cleanup; > + } > + > + ret = EXIT_SUCCESS; > + > +cleanup: > + if (ret != EXIT_SUCCESS) > + bpf_link__destroy(link); > + > + bpf_object__close(obj); > + return ret; > } [...] > > function attach_bpf { > - test_cgrp2_sock2 /tmp/cgroupv2/foo sock_flags_kern.o $1 > + ./test_cgrp2_sock2 /tmp/cgroupv2/foo sock_flags_kern.o $1 Can you please add Fixes: tag for this? > [ $? -ne 0 ] && exit 1 > } > > function cleanup { > - if [ -d /tmp/cgroupv2/foo ]; then > - test_cgrp2_sock -d /tmp/cgroupv2/foo > - fi > + rm -rf $LINK_PIN > ip link del veth0b > ip netns delete at_ns0 > umount /tmp/cgroupv2 > @@ -42,6 +51,7 @@ cleanup 2>/dev/null > set -e > config_device > config_cgroup > +config_bpffs > set +e > > # > @@ -62,6 +72,9 @@ if [ $? -eq 0 ]; then > exit 1 > fi > > +rm -rf $LINK_PIN > +sleep 1 # Wait for link detach > + > # > # Test 2 - fail ping > # > -- > 2.25.1 >
On Wed, Nov 18, 2020 at 12:02 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > This commit refactors the existing cgroup program with libbpf bpf > > loader. The original test_cgrp2_sock2 has keeped the bpf program > > attached to the cgroup hierarchy even after the exit of user program. > > To implement the same functionality with libbpf, this commit uses the > > BPF_LINK_PINNING to pin the link attachment even after it is closed. > > > > Since this uses LINK instead of ATTACH, detach of bpf program from > > cgroup with 'test_cgrp2_sock' is not used anymore. > > > > The code to mount the bpf was added to the .sh file in case the bpff > > was not mounted on /sys/fs/bpf. Additionally, to fix the problem that > > shell script cannot find the binary object from the current path, > > relative path './' has been added in front of binary. > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > --- > > samples/bpf/Makefile | 2 +- > > samples/bpf/test_cgrp2_sock2.c | 63 ++++++++++++++++++++++++--------- > > samples/bpf/test_cgrp2_sock2.sh | 21 ++++++++--- > > 3 files changed, 64 insertions(+), 22 deletions(-) > > > > [...] > > > > > - return EXIT_SUCCESS; > > + err = bpf_link__pin(link, link_pin_path); > > + if (err < 0) { > > + printf("err : %d\n", err); > > more meaningful error message would be helpful > Thanks for pointing out, I will fix it directly! > > + goto cleanup; > > + } > > + > > + ret = EXIT_SUCCESS; > > + > > +cleanup: > > + if (ret != EXIT_SUCCESS) > > + bpf_link__destroy(link); > > + > > + bpf_object__close(obj); > > + return ret; > > } > > [...] > > > > > function attach_bpf { > > - test_cgrp2_sock2 /tmp/cgroupv2/foo sock_flags_kern.o $1 > > + ./test_cgrp2_sock2 /tmp/cgroupv2/foo sock_flags_kern.o $1 > > Can you please add Fixes: tag for this? > Will add it in the next version of patch :) Thanks for your time and effort for the review.
On Tue, Nov 17, 2020 at 02:56:38PM +0000, Daniel T. Lee wrote: [ ... ] > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > index 01449d767122..7a643595ac6c 100644 > --- a/samples/bpf/Makefile > +++ b/samples/bpf/Makefile > @@ -82,7 +82,7 @@ test_overhead-objs := bpf_load.o test_overhead_user.o > test_cgrp2_array_pin-objs := test_cgrp2_array_pin.o > test_cgrp2_attach-objs := test_cgrp2_attach.o > test_cgrp2_sock-objs := test_cgrp2_sock.o > -test_cgrp2_sock2-objs := bpf_load.o test_cgrp2_sock2.o > +test_cgrp2_sock2-objs := test_cgrp2_sock2.o > xdp1-objs := xdp1_user.o > # reuse xdp1 source intentionally > xdp2-objs := xdp1_user.o > diff --git a/samples/bpf/test_cgrp2_sock2.c b/samples/bpf/test_cgrp2_sock2.c > index a9277b118c33..518526c7ce16 100644 > --- a/samples/bpf/test_cgrp2_sock2.c > +++ b/samples/bpf/test_cgrp2_sock2.c > @@ -20,9 +20,9 @@ > #include <net/if.h> > #include <linux/bpf.h> > #include <bpf/bpf.h> > +#include <bpf/libbpf.h> > > #include "bpf_insn.h" > -#include "bpf_load.h" > > static int usage(const char *argv0) > { > @@ -32,37 +32,66 @@ static int usage(const char *argv0) > > int main(int argc, char **argv) > { > - int cg_fd, ret, filter_id = 0; > + int cg_fd, err, ret = EXIT_FAILURE, filter_id = 0, prog_cnt = 0; > + const char *link_pin_path = "/sys/fs/bpf/test_cgrp2_sock2"; > + struct bpf_link *link = NULL; > + struct bpf_program *progs[2]; > + struct bpf_program *prog; > + struct bpf_object *obj; > > if (argc < 3) > return usage(argv[0]); > > + if (argc > 3) > + filter_id = atoi(argv[3]); > + > cg_fd = open(argv[1], O_DIRECTORY | O_RDONLY); > if (cg_fd < 0) { > printf("Failed to open cgroup path: '%s'\n", strerror(errno)); > - return EXIT_FAILURE; > + return ret; > } > > - if (load_bpf_file(argv[2])) > - return EXIT_FAILURE; > - > - printf("Output from kernel verifier:\n%s\n-------\n", bpf_log_buf); > + obj = bpf_object__open_file(argv[2], NULL); > + if (libbpf_get_error(obj)) { > + printf("ERROR: opening BPF object file failed\n"); > + return ret; > + } > > - if (argc > 3) > - filter_id = atoi(argv[3]); > + bpf_object__for_each_program(prog, obj) { > + progs[prog_cnt] = prog; > + prog_cnt++; > + } > > if (filter_id >= prog_cnt) { > printf("Invalid program id; program not found in file\n"); > - return EXIT_FAILURE; > + goto cleanup; > + } > + > + /* load BPF program */ > + if (bpf_object__load(obj)) { > + printf("ERROR: loading BPF object file failed\n"); > + goto cleanup; > } > > - ret = bpf_prog_attach(prog_fd[filter_id], cg_fd, > - BPF_CGROUP_INET_SOCK_CREATE, 0); > - if (ret < 0) { > - printf("Failed to attach prog to cgroup: '%s'\n", > - strerror(errno)); > - return EXIT_FAILURE; > + link = bpf_program__attach_cgroup(progs[filter_id], cg_fd); > + if (libbpf_get_error(link)) { > + printf("ERROR: bpf_program__attach failed\n"); > + link = NULL; > + goto cleanup; > } > > - return EXIT_SUCCESS; > + err = bpf_link__pin(link, link_pin_path); > + if (err < 0) { > + printf("err : %d\n", err); > + goto cleanup; > + } > + > + ret = EXIT_SUCCESS; > + > +cleanup: > + if (ret != EXIT_SUCCESS) > + bpf_link__destroy(link); This looks wrong. cleanup should be done regardless. > + > + bpf_object__close(obj); > + return ret; > }
On Wed, Nov 18, 2020 at 2:58 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Tue, Nov 17, 2020 at 02:56:38PM +0000, Daniel T. Lee wrote: > [ ... ] > > > + err = bpf_link__pin(link, link_pin_path); > > + if (err < 0) { > > + printf("err : %d\n", err); > > + goto cleanup; > > + } > > + > > + ret = EXIT_SUCCESS; > > + > > +cleanup: > > + if (ret != EXIT_SUCCESS) > > + bpf_link__destroy(link); > This looks wrong. cleanup should be done regardless. > At first, I thought destroying the link after the link__pin might unpin the link, but I just tested it and confirmed that it actually didn't and that the link kept pinned. Thanks for pointing it out! I will stick to this method. > > + > > + bpf_object__close(obj); > > + return ret; > > }
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile index 01449d767122..7a643595ac6c 100644 --- a/samples/bpf/Makefile +++ b/samples/bpf/Makefile @@ -82,7 +82,7 @@ test_overhead-objs := bpf_load.o test_overhead_user.o test_cgrp2_array_pin-objs := test_cgrp2_array_pin.o test_cgrp2_attach-objs := test_cgrp2_attach.o test_cgrp2_sock-objs := test_cgrp2_sock.o -test_cgrp2_sock2-objs := bpf_load.o test_cgrp2_sock2.o +test_cgrp2_sock2-objs := test_cgrp2_sock2.o xdp1-objs := xdp1_user.o # reuse xdp1 source intentionally xdp2-objs := xdp1_user.o diff --git a/samples/bpf/test_cgrp2_sock2.c b/samples/bpf/test_cgrp2_sock2.c index a9277b118c33..518526c7ce16 100644 --- a/samples/bpf/test_cgrp2_sock2.c +++ b/samples/bpf/test_cgrp2_sock2.c @@ -20,9 +20,9 @@ #include <net/if.h> #include <linux/bpf.h> #include <bpf/bpf.h> +#include <bpf/libbpf.h> #include "bpf_insn.h" -#include "bpf_load.h" static int usage(const char *argv0) { @@ -32,37 +32,66 @@ static int usage(const char *argv0) int main(int argc, char **argv) { - int cg_fd, ret, filter_id = 0; + int cg_fd, err, ret = EXIT_FAILURE, filter_id = 0, prog_cnt = 0; + const char *link_pin_path = "/sys/fs/bpf/test_cgrp2_sock2"; + struct bpf_link *link = NULL; + struct bpf_program *progs[2]; + struct bpf_program *prog; + struct bpf_object *obj; if (argc < 3) return usage(argv[0]); + if (argc > 3) + filter_id = atoi(argv[3]); + cg_fd = open(argv[1], O_DIRECTORY | O_RDONLY); if (cg_fd < 0) { printf("Failed to open cgroup path: '%s'\n", strerror(errno)); - return EXIT_FAILURE; + return ret; } - if (load_bpf_file(argv[2])) - return EXIT_FAILURE; - - printf("Output from kernel verifier:\n%s\n-------\n", bpf_log_buf); + obj = bpf_object__open_file(argv[2], NULL); + if (libbpf_get_error(obj)) { + printf("ERROR: opening BPF object file failed\n"); + return ret; + } - if (argc > 3) - filter_id = atoi(argv[3]); + bpf_object__for_each_program(prog, obj) { + progs[prog_cnt] = prog; + prog_cnt++; + } if (filter_id >= prog_cnt) { printf("Invalid program id; program not found in file\n"); - return EXIT_FAILURE; + goto cleanup; + } + + /* load BPF program */ + if (bpf_object__load(obj)) { + printf("ERROR: loading BPF object file failed\n"); + goto cleanup; } - ret = bpf_prog_attach(prog_fd[filter_id], cg_fd, - BPF_CGROUP_INET_SOCK_CREATE, 0); - if (ret < 0) { - printf("Failed to attach prog to cgroup: '%s'\n", - strerror(errno)); - return EXIT_FAILURE; + link = bpf_program__attach_cgroup(progs[filter_id], cg_fd); + if (libbpf_get_error(link)) { + printf("ERROR: bpf_program__attach failed\n"); + link = NULL; + goto cleanup; } - return EXIT_SUCCESS; + err = bpf_link__pin(link, link_pin_path); + if (err < 0) { + printf("err : %d\n", err); + goto cleanup; + } + + ret = EXIT_SUCCESS; + +cleanup: + if (ret != EXIT_SUCCESS) + bpf_link__destroy(link); + + bpf_object__close(obj); + return ret; } diff --git a/samples/bpf/test_cgrp2_sock2.sh b/samples/bpf/test_cgrp2_sock2.sh index 0f396a86e0cb..6a3dbe642b2b 100755 --- a/samples/bpf/test_cgrp2_sock2.sh +++ b/samples/bpf/test_cgrp2_sock2.sh @@ -1,6 +1,9 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0 +BPFFS=/sys/fs/bpf +LINK_PIN=$BPFFS/test_cgrp2_sock2 + function config_device { ip netns add at_ns0 ip link add veth0 type veth peer name veth0b @@ -21,16 +24,22 @@ function config_cgroup { echo $$ >> /tmp/cgroupv2/foo/cgroup.procs } +function config_bpffs { + if mount | grep $BPFFS > /dev/null; then + echo "bpffs already mounted" + else + echo "bpffs not mounted. Mounting..." + mount -t bpf none $BPFFS + fi +} function attach_bpf { - test_cgrp2_sock2 /tmp/cgroupv2/foo sock_flags_kern.o $1 + ./test_cgrp2_sock2 /tmp/cgroupv2/foo sock_flags_kern.o $1 [ $? -ne 0 ] && exit 1 } function cleanup { - if [ -d /tmp/cgroupv2/foo ]; then - test_cgrp2_sock -d /tmp/cgroupv2/foo - fi + rm -rf $LINK_PIN ip link del veth0b ip netns delete at_ns0 umount /tmp/cgroupv2 @@ -42,6 +51,7 @@ cleanup 2>/dev/null set -e config_device config_cgroup +config_bpffs set +e # @@ -62,6 +72,9 @@ if [ $? -eq 0 ]; then exit 1 fi +rm -rf $LINK_PIN +sleep 1 # Wait for link detach + # # Test 2 - fail ping #
This commit refactors the existing cgroup program with libbpf bpf loader. The original test_cgrp2_sock2 has keeped the bpf program attached to the cgroup hierarchy even after the exit of user program. To implement the same functionality with libbpf, this commit uses the BPF_LINK_PINNING to pin the link attachment even after it is closed. Since this uses LINK instead of ATTACH, detach of bpf program from cgroup with 'test_cgrp2_sock' is not used anymore. The code to mount the bpf was added to the .sh file in case the bpff was not mounted on /sys/fs/bpf. Additionally, to fix the problem that shell script cannot find the binary object from the current path, relative path './' has been added in front of binary. Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> --- samples/bpf/Makefile | 2 +- samples/bpf/test_cgrp2_sock2.c | 63 ++++++++++++++++++++++++--------- samples/bpf/test_cgrp2_sock2.sh | 21 ++++++++--- 3 files changed, 64 insertions(+), 22 deletions(-)