Re: [RFC PATCH bpf-next 4/6] bpf: Add bpf runtime hooks for tracking runtime acquire/release

[Alexei Starovoitov]

On Thu, Feb 13, 2025 at 4:36 PM Juntong Deng <juntong.deng@xxxxxxxxxxx> wrote:
>
> +void *bpf_runtime_acquire_hook(void *arg1, void *arg2, void *arg3,
> +                              void *arg4, void *arg5, void *arg6 /* kfunc addr */)
> +{
> +       struct btf_struct_kfunc *struct_kfunc, dummy_key;
> +       struct btf_struct_kfunc_tab *tab;
> +       struct bpf_run_ctx *bpf_ctx;
> +       struct bpf_ref_node *node;
> +       bpf_kfunc_t kfunc;
> +       struct btf *btf;
> +       void *kfunc_ret;
> +
> +       kfunc = (bpf_kfunc_t)arg6;
> +       kfunc_ret = kfunc(arg1, arg2, arg3, arg4, arg5);
> +
> +       if (!kfunc_ret)
> +               return kfunc_ret;
> +
> +       bpf_ctx = current->bpf_ctx;
> +       btf = bpf_get_btf_vmlinux();
> +
> +       tab = btf->acquire_kfunc_tab;
> +       if (!tab)
> +               return kfunc_ret;
> +
> +       dummy_key.kfunc_addr = (unsigned long)arg6;
> +       struct_kfunc = bsearch(&dummy_key, tab->set, tab->cnt,
> +                              sizeof(struct btf_struct_kfunc),
> +                              btf_kfunc_addr_cmp_func);
> +
> +       node = list_first_entry(&bpf_ctx->free_ref_list, struct bpf_ref_node, lnode);
> +       node->obj_addr = (unsigned long)kfunc_ret;
> +       node->struct_btf_id = struct_kfunc->struct_btf_id;
> +
> +       list_del(&node->lnode);
> +       hash_add(bpf_ctx->active_ref_list, &node->hnode, node->obj_addr);
> +
> +       pr_info("bpf prog acquire obj addr = %lx, btf id = %d\n",
> +               node->obj_addr, node->struct_btf_id);
> +       print_bpf_active_refs();
> +
> +       return kfunc_ret;
> +}
> +
> +void bpf_runtime_release_hook(void *arg1, void *arg2, void *arg3,
> +                             void *arg4, void *arg5, void *arg6 /* kfunc addr */)
> +{
> +       struct bpf_run_ctx *bpf_ctx;
> +       struct bpf_ref_node *node;
> +       bpf_kfunc_t kfunc;
> +
> +       kfunc = (bpf_kfunc_t)arg6;
> +       kfunc(arg1, arg2, arg3, arg4, arg5);
> +
> +       bpf_ctx = current->bpf_ctx;
> +
> +       hash_for_each_possible(bpf_ctx->active_ref_list, node, hnode, (unsigned long)arg1) {
> +               if (node->obj_addr == (unsigned long)arg1) {
> +                       hash_del(&node->hnode);
> +                       list_add(&node->lnode, &bpf_ctx->free_ref_list);
> +
> +                       pr_info("bpf prog release obj addr = %lx, btf id = %d\n",
> +                               node->obj_addr, node->struct_btf_id);
> +               }
> +       }
> +
> +       print_bpf_active_refs();
> +}

So for every acq/rel the above two function will be called
and you call this:
"
perhaps we can use some low overhead runtime solution first as a
not too bad alternative
"

low overhead ?!

acq/rel kfuncs can be very hot.
To the level that single atomic_inc() is a noticeable overhead.
Doing above is an obvious no-go in any production setup.

> Before the bpf program actually runs, we can allocate the maximum
> possible number of reference nodes to record reference information.

This is an incorrect assumption.
Look at register_btf_id_dtor_kfuncs()
that patch 1 is sort-of trying to reinvent.
Acquired objects can be stashed with single xchg instruction and
people are not happy with performance either.
An acquire kfunc plus inlined bpf_kptr_xchg is too slow in some cases.
A bunch of bpf progs operate under constraints where nanoseconds count.
That's why we rely on static verification where possible.
Everytime we introduce run-time safety checks (like bpf_arena) we
sacrifice some use cases.
So, no, this proposal is not a solution.