| Message ID | 20230308005050.255859-5-kuifeng@meta.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Transit between BPF TCP congestion controls. | expand |
On 3/7/23 4:50 PM, Kui-Feng Lee wrote: > @@ -11566,22 +11591,34 @@ 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) { map_fd < 0 should always be true? > + /* Fake bpf_link */ > + if (bpf_map_delete_elem(link->fd, &zero)) > + return -errno; > + return 0; > + } > + > + /* Doesn't support detaching. */ > + return -EOPNOTSUPP; > } > > 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 +11627,34 @@ 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) { > + err = -errno; > + free(link); > + return libbpf_err_ptr(err); > + } > > - 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; > + if (!(map->def.map_flags & BPF_F_LINK)) { > + /* Fake bpf_link */ > + link->link.fd = map->fd; > + link->map_fd = -1; > + link->link.detach = bpf_link__detach_struct_ops; > + return &link->link; > } > > - err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0); > - if (err) { > + fd = bpf_link_create(map->fd, -1, BPF_STRUCT_OPS, NULL); > + if (fd < 0) { > err = -errno; > free(link); > return libbpf_err_ptr(err); > } > > - link->detach = bpf_link__detach_struct_ops; > - link->fd = map->fd; > + link->link.fd = fd; > + link->map_fd = map->fd; Does it need to set link->link.detach? > > - return link; > + return &link->link; > } > > typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct perf_event_header *hdr,
On 3/8/23 13:42, Martin KaFai Lau wrote: > On 3/7/23 4:50 PM, Kui-Feng Lee wrote: >> @@ -11566,22 +11591,34 @@ 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) { > > map_fd < 0 should always be true? If the user pass a wrong link, it can fail. I check it here explicitly even the kernel returns an error for deleting an element of a struct_ops w/ link. > >> + /* Fake bpf_link */ >> + if (bpf_map_delete_elem(link->fd, &zero)) >> + return -errno; >> + return 0; >> + } >> + >> + /* Doesn't support detaching. */ >> + return -EOPNOTSUPP; >> } >> 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 +11627,34 @@ 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) { >> + err = -errno; >> + free(link); >> + return libbpf_err_ptr(err); >> + } >> - 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; >> + if (!(map->def.map_flags & BPF_F_LINK)) { >> + /* Fake bpf_link */ >> + link->link.fd = map->fd; >> + link->map_fd = -1; >> + link->link.detach = bpf_link__detach_struct_ops; >> + return &link->link; >> } >> - err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0); >> - if (err) { >> + fd = bpf_link_create(map->fd, -1, BPF_STRUCT_OPS, NULL); >> + if (fd < 0) { >> err = -errno; >> free(link); >> return libbpf_err_ptr(err); >> } >> - link->detach = bpf_link__detach_struct_ops; >> - link->fd = map->fd; >> + link->link.fd = fd; >> + link->map_fd = map->fd; > > Does it need to set link->link.detach? Yes, I have made some changes to this part. The new code will set link->link.detach for BPF_F_LINK as well to cleanup fd. > >> - return link; >> + return &link->link; >> } >> typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct >> perf_event_header *hdr, >
On 3/8/23 4:22 PM, Kui-Feng Lee wrote: > > > On 3/8/23 13:42, Martin KaFai Lau wrote: >> On 3/7/23 4:50 PM, Kui-Feng Lee wrote: >>> @@ -11566,22 +11591,34 @@ 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) { >> >> map_fd < 0 should always be true? > > If the user pass a wrong link, it can fail. I may have missed something. How can user directly pass a link to this static function? > I check it here explicitly even the kernel returns > an error for deleting an element of a struct_ops w/ link. Yep, the kernel should have stopped the delete if the user somehow corrupted the map_fd to -1. > >> >>> + /* Fake bpf_link */ >>> + if (bpf_map_delete_elem(link->fd, &zero)) >>> + return -errno; >>> + return 0; >>> + } >>> + >>> + /* Doesn't support detaching. */ >>> + return -EOPNOTSUPP;
On 3/9/23 09:09, Martin KaFai Lau wrote: > On 3/8/23 4:22 PM, Kui-Feng Lee wrote: >> >> >> On 3/8/23 13:42, Martin KaFai Lau wrote: >>> On 3/7/23 4:50 PM, Kui-Feng Lee wrote: >>>> @@ -11566,22 +11591,34 @@ 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) { >>> >>> map_fd < 0 should always be true? >> >> If the user pass a wrong link, it can fail. > > I may have missed something. How can user directly pass a link to this > static function? Ouch! You are right. This check is not necessary. I mixed it with the old detach feature. > >> I check it here explicitly even the kernel returns >> an error for deleting an element of a struct_ops w/ link. > Yep, the kernel should have stopped the delete if the user somehow > corrupted the map_fd to -1. > >> >>> >>>> + /* Fake bpf_link */ >>>> + if (bpf_map_delete_elem(link->fd, &zero)) >>>> + return -errno; >>>> + return 0; >>>> + } >>>> + >>>> + /* Doesn't support detaching. */ >>>> + return -EOPNOTSUPP; >
On 3/9/23 10:16, Kui-Feng Lee wrote: > > > On 3/9/23 09:09, Martin KaFai Lau wrote: >> On 3/8/23 4:22 PM, Kui-Feng Lee wrote: >>> >>> >>> On 3/8/23 13:42, Martin KaFai Lau wrote: >>>> On 3/7/23 4:50 PM, Kui-Feng Lee wrote: >>>>> @@ -11566,22 +11591,34 @@ 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) { >>>> >>>> map_fd < 0 should always be true? >>> >>> If the user pass a wrong link, it can fail. >> >> I may have missed something. How can user directly pass a link to this >> static function? > > Ouch! You are right. This check is not necessary. I mixed it with the > old detach feature. By the way, I will keep this test here since this function will handle the case w/o a link as well. > > >> >>> I check it here explicitly even the kernel returns >>> an error for deleting an element of a struct_ops w/ link. >> Yep, the kernel should have stopped the delete if the user somehow >> corrupted the map_fd to -1. >> >>> >>>> >>>>> + /* Fake bpf_link */ >>>>> + if (bpf_map_delete_elem(link->fd, &zero)) >>>>> + return -errno; >>>>> + return 0; >>>>> + } >>>>> + >>>>> + /* Doesn't support detaching. */ >>>>> + return -EOPNOTSUPP; >>
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index a557718401e4..f70b55c0f40e 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,26 @@ 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_load(struct bpf_object *obj, int extra_log_level, const char *target_btf_path) { int err, i; @@ -7728,6 +7749,10 @@ static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const ch btf__free(obj->btf_vmlinux); obj->btf_vmlinux = NULL; + for (i = 0; i < obj->nr_maps; i++) + if (bpf_map__is_struct_ops(&obj->maps[i])) + bpf_map_prepare_vdata(&obj->maps[i]); + obj->loaded = true; /* doesn't matter if successfully or not */ if (err) @@ -11566,22 +11591,34 @@ 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) { + /* Fake bpf_link */ + if (bpf_map_delete_elem(link->fd, &zero)) + return -errno; + return 0; + } + + /* Doesn't support detaching. */ + return -EOPNOTSUPP; } 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 +11627,34 @@ 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) { + err = -errno; + free(link); + return libbpf_err_ptr(err); + } - 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; + if (!(map->def.map_flags & BPF_F_LINK)) { + /* Fake bpf_link */ + link->link.fd = map->fd; + link->map_fd = -1; + link->link.detach = bpf_link__detach_struct_ops; + return &link->link; } - err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0); - if (err) { + fd = bpf_link_create(map->fd, -1, BPF_STRUCT_OPS, NULL); + if (fd < 0) { err = -errno; free(link); return libbpf_err_ptr(err); } - 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 | 84 +++++++++++++++++++++++++++++++----------- 1 file changed, 62 insertions(+), 22 deletions(-)