Message-ID: <CAEf4Bzb6FP4germQketqZ7NyOMMYEQ_qjVeqrngKMnQCNeokOA@mail.gmail.com>
Date:   Thu, 5 Mar 2020 13:50:07 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Eric Sage <eric@...e.org>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin Lau <kafai@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        john fastabend <john.fastabend@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH] [bpf] Make bpf program autoloading optional

On Thu, Mar 5, 2020 at 11:22 AM Eric Sage <eric@...e.org> wrote:
>
> Adds bpf_program__set_autoload which can be used to disable loading
> a bpf_prog when loading the bpf_object that contains it after the
> bpf_object has been opened. This behavior affect calling load directly
> and loading through BPF skel. A single flag is added to bpf_prog
> to make this work.
>
> Signed-off-by: Eric Sage <eric@...e.org>
> ---

This is a very useful feature for complicated scenarios, thanks for
working on this! You've based it off bpf tree, but all the new
features should go through bpf-next, please rebase.

>  tools/lib/bpf/libbpf.c                        |   9 +
>  tools/lib/bpf/libbpf.h                        |   2 +
>  tools/lib/bpf/libbpf.map                      |   5 +
>  tools/testing/selftests/bpf/Makefile          |   2 +-
>  .../selftests/bpf/progs/test_autoload_kern.c  |  24 +++
>  tools/testing/selftests/bpf/test_autoload.c   | 158 ++++++++++++++++++
>  6 files changed, 199 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/progs/test_autoload_kern.c
>  create mode 100644 tools/testing/selftests/bpf/test_autoload.c
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 514b1a524abb..fe156ca10d16 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -222,6 +222,7 @@ struct bpf_program {
>         bpf_program_prep_t preprocessor;
>
>         struct bpf_object *obj;
> +       bool autoload;

this will create unnecessarily 7 bytes of padding. Let's move this
field after `enum bpf_prog_type type;` few lines above, it will take
part of 4 byte padding there.

>         void *priv;
>         bpf_program_clear_priv_t clear_priv;
>
> @@ -499,6 +500,7 @@ bpf_program__init(void *data, size_t size, char *section_name, int idx,
>         prog->instances.fds = NULL;
>         prog->instances.nr = -1;
>         prog->type = BPF_PROG_TYPE_UNSPEC;
> +       prog->autoload = true;
>
>         return 0;
>  errout:
> @@ -4933,6 +4935,11 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
>         return ret;
>  }
>
> +void bpf_program__set_autoload(struct bpf_program *prog, bool autoload)
> +{
> +       prog->autoload = autoload;
> +}
> +
>  static int libbpf_find_attach_btf_id(struct bpf_program *prog);
>
>  int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
> @@ -5030,6 +5037,8 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
>         int err;
>
>         for (i = 0; i < obj->nr_programs; i++) {
> +               if (!obj->programs[i].autoload)
> +                       continue;
>                 if (bpf_program__is_function_storage(&obj->programs[i], obj))
>                         continue;
>                 obj->programs[i].log_level |= log_level;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 3fe12c9d1f92..e5f30f70bac1 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -204,6 +204,8 @@ LIBBPF_API const char *bpf_program__title(const struct bpf_program *prog,
>  /* returns program size in bytes */
>  LIBBPF_API size_t bpf_program__size(const struct bpf_program *prog);
>
> +LIBBPF_API void bpf_program__set_autoload(struct bpf_program *prog, bool autoload);
> +
>  LIBBPF_API int bpf_program__load(struct bpf_program *prog, char *license,
>                                  __u32 kern_version);
>  LIBBPF_API int bpf_program__fd(const struct bpf_program *prog);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index b035122142bb..1d7572806981 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -235,3 +235,8 @@ LIBBPF_0.0.7 {
>                 btf__align_of;
>                 libbpf_find_kernel_btf;
>  } LIBBPF_0.0.6;
> +
> +LIBBPF_0.0.8 {
> +  global:
> +    bpf_program__set_autoload;
> +} LIBBPF_0.0.7;
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 257a1aaaa37d..1ee62911992d 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -29,7 +29,7 @@ LDLIBS += -lcap -lelf -lz -lrt -lpthread
>  # Order correspond to 'make run_tests' order
>  TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
>         test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
> -       test_sock test_btf test_sockmap get_cgroup_id_user test_socket_cookie \
> +       test_sock test_btf test_sockmap test_autoload get_cgroup_id_user test_socket_cookie \

We normally add new tests into test_progs framework, can you please
add it there? See some notes regarding testing below as well.

>         test_cgroup_storage \
>         test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \
>         test_progs-no_alu32
> diff --git a/tools/testing/selftests/bpf/progs/test_autoload_kern.c b/tools/testing/selftests/bpf/progs/test_autoload_kern.c
> new file mode 100644
> index 000000000000..e4cfe9b90606
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_autoload_kern.c

nit: we usually name BPF programs as progs/test_<whatever> and their
user-space counterparts as prog_tests/<whatever>.c. _kern suffix is
rarely used now.

> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +SEC("xdp_prog_0")
> +int prog_0(struct xdp_md *xdp)
> +{
> +       return XDP_PASS;
> +}
> +
> +SEC("xdp_prog_1")
> +int prog_1(struct xdp_md *xdp)
> +{
> +       return XDP_PASS;
> +}
> +
> +SEC("xdp_prog_2")
> +int prog_2(struct xdp_md *xdp)
> +{
> +       return XDP_PASS;
> +}
> +

I've found that it's easiest to test BPF programs of
SEC("raw_tp/sys_enter") type, you can trigger them, e.g., with
usleep(1). I'd suggest switching them to that type, and each setting
its own global variable from 0 to 1. Then on user-space side you can
just validate that one of them wasn't triggered, while other(s) were.
No need for more code to check that program was loaded, etc.

> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/test_autoload.c b/tools/testing/selftests/bpf/test_autoload.c
> new file mode 100644
> index 000000000000..3294c167bbfd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_autoload.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <sys/resource.h>
> +
> +#include <bpf/bpf.h>
> +#include <bpf/libbpf.h>
> +
> +#include "test_autoload_kern.skel.h"
> +
> +#define AUTOLOAD_KERN "test_autoload_kern.o"
> +#define TEST_NO_AUTOLOAD_PROG "prog_2"
> +
> +int print_libbpf_log(enum libbpf_print_level lvl, const char *fmt, va_list args)
> +{
> +       return 0;
> +}
> +
> +int test_libbpf(void)
> +{
> +       struct bpf_object *obj;
> +       struct bpf_program *unloaded_prog, *prog;
> +       struct bpf_prog_info *info;
> +       __u32 info_len;
> +       int prog_fd;
> +
> +       obj = bpf_object__open(AUTOLOAD_KERN);
> +       if (obj == NULL) {
> +               fprintf(stderr, "failed to load %s\n", AUTOLOAD_KERN);
> +               return -1;
> +       }
> +
> +       unloaded_prog =
> +               bpf_object__find_program_by_name(obj, TEST_NO_AUTOLOAD_PROG);
> +       if (unloaded_prog == NULL) {
> +               fprintf(stderr, "failed to find test xdp prog %s\n",
> +                       TEST_NO_AUTOLOAD_PROG);
> +               goto fail;
> +       }
> +
> +       bpf_program__set_autoload(unloaded_prog, false);
> +
> +       bpf_object__load(obj);
> +
> +       bpf_object__for_each_program(prog, obj) {
> +               prog_fd = bpf_program__fd(prog);
> +
> +               if (unloaded_prog == prog) {
> +                       if (-prog_fd != EINVAL) {
> +                               fprintf(stderr,
> +                                       "non-autoloaded prog should not be loaded\n");
> +                               goto fail;
> +                       }
> +                       continue;
> +               }
> +
> +               info_len = sizeof(struct bpf_prog_info);
> +               info = calloc(1, info_len);
> +
> +               if (bpf_obj_get_info_by_fd(prog_fd, info, &info_len) < 0) {
> +                       fprintf(stderr, "could not get bpf prog info\n");
> +                       goto fail;
> +               }
> +
> +               if (info->id == 0) {
> +                       fprintf(stderr, "expected valid prog id\n");
> +                       goto fail;
> +               }
> +       }
> +
> +       bpf_object__close(obj);
> +       return 0;
> +fail:
> +       bpf_object__close(obj);
> +       return -1;
> +}
> +
> +int test_skel(void)
> +{
> +       struct test_autoload_kern *kern;
> +       struct bpf_object *obj;
> +       struct bpf_program *unloaded_prog, *prog;
> +       struct bpf_prog_info *info;
> +       __u32 info_len;
> +       int prog_fd;
> +
> +       kern = test_autoload_kern__open();

I think there's no need to test skeleton-based and
bpf_object__open()-based variants. Skeleton is using
bpf_object__open() either way, so I'd just use shorter skeleton
variant. See above about program type and global variables, that makes
test programs more concise.

> +       if (kern == NULL) {
> +               fprintf(stderr, "failed to autoload skel\n");
> +               return -1;
> +       }
> +
> +       obj = kern->obj;
> +
> +       unloaded_prog =
> +               bpf_object__find_program_by_name(obj, TEST_NO_AUTOLOAD_PROG);
> +       if (unloaded_prog == NULL) {
> +               fprintf(stderr, "failed to find test xdp prog %s\n",
> +                       TEST_NO_AUTOLOAD_PROG);
> +               goto fail;
> +       }
> +
> +       bpf_program__set_autoload(unloaded_prog, false);
> +
> +       bpf_object__load(obj);

CHECK that load succeeded?

> +
> +       bpf_object__for_each_program(prog, obj) {
> +               prog_fd = bpf_program__fd(prog);
> +
> +               if (unloaded_prog == prog) {
> +                       if (-prog_fd != EINVAL) {
> +                               fprintf(stderr,
> +                                       "non-autoloaded prog should not be loaded\n");
> +                               goto fail;
> +                       }
> +                       continue;
> +               }
> +
> +               info_len = sizeof(struct bpf_prog_info);
> +               info = calloc(1, info_len);
> +
> +               if (bpf_obj_get_info_by_fd(prog_fd, info, &info_len) < 0) {
> +                       fprintf(stderr, "could not get bpf prog info\n");
> +                       goto fail;
> +               }
> +
> +               if (info->id == 0) {
> +                       fprintf(stderr, "expected valid prog id\n");
> +                       goto fail;
> +               }
> +       }
> +
> +       test_autoload_kern__destroy(kern);
> +       return 0;
> +fail:
> +       test_autoload_kern__destroy(kern);
> +       return -1;
> +}
> +
> +int main(void)
> +{
> +       struct rlimit r = { RLIM_INFINITY, RLIM_INFINITY };
> +
> +       if (setrlimit(RLIMIT_MEMLOCK, &r)) {
> +               perror("setrlimit(RLIMIT_MEMLOCK)");
> +               return EXIT_FAILURE;
> +       }
> +
> +       libbpf_set_print(print_libbpf_log);
> +

all this is taken care of in test_progs framework

> +       if (test_libbpf() < 0)
> +               return EXIT_FAILURE;
> +
> +       if (test_skel() < 0)
> +               return EXIT_FAILURE;
> +}
> --
> 2.24.1
>

Powered by blists - more mailing lists