* [PATCH bpf-next] libbpf: deprecate bpf_object__unload() API @ 2021-09-08 15:35 Hengqi Chen 2021-09-09 4:38 ` Andrii Nakryiko 0 siblings, 1 reply; 3+ messages in thread From: Hengqi Chen @ 2021-09-08 15:35 UTC (permalink / raw) To: bpf; +Cc: ast, daniel, andrii, yhs, john.fastabend, hengqi.chen BPF objects are not re-loadable after unload. User are expected to use bpf_object__close() to unload and free up resources in one operation. No need to expose bpf_object__unload() as a public API, deprecate it.[0] Remove bpf_object__unload() inside bpf_object__load_xattr(), it is the caller's responsibility to free up resources, otherwise, the following code path will cause double-free problem when loading failed: bpf_prog_load bpf_prog_load_xattr bpf_object__load bpf_object__load_xattr Replace bpf_object__unload() inside bpf_object__close() with the necessary cleanup operations to avoid compilation error. [0] Closes: https://github.com/libbpf/libbpf/issues/290 Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> --- tools/lib/bpf/libbpf.c | 8 +++++--- tools/lib/bpf/libbpf.h | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 8f579c6666b2..c56b466c5461 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -6931,7 +6931,6 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr) if (obj->maps[i].pinned && !obj->maps[i].reused) bpf_map__unpin(&obj->maps[i], NULL); - bpf_object__unload(obj); pr_warn("failed to load object '%s'\n", obj->path); return libbpf_err(err); } @@ -7540,12 +7539,15 @@ void bpf_object__close(struct bpf_object *obj) bpf_gen__free(obj->gen_loader); bpf_object__elf_finish(obj); - bpf_object__unload(obj); btf__free(obj->btf); btf_ext__free(obj->btf_ext); - for (i = 0; i < obj->nr_maps; i++) + for (i = 0; i < obj->nr_maps; i++) { + zclose(obj->maps[i].fd); + if (obj->maps[i].st_ops) + zfree(&obj->maps[i].st_ops->kern_vdata); bpf_map__destroy(&obj->maps[i]); + } zfree(&obj->btf_custom_path); zfree(&obj->kconfig); diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 2f6f0e15d1e7..748f7dabe4c7 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -147,7 +147,8 @@ struct bpf_object_load_attr { /* Load/unload object into/from kernel */ LIBBPF_API int bpf_object__load(struct bpf_object *obj); LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr); -LIBBPF_API int bpf_object__unload(struct bpf_object *obj); +LIBBPF_API LIBBPF_DEPRECATED("bpf_object__unload() is deprecated, use bpf_object__close() instead") +int bpf_object__unload(struct bpf_object *obj); LIBBPF_API const char *bpf_object__name(const struct bpf_object *obj); LIBBPF_API unsigned int bpf_object__kversion(const struct bpf_object *obj); -- 2.25.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next] libbpf: deprecate bpf_object__unload() API 2021-09-08 15:35 [PATCH bpf-next] libbpf: deprecate bpf_object__unload() API Hengqi Chen @ 2021-09-09 4:38 ` Andrii Nakryiko 2021-10-02 16:07 ` Hengqi Chen 0 siblings, 1 reply; 3+ messages in thread From: Andrii Nakryiko @ 2021-09-09 4:38 UTC (permalink / raw) To: Hengqi Chen Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, john fastabend On Wed, Sep 8, 2021 at 8:35 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > BPF objects are not re-loadable after unload. User are expected to use > bpf_object__close() to unload and free up resources in one operation. > No need to expose bpf_object__unload() as a public API, deprecate it.[0] > Remove bpf_object__unload() inside bpf_object__load_xattr(), it is the > caller's responsibility to free up resources, otherwise, the following > code path will cause double-free problem when loading failed: > > bpf_prog_load > bpf_prog_load_xattr > bpf_object__load > bpf_object__load_xattr > Did you see this double-free ever happen? I'm looking at the code and not seeing it. Seems like bpf_object__unload() is idempotent, so no mater how many times we call it, it doesn't do any harm. Look at how zclose and zfree are implemented, they zero-out fields and also check for non-zero values before doing something. So unless I'm missing something, there is no problem. > Replace bpf_object__unload() inside bpf_object__close() with the necessary > cleanup operations to avoid compilation error. > > [0] Closes: https://github.com/libbpf/libbpf/issues/290 > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > --- > tools/lib/bpf/libbpf.c | 8 +++++--- > tools/lib/bpf/libbpf.h | 3 ++- > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 8f579c6666b2..c56b466c5461 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -6931,7 +6931,6 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr) > if (obj->maps[i].pinned && !obj->maps[i].reused) > bpf_map__unpin(&obj->maps[i], NULL); > > - bpf_object__unload(obj); I think unloading already loaded bpf programs is bpf_object__load()'s responsibility, so please don't remove this. > pr_warn("failed to load object '%s'\n", obj->path); > return libbpf_err(err); > } > @@ -7540,12 +7539,15 @@ void bpf_object__close(struct bpf_object *obj) > > bpf_gen__free(obj->gen_loader); > bpf_object__elf_finish(obj); > - bpf_object__unload(obj); same, this is fine, don't remove it > btf__free(obj->btf); > btf_ext__free(obj->btf_ext); > > - for (i = 0; i < obj->nr_maps; i++) > + for (i = 0; i < obj->nr_maps; i++) { > + zclose(obj->maps[i].fd); > + if (obj->maps[i].st_ops) > + zfree(&obj->maps[i].st_ops->kern_vdata); > bpf_map__destroy(&obj->maps[i]); > + } and no changes should be necessary here either > > zfree(&obj->btf_custom_path); > zfree(&obj->kconfig); > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index 2f6f0e15d1e7..748f7dabe4c7 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -147,7 +147,8 @@ struct bpf_object_load_attr { > /* Load/unload object into/from kernel */ > LIBBPF_API int bpf_object__load(struct bpf_object *obj); > LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr); > -LIBBPF_API int bpf_object__unload(struct bpf_object *obj); > +LIBBPF_API LIBBPF_DEPRECATED("bpf_object__unload() is deprecated, use bpf_object__close() instead") > +int bpf_object__unload(struct bpf_object *obj); > This is the right change, but let's also keep original bpf_object__unload() logic. I'd recommend renaming bpf_object__unload() into bpf_object_unload() (so that's naming is more clearly showing it's an internal function) and make it static. Then have a small shim of bpf_object__unload() calling into bpf_object_unload() until we remove that in libbpf 1.0. > LIBBPF_API const char *bpf_object__name(const struct bpf_object *obj); > LIBBPF_API unsigned int bpf_object__kversion(const struct bpf_object *obj); > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next] libbpf: deprecate bpf_object__unload() API 2021-09-09 4:38 ` Andrii Nakryiko @ 2021-10-02 16:07 ` Hengqi Chen 0 siblings, 0 replies; 3+ messages in thread From: Hengqi Chen @ 2021-10-02 16:07 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Yonghong Song, john fastabend On 9/9/21 12:38 PM, Andrii Nakryiko wrote: > On Wed, Sep 8, 2021 at 8:35 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: >> >> BPF objects are not re-loadable after unload. User are expected to use >> bpf_object__close() to unload and free up resources in one operation. >> No need to expose bpf_object__unload() as a public API, deprecate it.[0] >> Remove bpf_object__unload() inside bpf_object__load_xattr(), it is the >> caller's responsibility to free up resources, otherwise, the following >> code path will cause double-free problem when loading failed: >> >> bpf_prog_load >> bpf_prog_load_xattr >> bpf_object__load >> bpf_object__load_xattr >> > > Did you see this double-free ever happen? I'm looking at the code and > not seeing it. Seems like bpf_object__unload() is idempotent, so no > mater how many times we call it, it doesn't do any harm. Look at how > zclose and zfree are implemented, they zero-out fields and also check > for non-zero values before doing something. So unless I'm missing > something, there is no problem. > > Sorry, I made a stupid mistake. Did not realize zclose/zfree are implemented as macros. Will remove these changes. >> Replace bpf_object__unload() inside bpf_object__close() with the necessary >> cleanup operations to avoid compilation error. >> >> [0] Closes: https://github.com/libbpf/libbpf/issues/290 >> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >> --- >> tools/lib/bpf/libbpf.c | 8 +++++--- >> tools/lib/bpf/libbpf.h | 3 ++- >> 2 files changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 8f579c6666b2..c56b466c5461 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -6931,7 +6931,6 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr) >> if (obj->maps[i].pinned && !obj->maps[i].reused) >> bpf_map__unpin(&obj->maps[i], NULL); >> >> - bpf_object__unload(obj); > > I think unloading already loaded bpf programs is bpf_object__load()'s > responsibility, so please don't remove this. > >> pr_warn("failed to load object '%s'\n", obj->path); >> return libbpf_err(err); >> } >> @@ -7540,12 +7539,15 @@ void bpf_object__close(struct bpf_object *obj) >> >> bpf_gen__free(obj->gen_loader); >> bpf_object__elf_finish(obj); >> - bpf_object__unload(obj); > > same, this is fine, don't remove it > OK. >> btf__free(obj->btf); >> btf_ext__free(obj->btf_ext); >> >> - for (i = 0; i < obj->nr_maps; i++) >> + for (i = 0; i < obj->nr_maps; i++) { >> + zclose(obj->maps[i].fd); >> + if (obj->maps[i].st_ops) >> + zfree(&obj->maps[i].st_ops->kern_vdata); >> bpf_map__destroy(&obj->maps[i]); >> + } > > and no changes should be necessary here either > Acked. >> >> zfree(&obj->btf_custom_path); >> zfree(&obj->kconfig); >> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h >> index 2f6f0e15d1e7..748f7dabe4c7 100644 >> --- a/tools/lib/bpf/libbpf.h >> +++ b/tools/lib/bpf/libbpf.h >> @@ -147,7 +147,8 @@ struct bpf_object_load_attr { >> /* Load/unload object into/from kernel */ >> LIBBPF_API int bpf_object__load(struct bpf_object *obj); >> LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr); >> -LIBBPF_API int bpf_object__unload(struct bpf_object *obj); >> +LIBBPF_API LIBBPF_DEPRECATED("bpf_object__unload() is deprecated, use bpf_object__close() instead") >> +int bpf_object__unload(struct bpf_object *obj); >> > > This is the right change, but let's also keep original > bpf_object__unload() logic. I'd recommend renaming > bpf_object__unload() into bpf_object_unload() (so that's naming is > more clearly showing it's an internal function) and make it static. > Then have a small shim of bpf_object__unload() calling into > bpf_object_unload() until we remove that in libbpf 1.0. > OK, since LIBBPF_DEPRECATED_SINCE is landed, will use it instead. >> LIBBPF_API const char *bpf_object__name(const struct bpf_object *obj); >> LIBBPF_API unsigned int bpf_object__kversion(const struct bpf_object *obj); >> -- >> 2.25.1 >> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-02 16:07 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-08 15:35 [PATCH bpf-next] libbpf: deprecate bpf_object__unload() API Hengqi Chen 2021-09-09 4:38 ` Andrii Nakryiko 2021-10-02 16:07 ` Hengqi Chen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.