On Fri, Aug 11, 2023 at 01:13:45PM -0700, Dave Marchevsky wrote:
> The function signature of kfuncs can change at any time due to their
> intentional lack of stability guarantees. As kfuncs become more widely
> used, BPF program writers will need facilities to support calling
> different versions of a kfunc from a single BPF object. Consider this
> simplified example based on a real scenario we ran into at Meta:
>
> /* initial kfunc signature */
> int some_kfunc(void *ptr)
>
> /* Oops, we need to add some flag to modify behavior. No problem,
> change the kfunc. flags = 0 retains original behavior */
> int some_kfunc(void *ptr, long flags)
>
> If the initial version of the kfunc is deployed on some portion of the
> fleet and the new version on the rest, a fleetwide service that uses
> some_kfunc will currently need to load different BPF programs depending
> on which some_kfunc is available.
>
> Luckily CO-RE provides a facility to solve a very similar problem,
> struct definition changes, by allowing program writers to declare
> my_struct___old and my_struct___new, with ___suffix being considered a
> 'flavor' of the non-suffixed name and being ignored by
> bpf_core_type_exists and similar calls.
>
> This patch extends the 'flavor' facility to the kfunc extern
> relocation process. BPF program writers can now declare
>
> extern int some_kfunc___old(void *ptr)
> extern int some_kfunc___new(void *ptr, int flags)
>
> then test which version of the kfunc exists with bpf_ksym_exists.
> Relocation and verifier's dead code elimination will work in concert as
> expected, allowing this pattern:
>
> if (bpf_ksym_exists(some_kfunc___old))
> some_kfunc___old(ptr);
> else
> some_kfunc___new(ptr, 0);
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> ---
> tools/lib/bpf/libbpf.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 17883f5a44b9..8949d489a35f 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -550,6 +550,7 @@ struct extern_desc {
> int btf_id;
> int sec_btf_id;
> const char *name;
> + char *essent_name;
> bool is_set;
> bool is_weak;
> union {
> @@ -3770,6 +3771,7 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
> struct extern_desc *ext;
> int i, n, off, dummy_var_btf_id;
> const char *ext_name, *sec_name;
> + size_t ext_essent_len;
> Elf_Scn *scn;
> Elf64_Shdr *sh;
>
> @@ -3819,6 +3821,14 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
> ext->sym_idx = i;
> ext->is_weak = ELF64_ST_BIND(sym->st_info) == STB_WEAK;
>
> + ext_essent_len = bpf_core_essential_name_len(ext->name);
> + ext->essent_name = NULL;
> + if (ext_essent_len != strlen(ext->name)) {
> + ext->essent_name = malloc(ext_essent_len + 1);
> + memcpy(ext->essent_name, ext->name, ext_essent_len);
> + ext->essent_name[ext_essent_len] = '\0';
could we use strndup in here?
> + }
> +
> ext->sec_btf_id = find_extern_sec_btf_id(obj->btf, ext->btf_id);
> if (ext->sec_btf_id <= 0) {
> pr_warn("failed to find BTF for extern '%s' [%d] section: %d\n",
> @@ -7624,7 +7634,8 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
>
> local_func_proto_id = ext->ksym.type_id;
>
> - kfunc_id = find_ksym_btf_id(obj, ext->name, BTF_KIND_FUNC, &kern_btf, &mod_btf);
> + kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
> + &mod_btf);
> if (kfunc_id < 0) {
> if (kfunc_id == -ESRCH && ext->is_weak)
> return 0;
> @@ -7642,6 +7653,9 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
> pr_warn("extern (func ksym) '%s': func_proto [%d] incompatible with %s [%d]\n",
> ext->name, local_func_proto_id,
> mod_btf ? mod_btf->name : "vmlinux", kfunc_proto_id);
> +
> + if (ext->is_weak)
> + return 0;
> return -EINVAL;
> }
>
> @@ -8370,6 +8384,11 @@ void bpf_object__close(struct bpf_object *obj)
>
> zfree(&obj->btf_custom_path);
> zfree(&obj->kconfig);
> +
> + for (i = 0; i < obj->nr_extern; i++)
> + if (obj->externs[i].essent_name)
> + zfree(&obj->externs[i].essent_name);
no need to check the pointer, free will take care of that
jirka
> +
> zfree(&obj->externs);
> obj->nr_extern = 0;
>
> --
> 2.34.1
>
>
