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

[Hengqi Chen <hengqi.chen@gmail.com>]

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