Re: [PATCH bpf-next v9 1/9] bpf: refactor kfunc checks using table-driven approach in verifier

[Chengkaitao]

On Tue, Mar 31, 2026 at 1:05 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Sun, Mar 29, 2026 at 7:05 AM Chengkaitao <pilgrimtao@xxxxxxxxx> wrote:
> >
> > From: Kaitao Cheng <chengkaitao@xxxxxxxxxx>
> >
> > Replace per-kfunc btf_id chains check with btf_id_in_kfunc_table() and
> > static kfunc tables for easier maintenance.
> >
> > Prepare for future extensions to the bpf_list API family.
> >
> > Signed-off-by: Kaitao Cheng <chengkaitao@xxxxxxxxxx>
> > ---
> >  kernel/bpf/verifier.c | 261 +++++++++++++++++++++++-------------------
> >  1 file changed, 144 insertions(+), 117 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 4fbacd2149cd..f2d9863bb290 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -544,9 +544,6 @@ static bool is_async_callback_calling_kfunc(u32 btf_id);
> >  static bool is_callback_calling_kfunc(u32 btf_id);
> >  static bool is_bpf_throw_kfunc(struct bpf_insn *insn);
> >
> > -static bool is_bpf_wq_set_callback_kfunc(u32 btf_id);
> > -static bool is_task_work_add_kfunc(u32 func_id);
> > -
> >  static bool is_sync_callback_calling_function(enum bpf_func_id func_id)
> >  {
> >         return func_id == BPF_FUNC_for_each_map_elem ||
> > @@ -586,7 +583,7 @@ static bool is_async_cb_sleepable(struct bpf_verifier_env *env, struct bpf_insn
> >
> >         /* bpf_wq and bpf_task_work callbacks are always sleepable. */
> >         if (bpf_pseudo_kfunc_call(insn) && insn->off == 0 &&
> > -           (is_bpf_wq_set_callback_kfunc(insn->imm) || is_task_work_add_kfunc(insn->imm)))
> > +           is_async_callback_calling_kfunc(insn->imm))
> >                 return true;
> >
> >         verifier_bug(env, "unhandled async callback in is_async_cb_sleepable");
> > @@ -11203,31 +11200,6 @@ static int set_task_work_schedule_callback_state(struct bpf_verifier_env *env,
> >         return 0;
> >  }
> >
> > -static bool is_rbtree_lock_required_kfunc(u32 btf_id);
> > -
> > -/* Are we currently verifying the callback for a rbtree helper that must
> > - * be called with lock held? If so, no need to complain about unreleased
> > - * lock
> > - */
> > -static bool in_rbtree_lock_required_cb(struct bpf_verifier_env *env)
> > -{
> > -       struct bpf_verifier_state *state = env->cur_state;
> > -       struct bpf_insn *insn = env->prog->insnsi;
> > -       struct bpf_func_state *callee;
> > -       int kfunc_btf_id;
> > -
> > -       if (!state->curframe)
> > -               return false;
> > -
> > -       callee = state->frame[state->curframe];
> > -
> > -       if (!callee->in_callback_fn)
> > -               return false;
> > -
> > -       kfunc_btf_id = insn[callee->callsite].imm;
> > -       return is_rbtree_lock_required_kfunc(kfunc_btf_id);
> > -}
> > -
> >  static bool retval_range_within(struct bpf_retval_range range, const struct bpf_reg_state *reg)
> >  {
> >         if (range.return_32bit)
> > @@ -12639,11 +12611,103 @@ BTF_ID(func, bpf_session_is_return)
> >  BTF_ID(func, bpf_stream_vprintk)
> >  BTF_ID(func, bpf_stream_print_stack)
> >
> > -static bool is_task_work_add_kfunc(u32 func_id)
> > -{
> > -       return func_id == special_kfunc_list[KF_bpf_task_work_schedule_signal] ||
> > -              func_id == special_kfunc_list[KF_bpf_task_work_schedule_resume];
> > -}
> > +/* Kfunc family related to list. */
> > +static const enum special_kfunc_type bpf_list_api_kfuncs[] = {
> > +       KF_bpf_list_push_front_impl,
> > +       KF_bpf_list_push_back_impl,
> > +       KF_bpf_list_pop_front,
> > +       KF_bpf_list_pop_back,
> > +       KF_bpf_list_front,
> > +       KF_bpf_list_back,
> > +};
> > +
> > +/* Kfuncs that take a list node argument (bpf_list_node *). */
> > +static const enum special_kfunc_type bpf_list_node_api_kfuncs[] = {
> > +       KF_bpf_list_push_front_impl,
> > +       KF_bpf_list_push_back_impl,
> > +};
> > +
> > +/* Kfuncs that take an rbtree node argument (bpf_rb_node *). */
> > +static const enum special_kfunc_type bpf_rbtree_node_api_kfuncs[] = {
> > +       KF_bpf_rbtree_remove,
> > +       KF_bpf_rbtree_add_impl,
> > +       KF_bpf_rbtree_left,
> > +       KF_bpf_rbtree_right,
> > +};
> > +
> > +/* Kfunc family related to rbtree. */
> > +static const enum special_kfunc_type bpf_rbtree_api_kfuncs[] = {
> > +       KF_bpf_rbtree_add_impl,
> > +       KF_bpf_rbtree_remove,
> > +       KF_bpf_rbtree_first,
> > +       KF_bpf_rbtree_root,
> > +       KF_bpf_rbtree_left,
> > +       KF_bpf_rbtree_right,
> > +};
> > +
> > +/* Kfunc family related to spin_lock. */
> > +static const enum special_kfunc_type bpf_res_spin_lock_api_kfuncs[] = {
> > +       KF_bpf_res_spin_lock,
> > +       KF_bpf_res_spin_unlock,
> > +       KF_bpf_res_spin_lock_irqsave,
> > +       KF_bpf_res_spin_unlock_irqrestore,
> > +};
>
> I think it's a step in the wrong direction.
> I'd wait for Ihor's BTF_ID_NAMED cleanup.

After reading Ihor's messages on the list, if I understand correctly,
our two approaches seem to target different problems. What Ihor's
work appears to achieve is the ability to remove the entire enum
special_kfunc_type. My goal, on the other hand, is to replace many
scattered func_id == special_kfunc_list[...] comparisons with a
table-driven approach.

That said, once Ihor's solution lands, my approach would no longer
be applicable as-is. So I have been thinking about an alternative,
please see the patch linked below for details:
https://lore.kernel.org/bpf/20260403170900.58659-1-pilgrimtao@xxxxxxxxx/

> Kaitao Cheng,
>
> also please start your part of code reviews.
> Your patches are not going to be landing if you don't code review.
>
> https://lore.kernel.org/bpf/CAADnVQ+TKKptnNB25V3=bcdybh5G6c2DyW2sYtXvyRaVnPN8MA@xxxxxxxxxxxxxx/

I will do my best to carry this forward.

-- 
Yours,
Chengkaitao