Re: [PATCH bpf-next v3 3/8] libbpf: Add BPF_PROG_BIND_MAP syscall and use it on .metadata section

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Aug 28, 2020 at 12:37 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> From: YiFei Zhu <zhuyifei@google.com>
>
> The patch adds a simple wrapper bpf_prog_bind_map around the syscall.
> And when using libbpf to load a program, it will probe the kernel for
> the support of this syscall, and scan for the .metadata ELF section
> and load it as an internal map like a .data section.
>
> In the case that kernel supports the BPF_PROG_BIND_MAP syscall and
> a .metadata section exists, the map will be explicitly bound to
> the program via the syscall immediately after program is loaded.
> -EEXIST is ignored for this syscall.

Here is the question I have. How important is it that all this
metadata is in a separate map? What if libbpf just PROG_BIND_MAP all
the maps inside a single BPF .o file to all BPF programs in that file?
Including ARRAY maps created for .data, .rodata and .bss, even if the
BPF program doesn't use any of the global variables? If it's too
extreme, we could do it only for global data maps, leaving explicit
map definitions in SEC(".maps") alone. Would that be terrible?
Conceptually it makes sense, because when you program in user-space,
you expect global variables to be there, even if you don't reference
it directly, right? The only downside is that you won't have a special
".metadata" map, rather it will be part of ".rodata" one.


>
> Cc: YiFei Zhu <zhuyifei1999@gmail.com>
> Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  tools/lib/bpf/bpf.c      |  13 ++++
>  tools/lib/bpf/bpf.h      |   8 +++
>  tools/lib/bpf/libbpf.c   | 130 ++++++++++++++++++++++++++++++++-------
>  tools/lib/bpf/libbpf.map |   1 +
>  4 files changed, 131 insertions(+), 21 deletions(-)
>

[...]

> @@ -3592,18 +3619,13 @@ static int probe_kern_prog_name(void)
>         return probe_fd(ret);
>  }
>
> -static int probe_kern_global_data(void)
> +static void __probe_create_global_data(int *prog, int *map,
> +                                      struct bpf_insn *insns, size_t insns_cnt)

all those static functions are internal, no need for double underscore in names

>  {
>         struct bpf_load_program_attr prg_attr;
>         struct bpf_create_map_attr map_attr;
>         char *cp, errmsg[STRERR_BUFSIZE];
> -       struct bpf_insn insns[] = {
> -               BPF_LD_MAP_VALUE(BPF_REG_1, 0, 16),
> -               BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 42),
> -               BPF_MOV64_IMM(BPF_REG_0, 0),
> -               BPF_EXIT_INSN(),
> -       };
> -       int ret, map;
> +       int err;
>
>         memset(&map_attr, 0, sizeof(map_attr));
>         map_attr.map_type = BPF_MAP_TYPE_ARRAY;
> @@ -3611,26 +3633,40 @@ static int probe_kern_global_data(void)
>         map_attr.value_size = 32;
>         map_attr.max_entries = 1;
>
> -       map = bpf_create_map_xattr(&map_attr);
> -       if (map < 0) {
> -               ret = -errno;
> -               cp = libbpf_strerror_r(ret, errmsg, sizeof(errmsg));
> +       *map = bpf_create_map_xattr(&map_attr);
> +       if (*map < 0) {
> +               err = errno;
> +               cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
>                 pr_warn("Error in %s():%s(%d). Couldn't create simple array map.\n",
> -                       __func__, cp, -ret);
> -               return ret;
> +                       __func__, cp, -err);
> +               return;
>         }
>
> -       insns[0].imm = map;
> +       insns[0].imm = *map;

you are making confusing and error prone assumptions about the first
instruction passed in, I really-really don't like this, super easy to
miss and super easy to screw up.

>
>         memset(&prg_attr, 0, sizeof(prg_attr));
>         prg_attr.prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
>         prg_attr.insns = insns;
> -       prg_attr.insns_cnt = ARRAY_SIZE(insns);
> +       prg_attr.insns_cnt = insns_cnt;
>         prg_attr.license = "GPL";
>
> -       ret = bpf_load_program_xattr(&prg_attr, NULL, 0);
> +       *prog = bpf_load_program_xattr(&prg_attr, NULL, 0);
> +}
> +

[...]