On Wed, Nov 24, 2021 at 8:13 AM Hengqi Chen <hengqi.chen@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> 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",
[...]
