Messages in this thread |  | | From | Chengkaitao <> | | Date | Sat, 4 Apr 2026 18:38:21 +0800 | | Subject | Re: [PATCH bpf-next v9 1/9] bpf: refactor kfunc checks using table-driven approach in verifier |
| |
On Sat, Apr 4, 2026 at 12:49 PM Ihor Solodrai <ihor.solodrai@linux.dev> wrote: > > > > On 4/3/26 10:41 AM, Chengkaitao wrote: > > On Tue, Mar 31, 2026 at 1:05 AM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > >> > >> On Sun, Mar 29, 2026 at 7:05 AM Chengkaitao <pilgrimtao@gmail.com> wrote: > >>> > >>> From: Kaitao Cheng <chengkaitao@kylinos.cn> > >>> > >>> [...] > >>> + > >>> +/* 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. > > Hi Kaitao, > > I appreciate your efforts, however after a quick pass over the changes > you propose (both here and in the new series) with respect to BTF_ID > macros and special_kfuncs_list, I don't understand what problem you're > trying to solve. > > The inherent complexity is in the fact that the verifier must know > when a particular BTF id identifies a specific kfunc, or whether it > belongs to some pre-defined set of ids. This is why > special_kfuncs_list and other BTF_ID_SET/LIST-s exist. > > And so there is no way around defining those ids and sets *somewhere*, > and so far BTF_ID_* macros did a fine job of that, all things > considered. > > AFAICT your changes simply move around the same definitions from > functions with if statements to constant arrays with a runtime search > on them (which is slower by the way). What is the benefit of that vs > the current implementation? We still have to maintain those arrays in > the same way we have to maintain the is_foo_kfunc helpers. > > Your newer proposal [1] takes the same idea to the next level, by > introducing an entire new BTF kind, new ELF sections and a bunch of > macros that are no less complicated than existing. And all of that > just moves the same arrays "upstream" to the .BTF_ids section. Again, > I fail to see any benefits to that complexity. Having differentiation > between LIST and SET, and having to mark START and END is not a > problem that needs solving IMO.
Your analysis of the code implementation for the new proposal is correct. Let me elaborate on the purpose behind my approach.
****** Purpose 1 ******
As described in this patch: https://lore.kernel.org/bpf/20260303135219.33726-4-pilgrimtao@gmail.com/
If we want to add a new kfunc, bpf_list_add_impl, we would today have to add "btf_id == special_kfunc_list[KF_bpf_list_back]" (or similar) five times in verifier.c. Under the newer proposal, that is no longer necessary: defining the kfunc and its verifier metadata in one place is enough, for example:
__bpf_kfunc int bpf_list_add_impl(struct bpf_list_head *head, struct bpf_list_node *new, struct bpf_list_node *prev, void *meta__ign, u64 off) { /* kfunc implementation */ ....... } BPF_VERIF_KFUNC_DEF(bpf_list_add_impl, list_api, graph_node_api, ... )
If BPF_VERIF_KFUNC_DEF is extended further, BTF_ID(func, bpf_list_add_impl) and BTF_ID_FLAGS(func, bpf_list_add_impl) might also become unnecessary, so the snippet above could eventually be close to all the code required to add a new kfunc.
****** Purpose 2 ******
The kernel no longer needs enum special_kfunc_type to list every KF_bpf_* entry. That information is folded into the .BTF_ids.##sfx section instead, so kfunc authors do not have to touch or think about special_kfunc_type.
****** Purpose 3 ******
As described in this patch: https://lore.kernel.org/bpf/20260303135219.33726-6-pilgrimtao@gmail.com/
In is_bpf_list_api_kfunc(u32 btf_id) there are on the order of eleven "btf_id == special_kfunc_list[*]" comparisons. As more kfuncs are added, every is_bpf_* helper will only grow longer and the verifier will get more repetitive. With the new design, those is_bpf_* helpers can be removed entirely, including the awkward scattered "btf_id == *" checks.
****** Purpose 4 ******
It pushes us to untangle messy verifier safety cases and make them modular, so they can be expressed as parameters to BPF_VERIF_KFUNC_DEF
> The work I was discussing with Alexei [2] is targeted: get rid of > constant enums that mirror existing kfunc symbols by making BTF_ID > internals a bit smarter. And the benefit is clear and simple. > > If you could explain what exact issue you're trying to address with > your BTF_ID refactoring patches, maybe we can try converging on a > reasonable approach to it. > > Thanks! > > [1] https://lore.kernel.org/bpf/20260403170900.58659-1-pilgrimtao@gmail.com/ > [2] https://lore.kernel.org/bpf/21e5333c-0b57-46ce-99c8-f6c414270e70@linux.dev/ > > > > > > 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@gmail.com/ > > > >> 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@mail.gmail.com/ > > > > I will do my best to carry this forward.
-- Yours, Chengkaitao
|  |