* [PATCH bpf-next v3 0/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
@ 2025-05-19 20:33 Yonghong Song
2025-05-19 20:33 ` [PATCH bpf-next v3 1/2] " Yonghong Song
2025-05-19 20:33 ` [PATCH bpf-next v3 2/2] selftests/bpf: Add unit tests with bpf_unreachable() kfunc Yonghong Song
0 siblings, 2 replies; 13+ messages in thread
From: Yonghong Song @ 2025-05-19 20:33 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
Martin KaFai Lau
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.
Such optimization results in a verification log:
last insn is not an exit or jmp
User still needs to take quite some time to figure out what is
the root cause.
To give a better hint to user, bpf_unreachable() kfunc is introduced
in kernel and the compiler ([2]) will encode bpf_unreachable()
as needed. For example, compiler may generate 'unreachable' IR
after do optimizaiton by taking advantage of uninitialized variable,
and later bpf backend will translate such 'unreachable' IR to
bpf_unreachable() func in final binary. When kernel detects
bpf_unreachable(), it is able to issue much better verifier log, e.g.
unexpected bpf_unreachable() due to uninitialized variable?
[1] https://github.com/msune/clang_bpf/blob/main/Makefile#L3
[2] https://github.com/llvm/llvm-project/pull/131731
Changelogs:
v2 -> v3:
- v2: https://lore.kernel.org/bpf/CAADnVQL9A8vB-yRjnZn8bgMrfDSO17FFBtS_xOs5w-LSq+p74g@mail.gmail.com/
- The newer llvm patch (above [2]) added 'exit' insn if the last insn
in the function is bpf_unreachable(). This way, check_subprogs()
handling is unnecessary and removed.
- Remove the big C test (above [1]) and add a simple C test and three
inline asm tests.
v1 -> v2:
- v1: https://lore.kernel.org/bpf/20250511182744.1806792-1-yonghong.song@linux.dev/
- If bpf_unreachable() is hit during check_kfunc_call(), report the
verification failure.
- Add three inline asm test cases.
Yonghong Song (2):
bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized
variable
selftests/bpf: Add unit tests with bpf_unreachable() kfunc
kernel/bpf/helpers.c | 5 ++
kernel/bpf/verifier.c | 5 ++
.../selftests/bpf/prog_tests/verifier.c | 2 +
.../bpf/progs/verifier_bpf_unreachable.c | 61 +++++++++++++++++++
4 files changed, 73 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/verifier_bpf_unreachable.c
--
2.47.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
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 ` Yonghong Song
2025-05-19 22:48 ` Alexei Starovoitov
2025-05-19 20:33 ` [PATCH bpf-next v3 2/2] selftests/bpf: Add unit tests with bpf_unreachable() kfunc Yonghong Song
1 sibling, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2025-05-19 20:33 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
Martin KaFai Lau
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]) {
+ verbose(env, "unexpected bpf_unreachable() due to uninitialized variable?\n");
+ return -EFAULT;
}
if (is_kfunc_destructive(&meta) && !capable(CAP_SYS_BOOT)) {
--
2.47.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH bpf-next v3 2/2] selftests/bpf: Add unit tests with bpf_unreachable() kfunc
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 20:33 ` Yonghong Song
1 sibling, 0 replies; 13+ messages in thread
From: Yonghong Song @ 2025-05-19 20:33 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
Martin KaFai Lau
The compiler support for bpf_unreachable() ([1]) will be in llvm21,
but the current kernel will have a build failure with llvm21 ([2]).
So all unit tests have explicit references to bpf_unreachable().
The test where bpf_unreachable() is generated by compiler will be
provided later.
[1] https://github.com/llvm/llvm-project/pull/131731
[2] https://patchew.org/linux/20250506-default-const-init-clang-v2-1-fcfb69703264@kernel.org/
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
.../selftests/bpf/prog_tests/verifier.c | 2 +
.../bpf/progs/verifier_bpf_unreachable.c | 61 +++++++++++++++++++
2 files changed, 63 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/verifier_bpf_unreachable.c
diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index e66a57970d28..790fff264ec6 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -14,6 +14,7 @@
#include "verifier_bounds_deduction_non_const.skel.h"
#include "verifier_bounds_mix_sign_unsign.skel.h"
#include "verifier_bpf_get_stack.skel.h"
+#include "verifier_bpf_unreachable.skel.h"
#include "verifier_bswap.skel.h"
#include "verifier_btf_ctx_access.skel.h"
#include "verifier_btf_unreliable_prog.skel.h"
@@ -148,6 +149,7 @@ void test_verifier_bounds_deduction(void) { RUN(verifier_bounds_deduction);
void test_verifier_bounds_deduction_non_const(void) { RUN(verifier_bounds_deduction_non_const); }
void test_verifier_bounds_mix_sign_unsign(void) { RUN(verifier_bounds_mix_sign_unsign); }
void test_verifier_bpf_get_stack(void) { RUN(verifier_bpf_get_stack); }
+void test_verifier_bpf_unreachable(void) { RUN(verifier_bpf_unreachable); }
void test_verifier_bswap(void) { RUN(verifier_bswap); }
void test_verifier_btf_ctx_access(void) { RUN(verifier_btf_ctx_access); }
void test_verifier_btf_unreliable_prog(void) { RUN(verifier_btf_unreliable_prog); }
diff --git a/tools/testing/selftests/bpf/progs/verifier_bpf_unreachable.c b/tools/testing/selftests/bpf/progs/verifier_bpf_unreachable.c
new file mode 100644
index 000000000000..c8b14b6022a5
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_bpf_unreachable.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
+
+SEC("socket")
+__description("bpf_unreachable with simple c code")
+__failure __msg("unexpected bpf_unreachable() due to uninitialized variable?")
+void bpf_unreachable_with_simple_c(void)
+{
+ bpf_unreachable();
+}
+
+SEC("socket")
+__description("bpf_unreachable as the second-from-last insn")
+__failure __msg("unexpected bpf_unreachable() due to uninitialized variable?")
+__naked void bpf_unreachable_at_func_end(void)
+{
+ asm volatile (
+ "r0 = 0;"
+ "call %[bpf_unreachable];"
+ "exit;"
+ :
+ : __imm(bpf_unreachable)
+ : __clobber_all);
+}
+
+SEC("socket")
+__description("dead code bpf_unreachable() in the middle of code")
+__success
+__naked void dead_bpf_unreachable_in_middle(void)
+{
+ asm volatile (
+ "r0 = 0;"
+ "if r0 == 0 goto +1;"
+ "call %[bpf_unreachable];"
+ "r0 = 2;"
+ "exit;"
+ :
+ : __imm(bpf_unreachable)
+ : __clobber_all);
+}
+
+SEC("socket")
+__description("reachable bpf_unreachable() in the middle of code")
+__failure __msg("unexpected bpf_unreachable() due to uninitialized variable?")
+__naked void live_bpf_unreachable_in_middle(void)
+{
+ asm volatile (
+ "r0 = 0;"
+ "if r0 == 1 goto +1;"
+ "call %[bpf_unreachable];"
+ "r0 = 2;"
+ "exit;"
+ :
+ : __imm(bpf_unreachable)
+ : __clobber_all);
+}
+
+char _license[] SEC("license") = "GPL";
--
2.47.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
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
0 siblings, 1 reply; 13+ messages in thread
From: Alexei Starovoitov @ 2025-05-19 22:48 UTC (permalink / raw)
To: Yonghong Song, Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team, Martin KaFai Lau
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]
Digging through the code it looks like we made a bit of a mess there.
Like this part:
} else if (btf_type_is_void(t)) {
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_drop_impl] ||
meta.func_id ==
special_kfunc_list[KF_bpf_percpu_obj_drop_impl]) {
*obj_drop don't need to be in a set,
and btf_id_set_contains() doesn't need to be called.
Both kfuncs should be moved to new kfunc_ids[]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
2025-05-19 22:48 ` Alexei Starovoitov
@ 2025-05-20 15:25 ` Yonghong Song
2025-05-20 16:29 ` Alexei Starovoitov
0 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2025-05-20 15:25 UTC (permalink / raw)
To: Alexei Starovoitov, Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team, Martin KaFai Lau
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?
>
> Digging through the code it looks like we made a bit of a mess there.
> Like this part:
> } else if (btf_type_is_void(t)) {
> 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_drop_impl] ||
> meta.func_id ==
> special_kfunc_list[KF_bpf_percpu_obj_drop_impl]) {
>
>
> *obj_drop don't need to be in a set,
> and btf_id_set_contains() doesn't need to be called.
> Both kfuncs should be moved to new kfunc_ids[]
As you mentioned, for this one, we can do
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2cf00b06ae66..a3ff57eaa5f4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12110,7 +12110,6 @@ enum special_kfunc_list_type {
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)
@@ -12129,7 +12128,6 @@ 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)
-BTF_ID(func, bpf_percpu_obj_drop_impl)
BTF_SET_END(special_kfunc_set)
BTF_ID_LIST(special_kfunc_list)
@@ -13909,7 +13907,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
if (reg_may_point_to_spin_lock(®s[BPF_REG_0]) && !regs[BPF_REG_0].id)
regs[BPF_REG_0].id = ++env->id_gen;
} else if (btf_type_is_void(t)) {
- 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_drop_impl] ||
meta.func_id == special_kfunc_list[KF_bpf_percpu_obj_drop_impl]) {
insn_aux->kptr_struct_meta =
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
2025-05-20 15:25 ` Yonghong Song
@ 2025-05-20 16:29 ` Alexei Starovoitov
2025-05-20 18:01 ` Yonghong Song
0 siblings, 1 reply; 13+ messages in thread
From: Alexei Starovoitov @ 2025-05-20 16:29 UTC (permalink / raw)
To: Yonghong Song
Cc: Kumar Kartikeya Dwivedi, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team, Martin KaFai Lau
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[]
> >
> > Digging through the code it looks like we made a bit of a mess there.
> > Like this part:
> > } else if (btf_type_is_void(t)) {
> > 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_drop_impl] ||
> > meta.func_id ==
> > special_kfunc_list[KF_bpf_percpu_obj_drop_impl]) {
> >
> >
> > *obj_drop don't need to be in a set,
> > and btf_id_set_contains() doesn't need to be called.
> > Both kfuncs should be moved to new kfunc_ids[]
>
> As you mentioned, for this one, we can do
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2cf00b06ae66..a3ff57eaa5f4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -12110,7 +12110,6 @@ enum special_kfunc_list_type {
>
> 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)
> @@ -12129,7 +12128,6 @@ 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)
> -BTF_ID(func, bpf_percpu_obj_drop_impl)
> BTF_SET_END(special_kfunc_set)
>
> BTF_ID_LIST(special_kfunc_list)
> @@ -13909,7 +13907,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> if (reg_may_point_to_spin_lock(®s[BPF_REG_0]) && !regs[BPF_REG_0].id)
> regs[BPF_REG_0].id = ++env->id_gen;
> } else if (btf_type_is_void(t)) {
> - 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_drop_impl] ||
> meta.func_id == special_kfunc_list[KF_bpf_percpu_obj_drop_impl]) {
> insn_aux->kptr_struct_meta =
Yes. Something like this but with new enum and new kfunc_ids[], like:
if (meta.func_id == kfunc_ids[KF_bpf_obj_drop_impl] ..
There is a concern that two KF_* enums may be confusing,
since it's not obvious whether
special_kfunc_list[KF_foo] or kfunc_ids[KF_foo] should be used.
Need to think about how to resolve the ambiguity...
We also have these things for btf_ids of types:
extern u32 btf_tracing_ids[];
extern u32 bpf_cgroup_btf_id[];
extern u32 bpf_local_storage_map_btf_id[];
extern u32 btf_bpf_map_id[];
extern u32 bpf_kmem_cache_btf_id[];
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
2025-05-20 16:29 ` Alexei Starovoitov
@ 2025-05-20 18:01 ` Yonghong Song
2025-05-20 18:39 ` Alexei Starovoitov
0 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2025-05-20 18:01 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kumar Kartikeya Dwivedi, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team, Martin KaFai Lau
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)
+
+enum kfunc_ids_type {
+ KF_bpf_obj_drop_impl,
+ KF_bpf_rcu_read_lock,
+ KF_bpf_rcu_read_unlock,
+ KF_bpf_dynptr_from_skb,
+ KF_bpf_dynptr_from_xdp,
+ KF_bpf_dynptr_clone,
+ 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_ID_LIST(kfunc_ids)
+BTF_ID(func, bpf_obj_drop_impl)
+BTF_ID(func, bpf_rcu_read_lock)
+BTF_ID(func, bpf_rcu_read_unlock)
#ifdef CONFIG_NET
BTF_ID(func, bpf_dynptr_from_skb)
BTF_ID(func, bpf_dynptr_from_xdp)
@@ -12173,10 +12165,7 @@ BTF_ID(func, bpf_dynptr_from_xdp)
BTF_ID_UNUSED
BTF_ID_UNUSED
#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)
@@ -12223,22 +12212,22 @@ static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
static bool is_kfunc_bpf_rcu_read_lock(struct bpf_kfunc_call_arg_meta *meta)
{
- return meta->func_id == special_kfunc_list[KF_bpf_rcu_read_lock];
+ return meta->func_id == kfunc_ids[KF_bpf_rcu_read_lock];
}
static bool is_kfunc_bpf_rcu_read_unlock(struct bpf_kfunc_call_arg_meta *meta)
{
- return meta->func_id == special_kfunc_list[KF_bpf_rcu_read_unlock];
+ return meta->func_id == kfunc_ids[KF_bpf_rcu_read_unlock];
}
...
@@ -21470,13 +21459,13 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
insn_buf[2] = addr[1];
insn_buf[3] = *insn;
*cnt = 4;
- } else if (desc->func_id == special_kfunc_list[KF_bpf_obj_drop_impl] ||
- desc->func_id == special_kfunc_list[KF_bpf_percpu_obj_drop_impl] ||
+ } else if (desc->func_id == kfunc_ids[KF_bpf_obj_drop_impl] ||
+ desc->func_id == kfunc_ids[KF_bpf_percpu_obj_drop_impl] ||
desc->func_id == special_kfunc_list[KF_bpf_refcount_acquire_impl]) {
struct btf_struct_meta *kptr_struct_meta = env->insn_aux_data[insn_idx].kptr_struct_meta;
struct bpf_insn addr[2] = { BPF_LD_IMM64(BPF_REG_2, (long)kptr_struct_meta) };
- if (desc->func_id == special_kfunc_list[KF_bpf_percpu_obj_drop_impl] && kptr_struct_meta) {
+ if (desc->func_id == kfunc_ids[KF_bpf_percpu_obj_drop_impl] && kptr_struct_meta) {
verbose(env, "verifier internal error: NULL kptr_struct_meta expected at insn_idx %d\n",
insn_idx);
return -EFAULT;
So we have clear separation between special_kfunc (using btf_id_set_contains())
and list-only kfunc_ids.
>
>>> Digging through the code it looks like we made a bit of a mess there.
>>> Like this part:
>>> } else if (btf_type_is_void(t)) {
>>> 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_drop_impl] ||
>>> meta.func_id ==
>>> special_kfunc_list[KF_bpf_percpu_obj_drop_impl]) {
>>>
>>>
>>> *obj_drop don't need to be in a set,
>>> and btf_id_set_contains() doesn't need to be called.
>>> Both kfuncs should be moved to new kfunc_ids[]
>> As you mentioned, for this one, we can do
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 2cf00b06ae66..a3ff57eaa5f4 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -12110,7 +12110,6 @@ enum special_kfunc_list_type {
>>
>> 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)
>> @@ -12129,7 +12128,6 @@ 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)
>> -BTF_ID(func, bpf_percpu_obj_drop_impl)
>> BTF_SET_END(special_kfunc_set)
>>
>> BTF_ID_LIST(special_kfunc_list)
>> @@ -13909,7 +13907,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>> if (reg_may_point_to_spin_lock(®s[BPF_REG_0]) && !regs[BPF_REG_0].id)
>> regs[BPF_REG_0].id = ++env->id_gen;
>> } else if (btf_type_is_void(t)) {
>> - 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_drop_impl] ||
>> meta.func_id == special_kfunc_list[KF_bpf_percpu_obj_drop_impl]) {
>> insn_aux->kptr_struct_meta =
> Yes. Something like this but with new enum and new kfunc_ids[], like:
> if (meta.func_id == kfunc_ids[KF_bpf_obj_drop_impl] ..
Right. kfunc_ids is used here.
>
> There is a concern that two KF_* enums may be confusing,
> since it's not obvious whether
> special_kfunc_list[KF_foo] or kfunc_ids[KF_foo] should be used.
> Need to think about how to resolve the ambiguity...
Based on current verifier logic. special_kfunc_list[KF_foo] should
be used if KF_foo is involved with btf_id_set_contains(...) checking.
>
> We also have these things for btf_ids of types:
> extern u32 btf_tracing_ids[];
> extern u32 bpf_cgroup_btf_id[];
> extern u32 bpf_local_storage_map_btf_id[];
> extern u32 btf_bpf_map_id[];
> extern u32 bpf_kmem_cache_btf_id[];
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
2025-05-20 18:01 ` Yonghong Song
@ 2025-05-20 18:39 ` Alexei Starovoitov
2025-05-20 18:46 ` Alexei Starovoitov
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Alexei Starovoitov @ 2025-05-20 18:39 UTC (permalink / raw)
To: Yonghong Song
Cc: Kumar Kartikeya Dwivedi, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team, Martin KaFai Lau
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;
}
?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
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
2 siblings, 1 reply; 13+ messages in thread
From: Alexei Starovoitov @ 2025-05-20 18:46 UTC (permalink / raw)
To: Yonghong Song
Cc: Kumar Kartikeya Dwivedi, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team, Martin KaFai Lau
On Tue, May 20, 2025 at 11:39 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> 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;
> }
>
> ?
Found old Kumar's reply:
https://lore.kernel.org/bpf/20221120204625.ndtr7ygh7zgjxrsz@apollo/
and my old reply:
https://lore.kernel.org/bpf/20221120222922.udsuzkr5hcvjzot5@macbook-pro-5.dhcp.thefacebook.com/
I think we need to remove special_kfunc_set,
and then special_kfunc_list[] can stay as-is,
and we can keep adding new kfuncs to it like bpf_unreachable in this case.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
2025-05-20 18:39 ` Alexei Starovoitov
2025-05-20 18:46 ` Alexei Starovoitov
@ 2025-05-20 19:40 ` Yonghong Song
2025-05-20 20:59 ` Yonghong Song
2 siblings, 0 replies; 13+ messages in thread
From: Yonghong Song @ 2025-05-20 19:40 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kumar Kartikeya Dwivedi, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team, Martin KaFai Lau
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.
Right. the they both will be 0, but one should use
special_kfunc_list and the other need to use kfunc_ids.
But indeed people may make an mistake for this...
>
> 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.
I think the below change should work. This way, we only
have one enum and one list. It will make things easier.
But I don't know the background for using btf_id_set_contains().
>
> 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;
> }
>
> ?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
2025-05-20 18:46 ` Alexei Starovoitov
@ 2025-05-20 19:50 ` Yonghong Song
0 siblings, 0 replies; 13+ messages in thread
From: Yonghong Song @ 2025-05-20 19:50 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kumar Kartikeya Dwivedi, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team, Martin KaFai Lau
On 5/20/25 2:46 AM, Alexei Starovoitov wrote:
> On Tue, May 20, 2025 at 11:39 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> 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;
>> }
>>
>> ?
> Found old Kumar's reply:
> https://lore.kernel.org/bpf/20221120204625.ndtr7ygh7zgjxrsz@apollo/
>
> and my old reply:
> https://lore.kernel.org/bpf/20221120222922.udsuzkr5hcvjzot5@macbook-pro-5.dhcp.thefacebook.com/
>
> I think we need to remove special_kfunc_set,
> and then special_kfunc_list[] can stay as-is,
> and we can keep adding new kfuncs to it like bpf_unreachable in this case.
Okay, I will remove special_kfunc_set() then. Thanks!
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
2025-05-20 18:39 ` Alexei Starovoitov
2025-05-20 18:46 ` Alexei Starovoitov
2025-05-20 19:40 ` Yonghong Song
@ 2025-05-20 20:59 ` Yonghong Song
2025-05-20 21:14 ` Alexei Starovoitov
2 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2025-05-20 20:59 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kumar Kartikeya Dwivedi, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team, Martin KaFai Lau
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Warn with bpf_unreachable() kfunc maybe due to uninitialized variable
2025-05-20 20:59 ` Yonghong Song
@ 2025-05-20 21:14 ` Alexei Starovoitov
0 siblings, 0 replies; 13+ messages in thread
From: Alexei Starovoitov @ 2025-05-20 21:14 UTC (permalink / raw)
To: Yonghong Song
Cc: Kumar Kartikeya Dwivedi, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Kernel Team, Martin KaFai Lau
On Tue, May 20, 2025 at 1:59 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> 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
How about option 3:
factor out the whole chunk of
meta.func_id == special_kfunc_list[..]
into its own helper function
int check_special_kfunc(env, meta, regs)
{
if (meta.btf != btf_vmlinux)
return 0;
if (meta.func_id == special_kfunc_list[KF_bpf_obj_new_impl] ...
regs[BPF_REG_0] = ...
return 0;
}
return 0;
}
and here it will be:
err = check_special_kfunc(env, meta, regs);
if (err)
return err;
if (btf_type_is_void(ptr_type)) {
or something like this?
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-05-20 21:14 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.