Re: [PATCH v2 bpf-next 14/20] libbpf: Recognize __arena global varaibles.

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

On Thu, Feb 8, 2024 at 8:07 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> LLVM automatically places __arena variables into ".arena.1" ELF section.
> When libbpf sees such section it creates internal 'struct bpf_map' LIBBPF_MAP_ARENA
> that is connected to actual BPF_MAP_TYPE_ARENA 'struct bpf_map'.
> They share the same kernel's side bpf map and single map_fd.
> Both are emitted into skeleton. Real arena with the name given by bpf program
> in SEC(".maps") and another with "__arena_internal" name.
> All global variables from ".arena.1" section are accessible from user space
> via skel->arena->name_of_var.

This "real arena" vs "__arena_internal" seems like an unnecessary
complication. When processing .arena.1 ELF section, we search for
explicit BPF_MAP_TYPE_ARENA map, right? Great, at that point, we can
use that map and it's map->mmapable pointer (we mmap() anonymous
memory to hold initial values of global variables). We copy init
values into map->mmapable on actual arena struct bpf_map. Then during
map creation (during load) we do a new mmap(map_fd), taking into
account map_extra. Then memcpy() from the original anonymous mmap into
this arena-linked mmap. Then discard the original mmap.

This way we don't have fake maps anymore, we initialize actual map (we
might need to just remember smaller init mmap_sz, which doesn't seem
like a big complication). WDYT?

BTW, I think bpf_object__load_skeleton() can re-assign skeleton's
arena data pointer, so user accessing skel->arena->var before and
after skeleton load will be getting correct pointer.

>
> For bss/data/rodata the skeleton/libbpf perform the following sequence:
> 1. addr = mmap(MAP_ANONYMOUS)
> 2. user space optionally modifies global vars
> 3. map_fd = bpf_create_map()
> 4. bpf_update_map_elem(map_fd, addr) // to store values into the kernel
> 5. mmap(addr, MAP_FIXED, map_fd)
> after step 5 user spaces see the values it wrote at step 2 at the same addresses
>
> arena doesn't support update_map_elem. Hence skeleton/libbpf do:
> 1. addr = mmap(MAP_ANONYMOUS)
> 2. user space optionally modifies global vars
> 3. map_fd = bpf_create_map(MAP_TYPE_ARENA)
> 4. real_addr = mmap(map->map_extra, MAP_SHARED | MAP_FIXED, map_fd)
> 5. memcpy(real_addr, addr) // this will fault-in and allocate pages
> 6. munmap(addr)
>
> At the end look and feel of global data vs __arena global data is the same from bpf prog pov.
>
> Another complication is:
> struct {
>   __uint(type, BPF_MAP_TYPE_ARENA);
> } arena SEC(".maps");
>
> int __arena foo;
> int bar;
>
>   ptr1 = &foo;   // relocation against ".arena.1" section
>   ptr2 = &arena; // relocation against ".maps" section
>   ptr3 = &bar;   // relocation against ".bss" section
>
> Fo the kernel ptr1 and ptr2 has point to the same arena's map_fd
> while ptr3 points to a different global array's map_fd.
> For the verifier:
> ptr1->type == unknown_scalar
> ptr2->type == const_ptr_to_map
> ptr3->type == ptr_to_map_value
>
> after the verifier and for JIT all 3 ptr-s are normal ld_imm64 insns.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  tools/bpf/bpftool/gen.c |  13 ++++-
>  tools/lib/bpf/libbpf.c  | 102 +++++++++++++++++++++++++++++++++++-----
>  2 files changed, 101 insertions(+), 14 deletions(-)
>

[...]

> @@ -1718,10 +1722,34 @@ static int
>  bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
>                               const char *real_name, int sec_idx, void *data, size_t data_sz)
>  {
> +       const long page_sz = sysconf(_SC_PAGE_SIZE);
> +       struct bpf_map *map, *arena = NULL;
>         struct bpf_map_def *def;
> -       struct bpf_map *map;
>         size_t mmap_sz;
> -       int err;
> +       int err, i;
> +
> +       if (type == LIBBPF_MAP_ARENA) {
> +               for (i = 0; i < obj->nr_maps; i++) {
> +                       map = &obj->maps[i];
> +                       if (map->def.type != BPF_MAP_TYPE_ARENA)
> +                               continue;
> +                       arena = map;
> +                       real_name = "__arena_internal";
> +                       mmap_sz = bpf_map_mmap_sz(map);
> +                       if (roundup(data_sz, page_sz) > mmap_sz) {
> +                               pr_warn("Declared arena map size %zd is too small to hold"
> +                                       "global __arena variables of size %zd\n",
> +                                       mmap_sz, data_sz);
> +                               return -E2BIG;
> +                       }
> +                       break;
> +               }
> +               if (!arena) {
> +                       pr_warn("To use global __arena variables the arena map should"
> +                               "be declared explicitly in SEC(\".maps\")\n");
> +                       return -ENOENT;
> +               }

so basically here we found arena, let's arena->mmapable =
mmap(MAP_ANONYMOUS) here, memcpy(arena->mmapable, data, data_sz) and
exit early, not doing bpf_object__add_map()?


> +       }
>
>         map = bpf_object__add_map(obj);
>         if (IS_ERR(map))
> @@ -1732,6 +1760,7 @@ bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
>         map->sec_offset = 0;
>         map->real_name = strdup(real_name);
>         map->name = internal_map_name(obj, real_name);
> +       map->arena = arena;
>         if (!map->real_name || !map->name) {
>                 zfree(&map->real_name);
>                 zfree(&map->name);

[...]

> @@ -13437,6 +13508,11 @@ int bpf_object__load_skeleton(struct bpf_object_skeleton *s)
>                         continue;
>                 }
>
> +               if (map->arena) {
> +                       *mmaped = map->arena->mmaped;
> +                       continue;
> +               }
> +

yep, I was going to suggest that we can fix up this pointer in
load_skeleton, nice


>                 if (map->def.map_flags & BPF_F_RDONLY_PROG)
>                         prot = PROT_READ;
>                 else
> --
> 2.34.1
>