* [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.