Re: [PATCH bpf-next 1/2] libbpf: Support static initialization of BPF_MAP_TYPE_PROG_ARRAY

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

On Wed, Nov 24, 2021 at 8:13 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Hi, Andrii
>
> On 11/23/21 11:28 AM, Andrii Nakryiko wrote:
> > On Sun, Nov 21, 2021 at 5:55 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
> >>
> >> Support static initialization of BPF_MAP_TYPE_PROG_ARRAY with a
> >> syntax similar to map-in-map initialization ([0]):
> >>
> >>     SEC("socket")
> >>     int tailcall_1(void *ctx)
> >>     {
> >>         return 0;
> >>     }
> >>
> >>     struct {
> >>         __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> >>         __uint(max_entries, 2);
> >>         __uint(key_size, sizeof(__u32));
> >>         __array(values, int (void *));
> >>     } prog_array_init SEC(".maps") = {
> >>         .values = {
> >>             [1] = (void *)&tailcall_1,
> >>         },
> >>     };
> >>
> >> Here's the relevant part of libbpf debug log showing what's
> >> going on with prog-array initialization:
> >>
> >> libbpf: sec '.relsocket': collecting relocation for section(3) 'socket'
> >> libbpf: sec '.relsocket': relo #0: insn #2 against 'prog_array_init'
> >> libbpf: prog 'entry': found map 0 (prog_array_init, sec 4, off 0) for insn #0
> >> libbpf: .maps relo #0: for 3 value 0 rel->r_offset 32 name 53 ('tailcall_1')
> >> libbpf: .maps relo #0: map 'prog_array_init' slot [1] points to prog 'tailcall_1'
> >> libbpf: map 'prog_array_init': created successfully, fd=5
> >> libbpf: map 'prog_array_init': slot [1] set to prog 'tailcall_1' fd=6
> >>
> >>   [0] Closes: https://github.com/libbpf/libbpf/issues/354
> >>
> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> >> ---
> >>  tools/lib/bpf/libbpf.c | 146 ++++++++++++++++++++++++++++++++++-------
> >>  1 file changed, 122 insertions(+), 24 deletions(-)
> >>
> >
> > Just a few nits and suggestions below, but it looks great overall, thanks!
> >
> > [...]
> >
> >>                         t = skip_mods_and_typedefs(btf, btf_array(t)->type, NULL);
> >>                         if (!btf_is_ptr(t)) {
> >> -                               pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> >> -                                       map_name, btf_kind_str(t));
> >> +                               if (is_map_in_map)
> >> +                                       pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n",
> >> +                                               map_name, btf_kind_str(t));
> >> +                               else
> >> +                                       pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n",
> >
> > maybe let's do
> >
> > const char *desc = is_map_in_map ? "map-in-map inner" : "prog-array value";
> >
> > and use desc in those three pr_warn() messages?
> >
>
> Ack.
>
> >> +                                               map_name, btf_kind_str(t));
> >>                                 return -EINVAL;
> >>                         }
> >>                         t = skip_mods_and_typedefs(btf, t->type, NULL);
> >> -                       if (!btf_is_struct(t)) {
> >> +                       if (is_prog_array) {
> >> +                               if (btf_is_func_proto(t))
> >> +                                       return 0;
> >
> > you can't return on success here, there could technically be other
> > fields after "values". Can you please also invert the condition so
> > that error handling happens first and then we continue:
> >
> According to the original code ([0]), the "values" field is intended to be
>
> the last field ?

yeah, but we don't need to make this assumption here and exit early,
given the other code doesn't do that. Let's not try to shoot ourselves
in the foot unnecessarily.

>
> > if (!btf_is_func_proto(t)) {
> >     pr_warn(..);
> >     return -EINVAl;
> > }
> > continue;
> >
> > It's more consistent with the other logic in this function
> >
> >

[...]

> >> @@ -6229,8 +6304,20 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
> >>                         return -EINVAL;
> >>                 }
> >>
> >> -               if (!bpf_map_type__is_map_in_map(map->def.type))
> >> +               is_map_in_map = bpf_map_type__is_map_in_map(map->def.type);
> >> +               is_prog_array = map->def.type == BPF_MAP_TYPE_PROG_ARRAY;
> >> +               if (!is_map_in_map && !is_prog_array)
> >>                         return -EINVAL;
> >> +               if (is_map_in_map && sym->st_shndx != obj->efile.btf_maps_shndx) {
> >> +                       pr_warn(".maps relo #%d: '%s' isn't a BTF-defined map\n",
> >> +                               i, name);
> >> +                       return -LIBBPF_ERRNO__RELOC;
> >> +               }
> >> +               if (is_prog_array && !bpf_object__find_program_by_name(obj, name)) {
> >
> > let's do an additional check on the program you found with find_program_by_name:
> >
> >   1. prog->sec_idx == sym->st_shndx
> >   2. prog->sec_insn_off * 8 == sym->st_value
> >   3. !prog_is_subprog(obj, prog)
> >
> > This will make sure you have the correct entry-point BPF program (not
> > a subprog) and we point to its beginning (no offset into the program).
> >
> >
>
> OK, will do, maybe we should also add some tests for these cases ?

yeah, negative test validating that you can't put a global variable
and/or a map into the PROG_ARRAY slot would be great, thanks!

>
> >> +                       pr_warn(".maps relo #%d: '%s' isn't a BPF program\n",
> >
> > "entry-point" is an important distinction, please mention that. You
> > can't put sub-programs into PROG_ARRAY.
> >
> >> +                               i, name);
> >> +                       return -LIBBPF_ERRNO__RELOC;
> >> +               }
> >>                 if (map->def.type == BPF_MAP_TYPE_HASH_OF_MAPS &&
> >>                     map->def.key_size != sizeof(int)) {
> >>                         pr_warn(".maps relo #%d: hash-of-maps '%s' should have key size %zu.\n",

[...]