Re: [PATCH v2 bpf-next 7/8] bpf: Allow pro/epilogue to call kfunc

[Martin KaFai Lau <martin.lau@linux.dev>]

On 8/21/24 6:32 PM, Alexei Starovoitov wrote:
> On Wed, Aug 21, 2024 at 4:35 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> The existing prologue has been able to call bpf helper but not a kfunc.
>> This patch allows the prologue/epilogue to call the kfunc.
>>
>> The subsystem that implements the .gen_prologue and .gen_epilogue
>> can add the BPF_PSEUDO_KFUNC_CALL instruction with insn->imm
>> set to the btf func_id of the kfunc call. This part is the same
>> as the bpf prog loaded from the sys_bpf.
> 
> I don't understand the value of this feature, since it seems
> pretty hard to use.
> The module (qdisc-bpf or else) would need to do something
> like patch 8/8:
> +BTF_ID_LIST(st_ops_epilogue_kfunc_list)
> +BTF_ID(func, bpf_kfunc_st_ops_inc10)
> +BTF_ID(func, bpf_kfunc_st_ops_inc100)
> 
> just to be able to:
>    BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0,
>                 st_ops_epilogue_kfunc_list[0]);
> 
> So a bunch of extra work on the module side and
> a bunch of work in this patch to enable such a pattern,
> but what is the value?
> 
> gen_epilogue() can call arbitrary kernel function.
> It doesn't have to be a helper.
> kfunc-s provide calling convention conversion from bpf to native,
> but the same thing is achieved by BPF_CALL_N macro.
> The module can use that macro without adding an actual bpf helper
> to uapi bpf.h.
> Then in gen_epilogue() the extra bpf insn can use:
> BPF_EMIT_CALL(module_provided_helper_that_is_not_helper)
> which will use
> BPF_CALL_IMM(x) ((void *)(x) - (void *)__bpf_call_base)

BPF_EMIT_CALL() was my earlier thought. I switched to the kfunc in this patch 
because of the bpf_jit_supports_far_kfunc_call() support for the kernel module. 
Using kfunc call will make supporting it the same.

I think the future bpf-qdisc can enforce built-in. bpf-tcp-cc has already been 
built-in only also. I think the hid_bpf is built-in only also.

Another consideration is also holding the module refcnt when having an 
attachable bpf prog calling a kernel func implemented in a kernel module. iiuc, 
this is the reason why aux->kfunc_btf_tab holds a reference to the kernel 
module. This should not be a problem to struct_ops though because the struct_ops 
map is the one that is attachable instead of the struct_ops prog. The struct_ops 
map has already held a refcnt of the module.

> to populate imm.
> And JITs will emit jump to that wrapper code provided by
> BPF_CALL_N.
> 
> And no need for this extra complexity in the verifier and
> its consumers that have to figure out (module_fd, btf_id) for
> kfunc just to fit into kfunc pattern with btf_distill_func_proto().
> 
> I guess one can argue that if such kfunc is already available
> to bpf prog then extra BPF_CALL_N wrapper for the same thing
> is a waste of kernel text, but this patch also adds quite a bit of
> kernel text. So the cost of BPF_CALL_N (which is a zero on x86)
> is acceptable.