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

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Hengqi Chen <hengqi.chen@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>, Yonghong Song <yhs@fb.com>,
	john fastabend <john.fastabend@gmail.com>
Subject: Re: [PATCH bpf-next] libbpf: deprecate bpf_object__unload() API
Date: Wed, 8 Sep 2021 21:38:45 -0700	[thread overview]
Message-ID: <CAEf4BzYhYcyVOJ84REys1nyF8eMaDa0JgAinjgwU_EMvMqOo-g@mail.gmail.com> (raw)
In-Reply-To: <20210908153544.749101-1-hengqi.chen@gmail.com>

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
>