| Message ID | 20211121135440.3205482-2-hengqi.chen@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Support static initialization of BPF_MAP_TYPE_PROG_ARRAY | expand |
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? > + 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: if (!btf_is_func_proto(t)) { pr_warn(..); return -EINVAl; } continue; It's more consistent with the other logic in this function > + pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n", > + map_name, btf_kind_str(t)); > + return -EINVAL; > + } > + if (is_map_in_map && !btf_is_struct(t)) { well, it can't be anything else, so I guess drop the is_map_in_map check? > pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n", > map_name, btf_kind_str(t)); > return -EINVAL; > @@ -4964,12 +4985,16 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map) > unsigned int i; > int fd, err = 0; > > + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY) > + return 0; let's leave a comment that PROG_ARRAY can only be initialized once all the programs are loaded, so this will be done later Better still, it would be good to also rename init_map_slots to init_map_in_map_slots and do the PROG_ARRAY check outside, in bpf_object__create_maps(). This creates a nice symmetry with init_prog_array (should it be named init_prog_array_slots for consistency?). WDYT? > + > for (i = 0; i < map->init_slots_sz; i++) { > if (!map->init_slots[i]) > continue; > > targ_map = map->init_slots[i]; > fd = bpf_map__fd(targ_map); > + > if (obj->gen_loader) { > pr_warn("// TODO map_update_elem: idx %td key %d value==map_idx %td\n", > map - obj->maps, i, targ_map - obj->maps); > @@ -4980,8 +5005,7 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map) > if (err) { > err = -errno; > pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n", > - map->name, i, targ_map->name, > - fd, err); > + map->name, i, targ_map->name, fd, err); > return err; > } > pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n", > @@ -4994,6 +5018,60 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map) > return 0; > } > > +static int init_prog_array(struct bpf_object *obj, struct bpf_map *map) > +{ > + const struct bpf_program *targ_prog; > + unsigned int i; > + int fd, err = 0; you always set err, no need to zero-initialize it > + > + for (i = 0; i < map->init_slots_sz; i++) { > + if (!map->init_slots[i]) > + continue; > + > + targ_prog = map->init_slots[i]; > + fd = bpf_program__fd(targ_prog); > + > + if (obj->gen_loader) { > + return -ENOTSUP; > + } else { > + err = bpf_map_update_elem(map->fd, &i, &fd, 0); > + } > + if (err) { > + err = -errno; > + pr_warn("map '%s': failed to initialize slot [%d] to prog '%s' fd=%d: %d\n", > + map->name, i, targ_prog->name, fd, err); > + return err; > + } > + pr_debug("map '%s': slot [%d] set to prog '%s' fd=%d\n", > + map->name, i, targ_prog->name, fd); > + } > + > + zfree(&map->init_slots); > + map->init_slots_sz = 0; > + > + return 0; > +} > + > +static int bpf_object_init_prog_array(struct bpf_object *obj) s/prog_array/prog_arrays/ > +{ > + struct bpf_map *map; > + int i, err; > + > + for (i = 0; i < obj->nr_maps; i++) { > + map = &obj->maps[i]; > + > + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY && > + map->init_slots_sz) { nit: longer single line is fine here (but you can also invert the condition and continue early to avoid extra level of nestedness) > + err = init_prog_array(obj, map); > + if (err < 0) { > + zclose(map->fd); > + return err; > + } > + } > + } > + return 0; > +} > + > static int > bpf_object__create_maps(struct bpf_object *obj) > { > @@ -6174,7 +6252,9 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, > int i, j, nrels, new_sz; > const struct btf_var_secinfo *vi = NULL; > const struct btf_type *sec, *var, *def; > - struct bpf_map *map = NULL, *targ_map; > + struct bpf_map *map = NULL, *targ_map = NULL; > + struct bpf_program *targ_prog = NULL; > + bool is_prog_array, is_map_in_map; > const struct btf_member *member; > const char *name, *mname; > unsigned int moff; > @@ -6203,11 +6283,6 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, > return -LIBBPF_ERRNO__FORMAT; > } > name = elf_sym_str(obj, sym->st_name) ?: "<?>"; > - if (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; > - } > > pr_debug(".maps relo #%d: for %zd value %zd rel->r_offset %zu name %d ('%s')\n", > i, (ssize_t)(rel->r_info >> 32), (size_t)sym->st_value, > @@ -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). > + 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", > @@ -6238,9 +6325,15 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, > return -EINVAL; > } > > - targ_map = bpf_object__find_map_by_name(obj, name); > - if (!targ_map) > - return -ESRCH; > + if (is_map_in_map) { > + targ_map = bpf_object__find_map_by_name(obj, name); > + if (!targ_map) > + return -ESRCH; > + } else { > + targ_prog = bpf_object__find_program_by_name(obj, name); > + if (!targ_prog) > + return -ESRCH; > + } > > var = btf__type_by_id(obj->btf, vi->type); > def = skip_mods_and_typedefs(obj->btf, var->type, NULL); > @@ -6272,10 +6365,14 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, > (new_sz - map->init_slots_sz) * host_ptr_sz); > map->init_slots_sz = new_sz; > } > - map->init_slots[moff] = targ_map; > + map->init_slots[moff] = is_map_in_map ? (void *)targ_map : (void *)targ_prog; > > - pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n", > - i, map->name, moff, name); > + if (is_map_in_map) > + pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n", > + i, map->name, moff, name); > + else > + pr_debug(".maps relo #%d: map '%s' slot [%d] points to prog '%s'\n", > + i, map->name, moff, name); similar as above `is_map_in_map ? "map" : "prog"` will keep it short and not duplicated > } > > return 0; > @@ -7293,6 +7390,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr) > err = err ? : bpf_object__create_maps(obj); > err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : attr->target_btf_path); > err = err ? : bpf_object__load_progs(obj, attr->log_level); > + err = err ? : bpf_object_init_prog_array(obj); > > if (obj->gen_loader) { > /* reset FDs */ > -- > 2.30.2
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 ? > if (!btf_is_func_proto(t)) { > pr_warn(..); > return -EINVAl; > } > continue; > > It's more consistent with the other logic in this function > > >> + pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n", >> + map_name, btf_kind_str(t)); >> + return -EINVAL; >> + } >> + if (is_map_in_map && !btf_is_struct(t)) { > > well, it can't be anything else, so I guess drop the is_map_in_map check? > Right, will do. >> pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n", >> map_name, btf_kind_str(t)); >> return -EINVAL; >> @@ -4964,12 +4985,16 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map) >> unsigned int i; >> int fd, err = 0; >> >> + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY) >> + return 0; > > let's leave a comment that PROG_ARRAY can only be initialized once all > the programs are loaded, so this will be done later > > Better still, it would be good to also rename init_map_slots to > init_map_in_map_slots and do the PROG_ARRAY check outside, in > bpf_object__create_maps(). This creates a nice symmetry with > init_prog_array (should it be named init_prog_array_slots for > consistency?). WDYT? > Ack. >> + >> for (i = 0; i < map->init_slots_sz; i++) { >> if (!map->init_slots[i]) >> continue; >> >> targ_map = map->init_slots[i]; >> fd = bpf_map__fd(targ_map); >> + >> if (obj->gen_loader) { >> pr_warn("// TODO map_update_elem: idx %td key %d value==map_idx %td\n", >> map - obj->maps, i, targ_map - obj->maps); >> @@ -4980,8 +5005,7 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map) >> if (err) { >> err = -errno; >> pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n", >> - map->name, i, targ_map->name, >> - fd, err); >> + map->name, i, targ_map->name, fd, err); >> return err; >> } >> pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n", >> @@ -4994,6 +5018,60 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map) >> return 0; >> } >> >> +static int init_prog_array(struct bpf_object *obj, struct bpf_map *map) >> +{ >> + const struct bpf_program *targ_prog; >> + unsigned int i; >> + int fd, err = 0; > > you always set err, no need to zero-initialize it > OK, will remove it. >> + >> + for (i = 0; i < map->init_slots_sz; i++) { >> + if (!map->init_slots[i]) >> + continue; >> + >> + targ_prog = map->init_slots[i]; >> + fd = bpf_program__fd(targ_prog); >> + >> + if (obj->gen_loader) { >> + return -ENOTSUP; >> + } else { >> + err = bpf_map_update_elem(map->fd, &i, &fd, 0); >> + } >> + if (err) { >> + err = -errno; >> + pr_warn("map '%s': failed to initialize slot [%d] to prog '%s' fd=%d: %d\n", >> + map->name, i, targ_prog->name, fd, err); >> + return err; >> + } >> + pr_debug("map '%s': slot [%d] set to prog '%s' fd=%d\n", >> + map->name, i, targ_prog->name, fd); >> + } >> + >> + zfree(&map->init_slots); >> + map->init_slots_sz = 0; >> + >> + return 0; >> +} >> + >> +static int bpf_object_init_prog_array(struct bpf_object *obj) > > s/prog_array/prog_arrays/ > >> +{ >> + struct bpf_map *map; >> + int i, err; >> + >> + for (i = 0; i < obj->nr_maps; i++) { >> + map = &obj->maps[i]; >> + >> + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY && >> + map->init_slots_sz) { > > nit: longer single line is fine here (but you can also invert the > condition and continue early to avoid extra level of nestedness) > Will do. >> + err = init_prog_array(obj, map); >> + if (err < 0) { >> + zclose(map->fd); >> + return err; >> + } >> + } >> + } >> + return 0; >> +} >> + >> static int >> bpf_object__create_maps(struct bpf_object *obj) >> { >> @@ -6174,7 +6252,9 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, >> int i, j, nrels, new_sz; >> const struct btf_var_secinfo *vi = NULL; >> const struct btf_type *sec, *var, *def; >> - struct bpf_map *map = NULL, *targ_map; >> + struct bpf_map *map = NULL, *targ_map = NULL; >> + struct bpf_program *targ_prog = NULL; >> + bool is_prog_array, is_map_in_map; >> const struct btf_member *member; >> const char *name, *mname; >> unsigned int moff; >> @@ -6203,11 +6283,6 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, >> return -LIBBPF_ERRNO__FORMAT; >> } >> name = elf_sym_str(obj, sym->st_name) ?: "<?>"; >> - if (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; >> - } >> >> pr_debug(".maps relo #%d: for %zd value %zd rel->r_offset %zu name %d ('%s')\n", >> i, (ssize_t)(rel->r_info >> 32), (size_t)sym->st_value, >> @@ -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 ? >> + 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", >> @@ -6238,9 +6325,15 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, >> return -EINVAL; >> } >> >> - targ_map = bpf_object__find_map_by_name(obj, name); >> - if (!targ_map) >> - return -ESRCH; >> + if (is_map_in_map) { >> + targ_map = bpf_object__find_map_by_name(obj, name); >> + if (!targ_map) >> + return -ESRCH; >> + } else { >> + targ_prog = bpf_object__find_program_by_name(obj, name); >> + if (!targ_prog) >> + return -ESRCH; >> + } >> >> var = btf__type_by_id(obj->btf, vi->type); >> def = skip_mods_and_typedefs(obj->btf, var->type, NULL); >> @@ -6272,10 +6365,14 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, >> (new_sz - map->init_slots_sz) * host_ptr_sz); >> map->init_slots_sz = new_sz; >> } >> - map->init_slots[moff] = targ_map; >> + map->init_slots[moff] = is_map_in_map ? (void *)targ_map : (void *)targ_prog; >> >> - pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n", >> - i, map->name, moff, name); >> + if (is_map_in_map) >> + pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n", >> + i, map->name, moff, name); >> + else >> + pr_debug(".maps relo #%d: map '%s' slot [%d] points to prog '%s'\n", >> + i, map->name, moff, name); > > similar as above `is_map_in_map ? "map" : "prog"` will keep it short > and not duplicated > > Ack. >> } >> >> return 0; >> @@ -7293,6 +7390,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr) >> err = err ? : bpf_object__create_maps(obj); >> err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : attr->target_btf_path); >> err = err ? : bpf_object__load_progs(obj, attr->log_level); >> + err = err ? : bpf_object_init_prog_array(obj); >> >> if (obj->gen_loader) { >> /* reset FDs */ >> -- >> 2.30.2 [0]: https://github.com/libbpf/libbpf/blob/master/src/libbpf.c#L2288-L2292 Cheers, --- Hengqi
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", [...]
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index af405c38aadc..6b4a175d717d 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -2277,6 +2277,8 @@ int parse_btf_map_def(const char *map_name, struct btf *btf, map_def->parts |= MAP_DEF_VALUE_SIZE | MAP_DEF_VALUE_TYPE; } else if (strcmp(name, "values") == 0) { + bool is_map_in_map = bpf_map_type__is_map_in_map(map_def->map_type); + bool is_prog_array = map_def->map_type == BPF_MAP_TYPE_PROG_ARRAY; char inner_map_name[128]; int err; @@ -2290,8 +2292,8 @@ int parse_btf_map_def(const char *map_name, struct btf *btf, map_name, name); return -EINVAL; } - if (!bpf_map_type__is_map_in_map(map_def->map_type)) { - pr_warn("map '%s': should be map-in-map.\n", + if (!is_map_in_map && !is_prog_array) { + pr_warn("map '%s': should be map-in-map or prog-array.\n", map_name); return -ENOTSUP; } @@ -2303,23 +2305,42 @@ int parse_btf_map_def(const char *map_name, struct btf *btf, map_def->value_size = 4; t = btf__type_by_id(btf, m->type); if (!t) { - pr_warn("map '%s': map-in-map inner type [%d] not found.\n", - map_name, m->type); + if (is_map_in_map) + pr_warn("map '%s': map-in-map inner type [%d] not found.\n", + map_name, m->type); + else + pr_warn("map '%s': prog-array value type [%d] not found.\n", + map_name, m->type); return -EINVAL; } if (!btf_is_array(t) || btf_array(t)->nelems) { - pr_warn("map '%s': map-in-map inner spec is not a zero-sized array.\n", - map_name); + if (is_map_in_map) + pr_warn("map '%s': map-in-map inner spec is not a zero-sized array.\n", + map_name); + else + pr_warn("map '%s': prog-array value spec is not a zero-sized array.\n", + map_name); return -EINVAL; } 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", + 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; + pr_warn("map '%s': prog-array value def is of unexpected kind %s.\n", + map_name, btf_kind_str(t)); + return -EINVAL; + } + if (is_map_in_map && !btf_is_struct(t)) { pr_warn("map '%s': map-in-map inner def is of unexpected kind %s.\n", map_name, btf_kind_str(t)); return -EINVAL; @@ -4964,12 +4985,16 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map) unsigned int i; int fd, err = 0; + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY) + return 0; + for (i = 0; i < map->init_slots_sz; i++) { if (!map->init_slots[i]) continue; targ_map = map->init_slots[i]; fd = bpf_map__fd(targ_map); + if (obj->gen_loader) { pr_warn("// TODO map_update_elem: idx %td key %d value==map_idx %td\n", map - obj->maps, i, targ_map - obj->maps); @@ -4980,8 +5005,7 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map) if (err) { err = -errno; pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n", - map->name, i, targ_map->name, - fd, err); + map->name, i, targ_map->name, fd, err); return err; } pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n", @@ -4994,6 +5018,60 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map) return 0; } +static int init_prog_array(struct bpf_object *obj, struct bpf_map *map) +{ + const struct bpf_program *targ_prog; + unsigned int i; + int fd, err = 0; + + for (i = 0; i < map->init_slots_sz; i++) { + if (!map->init_slots[i]) + continue; + + targ_prog = map->init_slots[i]; + fd = bpf_program__fd(targ_prog); + + if (obj->gen_loader) { + return -ENOTSUP; + } else { + err = bpf_map_update_elem(map->fd, &i, &fd, 0); + } + if (err) { + err = -errno; + pr_warn("map '%s': failed to initialize slot [%d] to prog '%s' fd=%d: %d\n", + map->name, i, targ_prog->name, fd, err); + return err; + } + pr_debug("map '%s': slot [%d] set to prog '%s' fd=%d\n", + map->name, i, targ_prog->name, fd); + } + + zfree(&map->init_slots); + map->init_slots_sz = 0; + + return 0; +} + +static int bpf_object_init_prog_array(struct bpf_object *obj) +{ + struct bpf_map *map; + int i, err; + + for (i = 0; i < obj->nr_maps; i++) { + map = &obj->maps[i]; + + if (map->def.type == BPF_MAP_TYPE_PROG_ARRAY && + map->init_slots_sz) { + err = init_prog_array(obj, map); + if (err < 0) { + zclose(map->fd); + return err; + } + } + } + return 0; +} + static int bpf_object__create_maps(struct bpf_object *obj) { @@ -6174,7 +6252,9 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, int i, j, nrels, new_sz; const struct btf_var_secinfo *vi = NULL; const struct btf_type *sec, *var, *def; - struct bpf_map *map = NULL, *targ_map; + struct bpf_map *map = NULL, *targ_map = NULL; + struct bpf_program *targ_prog = NULL; + bool is_prog_array, is_map_in_map; const struct btf_member *member; const char *name, *mname; unsigned int moff; @@ -6203,11 +6283,6 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, return -LIBBPF_ERRNO__FORMAT; } name = elf_sym_str(obj, sym->st_name) ?: "<?>"; - if (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; - } pr_debug(".maps relo #%d: for %zd value %zd rel->r_offset %zu name %d ('%s')\n", i, (ssize_t)(rel->r_info >> 32), (size_t)sym->st_value, @@ -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)) { + pr_warn(".maps relo #%d: '%s' isn't a BPF program\n", + 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", @@ -6238,9 +6325,15 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, return -EINVAL; } - targ_map = bpf_object__find_map_by_name(obj, name); - if (!targ_map) - return -ESRCH; + if (is_map_in_map) { + targ_map = bpf_object__find_map_by_name(obj, name); + if (!targ_map) + return -ESRCH; + } else { + targ_prog = bpf_object__find_program_by_name(obj, name); + if (!targ_prog) + return -ESRCH; + } var = btf__type_by_id(obj->btf, vi->type); def = skip_mods_and_typedefs(obj->btf, var->type, NULL); @@ -6272,10 +6365,14 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj, (new_sz - map->init_slots_sz) * host_ptr_sz); map->init_slots_sz = new_sz; } - map->init_slots[moff] = targ_map; + map->init_slots[moff] = is_map_in_map ? (void *)targ_map : (void *)targ_prog; - pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n", - i, map->name, moff, name); + if (is_map_in_map) + pr_debug(".maps relo #%d: map '%s' slot [%d] points to map '%s'\n", + i, map->name, moff, name); + else + pr_debug(".maps relo #%d: map '%s' slot [%d] points to prog '%s'\n", + i, map->name, moff, name); } return 0; @@ -7293,6 +7390,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr) err = err ? : bpf_object__create_maps(obj); err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : attr->target_btf_path); err = err ? : bpf_object__load_progs(obj, attr->log_level); + err = err ? : bpf_object_init_prog_array(obj); if (obj->gen_loader) { /* reset FDs */
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(-) -- 2.30.2