Message ID | 20230316023641.2092778-5-kuifeng@meta.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | BPF |
Headers | show |
Series | Transit between BPF TCP congestion controls. | expand |
On 3/15/23 7:36 PM, Kui-Feng Lee wrote: > @@ -11590,31 +11631,32 @@ struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map) > if (!link) > return libbpf_err_ptr(-EINVAL); > > - st_ops = map->st_ops; > - for (i = 0; i < btf_vlen(st_ops->type); i++) { > - struct bpf_program *prog = st_ops->progs[i]; > - void *kern_data; > - int prog_fd; > + /* kern_vdata should be prepared during the loading phase. */ > + err = bpf_map_update_elem(map->fd, &zero, map->st_ops->kern_vdata, 0); > + if (err) { It should not fail for BPF_F_LINK struct_ops when err is EBUSY. The struct_ops map can attach, detach, and then attach again. It needs a test for this case. > + free(link); > + return libbpf_err_ptr(err); > + }
On 3/17/23 11:44, Martin KaFai Lau wrote: > On 3/15/23 7:36 PM, Kui-Feng Lee wrote: >> @@ -11590,31 +11631,32 @@ struct bpf_link >> *bpf_map__attach_struct_ops(const struct bpf_map *map) >> if (!link) >> return libbpf_err_ptr(-EINVAL); >> - st_ops = map->st_ops; >> - for (i = 0; i < btf_vlen(st_ops->type); i++) { >> - struct bpf_program *prog = st_ops->progs[i]; >> - void *kern_data; >> - int prog_fd; >> + /* kern_vdata should be prepared during the loading phase. */ >> + err = bpf_map_update_elem(map->fd, &zero, >> map->st_ops->kern_vdata, 0); >> + if (err) { > > It should not fail for BPF_F_LINK struct_ops when err is EBUSY. > The struct_ops map can attach, detach, and then attach again. > > It needs a test for this case. Got it! > >> + free(link); >> + return libbpf_err_ptr(err); >> + } >
On Wed, Mar 15, 2023 at 7:37 PM Kui-Feng Lee <kuifeng@meta.com> wrote: > > bpf_map__attach_struct_ops() was creating a dummy bpf_link as a > placeholder, but now it is constructing an authentic one by calling > bpf_link_create() if the map has the BPF_F_LINK flag. > > You can flag a struct_ops map with BPF_F_LINK by calling > bpf_map__set_map_flags(). > > Signed-off-by: Kui-Feng Lee <kuifeng@meta.com> > --- > tools/lib/bpf/libbpf.c | 90 +++++++++++++++++++++++++++++++----------- > 1 file changed, 66 insertions(+), 24 deletions(-) > [...] > - if (!prog) > - continue; > + link->link.detach = bpf_link__detach_struct_ops; > > - prog_fd = bpf_program__fd(prog); > - kern_data = st_ops->kern_vdata + st_ops->kern_func_off[i]; > - *(unsigned long *)kern_data = prog_fd; > + if (!(map->def.map_flags & BPF_F_LINK)) { > + /* w/o a real link */ > + link->link.fd = map->fd; > + link->map_fd = -1; > + return &link->link; > } > > - err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0); > - if (err) { > - err = -errno; > + fd = bpf_link_create(map->fd, -1, BPF_STRUCT_OPS, NULL); pass 0, not -1. BPF APIs have a convention that fd=0 means "no fd was provided". And actually kernel should have rejected this -1, so please check why that didn't happen in your testing, we might be missing some kernel validation. > + if (fd < 0) { > free(link); > - return libbpf_err_ptr(err); > + return libbpf_err_ptr(fd); > } > > - link->detach = bpf_link__detach_struct_ops; > - link->fd = map->fd; > + link->link.fd = fd; > + link->map_fd = map->fd; > > - return link; > + return &link->link; > } > > typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct perf_event_header *hdr, > -- > 2.34.1 >
On 3/17/23 15:23, Andrii Nakryiko wrote: > On Wed, Mar 15, 2023 at 7:37 PM Kui-Feng Lee <kuifeng@meta.com> wrote: >> >> bpf_map__attach_struct_ops() was creating a dummy bpf_link as a >> placeholder, but now it is constructing an authentic one by calling >> bpf_link_create() if the map has the BPF_F_LINK flag. >> >> You can flag a struct_ops map with BPF_F_LINK by calling >> bpf_map__set_map_flags(). >> >> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com> >> --- >> tools/lib/bpf/libbpf.c | 90 +++++++++++++++++++++++++++++++----------- >> 1 file changed, 66 insertions(+), 24 deletions(-) >> > > [...] > >> - if (!prog) >> - continue; >> + link->link.detach = bpf_link__detach_struct_ops; >> >> - prog_fd = bpf_program__fd(prog); >> - kern_data = st_ops->kern_vdata + st_ops->kern_func_off[i]; >> - *(unsigned long *)kern_data = prog_fd; >> + if (!(map->def.map_flags & BPF_F_LINK)) { >> + /* w/o a real link */ >> + link->link.fd = map->fd; >> + link->map_fd = -1; >> + return &link->link; >> } >> >> - err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0); >> - if (err) { >> - err = -errno; >> + fd = bpf_link_create(map->fd, -1, BPF_STRUCT_OPS, NULL); > > pass 0, not -1. BPF APIs have a convention that fd=0 means "no fd was > provided". And actually kernel should have rejected this -1, so please > check why that didn't happen in your testing, we might be missing some > kernel validation. Oh! probe_perf_link() also pass -1 as well. I will fix it. > > >> + if (fd < 0) { >> free(link); >> - return libbpf_err_ptr(err); >> + return libbpf_err_ptr(fd); >> } >> >> - link->detach = bpf_link__detach_struct_ops; >> - link->fd = map->fd; >> + link->link.fd = fd; >> + link->map_fd = map->fd; >> >> - return link; >> + return &link->link; >> } >> >> typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct perf_event_header *hdr, >> -- >> 2.34.1 >>
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index a557718401e4..6dbae7ffab48 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -116,6 +116,7 @@ static const char * const attach_type_name[] = { [BPF_SK_REUSEPORT_SELECT_OR_MIGRATE] = "sk_reuseport_select_or_migrate", [BPF_PERF_EVENT] = "perf_event", [BPF_TRACE_KPROBE_MULTI] = "trace_kprobe_multi", + [BPF_STRUCT_OPS] = "struct_ops", }; static const char * const link_type_name[] = { @@ -7677,6 +7678,37 @@ static int bpf_object__resolve_externs(struct bpf_object *obj, return 0; } +static void bpf_map_prepare_vdata(const struct bpf_map *map) +{ + struct bpf_struct_ops *st_ops; + __u32 i; + + st_ops = map->st_ops; + for (i = 0; i < btf_vlen(st_ops->type); i++) { + struct bpf_program *prog = st_ops->progs[i]; + void *kern_data; + int prog_fd; + + if (!prog) + continue; + + prog_fd = bpf_program__fd(prog); + kern_data = st_ops->kern_vdata + st_ops->kern_func_off[i]; + *(unsigned long *)kern_data = prog_fd; + } +} + +static int bpf_object_prepare_struct_ops(struct bpf_object *obj) +{ + int i; + + for (i = 0; i < obj->nr_maps; i++) + if (bpf_map__is_struct_ops(&obj->maps[i])) + bpf_map_prepare_vdata(&obj->maps[i]); + + return 0; +} + static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const char *target_btf_path) { int err, i; @@ -7702,6 +7734,7 @@ static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const ch err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : target_btf_path); err = err ? : bpf_object__load_progs(obj, extra_log_level); err = err ? : bpf_object_init_prog_arrays(obj); + err = err ? : bpf_object_prepare_struct_ops(obj); if (obj->gen_loader) { /* reset FDs */ @@ -11566,22 +11599,30 @@ struct bpf_link *bpf_program__attach(const struct bpf_program *prog) return link; } +struct bpf_link_struct_ops { + struct bpf_link link; + int map_fd; +}; + static int bpf_link__detach_struct_ops(struct bpf_link *link) { + struct bpf_link_struct_ops *st_link; __u32 zero = 0; - if (bpf_map_delete_elem(link->fd, &zero)) - return -errno; + st_link = container_of(link, struct bpf_link_struct_ops, link); - return 0; + if (st_link->map_fd < 0) + /* w/o a real link */ + return bpf_map_delete_elem(link->fd, &zero); + + return close(link->fd); } struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map) { - struct bpf_struct_ops *st_ops; - struct bpf_link *link; - __u32 i, zero = 0; - int err; + struct bpf_link_struct_ops *link; + __u32 zero = 0; + int err, fd; if (!bpf_map__is_struct_ops(map) || map->fd == -1) return libbpf_err_ptr(-EINVAL); @@ -11590,31 +11631,32 @@ struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map) if (!link) return libbpf_err_ptr(-EINVAL); - st_ops = map->st_ops; - for (i = 0; i < btf_vlen(st_ops->type); i++) { - struct bpf_program *prog = st_ops->progs[i]; - void *kern_data; - int prog_fd; + /* kern_vdata should be prepared during the loading phase. */ + err = bpf_map_update_elem(map->fd, &zero, map->st_ops->kern_vdata, 0); + if (err) { + free(link); + return libbpf_err_ptr(err); + } - if (!prog) - continue; + link->link.detach = bpf_link__detach_struct_ops; - prog_fd = bpf_program__fd(prog); - kern_data = st_ops->kern_vdata + st_ops->kern_func_off[i]; - *(unsigned long *)kern_data = prog_fd; + if (!(map->def.map_flags & BPF_F_LINK)) { + /* w/o a real link */ + link->link.fd = map->fd; + link->map_fd = -1; + return &link->link; } - err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0); - if (err) { - err = -errno; + fd = bpf_link_create(map->fd, -1, BPF_STRUCT_OPS, NULL); + if (fd < 0) { free(link); - return libbpf_err_ptr(err); + return libbpf_err_ptr(fd); } - link->detach = bpf_link__detach_struct_ops; - link->fd = map->fd; + link->link.fd = fd; + link->map_fd = map->fd; - return link; + return &link->link; } typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct perf_event_header *hdr,
bpf_map__attach_struct_ops() was creating a dummy bpf_link as a placeholder, but now it is constructing an authentic one by calling bpf_link_create() if the map has the BPF_F_LINK flag. You can flag a struct_ops map with BPF_F_LINK by calling bpf_map__set_map_flags(). Signed-off-by: Kui-Feng Lee <kuifeng@meta.com> --- tools/lib/bpf/libbpf.c | 90 +++++++++++++++++++++++++++++++----------- 1 file changed, 66 insertions(+), 24 deletions(-)