From: Yonghong Song <yonghong.song@linux.dev>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>,
bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Kernel Team <kernel-team@fb.com>,
Martin KaFai Lau <martin.lau@kernel.org>
Subject: Re: [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
Date: Tue, 20 May 2025 13:59:18 -0700 [thread overview]
Message-ID: <c9972e17-1a8a-4178-be33-9c18b2d39d57@linux.dev> (raw)
In-Reply-To: <CAADnVQKipc=mML1ZjcMjKgKrP_L+wUPhAo0feFf6=DVqdWpCPQ@mail.gmail.com>
On 5/20/25 2:39 AM, Alexei Starovoitov wrote:
> On Tue, May 20, 2025 at 11:01 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>>
>> On 5/20/25 12:29 AM, Alexei Starovoitov wrote:
>>> On Tue, May 20, 2025 at 8:25 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>>
>>>> On 5/19/25 6:48 AM, Alexei Starovoitov wrote:
>>>>> On Mon, May 19, 2025 at 1:34 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>>>> Marc Suñé (Isovalent, part of Cisco) reported an issue where an
>>>>>> uninitialized variable caused generating bpf prog binary code not
>>>>>> working as expected. The reproducer is in [1] where the flags
>>>>>> “-Wall -Werror” are enabled, but there is no warning as the compiler
>>>>>> takes advantage of uninitialized variable to do aggressive optimization.
>>>>>> The optimized code looks like below:
>>>>>>
>>>>>> ; {
>>>>>> 0: bf 16 00 00 00 00 00 00 r6 = r1
>>>>>> ; bpf_printk("Start");
>>>>>> 1: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x0 ll
>>>>>> 0000000000000008: R_BPF_64_64 .rodata
>>>>>> 3: b4 02 00 00 06 00 00 00 w2 = 0x6
>>>>>> 4: 85 00 00 00 06 00 00 00 call 0x6
>>>>>> ; DEFINE_FUNC_CTX_POINTER(data)
>>>>>> 5: 61 61 4c 00 00 00 00 00 w1 = *(u32 *)(r6 + 0x4c)
>>>>>> ; bpf_printk("pre ipv6_hdrlen_offset");
>>>>>> 6: 18 01 00 00 06 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x6 ll
>>>>>> 0000000000000030: R_BPF_64_64 .rodata
>>>>>> 8: b4 02 00 00 17 00 00 00 w2 = 0x17
>>>>>> 9: 85 00 00 00 06 00 00 00 call 0x6
>>>>>> <END>
>>>>>>
>>>>>> The verifier will report the following failure:
>>>>>> 9: (85) call bpf_trace_printk#6
>>>>>> last insn is not an exit or jmp
>>>>>>
>>>>>> The above verifier log does not give a clear hint about how to fix
>>>>>> the problem and user may take quite some time to figure out that
>>>>>> the issue is due to compiler taking advantage of uninitialized variable.
>>>>>>
>>>>>> In llvm internals, uninitialized variable usage may generate
>>>>>> 'unreachable' IR insn and these 'unreachable' IR insns may indicate
>>>>>> uninitialized variable impact on code optimization. So far, llvm
>>>>>> BPF backend ignores 'unreachable' IR hence the above code is generated.
>>>>>> With clang21 patch [2], those 'unreachable' IR insn are converted
>>>>>> to func bpf_unreachable(). In order to maintain proper control flow
>>>>>> graph for bpf progs, [2] also adds an 'exit' insn after bpf_unreachable()
>>>>>> if bpf_unreachable() is the last insn in the function.
>>>>>> The new code looks like:
>>>>>>
>>>>>> ; {
>>>>>> 0: bf 16 00 00 00 00 00 00 r6 = r1
>>>>>> ; bpf_printk("Start");
>>>>>> 1: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x0 ll
>>>>>> 0000000000000008: R_BPF_64_64 .rodata
>>>>>> 3: b4 02 00 00 06 00 00 00 w2 = 0x6
>>>>>> 4: 85 00 00 00 06 00 00 00 call 0x6
>>>>>> ; DEFINE_FUNC_CTX_POINTER(data)
>>>>>> 5: 61 61 4c 00 00 00 00 00 w1 = *(u32 *)(r6 + 0x4c)
>>>>>> ; bpf_printk("pre ipv6_hdrlen_offset");
>>>>>> 6: 18 01 00 00 06 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x6 ll
>>>>>> 0000000000000030: R_BPF_64_64 .rodata
>>>>>> 8: b4 02 00 00 17 00 00 00 w2 = 0x17
>>>>>> 9: 85 00 00 00 06 00 00 00 call 0x6
>>>>>> 10: 85 10 00 00 ff ff ff ff call -0x1
>>>>>> 0000000000000050: R_BPF_64_32 bpf_unreachable
>>>>>> 11: 95 00 00 00 00 00 00 00 exit
>>>>>> <END>
>>>>>>
>>>>>> In kernel, a new kfunc bpf_unreachable() is added. During insn
>>>>>> verification, any hit with bpf_unreachable() will result in
>>>>>> verification failure. The kernel is able to provide better
>>>>>> log message for debugging.
>>>>>>
>>>>>> With llvm patch [2] and without this patch (no bpf_unreachable()
>>>>>> kfunc for existing kernel), e.g., for old kernels, the verifier
>>>>>> outputs
>>>>>> 10: <invalid kfunc call>
>>>>>> kfunc 'bpf_unreachable' is referenced but wasn't resolved
>>>>>> Basically, kernel does not support bpf_unreachable() kfunc.
>>>>>> This still didn't give clear signals about possible reason.
>>>>>>
>>>>>> With llvm patch [2] and with this patch, the verifier outputs
>>>>>> 10: (85) call bpf_unreachable#74479
>>>>>> unexpected bpf_unreachable() due to uninitialized variable?
>>>>>> It gives much better hints for verification failure.
>>>>>>
>>>>>> [1] https://github.com/msune/clang_bpf/blob/main/Makefile#L3
>>>>>> [2] https://github.com/llvm/llvm-project/pull/131731
>>>>>>
>>>>>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>>>>>> ---
>>>>>> kernel/bpf/helpers.c | 5 +++++
>>>>>> kernel/bpf/verifier.c | 5 +++++
>>>>>> 2 files changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>>>>>> index c1113b74e1e2..4852c36b1c51 100644
>>>>>> --- a/kernel/bpf/helpers.c
>>>>>> +++ b/kernel/bpf/helpers.c
>>>>>> @@ -3273,6 +3273,10 @@ __bpf_kfunc void bpf_local_irq_restore(unsigned long *flags__irq_flag)
>>>>>> local_irq_restore(*flags__irq_flag);
>>>>>> }
>>>>>>
>>>>>> +__bpf_kfunc void bpf_unreachable(void)
>>>>>> +{
>>>>>> +}
>>>>>> +
>>>>>> __bpf_kfunc_end_defs();
>>>>>>
>>>>>> BTF_KFUNCS_START(generic_btf_ids)
>>>>>> @@ -3386,6 +3390,7 @@ BTF_ID_FLAGS(func, bpf_copy_from_user_dynptr, KF_SLEEPABLE)
>>>>>> BTF_ID_FLAGS(func, bpf_copy_from_user_str_dynptr, KF_SLEEPABLE)
>>>>>> BTF_ID_FLAGS(func, bpf_copy_from_user_task_dynptr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
>>>>>> BTF_ID_FLAGS(func, bpf_copy_from_user_task_str_dynptr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
>>>>>> +BTF_ID_FLAGS(func, bpf_unreachable)
>>>>>> BTF_KFUNCS_END(common_btf_ids)
>>>>>>
>>>>>> static const struct btf_kfunc_id_set common_kfunc_set = {
>>>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>>>> index d5807d2efc92..08013e2e1697 100644
>>>>>> --- a/kernel/bpf/verifier.c
>>>>>> +++ b/kernel/bpf/verifier.c
>>>>>> @@ -12105,6 +12105,7 @@ enum special_kfunc_type {
>>>>>> KF_bpf_res_spin_unlock,
>>>>>> KF_bpf_res_spin_lock_irqsave,
>>>>>> KF_bpf_res_spin_unlock_irqrestore,
>>>>>> + KF_bpf_unreachable,
>>>>>> };
>>>>>>
>>>>>> BTF_SET_START(special_kfunc_set)
>>>>>> @@ -12208,6 +12209,7 @@ BTF_ID(func, bpf_res_spin_lock)
>>>>>> BTF_ID(func, bpf_res_spin_unlock)
>>>>>> BTF_ID(func, bpf_res_spin_lock_irqsave)
>>>>>> BTF_ID(func, bpf_res_spin_unlock_irqrestore)
>>>>>> +BTF_ID(func, bpf_unreachable)
>>>>>>
>>>>>> static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
>>>>>> {
>>>>>> @@ -13508,6 +13510,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>>>> return err;
>>>>>> }
>>>>>> __mark_btf_func_reg_size(env, regs, BPF_REG_0, sizeof(u32));
>>>>>> + } else if (!insn->off && insn->imm == special_kfunc_list[KF_bpf_unreachable]) {
>>>>> Looks good, but let's not abuse special_kfunc_list[] for this case.
>>>>> special_kfunc_type supposed to be in both set[] and list[].
>>>>> This is not the case here.
>>>>> It was wrong to add KF_bpf_set_dentry_xattr, bpf_iter_css_task_new,
>>>>> bpf_dynptr_from_skb, and many others.
>>>>> Let's fix this tech debt that we accumulated.
>>>>>
>>>>> special_kfunc_type should include only kfuncs that return
>>>>> a pointer, so that this part is triggered:
>>>>>
>>>>> } else if (btf_type_is_ptr(t)) {
>>>>> ptr_type = btf_type_skip_modifiers(desc_btf, t->type,
>>>>> &ptr_type_id);
>>>>>
>>>>> if (meta.btf == btf_vmlinux &&
>>>>> btf_id_set_contains(&/special_kfunc_set, meta.func_id)) {
>>>>>
>>>>> All other kfuncs shouldn't be there. They don't need to be in
>>>>> the special_kfunc_set.
>>>>>
>>>>> Let's split enum special_kfunc_type into what it meant to be
>>>>> originally (both set and list), and move all list-only kfuncs
>>>>> into a new array.
>>>>> Let's call it kfunc_ids.
>>>>> Then the check in this patch will look like:
>>>>> insn->imm == kfunc_ids[KF_bpf_unreachable]
>>>> IIUC, the main goal is to remove some kfuncs from special_kfunc_set
>>>> since they are unnecessary.
>>>>
>>>> I think we do not need an 'enum' type for special_kfunc_set since
>>>> the for all kfuncs in special_kfunc_set, btf_id_set_contains()
>>>> is used to find corresponding btf_id. So current 'enum special_kfunc_type'
>>>> is only used for special_kfunc_list to find proper kfunc_id's.
>>>>
>>>> I think the following change should achieve this:
>>>>
>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>> index 08013e2e1697..2cf00b06ae66 100644
>>>> --- a/kernel/bpf/verifier.c
>>>> +++ b/kernel/bpf/verifier.c
>>>> @@ -12060,7 +12060,7 @@ enum kfunc_ptr_arg_type {
>>>> KF_ARG_PTR_TO_RES_SPIN_LOCK,
>>>> };
>>>>
>>>> -enum special_kfunc_type {
>>>> +enum special_kfunc_list_type {
>>>> KF_bpf_obj_new_impl,
>>>> KF_bpf_obj_drop_impl,
>>>> KF_bpf_refcount_acquire_impl,
>>>> @@ -12126,24 +12126,10 @@ BTF_ID(func, bpf_rbtree_first)
>>>> BTF_ID(func, bpf_rbtree_root)
>>>> BTF_ID(func, bpf_rbtree_left)
>>>> BTF_ID(func, bpf_rbtree_right)
>>>> -#ifdef CONFIG_NET
>>>> -BTF_ID(func, bpf_dynptr_from_skb)
>>>> -BTF_ID(func, bpf_dynptr_from_xdp)
>>>> -#endif
>>>> BTF_ID(func, bpf_dynptr_slice)
>>>> BTF_ID(func, bpf_dynptr_slice_rdwr)
>>>> -BTF_ID(func, bpf_dynptr_clone)
>>>> BTF_ID(func, bpf_percpu_obj_new_impl)
>>>> BTF_ID(func, bpf_percpu_obj_drop_impl)
>>>> -BTF_ID(func, bpf_throw)
>>>> -BTF_ID(func, bpf_wq_set_callback_impl)
>>>> -#ifdef CONFIG_CGROUPS
>>>> -BTF_ID(func, bpf_iter_css_task_new)
>>>> -#endif
>>>> -#ifdef CONFIG_BPF_LSM
>>>> -BTF_ID(func, bpf_set_dentry_xattr)
>>>> -BTF_ID(func, bpf_remove_dentry_xattr)
>>>> -#endif
>>>> BTF_SET_END(special_kfunc_set)
>>>>
>>>> BTF_ID_LIST(special_kfunc_list)
>>>>
>>>> I renamed 'enum special_kfunc_type' to 'enum special_kfunc_list_type'
>>>> implying that the enum values in special_kfunc_lit_type has
>>>> 1:1 relation to special_kfunc_list.
>>>>
>>>> WDYT?
>>> I think this is not going far enough.
>>> We confused ourselves with the current special_kfunc_type.
>>> I prefer a full split where enum special_kfunc_type
>>> contains only kfuncs for special_kfunc_set and _list,
>>> and a separate enum that covers kfuncs in a new kfunc_ids[]
>> Okay, I see. we should have
>> `enum special_kfunc_type`, special_kfunc_set and special_kfunc_list
>> for kfuncs which are used in btf_id_set_contains().
>>
>> For all other kfuncs, we will have
>> `enum kfunc_ids_type` and kfunc_ids
>>
>> Something like below:
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 08013e2e1697..66d0163c7ddb 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -12062,7 +12062,6 @@ enum kfunc_ptr_arg_type {
>>
>> enum special_kfunc_type {
>> KF_bpf_obj_new_impl,
>> - KF_bpf_obj_drop_impl,
>> KF_bpf_refcount_acquire_impl,
>> KF_bpf_list_push_front_impl,
>> KF_bpf_list_push_back_impl,
>> @@ -12072,45 +12071,19 @@ enum special_kfunc_type {
>> KF_bpf_list_back,
>> KF_bpf_cast_to_kern_ctx,
>> KF_bpf_rdonly_cast,
>> - KF_bpf_rcu_read_lock,
>> - KF_bpf_rcu_read_unlock,
>> KF_bpf_rbtree_remove,
>> KF_bpf_rbtree_add_impl,
>> KF_bpf_rbtree_first,
>> KF_bpf_rbtree_root,
>> KF_bpf_rbtree_left,
>> KF_bpf_rbtree_right,
>> - KF_bpf_dynptr_from_skb,
>> - KF_bpf_dynptr_from_xdp,
>> KF_bpf_dynptr_slice,
>> KF_bpf_dynptr_slice_rdwr,
>> - KF_bpf_dynptr_clone,
>> KF_bpf_percpu_obj_new_impl,
>> - KF_bpf_percpu_obj_drop_impl,
>> - KF_bpf_throw,
>> - KF_bpf_wq_set_callback_impl,
>> - KF_bpf_preempt_disable,
>> - KF_bpf_preempt_enable,
>> - KF_bpf_iter_css_task_new,
>> - KF_bpf_session_cookie,
>> - KF_bpf_get_kmem_cache,
>> - KF_bpf_local_irq_save,
>> - KF_bpf_local_irq_restore,
>> - KF_bpf_iter_num_new,
>> - KF_bpf_iter_num_next,
>> - KF_bpf_iter_num_destroy,
>> - KF_bpf_set_dentry_xattr,
>> - KF_bpf_remove_dentry_xattr,
>> - KF_bpf_res_spin_lock,
>> - KF_bpf_res_spin_unlock,
>> - KF_bpf_res_spin_lock_irqsave,
>> - KF_bpf_res_spin_unlock_irqrestore,
>> - KF_bpf_unreachable,
>> };
>>
>> BTF_SET_START(special_kfunc_set)
>> BTF_ID(func, bpf_obj_new_impl)
>> -BTF_ID(func, bpf_obj_drop_impl)
>> BTF_ID(func, bpf_refcount_acquire_impl)
>> BTF_ID(func, bpf_list_push_front_impl)
>> BTF_ID(func, bpf_list_push_back_impl)
>> @@ -12126,29 +12099,13 @@ BTF_ID(func, bpf_rbtree_first)
>> BTF_ID(func, bpf_rbtree_root)
>> BTF_ID(func, bpf_rbtree_left)
>> BTF_ID(func, bpf_rbtree_right)
>> -#ifdef CONFIG_NET
>> -BTF_ID(func, bpf_dynptr_from_skb)
>> -BTF_ID(func, bpf_dynptr_from_xdp)
>> -#endif
>> BTF_ID(func, bpf_dynptr_slice)
>> BTF_ID(func, bpf_dynptr_slice_rdwr)
>> -BTF_ID(func, bpf_dynptr_clone)
>> BTF_ID(func, bpf_percpu_obj_new_impl)
>> -BTF_ID(func, bpf_percpu_obj_drop_impl)
>> -BTF_ID(func, bpf_throw)
>> -BTF_ID(func, bpf_wq_set_callback_impl)
>> -#ifdef CONFIG_CGROUPS
>> -BTF_ID(func, bpf_iter_css_task_new)
>> -#endif
>> -#ifdef CONFIG_BPF_LSM
>> -BTF_ID(func, bpf_set_dentry_xattr)
>> -BTF_ID(func, bpf_remove_dentry_xattr)
>> -#endif
>> BTF_SET_END(special_kfunc_set)
>>
>> BTF_ID_LIST(special_kfunc_list)
>> BTF_ID(func, bpf_obj_new_impl)
>> -BTF_ID(func, bpf_obj_drop_impl)
>> BTF_ID(func, bpf_refcount_acquire_impl)
>> BTF_ID(func, bpf_list_push_front_impl)
>> BTF_ID(func, bpf_list_push_back_impl)
>> @@ -12158,14 +12115,49 @@ BTF_ID(func, bpf_list_front)
>> BTF_ID(func, bpf_list_back)
>> BTF_ID(func, bpf_cast_to_kern_ctx)
>> BTF_ID(func, bpf_rdonly_cast)
>> -BTF_ID(func, bpf_rcu_read_lock)
>> -BTF_ID(func, bpf_rcu_read_unlock)
>> BTF_ID(func, bpf_rbtree_remove)
>> BTF_ID(func, bpf_rbtree_add_impl)
>> BTF_ID(func, bpf_rbtree_first)
>> BTF_ID(func, bpf_rbtree_root)
>> BTF_ID(func, bpf_rbtree_left)
>> BTF_ID(func, bpf_rbtree_right)
>> +BTF_ID(func, bpf_dynptr_slice)
>> +BTF_ID(func, bpf_dynptr_slice_rdwr)
>> +BTF_ID(func, bpf_percpu_obj_new_impl)
>> +
> Yes. Something like that.
>
>
>> +enum kfunc_ids_type {
>> + KF_bpf_obj_drop_impl,
> The concern I had that now KF_bpf_obj_drop_impl == 0
> and KF_bpf_obj_new_impl == 0.
>
> So another idea...
>
> maybe we should remove special_kfunc_set instead?
>
> I recall we argued with Kumar years ago about it.
> I don't remember why we kept it
> and what purpose it serves now.
>
> What will break if we do:
>
> - if (meta.btf == btf_vmlinux &&
> btf_id_set_contains(&special_kfunc_set, meta.func_id)) {
> + if (meta.btf == btf_vmlinux) {
> if (meta.func_id ==
> special_kfunc_list[KF_bpf_obj_new_impl] ||
> meta.func_id ==
> special_kfunc_list[KF_bpf_percpu_obj_new_impl]) {
> struct btf_struct_meta *struct_meta;
> @@ -13838,10 +13838,6 @@ static int check_kfunc_call(struct
> bpf_verifier_env *env, struct bpf_insn *insn,
> * because packet slices are not refcounted (see
> * dynptr_type_refcounted)
> */
> - } else {
> - verbose(env, "kernel function %s
> unhandled dynamic return type\n",
> - meta.func_name);
> - return -EFAULT;
> }
>
> ?
The above change won't work. In original code, if the branch
if (meta.btf == btf_vmlinux && btf_id_set_contains(&special_kfunc_set, meta.func_id))
is false, the control will go the next one like
} else if (btf_type_is_void(ptr_type)) {
With the above code, if meta.btf == btf_vmlinux, but
btf_id_set_contains(&special_kfunc_set, meta.func_id) is false, the control
will miss
} else if (btf_type_is_void(ptr_type))
checking and verification will not work any more.
There are two possible ways to fix the problem:
Option 1 to change btf_id_set_contains to another function to check meta.func_id like
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d5807d2efc92..5129da4f604f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12107,44 +12107,6 @@ enum special_kfunc_type {
KF_bpf_res_spin_unlock_irqrestore,
};
-BTF_SET_START(special_kfunc_set)
-BTF_ID(func, bpf_obj_new_impl)
-BTF_ID(func, bpf_obj_drop_impl)
...
-BTF_ID(func, bpf_set_dentry_xattr)
-BTF_ID(func, bpf_remove_dentry_xattr)
-#endif
-BTF_SET_END(special_kfunc_set)
-
BTF_ID_LIST(special_kfunc_list)
BTF_ID(func, bpf_obj_new_impl)
BTF_ID(func, bpf_obj_drop_impl)
@@ -13452,6 +13414,22 @@ static int fetch_kfunc_meta(struct bpf_verifier_env *env,
return 0;
}
+static bool kfunc_id_allowed(int func_id, const struct btf_type *ptr_type)
+{
+ if (func_id == special_kfunc_list[KF_bpf_obj_new_impl] ||
+ func_id == special_kfunc_list[KF_bpf_percpu_obj_new_impl] ||
+ func_id == special_kfunc_list[KF_bpf_refcount_acquire_impl] ||
+ is_list_node_type(ptr_type) ||
+ is_rbtree_node_type(ptr_type) ||
+ func_id == special_kfunc_list[KF_bpf_cast_to_kern_ctx] ||
+ func_id == special_kfunc_list[KF_bpf_rdonly_cast] ||
+ func_id == special_kfunc_list[KF_bpf_dynptr_slice] ||
+ func_id == special_kfunc_list[KF_bpf_dynptr_slice_rdwr])
+ return true;
+
+ return false;
+}
+
static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
@@ -13687,7 +13665,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
} else if (btf_type_is_ptr(t)) {
ptr_type = btf_type_skip_modifiers(desc_btf, t->type, &ptr_type_id);
- if (meta.btf == btf_vmlinux && btf_id_set_contains(&special_kfunc_set, meta.func_id)) {
+ if (meta.btf == btf_vmlinux && kfunc_id_allowed(meta.func_id, ptr_type)) {
if (meta.func_id == special_kfunc_list[KF_bpf_obj_new_impl] ||
...
Option 2 is to add meta.btf == btf_vmlinux to each checking under branch with 'if(btf_type_is_ptr(t)' like
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d5807d2efc92..2b5fbfb961d0 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12107,44 +12107,6 @@ enum special_kfunc_type {
KF_bpf_res_spin_unlock_irqrestore,
};
-BTF_SET_START(special_kfunc_set)
-BTF_ID(func, bpf_obj_new_impl)
-BTF_ID(func, bpf_obj_drop_impl)
...
-BTF_ID(func, bpf_set_dentry_xattr)
-BTF_ID(func, bpf_remove_dentry_xattr)
-#endif
-BTF_SET_END(special_kfunc_set)
-
BTF_ID_LIST(special_kfunc_list)
BTF_ID(func, bpf_obj_new_impl)
BTF_ID(func, bpf_obj_drop_impl)
@@ -13687,162 +13649,161 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
} else if (btf_type_is_ptr(t)) {
ptr_type = btf_type_skip_modifiers(desc_btf, t->type, &ptr_type_id);
- if (meta.btf == btf_vmlinux && btf_id_set_contains(&special_kfunc_set, meta.func_id)) {
- if (meta.func_id == special_kfunc_list[KF_bpf_obj_new_impl] ||
- meta.func_id == special_kfunc_list[KF_bpf_percpu_obj_new_impl]) {
- struct btf_struct_meta *struct_meta;
- struct btf *ret_btf;
- u32 ret_btf_id;
+ if (meta.btf == btf_vmlinux &&
+ (meta.func_id == special_kfunc_list[KF_bpf_obj_new_impl] ||
+ meta.func_id == special_kfunc_list[KF_bpf_percpu_obj_new_impl])) {
+ struct btf_struct_meta *struct_meta;
+ struct btf *ret_btf;
+ u32 ret_btf_id;
...
+ } else if (meta.btf == btf_vmlinux &&
+ meta.func_id == special_kfunc_list[KF_bpf_refcount_acquire_impl]) {
+ mark_reg_known_zero(env, regs, BPF_REG_0);
+ regs[BPF_REG_0].type = PTR_TO_BTF_ID | MEM_ALLOC;
+ regs[BPF_REG_0].btf = meta.arg_btf;
+ regs[BPF_REG_0].btf_id = meta.arg_btf_id;
+
+ insn_aux->kptr_struct_meta =
+ btf_find_struct_meta(meta.arg_btf,
+ meta.arg_btf_id);
+ } else if (meta.btf == btf_vmlinux && is_list_node_type(ptr_type)) {
+ struct btf_field *field = meta.arg_list_head.field;
+
+ mark_reg_graph_node(regs, BPF_REG_0, &field->graph_root);
...
+ } else if (meta.btf == btf_vmlinux &&
+ (meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice] ||
+ meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice_rdwr])) {
+ enum bpf_type_flag type_flag = get_dynptr_type_flag(meta.initialized_dynptr.type);
...
+ */
} else if (btf_type_is_void(ptr_type)) {
...
The option 2 probably better as we can avoid redundant check like kfunc_id_allowed().
Also we can have
bool is_meta_btf_vmlinux = meta.btf == btf_vmlinux;
and use is_meta_btf_vmlinux instead of meta.btf == btf_vmlinux
next prev parent reply other threads:[~2025-05-20 20:59 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-19 20:33 [PATCH bpf-next v3 0/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable Yonghong Song
2025-05-19 20:33 ` [PATCH bpf-next v3 1/2] " Yonghong Song
2025-05-19 22:48 ` Alexei Starovoitov
2025-05-20 15:25 ` Yonghong Song
2025-05-20 16:29 ` Alexei Starovoitov
2025-05-20 18:01 ` Yonghong Song
2025-05-20 18:39 ` Alexei Starovoitov
2025-05-20 18:46 ` Alexei Starovoitov
2025-05-20 19:50 ` Yonghong Song
2025-05-20 19:40 ` Yonghong Song
2025-05-20 20:59 ` Yonghong Song [this message]
2025-05-20 21:14 ` Alexei Starovoitov
2025-05-19 20:33 ` [PATCH bpf-next v3 2/2] selftests/bpf: Add unit tests with bpf_unreachable() kfunc Yonghong Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c9972e17-1a8a-4178-be33-9c18b2d39d57@linux.dev \
--to=yonghong.song@linux.dev \
--cc=alexei.starovoitov@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=martin.lau@kernel.org \
--cc=memxor@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox