Re: [PATCH bpf-next v1 3/8] bpf: Support for kfuncs with KF_MAGIC_ARGS

Eduard Zingerman
2025-10-29T23:54:37Z

On Wed, 2025-10-29 at 12:01 -0700, Ihor Solodrai wrote:
> A kernel function bpf_foo with KF_MAGIC_ARGS flag is expected to have
^^^^^^^^^^^^^
I don't like this name very much.
It bears very little context.
Imo, KF_IMPLICIT_ARGS fits the use case much better.

> two types in BTF:
> * `bpf_foo` with a function prototype that omits __magic arguments
> * `bpf_foo_impl` with a function prototype that matches kernel
> declaration, but doesn't have a ksym associated with its name

Could you please start with an example here?
Stating how `bpf_foo` needs to be declared in kernel, and what are the
options to invoke it from bpf. Then proceed with BTF details, etc.

> In order to support magic kfuncs the verifier has to know how to
> resolve calls both of `bpf_foo` and `bpf_foo_impl` to the correct BTF
> function prototype and address.
>
> In add_kfunc_call() kfunc flags are inspected to detect a magic kfunc
> or its _impl, and then the address and func_proto are adjusted for the
> kfunc descriptor.
>
> In fetch_kfunc_meta() similar logic is used to fixup the contents of
> struct bpf_kfunc_call_arg_meta.
>
> In check_kfunc_call() reset the subreg_def of registers holding magic
> arguments to correctly track zero extensions.
>
> Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
> ---
> include/linux/btf.h | 1 +
> kernel/bpf/verifier.c | 123 ++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 120 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 9c64bc5e5789..3fe20514692f 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -79,6 +79,7 @@
> #define KF_ARENA_RET (1 << 13) /* kfunc returns an arena pointer */
> #define KF_ARENA_ARG1 (1 << 14) /* kfunc takes an arena pointer as its first argument */
> #define KF_ARENA_ARG2 (1 << 15) /* kfunc takes an arena pointer as its second argument */
> +#define KF_MAGIC_ARGS (1 << 16) /* kfunc signature is different from its BPF signature */
>
> /*
> * Tag marking a kernel function as a kfunc. This is meant to minimize the
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index cb1b483be0fa..fcf0872b9e3d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3263,17 +3263,68 @@ static struct btf *find_kfunc_desc_btf(struct bpf_verifier_env *env, s16 offset)
> return btf_vmlinux ?: ERR_PTR(-ENOENT);
> }
>
> +/*
> + * magic_kfuncs is used as a list of (foo, foo_impl) pairs
> + */
> +BTF_ID_LIST(magic_kfuncs)
> +BTF_ID_UNUSED
> +BTF_ID_LIST_END(magic_kfuncs)
> +
> +static s32 magic_kfunc_by_impl(s32 impl_func_id)
> +{
> + int i;
> +
> + for (i = 1; i < BTF_ID_LIST_SIZE(magic_kfuncs); i += 2) {
> + if (magic_kfuncs[i] == impl_func_id)
^^^^^
Nit: similarly, I'd rename this to something like "implicit_func_id"
or "fake_func_id. "impl" is confusing because this id has nothing
to do with implementation.

> + return magic_kfuncs[i - 1];
> + }
> + return -ENOENT;
> +}
> +
> +static s32 impl_by_magic_kfunc(s32 func_id)
> +{
> + int i;
> +
> + for (i = 0; i < BTF_ID_LIST_SIZE(magic_kfuncs); i += 2) {
> + if (magic_kfuncs[i] == func_id)
> + return magic_kfuncs[i + 1];
> + }
> + return -ENOENT;
> +}
> +
> +static const struct btf_type *find_magic_kfunc_proto(struct btf *desc_btf, s32 func_id)
> +{
> + const struct btf_type *impl_func, *func_proto;
> + u32 impl_func_id;
> +
> + impl_func_id = impl_by_magic_kfunc(func_id);
> + if (impl_func_id < 0)
> + return NULL;
> +
> + impl_func = btf_type_by_id(desc_btf, impl_func_id);
> + if (!impl_func || !btf_type_is_func(impl_func))
> + return NULL;
> +
> + func_proto = btf_type_by_id(desc_btf, impl_func->type);
> + if (!func_proto || !btf_type_is_func_proto(func_proto))
> + return NULL;
> +
> + return func_proto;
> +}
> +
> static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
> {
> - const struct btf_type *func, *func_proto;
> + const struct btf_type *func, *func_proto, *tmp_func;
> struct bpf_kfunc_btf_tab *btf_tab;
> + const char *func_name, *tmp_name;
> struct btf_func_model func_model;
> struct bpf_kfunc_desc_tab *tab;
> struct bpf_prog_aux *prog_aux;
> struct bpf_kfunc_desc *desc;
> - const char *func_name;
> struct btf *desc_btf;
> unsigned long addr;
> + u32 *kfunc_flags;
> + s32 tmp_func_id;
> int err;
>
> prog_aux = env->prog->aux;
> @@ -3349,8 +3400,37 @@ static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
> return -EINVAL;
> }
>
> + kfunc_flags = btf_kfunc_flags(desc_btf, func_id, env->prog);
> func_name = btf_name_by_offset(desc_btf, func->name_off);
> addr = kallsyms_lookup_name(func_name);
> +
> + /* This may be an _impl kfunc with KF_MAGIC_ARGS counterpart */
> + if (unlikely(!addr && !kfunc_flags)) {
> + tmp_func_id = magic_kfunc_by_impl(func_id);

I think there is no need to hide magic_kfunc_by_impl() call behind the
above condition. It can be moved before kfunc_flags assignment.
Then it wont be necessary to textually repeat btf_name_by_offset() and
kallsyms_lookup_name() calls.

> + if (tmp_func_id < 0)
> + return -EACCES;

Nit: this skips proper error reporting: "cannot find address for kernel function %s\n".

> + tmp_func = btf_type_by_id(desc_btf, tmp_func_id);
> + if (!tmp_func || !btf_type_is_func(tmp_func))

Nit: this condition indicates a verifier bug, should it be reported as such?

> + return -EACCES;
> + tmp_name = btf_name_by_offset(desc_btf, tmp_func->name_off);
> + addr = kallsyms_lookup_name(tmp_name);
> + }
> +
> + /*
> + * Note that kfunc_flags may be NULL at this point, which means that we couldn't find
> + * func_id in any relevant kfunc_id_set. This most likely indicates an invalid kfunc call.
> + * However we don't want to fail the verification here, because invalid calls may be
> + * eliminated as dead code later.
> + */
> + if (unlikely(kfunc_flags && KF_MAGIC_ARGS & *kfunc_flags)) {
> + func_proto = find_magic_kfunc_proto(desc_btf, func_id);
> + if (!func_proto) {
> + verbose(env, "cannot find _impl proto for kernel function %s\n",
> + func_name);
> + return -EINVAL;
> + }
> + }
> +
> if (!addr) {
> verbose(env, "cannot find address for kernel function %s\n",
> func_name);
> @@ -12051,6 +12131,11 @@ static bool is_kfunc_arg_irq_flag(const struct btf *btf, const struct btf_param
> return btf_param_match_suffix(btf, arg, "__irq_flag");
> }
>
> +static bool is_kfunc_arg_magic(const struct btf *btf, const struct btf_param *arg)
> +{
> + return btf_param_match_suffix(btf, arg, "__magic");
> +}
> +
> static bool is_kfunc_arg_prog(const struct btf *btf, const struct btf_param *arg)
> {
> return btf_param_match_suffix(btf, arg, "__prog");
> @@ -13613,6 +13698,7 @@ static int fetch_kfunc_meta(struct bpf_verifier_env *env,
> u32 func_id, *kfunc_flags;
> const char *func_name;
> struct btf *desc_btf;
> + s32 tmp_func_id;
>
> if (kfunc_name)
> *kfunc_name = NULL;
> @@ -13632,10 +13718,28 @@ static int fetch_kfunc_meta(struct bpf_verifier_env *env,
> func_proto = btf_type_by_id(desc_btf, func->type);
>
> kfunc_flags = btf_kfunc_flags_if_allowed(desc_btf, func_id, env->prog);
> - if (!kfunc_flags) {
> - return -EACCES;
> + if (unlikely(!kfunc_flags)) {

What if we patch insn->imm to use the "fake" function id in add_kfunc_call()?
Then modifications to fetch_kfunc_meta() wont be necessary.

> + /*
> + * An _impl kfunc with KF_MAGIC_ARGS counterpart
> + * does not have its own kfunc flags.
> + */
> + tmp_func_id = magic_kfunc_by_impl(func_id);
> + if (tmp_func_id < 0)
> + return -EACCES;
> + kfunc_flags = btf_kfunc_flags_if_allowed(desc_btf, tmp_func_id, env->prog);
> + if (!kfunc_flags)
> + return -EACCES;
> + } else if (unlikely(KF_MAGIC_ARGS & *kfunc_flags)) {
> + /*
> + * An actual func_proto of a kfunc with KF_MAGIC_ARGS flag
> + * can be found through the corresponding _impl kfunc.
> + */
> + func_proto = find_magic_kfunc_proto(desc_btf, func_id);
> }
>
> + if (!func_proto)
> + return -EACCES;
> +
> memset(meta, 0, sizeof(*meta));
> meta->btf = desc_btf;
> meta->func_id = func_id;
> @@ -14189,6 +14293,17 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> for (i = 0; i < nargs; i++) {
> u32 regno = i + 1;
>
> + /*
> + * Magic arguments are set after main verification pass.
> + * For correct tracking of zero-extensions we have to reset subreg_def for such
> + * args. Otherwise mark_btf_func_reg_size() will be inspecting subreg_def of regs
> + * from an earlier (irrelevant) point in the program, which may lead to an error
> + * in opt_subreg_zext_lo32_rnd_hi32().
> + */
> + if (unlikely(KF_MAGIC_ARGS & meta.kfunc_flags
> + && is_kfunc_arg_magic(desc_btf, &args[i])))
> + regs[regno].subreg_def = DEF_NOT_SUBREG;
> +
> t = btf_type_skip_modifiers(desc_btf, args[i].type, NULL);
> if (btf_type_is_ptr(t))
> mark_btf_func_reg_size(env, regno, sizeof(void *));