BPF List
 help / color / mirror / Atom feed
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


  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