* [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var @ 2025-05-11 18:27 Yonghong Song 2025-05-11 18:27 ` [PATCH bpf-next 2/2] selftests/bpf: Add a test with bpf_unreachable() kfunc Yonghong Song 2025-05-12 0:30 ` [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var Alexei Starovoitov 0 siblings, 2 replies; 6+ messages in thread From: Yonghong Song @ 2025-05-11 18:27 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 and compiler may take advantage of uninit variable to do aggressive optimization. In llvm internals, uninitialized variable usage may generate 'unreachable' IR insn and these 'unreachable' IR insns may indicate uninit var impact on code optimization. With clang21 patch [2], those 'unreachable' IR insn are converted to func bpf_unreachable(). In kernel, a new kfunc bpf_unreachable() is added. If this kfunc (generated by [2]) is the last insn in the main prog or a subprog, the verifier will suggest the verification failure may be due to uninitialized var, so user can check their source code to find the root cause. Without this patch, the verifier will output last insn is not an exit or jmp and user will not know what is the potential root cause and it will take more time to debug this 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 | 17 ++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) In order to compile kernel successfully with the above [2], the following change is needed due to clang21 changes: --- a/Makefile +++ b/Makefile @@ -852,7 +852,7 @@ endif endif # may-sync-config endif # need-config -KBUILD_CFLAGS += -fno-delete-null-pointer-checks +KBUILD_CFLAGS += -fno-delete-null-pointer-checks -Wno-default-const-init-field-unsafe --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -19,6 +19,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, frame-address) KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) KBUILD_CFLAGS += -Wmissing-declarations KBUILD_CFLAGS += -Wmissing-prototypes +KBUILD_CFLAGS += -Wno-default-const-init-var-unsafe diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index fed53da75025..6048d7e19d4c 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -3283,6 +3283,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) @@ -3388,6 +3392,7 @@ BTF_ID_FLAGS(func, bpf_iter_kmem_cache_next, KF_ITER_NEXT | KF_RET_NULL | KF_SLE BTF_ID_FLAGS(func, bpf_iter_kmem_cache_destroy, KF_ITER_DESTROY | KF_SLEEPABLE) BTF_ID_FLAGS(func, bpf_local_irq_save) BTF_ID_FLAGS(func, bpf_local_irq_restore) +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 28f5a7899bd6..d26aec0a90d0 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -206,6 +206,7 @@ static int ref_set_non_owning(struct bpf_verifier_env *env, static void specialize_kfunc(struct bpf_verifier_env *env, u32 func_id, u16 offset, unsigned long *addr); static bool is_trusted_reg(const struct bpf_reg_state *reg); +static void verbose_insn(struct bpf_verifier_env *env, struct bpf_insn *insn); static bool bpf_map_ptr_poisoned(const struct bpf_insn_aux_data *aux) { @@ -3398,7 +3399,10 @@ static int check_subprogs(struct bpf_verifier_env *env) int i, subprog_start, subprog_end, off, cur_subprog = 0; struct bpf_subprog_info *subprog = env->subprog_info; struct bpf_insn *insn = env->prog->insnsi; + bool is_bpf_unreachable = false; int insn_cnt = env->prog->len; + const struct btf_type *t; + const char *tname; /* now check that all jumps are within the same subprog */ subprog_start = subprog[cur_subprog].start; @@ -3433,7 +3437,18 @@ static int check_subprogs(struct bpf_verifier_env *env) if (code != (BPF_JMP | BPF_EXIT) && code != (BPF_JMP32 | BPF_JA) && code != (BPF_JMP | BPF_JA)) { - verbose(env, "last insn is not an exit or jmp\n"); + verbose_insn(env, &insn[i]); + if (btf_vmlinux && insn[i].code == (BPF_CALL | BPF_JMP) && + insn[i].src_reg == BPF_PSEUDO_KFUNC_CALL) { + t = btf_type_by_id(btf_vmlinux, insn[i].imm); + tname = btf_name_by_offset(btf_vmlinux, t->name_off); + if (strcmp(tname, "bpf_unreachable") == 0) + is_bpf_unreachable = true; + } + if (is_bpf_unreachable) + verbose(env, "last insn is bpf_unreachable, due to uninitialized var?\n"); + else + verbose(env, "last insn is not an exit or jmp\n"); return -EINVAL; } subprog_start = subprog_end; -- 2.47.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Add a test with bpf_unreachable() kfunc 2025-05-11 18:27 [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var Yonghong Song @ 2025-05-11 18:27 ` Yonghong Song 2025-05-12 0:30 ` [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var Alexei Starovoitov 1 sibling, 0 replies; 6+ messages in thread From: Yonghong Song @ 2025-05-11 18:27 UTC (permalink / raw) To: bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team, Martin KaFai Lau The test case is from [1] reported by Marc Suñé. When compiled with [2], the object code looks like: 0000000000000000 <repro>: ; { 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 You can see the last insn is bpf_unreachable() func and bpf verifier can take advantage of such information and emits the following error message: (85) call bpf_unreachable#74465 last insn is bpf_unreachable, due to uninitialized var? [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> --- .../selftests/bpf/prog_tests/verifier.c | 2 + .../selftests/bpf/progs/verifier_uninit_var.c | 203 ++++++++++++++++++ 2 files changed, 205 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/verifier_uninit_var.c diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c index e66a57970d28..bc2765d130c0 100644 --- a/tools/testing/selftests/bpf/prog_tests/verifier.c +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c @@ -87,6 +87,7 @@ #include "verifier_tailcall_jit.skel.h" #include "verifier_typedef.skel.h" #include "verifier_uninit.skel.h" +#include "verifier_uninit_var.skel.h" #include "verifier_unpriv.skel.h" #include "verifier_unpriv_perf.skel.h" #include "verifier_value_adj_spill.skel.h" @@ -220,6 +221,7 @@ void test_verifier_subreg(void) { RUN(verifier_subreg); } void test_verifier_tailcall_jit(void) { RUN(verifier_tailcall_jit); } void test_verifier_typedef(void) { RUN(verifier_typedef); } void test_verifier_uninit(void) { RUN(verifier_uninit); } +void test_verifier_uninit_var(void) { RUN(verifier_uninit_var); } void test_verifier_unpriv(void) { RUN(verifier_unpriv); } void test_verifier_unpriv_perf(void) { RUN(verifier_unpriv_perf); } void test_verifier_value_adj_spill(void) { RUN(verifier_value_adj_spill); } diff --git a/tools/testing/selftests/bpf/progs/verifier_uninit_var.c b/tools/testing/selftests/bpf/progs/verifier_uninit_var.c new file mode 100644 index 000000000000..5ec01b492623 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/verifier_uninit_var.c @@ -0,0 +1,203 @@ +#include <linux/bpf.h> +#include <linux/pkt_cls.h> +#include <linux/if_ether.h> +#include <linux/in.h> +#include <linux/ip.h> +#include <linux/ipv6.h> +#include <linux/udp.h> +#include <linux/tcp.h> +#include <stdbool.h> +#include <linux/icmpv6.h> +#include <linux/in.h> + +#include <bpf/bpf_endian.h> +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> +#include "bpf_misc.h" + +union macaddr { + struct { + __u32 p1; + __u16 p2; + }; + __u8 addr[6]; +}; + +union v6addr { + struct { + __u32 p1; + __u32 p2; + __u32 p3; + __u32 p4; + }; + struct { + __u64 d1; + __u64 d2; + }; + __u8 addr[16]; +}; + +/* Number of extension headers that can be skipped */ +#define IPV6_MAX_HEADERS 4 + +#define NEXTHDR_HOP 0 /* Hop-by-hop option header. */ +#define NEXTHDR_TCP 6 /* TCP segment. */ +#define NEXTHDR_UDP 17 /* UDP message. */ +#define NEXTHDR_IPV6 41 /* IPv6 in IPv6 */ +#define NEXTHDR_ROUTING 43 /* Routing header. */ +#define NEXTHDR_FRAGMENT 44 /* Fragmentation/reassembly header. */ +#define NEXTHDR_GRE 47 /* GRE header. */ +#define NEXTHDR_ESP 50 /* Encapsulating security payload. */ +#define NEXTHDR_AUTH 51 /* Authentication header. */ +#define NEXTHDR_ICMP 58 /* ICMP for IPv6. */ +#define NEXTHDR_NONE 59 /* No next header */ +#define NEXTHDR_DEST 60 /* Destination options header. */ +#define NEXTHDR_SCTP 132 /* SCTP message. */ +#define NEXTHDR_MOBILITY 135 /* Mobility header. */ + +#define NEXTHDR_MAX 255 + +#define IPV6_SADDR_OFF offsetof(struct ipv6hdr, saddr) +#define IPV6_DADDR_OFF offsetof(struct ipv6hdr, daddr) + +#define NEXTHDR_ICMP 58 +#define ICMP6_NS_MSG_TYPE 135 + +#define DROP_INVALID -134 +#define DROP_INVALID_EXTHDR -156 +#define DROP_FRAG_NOSUPPORT -157 + +#define ctx_load_bytes skb_load_bytes + +#define DEFINE_FUNC_CTX_POINTER(FIELD) \ +static __always_inline void * \ +ctx_ ## FIELD(const struct __sk_buff *ctx) \ +{ \ + void *ptr; \ + \ + /* LLVM may generate u32 assignments of ctx->{data,data_end,data_meta}. \ + * With this inline asm, LLVM loses track of the fact this field is on \ + * 32 bits. \ + */ \ + asm volatile("%0 = *(u32 *)(%1 + %2)" \ + : "=r"(ptr) \ + : "r"(ctx), "i"(offsetof(struct __sk_buff, FIELD))); \ + return ptr; \ +} +/* This defines ctx_data(). */ +DEFINE_FUNC_CTX_POINTER(data) +#undef DEFINE_FUNC_CTX_POINTER + + +static __always_inline int ipv6_optlen(const struct ipv6_opt_hdr *opthdr) +{ + return (opthdr->hdrlen + 1) << 3; +} + +static __always_inline int ipv6_authlen(const struct ipv6_opt_hdr *opthdr) +{ + return (opthdr->hdrlen + 2) << 2; +} + +static __always_inline int ipv6_hdrlen_offset(struct __sk_buff *ctx, + __u8 *nexthdr, int l3_off) +{ + int i, len = sizeof(struct ipv6hdr); + struct ipv6_opt_hdr opthdr __attribute__((aligned(8))); + __u8 nh = *nexthdr; + +#pragma unroll + for (i = 0; i < IPV6_MAX_HEADERS; i++) { + switch (nh) { + case NEXTHDR_NONE: + return DROP_INVALID_EXTHDR; + + case NEXTHDR_FRAGMENT: + return DROP_FRAG_NOSUPPORT; + + case NEXTHDR_HOP: + case NEXTHDR_ROUTING: + case NEXTHDR_AUTH: + case NEXTHDR_DEST: + if (bpf_skb_load_bytes(ctx, l3_off + len, &opthdr, + sizeof(opthdr)) < 0) + return DROP_INVALID; + + if (nh == NEXTHDR_AUTH) + len += ipv6_authlen(&opthdr); + else + len += ipv6_optlen(&opthdr); + + nh = opthdr.nexthdr; + break; + + default: + bpf_printk("OKOK %d, len: %d", *nexthdr, len); + *nexthdr = nh; + return len; + } + } + + bpf_printk("KO INVALID EXTHDR"); + + /* Reached limit of supported extension headers */ + return DROP_INVALID_EXTHDR; +} +static __always_inline +bool icmp6_ndisc_validate(struct __sk_buff *ctx, struct ipv6hdr *ip6, + union macaddr *mac, union macaddr *smac, + union v6addr *sip, union v6addr *tip) +{ + __u8 nexthdr; + struct icmp6hdr *icmp; + int l3_off, l4_off; + + l3_off = (__u8 *)ip6 - (__u8 *)ctx_data(ctx); + bpf_printk("pre ipv6_hdrlen_offset"); + l4_off = ipv6_hdrlen_offset(ctx, &nexthdr, l3_off); + bpf_printk("post ipv6_hdrlen_offset"); + + if (l4_off < 0 || nexthdr != NEXTHDR_ICMP) { + bpf_printk("KO 1"); + return false; + } + + icmp = (struct icmp6hdr *)((__u8 *)ctx_data(ctx) + l4_off); + if (icmp->icmp6_type != ICMP6_NS_MSG_TYPE) { + bpf_printk("KO 2"); + return false; + } + + /* Extract fields */ +#if 0 + eth_load_saddr(ctx, &smac->addr[0], 0); + eth_load_daddr(ctx, &mac->addr[0], 0); + ipv6_load_saddr(ctx, l3_off, sip); + ipv6_load_daddr(ctx, l3_off, tip); +#endif + bpf_printk("ACK "); + + return true; +} + +SEC("classifier") +__description("agressive optimization due to uninitialized variable") +#if __clang_major__ >= 21 +__failure __msg("last insn is bpf_unreachable, due to uninitialized var") +#else +__failure __msg("last insn is not an exit or jmp") +#endif +int classifier_uninit_var(struct __sk_buff *skb) +{ + struct ipv6hdr* ip6 = NULL; + union macaddr mac, smac; + union v6addr sip, tip; + + bpf_printk("Start"); + icmp6_ndisc_validate(skb, ip6, &mac, &smac, &sip, &tip); + bpf_printk("End"); + + return 0; +} + +char _license[] SEC("license") = "GPL"; -- 2.47.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var 2025-05-11 18:27 [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var Yonghong Song 2025-05-11 18:27 ` [PATCH bpf-next 2/2] selftests/bpf: Add a test with bpf_unreachable() kfunc Yonghong Song @ 2025-05-12 0:30 ` Alexei Starovoitov 2025-05-13 5:49 ` Yonghong Song 1 sibling, 1 reply; 6+ messages in thread From: Alexei Starovoitov @ 2025-05-12 0:30 UTC (permalink / raw) To: Yonghong Song Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team, Martin KaFai Lau On Sun, May 11, 2025 at 11:28 AM 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 and compiler > may take advantage of uninit variable to do aggressive optimization. > > In llvm internals, uninitialized variable usage may generate > 'unreachable' IR insn and these 'unreachable' IR insns may indicate > uninit var impact on code optimization. With clang21 patch [2], > those 'unreachable' IR insn are converted to func bpf_unreachable(). > > In kernel, a new kfunc bpf_unreachable() is added. If this kfunc > (generated by [2]) is the last insn in the main prog or a subprog, > the verifier will suggest the verification failure may be due to > uninitialized var, so user can check their source code to find the > root cause. > > Without this patch, the verifier will output > last insn is not an exit or jmp > and user will not know what is the potential root cause and > it will take more time to debug this 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 | 17 ++++++++++++++++- > 2 files changed, 21 insertions(+), 1 deletion(-) > > In order to compile kernel successfully with the above [2], the following > change is needed due to clang21 changes: > > --- a/Makefile > +++ b/Makefile > @@ -852,7 +852,7 @@ endif > endif # may-sync-config > endif # need-config > > -KBUILD_CFLAGS += -fno-delete-null-pointer-checks > +KBUILD_CFLAGS += -fno-delete-null-pointer-checks -Wno-default-const-init-field-unsafe > > --- a/scripts/Makefile.extrawarn > +++ b/scripts/Makefile.extrawarn > @@ -19,6 +19,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, frame-address) > KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) > KBUILD_CFLAGS += -Wmissing-declarations > KBUILD_CFLAGS += -Wmissing-prototypes > +KBUILD_CFLAGS += -Wno-default-const-init-var-unsafe > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index fed53da75025..6048d7e19d4c 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -3283,6 +3283,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) > +{ > +} Does it have to be an actual function with the body? Can it be a kfunc that doesn't consume any .text ? > + > __bpf_kfunc_end_defs(); > > BTF_KFUNCS_START(generic_btf_ids) > @@ -3388,6 +3392,7 @@ BTF_ID_FLAGS(func, bpf_iter_kmem_cache_next, KF_ITER_NEXT | KF_RET_NULL | KF_SLE > BTF_ID_FLAGS(func, bpf_iter_kmem_cache_destroy, KF_ITER_DESTROY | KF_SLEEPABLE) > BTF_ID_FLAGS(func, bpf_local_irq_save) > BTF_ID_FLAGS(func, bpf_local_irq_restore) > +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 28f5a7899bd6..d26aec0a90d0 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -206,6 +206,7 @@ static int ref_set_non_owning(struct bpf_verifier_env *env, > static void specialize_kfunc(struct bpf_verifier_env *env, > u32 func_id, u16 offset, unsigned long *addr); > static bool is_trusted_reg(const struct bpf_reg_state *reg); > +static void verbose_insn(struct bpf_verifier_env *env, struct bpf_insn *insn); > > static bool bpf_map_ptr_poisoned(const struct bpf_insn_aux_data *aux) > { > @@ -3398,7 +3399,10 @@ static int check_subprogs(struct bpf_verifier_env *env) > int i, subprog_start, subprog_end, off, cur_subprog = 0; > struct bpf_subprog_info *subprog = env->subprog_info; > struct bpf_insn *insn = env->prog->insnsi; > + bool is_bpf_unreachable = false; > int insn_cnt = env->prog->len; > + const struct btf_type *t; > + const char *tname; > > /* now check that all jumps are within the same subprog */ > subprog_start = subprog[cur_subprog].start; > @@ -3433,7 +3437,18 @@ static int check_subprogs(struct bpf_verifier_env *env) > if (code != (BPF_JMP | BPF_EXIT) && > code != (BPF_JMP32 | BPF_JA) && > code != (BPF_JMP | BPF_JA)) { > - verbose(env, "last insn is not an exit or jmp\n"); > + verbose_insn(env, &insn[i]); > + if (btf_vmlinux && insn[i].code == (BPF_CALL | BPF_JMP) && > + insn[i].src_reg == BPF_PSEUDO_KFUNC_CALL) { > + t = btf_type_by_id(btf_vmlinux, insn[i].imm); > + tname = btf_name_by_offset(btf_vmlinux, t->name_off); > + if (strcmp(tname, "bpf_unreachable") == 0) the check by name is not pretty. > + is_bpf_unreachable = true; > + } > + if (is_bpf_unreachable) > + verbose(env, "last insn is bpf_unreachable, due to uninitialized var?\n"); > + else > + verbose(env, "last insn is not an exit or jmp\n"); This is too specific imo. add_subprog_and_kfunc() -> add_kfunc_call() should probably handle it instead, and print that error. It doesn't matter that call bpf_unreachable is the last insn of a program or subprogram. I suspect llvm can emit it anywhere. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var 2025-05-12 0:30 ` [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var Alexei Starovoitov @ 2025-05-13 5:49 ` Yonghong Song 2025-05-13 14:54 ` Alexei Starovoitov 0 siblings, 1 reply; 6+ messages in thread From: Yonghong Song @ 2025-05-13 5:49 UTC (permalink / raw) To: Alexei Starovoitov Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team, Martin KaFai Lau On 5/11/25 8:30 AM, Alexei Starovoitov wrote: > On Sun, May 11, 2025 at 11:28 AM 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 and compiler >> may take advantage of uninit variable to do aggressive optimization. >> >> In llvm internals, uninitialized variable usage may generate >> 'unreachable' IR insn and these 'unreachable' IR insns may indicate >> uninit var impact on code optimization. With clang21 patch [2], >> those 'unreachable' IR insn are converted to func bpf_unreachable(). >> >> In kernel, a new kfunc bpf_unreachable() is added. If this kfunc >> (generated by [2]) is the last insn in the main prog or a subprog, >> the verifier will suggest the verification failure may be due to >> uninitialized var, so user can check their source code to find the >> root cause. >> >> Without this patch, the verifier will output >> last insn is not an exit or jmp >> and user will not know what is the potential root cause and >> it will take more time to debug this 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 | 17 ++++++++++++++++- >> 2 files changed, 21 insertions(+), 1 deletion(-) >> >> In order to compile kernel successfully with the above [2], the following >> change is needed due to clang21 changes: >> >> --- a/Makefile >> +++ b/Makefile >> @@ -852,7 +852,7 @@ endif >> endif # may-sync-config >> endif # need-config >> >> -KBUILD_CFLAGS += -fno-delete-null-pointer-checks >> +KBUILD_CFLAGS += -fno-delete-null-pointer-checks -Wno-default-const-init-field-unsafe >> >> --- a/scripts/Makefile.extrawarn >> +++ b/scripts/Makefile.extrawarn >> @@ -19,6 +19,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, frame-address) >> KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) >> KBUILD_CFLAGS += -Wmissing-declarations >> KBUILD_CFLAGS += -Wmissing-prototypes >> +KBUILD_CFLAGS += -Wno-default-const-init-var-unsafe >> >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c >> index fed53da75025..6048d7e19d4c 100644 >> --- a/kernel/bpf/helpers.c >> +++ b/kernel/bpf/helpers.c >> @@ -3283,6 +3283,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) >> +{ >> +} > Does it have to be an actual function with the body? > Can it be a kfunc that doesn't consume any .text ? I tried to define bpf_unreachable as an extern function, but it does not work as a __bpf_kfunc. I agree that we do not need to consume any bytes in .text section for bpf_unreachable. I have not found a solution for that yet. > >> + >> __bpf_kfunc_end_defs(); >> >> BTF_KFUNCS_START(generic_btf_ids) >> @@ -3388,6 +3392,7 @@ BTF_ID_FLAGS(func, bpf_iter_kmem_cache_next, KF_ITER_NEXT | KF_RET_NULL | KF_SLE >> BTF_ID_FLAGS(func, bpf_iter_kmem_cache_destroy, KF_ITER_DESTROY | KF_SLEEPABLE) >> BTF_ID_FLAGS(func, bpf_local_irq_save) >> BTF_ID_FLAGS(func, bpf_local_irq_restore) >> +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 28f5a7899bd6..d26aec0a90d0 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -206,6 +206,7 @@ static int ref_set_non_owning(struct bpf_verifier_env *env, >> static void specialize_kfunc(struct bpf_verifier_env *env, >> u32 func_id, u16 offset, unsigned long *addr); >> static bool is_trusted_reg(const struct bpf_reg_state *reg); >> +static void verbose_insn(struct bpf_verifier_env *env, struct bpf_insn *insn); >> >> static bool bpf_map_ptr_poisoned(const struct bpf_insn_aux_data *aux) >> { >> @@ -3398,7 +3399,10 @@ static int check_subprogs(struct bpf_verifier_env *env) >> int i, subprog_start, subprog_end, off, cur_subprog = 0; >> struct bpf_subprog_info *subprog = env->subprog_info; >> struct bpf_insn *insn = env->prog->insnsi; >> + bool is_bpf_unreachable = false; >> int insn_cnt = env->prog->len; >> + const struct btf_type *t; >> + const char *tname; >> >> /* now check that all jumps are within the same subprog */ >> subprog_start = subprog[cur_subprog].start; >> @@ -3433,7 +3437,18 @@ static int check_subprogs(struct bpf_verifier_env *env) >> if (code != (BPF_JMP | BPF_EXIT) && >> code != (BPF_JMP32 | BPF_JA) && >> code != (BPF_JMP | BPF_JA)) { >> - verbose(env, "last insn is not an exit or jmp\n"); >> + verbose_insn(env, &insn[i]); >> + if (btf_vmlinux && insn[i].code == (BPF_CALL | BPF_JMP) && >> + insn[i].src_reg == BPF_PSEUDO_KFUNC_CALL) { >> + t = btf_type_by_id(btf_vmlinux, insn[i].imm); >> + tname = btf_name_by_offset(btf_vmlinux, t->name_off); >> + if (strcmp(tname, "bpf_unreachable") == 0) > the check by name is not pretty. > >> + is_bpf_unreachable = true; >> + } >> + if (is_bpf_unreachable) >> + verbose(env, "last insn is bpf_unreachable, due to uninitialized var?\n"); >> + else >> + verbose(env, "last insn is not an exit or jmp\n"); > This is too specific imo. > add_subprog_and_kfunc() -> add_kfunc_call() > should probably handle it instead, > and print that error. add_subprog_and_kfunc() -> add_kfunc_call() approach probably won't work. The error should be emitted only if the verifier (through path sensitive analysis) reaches bpf_unreachable(). if bpf_unreachable() exists in the bpf prog, but verifier cannot reach it during verification process, error will not printed. > It doesn't matter that call bpf_unreachable is the last insn > of a program or subprogram. > I suspect llvm can emit it anywhere. It is totally possible that bpf_unreachable may appear in the middle of code. But based on past examples, bpf_unreachable tends to be in the last insn and it may be targetted from multiple sources. This also makes code easier to understand. I can dig into llvm internal a little bit more to find how llvm places 'unreachable' IR insns. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var 2025-05-13 5:49 ` Yonghong Song @ 2025-05-13 14:54 ` Alexei Starovoitov 2025-05-13 18:05 ` Yonghong Song 0 siblings, 1 reply; 6+ messages in thread From: Alexei Starovoitov @ 2025-05-13 14:54 UTC (permalink / raw) To: Yonghong Song Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team, Martin KaFai Lau On Mon, May 12, 2025 at 10:49 PM Yonghong Song <yonghong.song@linux.dev> wrote: > > > > On 5/11/25 8:30 AM, Alexei Starovoitov wrote: > > On Sun, May 11, 2025 at 11:28 AM 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 and compiler > >> may take advantage of uninit variable to do aggressive optimization. > >> > >> In llvm internals, uninitialized variable usage may generate > >> 'unreachable' IR insn and these 'unreachable' IR insns may indicate > >> uninit var impact on code optimization. With clang21 patch [2], > >> those 'unreachable' IR insn are converted to func bpf_unreachable(). > >> > >> In kernel, a new kfunc bpf_unreachable() is added. If this kfunc > >> (generated by [2]) is the last insn in the main prog or a subprog, > >> the verifier will suggest the verification failure may be due to > >> uninitialized var, so user can check their source code to find the > >> root cause. > >> > >> Without this patch, the verifier will output > >> last insn is not an exit or jmp > >> and user will not know what is the potential root cause and > >> it will take more time to debug this 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 | 17 ++++++++++++++++- > >> 2 files changed, 21 insertions(+), 1 deletion(-) > >> > >> In order to compile kernel successfully with the above [2], the following > >> change is needed due to clang21 changes: > >> > >> --- a/Makefile > >> +++ b/Makefile > >> @@ -852,7 +852,7 @@ endif > >> endif # may-sync-config > >> endif # need-config > >> > >> -KBUILD_CFLAGS += -fno-delete-null-pointer-checks > >> +KBUILD_CFLAGS += -fno-delete-null-pointer-checks -Wno-default-const-init-field-unsafe > >> > >> --- a/scripts/Makefile.extrawarn > >> +++ b/scripts/Makefile.extrawarn > >> @@ -19,6 +19,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, frame-address) > >> KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) > >> KBUILD_CFLAGS += -Wmissing-declarations > >> KBUILD_CFLAGS += -Wmissing-prototypes > >> +KBUILD_CFLAGS += -Wno-default-const-init-var-unsafe > >> > >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > >> index fed53da75025..6048d7e19d4c 100644 > >> --- a/kernel/bpf/helpers.c > >> +++ b/kernel/bpf/helpers.c > >> @@ -3283,6 +3283,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) > >> +{ > >> +} > > Does it have to be an actual function with the body? > > Can it be a kfunc that doesn't consume any .text ? > > I tried to define bpf_unreachable as an extern function, but > it does not work as a __bpf_kfunc. I agree that we do not > need to consume any bytes in .text section for bpf_unreachable. > I have not found a solution for that yet. Have you tried marking it as 'naked' and empty body? > > > >> + > >> __bpf_kfunc_end_defs(); > >> > >> BTF_KFUNCS_START(generic_btf_ids) > >> @@ -3388,6 +3392,7 @@ BTF_ID_FLAGS(func, bpf_iter_kmem_cache_next, KF_ITER_NEXT | KF_RET_NULL | KF_SLE > >> BTF_ID_FLAGS(func, bpf_iter_kmem_cache_destroy, KF_ITER_DESTROY | KF_SLEEPABLE) > >> BTF_ID_FLAGS(func, bpf_local_irq_save) > >> BTF_ID_FLAGS(func, bpf_local_irq_restore) > >> +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 28f5a7899bd6..d26aec0a90d0 100644 > >> --- a/kernel/bpf/verifier.c > >> +++ b/kernel/bpf/verifier.c > >> @@ -206,6 +206,7 @@ static int ref_set_non_owning(struct bpf_verifier_env *env, > >> static void specialize_kfunc(struct bpf_verifier_env *env, > >> u32 func_id, u16 offset, unsigned long *addr); > >> static bool is_trusted_reg(const struct bpf_reg_state *reg); > >> +static void verbose_insn(struct bpf_verifier_env *env, struct bpf_insn *insn); > >> > >> static bool bpf_map_ptr_poisoned(const struct bpf_insn_aux_data *aux) > >> { > >> @@ -3398,7 +3399,10 @@ static int check_subprogs(struct bpf_verifier_env *env) > >> int i, subprog_start, subprog_end, off, cur_subprog = 0; > >> struct bpf_subprog_info *subprog = env->subprog_info; > >> struct bpf_insn *insn = env->prog->insnsi; > >> + bool is_bpf_unreachable = false; > >> int insn_cnt = env->prog->len; > >> + const struct btf_type *t; > >> + const char *tname; > >> > >> /* now check that all jumps are within the same subprog */ > >> subprog_start = subprog[cur_subprog].start; > >> @@ -3433,7 +3437,18 @@ static int check_subprogs(struct bpf_verifier_env *env) > >> if (code != (BPF_JMP | BPF_EXIT) && > >> code != (BPF_JMP32 | BPF_JA) && > >> code != (BPF_JMP | BPF_JA)) { > >> - verbose(env, "last insn is not an exit or jmp\n"); > >> + verbose_insn(env, &insn[i]); > >> + if (btf_vmlinux && insn[i].code == (BPF_CALL | BPF_JMP) && > >> + insn[i].src_reg == BPF_PSEUDO_KFUNC_CALL) { > >> + t = btf_type_by_id(btf_vmlinux, insn[i].imm); > >> + tname = btf_name_by_offset(btf_vmlinux, t->name_off); > >> + if (strcmp(tname, "bpf_unreachable") == 0) > > the check by name is not pretty. > > > >> + is_bpf_unreachable = true; > >> + } > >> + if (is_bpf_unreachable) > >> + verbose(env, "last insn is bpf_unreachable, due to uninitialized var?\n"); > >> + else > >> + verbose(env, "last insn is not an exit or jmp\n"); > > This is too specific imo. > > add_subprog_and_kfunc() -> add_kfunc_call() > > should probably handle it instead, > > and print that error. > > add_subprog_and_kfunc() -> add_kfunc_call() approach probably won't work. > The error should be emitted only if the verifier (through path sensitive > analysis) reaches bpf_unreachable(). > > if bpf_unreachable() exists in the bpf prog, but verifier cannot reach it > during verification process, error will not printed. you mean when bpf_unreachable() in the actual dead code then the verifier shouldn't error ? Makes sense. > > It doesn't matter that call bpf_unreachable is the last insn > > of a program or subprogram. > > I suspect llvm can emit it anywhere. > > It is totally possible that bpf_unreachable may appear in the middle of > code. But based on past examples, bpf_unreachable tends to be in the > last insn and it may be targetted from multiple sources. This also makes > code easier to understand. I can dig into llvm internal a little bit > more to find how llvm places 'unreachable' IR insns. I recall Anton hit some odd case of unreachable code with upcoming indirect goto/call work. If llmv starts emitting call bpf_unreachable that may be another case. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var 2025-05-13 14:54 ` Alexei Starovoitov @ 2025-05-13 18:05 ` Yonghong Song 0 siblings, 0 replies; 6+ messages in thread From: Yonghong Song @ 2025-05-13 18:05 UTC (permalink / raw) To: Alexei Starovoitov Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Kernel Team, Martin KaFai Lau On 5/13/25 10:54 PM, Alexei Starovoitov wrote: > On Mon, May 12, 2025 at 10:49 PM Yonghong Song <yonghong.song@linux.dev> wrote: >> >> >> On 5/11/25 8:30 AM, Alexei Starovoitov wrote: >>> On Sun, May 11, 2025 at 11:28 AM 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 and compiler >>>> may take advantage of uninit variable to do aggressive optimization. >>>> >>>> In llvm internals, uninitialized variable usage may generate >>>> 'unreachable' IR insn and these 'unreachable' IR insns may indicate >>>> uninit var impact on code optimization. With clang21 patch [2], >>>> those 'unreachable' IR insn are converted to func bpf_unreachable(). >>>> >>>> In kernel, a new kfunc bpf_unreachable() is added. If this kfunc >>>> (generated by [2]) is the last insn in the main prog or a subprog, >>>> the verifier will suggest the verification failure may be due to >>>> uninitialized var, so user can check their source code to find the >>>> root cause. >>>> >>>> Without this patch, the verifier will output >>>> last insn is not an exit or jmp >>>> and user will not know what is the potential root cause and >>>> it will take more time to debug this 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 | 17 ++++++++++++++++- >>>> 2 files changed, 21 insertions(+), 1 deletion(-) >>>> >>>> In order to compile kernel successfully with the above [2], the following >>>> change is needed due to clang21 changes: >>>> >>>> --- a/Makefile >>>> +++ b/Makefile >>>> @@ -852,7 +852,7 @@ endif >>>> endif # may-sync-config >>>> endif # need-config >>>> >>>> -KBUILD_CFLAGS += -fno-delete-null-pointer-checks >>>> +KBUILD_CFLAGS += -fno-delete-null-pointer-checks -Wno-default-const-init-field-unsafe >>>> >>>> --- a/scripts/Makefile.extrawarn >>>> +++ b/scripts/Makefile.extrawarn >>>> @@ -19,6 +19,7 @@ KBUILD_CFLAGS += $(call cc-disable-warning, frame-address) >>>> KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) >>>> KBUILD_CFLAGS += -Wmissing-declarations >>>> KBUILD_CFLAGS += -Wmissing-prototypes >>>> +KBUILD_CFLAGS += -Wno-default-const-init-var-unsafe >>>> >>>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c >>>> index fed53da75025..6048d7e19d4c 100644 >>>> --- a/kernel/bpf/helpers.c >>>> +++ b/kernel/bpf/helpers.c >>>> @@ -3283,6 +3283,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) >>>> +{ >>>> +} >>> Does it have to be an actual function with the body? >>> Can it be a kfunc that doesn't consume any .text ? >> I tried to define bpf_unreachable as an extern function, but >> it does not work as a __bpf_kfunc. I agree that we do not >> need to consume any bytes in .text section for bpf_unreachable. >> I have not found a solution for that yet. > Have you tried marking it as 'naked' and empty body? With or without 'naked' attribute, 32 byte text will be consumed: ffffffff8188daa0 <__pfx_bpf_unreachable>: ffffffff8188daa0: 90 nop ffffffff8188daa1: 90 nop ffffffff8188daa2: 90 nop ffffffff8188daa3: 90 nop ffffffff8188daa4: 90 nop ffffffff8188daa5: 90 nop ffffffff8188daa6: 90 nop ffffffff8188daa7: 90 nop ffffffff8188daa8: 90 nop ffffffff8188daa9: 90 nop ffffffff8188daaa: 90 nop ffffffff8188daab: 90 nop ffffffff8188daac: 90 nop ffffffff8188daad: 90 nop ffffffff8188daae: 90 nop ffffffff8188daaf: 90 nop ffffffff8188dab0 <bpf_unreachable>: ffffffff8188dab0: f3 0f 1e fa endbr64 ffffffff8188dab4: 0f 1f 44 00 00 nopl (%rax,%rax) ffffffff8188dab9: 0f 1f 80 00 00 00 00 nopl (%rax) > >>>> + >>>> __bpf_kfunc_end_defs(); >>>> >>>> BTF_KFUNCS_START(generic_btf_ids) >>>> @@ -3388,6 +3392,7 @@ BTF_ID_FLAGS(func, bpf_iter_kmem_cache_next, KF_ITER_NEXT | KF_RET_NULL | KF_SLE >>>> BTF_ID_FLAGS(func, bpf_iter_kmem_cache_destroy, KF_ITER_DESTROY | KF_SLEEPABLE) >>>> BTF_ID_FLAGS(func, bpf_local_irq_save) >>>> BTF_ID_FLAGS(func, bpf_local_irq_restore) >>>> +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 28f5a7899bd6..d26aec0a90d0 100644 >>>> --- a/kernel/bpf/verifier.c >>>> +++ b/kernel/bpf/verifier.c >>>> @@ -206,6 +206,7 @@ static int ref_set_non_owning(struct bpf_verifier_env *env, >>>> static void specialize_kfunc(struct bpf_verifier_env *env, >>>> u32 func_id, u16 offset, unsigned long *addr); >>>> static bool is_trusted_reg(const struct bpf_reg_state *reg); >>>> +static void verbose_insn(struct bpf_verifier_env *env, struct bpf_insn *insn); >>>> >>>> static bool bpf_map_ptr_poisoned(const struct bpf_insn_aux_data *aux) >>>> { >>>> @@ -3398,7 +3399,10 @@ static int check_subprogs(struct bpf_verifier_env *env) >>>> int i, subprog_start, subprog_end, off, cur_subprog = 0; >>>> struct bpf_subprog_info *subprog = env->subprog_info; >>>> struct bpf_insn *insn = env->prog->insnsi; >>>> + bool is_bpf_unreachable = false; >>>> int insn_cnt = env->prog->len; >>>> + const struct btf_type *t; >>>> + const char *tname; >>>> >>>> /* now check that all jumps are within the same subprog */ >>>> subprog_start = subprog[cur_subprog].start; >>>> @@ -3433,7 +3437,18 @@ static int check_subprogs(struct bpf_verifier_env *env) >>>> if (code != (BPF_JMP | BPF_EXIT) && >>>> code != (BPF_JMP32 | BPF_JA) && >>>> code != (BPF_JMP | BPF_JA)) { >>>> - verbose(env, "last insn is not an exit or jmp\n"); >>>> + verbose_insn(env, &insn[i]); >>>> + if (btf_vmlinux && insn[i].code == (BPF_CALL | BPF_JMP) && >>>> + insn[i].src_reg == BPF_PSEUDO_KFUNC_CALL) { >>>> + t = btf_type_by_id(btf_vmlinux, insn[i].imm); >>>> + tname = btf_name_by_offset(btf_vmlinux, t->name_off); >>>> + if (strcmp(tname, "bpf_unreachable") == 0) >>> the check by name is not pretty. >>> >>>> + is_bpf_unreachable = true; >>>> + } >>>> + if (is_bpf_unreachable) >>>> + verbose(env, "last insn is bpf_unreachable, due to uninitialized var?\n"); >>>> + else >>>> + verbose(env, "last insn is not an exit or jmp\n"); >>> This is too specific imo. >>> add_subprog_and_kfunc() -> add_kfunc_call() >>> should probably handle it instead, >>> and print that error. >> add_subprog_and_kfunc() -> add_kfunc_call() approach probably won't work. >> The error should be emitted only if the verifier (through path sensitive >> analysis) reaches bpf_unreachable(). >> >> if bpf_unreachable() exists in the bpf prog, but verifier cannot reach it >> during verification process, error will not printed. > you mean when bpf_unreachable() in the actual dead code > then the verifier shouldn't error ? Makes sense. Right. This is what I intend to do. > >>> It doesn't matter that call bpf_unreachable is the last insn >>> of a program or subprogram. >>> I suspect llvm can emit it anywhere. >> It is totally possible that bpf_unreachable may appear in the middle of >> code. But based on past examples, bpf_unreachable tends to be in the >> last insn and it may be targetted from multiple sources. This also makes >> code easier to understand. I can dig into llvm internal a little bit >> more to find how llvm places 'unreachable' IR insns. > I recall Anton hit some odd case of unreachable code with > upcoming indirect goto/call work. > If llmv starts emitting call bpf_unreachable that may be another case. The following is the related jump table code: 0000000000000700 <foo>: ; switch (ctx->x) { 224: 79 11 00 00 00 00 00 00 r1 = *(u64 *)(r1 + 0x0) 225: 25 01 0f 00 1f 00 00 00 if r1 > 0x1f goto +0xf <foo+0x88> 226: 67 01 00 00 03 00 00 00 r1 <<= 0x3 227: 18 02 00 00 a8 00 00 00 00 00 00 00 00 00 00 00 r2 = 0xa8 ll 0000000000000718: R_BPF_64_64 .rodata 229: 0f 12 00 00 00 00 00 00 r2 += r1 230: 79 21 00 00 00 00 00 00 r1 = *(u64 *)(r2 + 0x0) 231: 0d 01 00 00 00 00 00 00 gotox r1 232: 05 00 08 00 00 00 00 00 goto +0x8 <foo+0x88> 233: b7 01 00 00 02 00 00 00 r1 = 0x2 ; switch (ctx->x) { 234: 05 00 07 00 00 00 00 00 goto +0x7 <foo+0x90> 235: b7 01 00 00 04 00 00 00 r1 = 0x4 ; break; 236: 05 00 05 00 00 00 00 00 goto +0x5 <foo+0x90> 237: b7 01 00 00 03 00 00 00 r1 = 0x3 ; break; 238: 05 00 03 00 00 00 00 00 goto +0x3 <foo+0x90> 239: b7 01 00 00 05 00 00 00 r1 = 0x5 ; break; 240: 05 00 01 00 00 00 00 00 goto +0x1 <foo+0x90> 241: b7 01 00 00 13 00 00 00 r1 = 0x13 242: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0x0 ll 0000000000000790: R_BPF_64_64 ret_user 244: 7b 12 00 00 00 00 00 00 *(u64 *)(r2 + 0x0) = r1 ; return 0; 245: b4 00 00 00 00 00 00 00 w0 = 0x0 246: 95 00 00 00 00 00 00 00 exit The insn 232: 05 00 08 00 00 00 00 00 goto +0x8 <foo+0x88> is actually unreachable although LLVM generates 'goto +0x8'. So it is possible that llvm may generate 'unreachable' insn for the above insn 232. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-05-13 18:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-05-11 18:27 [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var Yonghong Song 2025-05-11 18:27 ` [PATCH bpf-next 2/2] selftests/bpf: Add a test with bpf_unreachable() kfunc Yonghong Song 2025-05-12 0:30 ` [PATCH bpf-next 1/2] bpf: Warn with new bpf_unreachable() kfunc maybe due to uninitialized var Alexei Starovoitov 2025-05-13 5:49 ` Yonghong Song 2025-05-13 14:54 ` Alexei Starovoitov 2025-05-13 18:05 ` Yonghong Song
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).