bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).