[PATCH bpf-next] libbpf: deprecate bpf_object__unload() API

[PATCH bpf-next] libbpf: deprecate bpf_object__unload() API

[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

Re: [PATCH bpf-next] libbpf: deprecate bpf_object__unload() API

[Andrii Nakryiko]

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
>

Re: [PATCH bpf-next] libbpf: deprecate bpf_object__unload() API

[Hengqi Chen]

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
>>