[RFC PATCH bpf-next v1 00/11] Unwinding for synchronous callbacks

[RFC PATCH bpf-next v1 00/11] Unwinding for synchronous callbacks

[Kumar Kartikeya Dwivedi]

A callback that a helper calls cannot unwind: the helper is a C frame
in between, and it holds things only its epilogue drops.

Code that unwinds due to panics or exceptions doesn't often control
where they can be invoked, and reusing the same code in callbacks would
lead to verifier rejections since unwinding across such synchronous
callbacks is not supported.

With this series, we allow a helper gets a pad of its own. A new macro
bpf_call_cb() calls the callback from an asm goto with an extable entry
at the return address that points to a label in the helper.  unwind_exit
of the callback lands there, the helper drops what it holds, and
bpf_unwind_leave() sends it on to the pad after its own call, with the
r0 the callback unwound with. The normal path pays nothing.

To the verifier such a call is a call of something that can unwind:
unwind_goto has to follow it, and the pad gets the r0 of the callback
with its state. bpf_loop(), bpf_for_each_map_elem(), bpf_find_vma(),
bpf_user_ringbuf_drain() and the comparator of bpf_rbtree_add() are
opened up one by one. A callback may unwind with what it may store into
callback_ctx already, and the fix for the pointers that don't outlive
the helper [1] will apply to r0 as well.

Tested on x86-64 with clang, with and without kCFI, and gcc for the
objects. The arm64 objects compile, but the arm64 run is left to CI.

  [1]: https://lore.kernel.org/bpf/d2369629-846e-4fd8-b567-a0fd8863256c@linux.dev/

Kumar Kartikeya Dwivedi (11):
  bpf: Give a helper a way to call its callback with a pad
  bpf, arm64: Let the pad after a helper call have r0
  bpf: Let a callback unwind into the pad of its helper
  bpf: Give the inlined bpf_loop a pad
  bpf: Let the callback of bpf_loop unwind
  bpf: Let the callback of bpf_for_each_map_elem unwind
  bpf: Let the callback of bpf_find_vma unwind
  bpf: Let the callback of bpf_user_ringbuf_drain unwind
  bpf: Let the comparator of bpf_rbtree_add unwind
  selftests/bpf: Test callbacks that unwind
  selftests/bpf: Test what a callback unwinds with

 arch/arm64/net/bpf_jit_comp.c                 |  129 +-
 arch/x86/net/bpf_jit_comp.c                   |   25 +-
 include/linux/bpf_unwind.h                    |  282 +++++
 include/linux/bpf_verifier.h                  |    6 +-
 include/linux/filter.h                        |    1 +
 kernel/bpf/arraymap.c                         |   11 +-
 kernel/bpf/backtrack.c                        |   21 +-
 kernel/bpf/bpf_iter.c                         |   32 +-
 kernel/bpf/core.c                             |   10 +
 kernel/bpf/fixups.c                           |   43 +-
 kernel/bpf/hashtab.c                          |   26 +-
 kernel/bpf/helpers.c                          |   50 +-
 kernel/bpf/liveness.c                         |    7 +
 kernel/bpf/ringbuf.c                          |   23 +-
 kernel/bpf/task_iter.c                        |   21 +-
 kernel/bpf/verifier.c                         |  162 ++-
 .../selftests/bpf/prog_tests/unwind_drain.c   |   71 ++
 .../selftests/bpf/prog_tests/verifier.c       |    2 +
 .../selftests/bpf/progs/unwind_drain.c        |   69 ++
 .../selftests/bpf/progs/verifier_unwind.c     |    3 +-
 .../selftests/bpf/progs/verifier_unwind_cb.c  | 1064 +++++++++++++++++
 21 files changed, 1974 insertions(+), 84 deletions(-)
 create mode 100644 include/linux/bpf_unwind.h
 create mode 100644 tools/testing/selftests/bpf/prog_tests/unwind_drain.c
 create mode 100644 tools/testing/selftests/bpf/progs/unwind_drain.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_unwind_cb.c


base-commit: 4169c9c90532df9585c0a2ae513337a58f5e59ab
-- 
2.53.0

[RFC PATCH bpf-next v1 01/11] bpf: Give a helper a way to call its callback with a pad

[Kumar Kartikeya Dwivedi]

A callback that bpf_loop() or another helper calls cannot unwind. The
helper is a C frame between the callback and the BPF code that called the
helper: unwind_exit of the callback finds no pad at its return address and
returns, the helper reads r0 and goes on, and the exception is lost in the
middle of the loop. The callback cannot skip the frame either, since the
helper holds things that only its epilogue drops: the mmap lock in
bpf_find_vma(), the RCU section of the bucket in the hash iterators, the
busy bit and the sample in bpf_user_ringbuf_drain().

Let the helper have a pad of its own. bpf_call_cb() calls the callback
from inline asm with an extable entry at the return address that points to
a label in the helper, and asm goto tells the compiler that control may
leave for it. unwind_exit of the callback finds the entry and lands on the
label, with the callee-saved registers and the stack as after a return.
There the helper stops, drops what it holds, and leaves the way a BPF frame
leaves at unwind_exit: bpf_unwind_leave() makes its return address the pad
after the call of the helper, which the verifier insists on. Nothing is
signalled anywhere: the callback leaves as any subprog does, and so does
the helper.

The return address of the helper sits where the compiler knows it to be,
below the canonical frame address, which __builtin_dwarf_cfa() gives as an
offset from the stack pointer. So the normal path pays nothing: no stub, no
frame set aside, no unwinder. On arm64 the frame record holds it, signed
with the stack pointer at entry, which is that same address, so the new one
is signed the same way, and written to the shadow call stack as well, which
the epilogue prefers, when it is on: a dynamic one stays off where there
is pointer authentication. clang has __builtin_dwarf_cfa() stand for the
frame pointer on arm64, so the modifier is __builtin_sponentry() there. On
x86-64 clang takes the canonical frame address through a frame pointer, so
there the helpers get one: push, mov and pop, on a kernel that otherwise
does without. That makes the pad part of the frame that the BPF code
called, or of one called with a pad in turn: a body that two kfuncs share
is inlined into both, not called by them, and a function with a pad is
noinline, so that the compiler neither inlines it into another frame nor
splits the pad, cold as it is, out of it into a part of its own, as gcc's
ipa-split otherwise does. A miss at the return address of the helper is a
bug then, and bpf_unwind_leave() warns of it, since the exception would
come back as a return.

The call itself is the one the compiler would make, kCFI check included.
The check is spelled out byte for byte, with the type id that clang emits
for the declaration of __bpf_callback_fn, negated by the assembler, the
hash where the JIT and the compiler put it, the trap in .kcfi_traps and the
target in r11: apply_fineibt() and handle_cfi_failure() take the site for
one of the compiler's. On arm64 it's the ldur, movz, movk, cmp and brk of
the compiler, with the type in w17 and the target in x15.

The entries are EX_TYPE_BPF, since only bpf_unwind_target() looks them up,
and they are keyed at the return address itself. No fault lands there as
long as the insn after bpf_call_cb() carries no fixup of its own, which in
the helpers it never does. objtool reads __ex_table as alternatives and
wants the key to be an insn, which is why it isn't ret_addr - 1 as in the
JITs. bpf_unwind_target() asks the kernel extables after the BPF ones, for
entries of that kind only, so that a fixup of some caller's own at the
return address of a main prog is not taken for a pad, and is shared with
the helpers through filter.h, with a weak default for the arches whose JIT
does no unwinding.

The pad leaves with the r0 the callback unwound with, as the pad of a BPF
caller does: r0 is an output of the asm, and the outputs of an asm goto are
valid on the way to a label as well as on the way out with every compiler
that has them and that the kernel takes, see CC_HAS_ASM_GOTO_OUTPUT. On
x86-64 that output is rax, the caller's variable. On arm64 it is x0,
which only a register variable of the macro can name, so the macro lands
at a label of its own first, assigns it there and goes on to the pad of
the helper.

The registers the call clobbers are outputs of the asm, so that none of
them is an operand. On arm64 x18 is among the clobbers unless it holds the
shadow call stack, which the callee keeps and the compiler has reserved.
That takes asm goto with outputs. Without it, or on an arch other than
x86-64 and arm64, bpf_call_cb() is the plain call and BPF_HAS_CB_PADS is 0:
the verifier keeps refusing a callback that unwinds.

Nothing uses this yet. The verifier learns next what a callback that
unwinds into such a pad means, and the helpers follow one by one, each
opened as it gets its pad.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 arch/arm64/net/bpf_jit_comp.c |  32 +++-
 arch/x86/net/bpf_jit_comp.c   |  25 ++-
 include/linux/bpf_unwind.h    | 282 ++++++++++++++++++++++++++++++++++
 include/linux/filter.h        |   1 +
 kernel/bpf/core.c             |  10 ++
 5 files changed, 340 insertions(+), 10 deletions(-)
 create mode 100644 include/linux/bpf_unwind.h

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index aaa34c739004..b8fb49b00063 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -1266,16 +1266,38 @@ static int add_unwind_exentry(struct jit_ctx *ctx, int pad)
 	return 0;
 }
 
-static long bpf_unwind_fixup(u64 lr)
+/*
+ * The verifier made sure that unwind_goto follows a call in a prog. When the
+ * kernel called an async callback, it's exit. A helper that calls a callback
+ * has a pad when it wants one, see bpf_call_cb().
+ */
+u64 bpf_unwind_target(u64 ret_addr)
 {
 	const struct exception_table_entry *e;
 
-	e = search_bpf_extables(ptrauth_strip_kernel_insn_pac(lr) - 1);
+	e = search_bpf_extables(ret_addr - 1);
+	if (e)
+		return ret_addr + e->fixup;
 	/*
-	 * The verifier made sure that unwind_goto follows a call in a prog. When
-	 * the kernel called an async callback, it's exit.
+	 * The pad of a helper that called a callback with bpf_call_cb(): its
+	 * entries are the EX_TYPE_BPF ones of the kernel and module tables.
+	 * Anything else at a return address is some caller's own fixup, and
+	 * what a JIT keyed there is a fixup of the insn after the call.
 	 */
-	return e ? e->fixup : 0;
+	e = search_kernel_exception_table(ret_addr);
+	if (!e)
+		e = search_module_extables(ret_addr);
+	if (e && e->type == EX_TYPE_BPF)
+		return (unsigned long)&e->fixup + e->fixup;
+	return ret_addr;
+}
+
+/* The offset that the epilogue adds to LR once it is authenticated */
+static long bpf_unwind_fixup(u64 lr)
+{
+	u64 ret_addr = ptrauth_strip_kernel_insn_pac(lr);
+
+	return bpf_unwind_target(ret_addr) - ret_addr;
 }
 
 /*
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 6a2fe3d0039d..a092a84157a9 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -2546,15 +2546,30 @@ static int add_unwind_exentry(struct bpf_prog *prog, u8 *image, u8 *rw_image, in
 	return 0;
 }
 
-static u64 bpf_unwind_target(u64 ret_addr)
+/*
+ * The verifier made sure that unwind_goto follows a call in a prog. When the
+ * kernel called an async callback, it's exit. A helper that calls a callback
+ * has a pad when it wants one, see bpf_call_cb().
+ */
+u64 bpf_unwind_target(u64 ret_addr)
 {
-	const struct exception_table_entry *e = search_bpf_extables(ret_addr - 1);
+	const struct exception_table_entry *e;
 
+	e = search_bpf_extables(ret_addr - 1);
+	if (e)
+		return ret_addr + e->fixup;
 	/*
-	 * The verifier made sure that unwind_goto follows a call in a prog. When
-	 * the kernel called an async callback, it's exit.
+	 * The pad of a helper that called a callback with bpf_call_cb(): its
+	 * entries are the EX_TYPE_BPF ones of the kernel and module tables.
+	 * Anything else at a return address is some caller's own fixup, and
+	 * what a JIT keyed there is a fixup of the insn after the call.
 	 */
-	return e ? ret_addr + e->fixup : ret_addr;
+	e = search_kernel_exception_table(ret_addr);
+	if (!e)
+		e = search_module_extables(ret_addr);
+	if (e && (e->data & EX_DATA_TYPE_MASK) == EX_TYPE_BPF)
+		return (unsigned long)&e->fixup + e->fixup;
+	return ret_addr;
 }
 
 /* unwind_exit: fix the return address of this frame. r0 and r2 are returned. */
diff --git a/include/linux/bpf_unwind.h b/include/linux/bpf_unwind.h
new file mode 100644
index 000000000000..190d6dbae856
--- /dev/null
+++ b/include/linux/bpf_unwind.h
@@ -0,0 +1,282 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_BPF_UNWIND_H
+#define _LINUX_BPF_UNWIND_H
+
+/*
+ * A helper that calls a callback is a C frame between the callback and the BPF
+ * code that called the helper. unwind_exit of the callback returns to the pad of
+ * its caller when the caller has one, so a helper lets its callback unwind by
+ * having one: bpf_call_cb() calls the callback with an extable entry at the
+ * return address that points to a label in the helper. There the helper drops
+ * what it holds and leaves the way a BPF frame leaves at unwind_exit, with
+ * bpf_unwind_leave(): its return address becomes the pad of its caller, which
+ * the verifier insists on, and r0 is what the callback unwound with, since the
+ * output of the call is valid on both edges. The slot that holds the return
+ * address is where the compiler knows it to be, at the canonical frame address
+ * of the helper, so nothing is looked up and nothing is saved beforehand. That
+ * makes the pad part of the frame that the BPF code called, or of one called
+ * with a pad in turn: a body that two entry points share is inlined into both,
+ * not called by them, and a function with a pad is noinline, so that the
+ * compiler neither inlines it into another frame nor splits the pad, cold as it
+ * is, out of it into a part of its own, as gcc's ipa-split otherwise does.
+ *
+ * The call is the one the compiler would make, kCFI check included: against
+ * the type of a callback, or, with __bpf_call_cb(), against the type of a
+ * function declared with BPF_CB_PAD_TYPE(). The entry is EX_TYPE_BPF, keyed at
+ * the return address: only bpf_unwind_target() looks it up, and takes no other
+ * kind from the kernel tables, and no fault lands on an insn after a call of
+ * ours. For that the insn after bpf_call_cb() must not carry a fixup of its
+ * own, which in the helpers it never does. objtool reads __ex_table and wants
+ * the key to be an insn, which is why it isn't ret_addr - 1 as in the JITs.
+ *
+ * The outputs of an asm goto are valid on the way to a label as well as on the
+ * way out with every compiler that has them and that the kernel takes, see
+ * CC_HAS_ASM_GOTO_OUTPUT. So r0 is what the callback left on either edge.
+ * Without asm goto outputs, or on an arch whose JIT does no unwinding, the
+ * callback is called the plain way and BPF_HAS_CB_PADS tells the verifier that
+ * a callback cannot unwind.
+ */
+
+#include <linux/bug.h>
+#include <linux/types.h>
+#include <linux/filter.h>
+#include <linux/stringify.h>
+
+#if defined(CONFIG_X86_64) && defined(CONFIG_CC_HAS_ASM_GOTO_OUTPUT)
+
+#include <asm/asm.h>
+#include <asm/extable_fixup_types.h>
+#include <asm/nospec-branch.h>
+
+#define BPF_HAS_CB_PADS 1
+
+#ifdef CONFIG_CFI
+/*
+ * The kCFI check of the compiler, byte for byte: the hash where the compiler
+ * and the JIT put it, the trap in .kcfi_traps, the target in r11 for the
+ * FineIBT form. apply_fineibt() and handle_cfi_failure() take the site for one
+ * of the compiler's. The type id is the symbol clang emits for a declaration
+ * of that type once its address is taken, which BPF_CB_PAD_TYPE() sees to, and
+ * the assembler negates it, since the symbol is absolute.
+ */
+#define BPF_CB_PAD_TYPE(name, type)						\
+	extern type name;							\
+	__ADDRESSABLE(name)
+
+#ifdef CONFIG_CALL_PADDING
+#define BPF_KCFI_HASH_OFF	(4 + CONFIG_FUNCTION_PADDING_CFI)
+#else
+#define BPF_KCFI_HASH_OFF	4
+#endif
+
+#define BPF_KCFI_CHECK(type)							\
+	"	movl $-__kcfi_typeid_" #type ", %%r10d\n"			\
+	"	addl -" __stringify(BPF_KCFI_HASH_OFF) "(%%r11), %%r10d\n"	\
+	"	je 2f\n"							\
+	"3:	ud2\n"								\
+	"	.pushsection .kcfi_traps, \"ao\", @progbits, .text\n"		\
+	"	.long 3b - .\n"							\
+	"	.popsection\n"							\
+	"2:\n"
+#else
+#define BPF_CB_PAD_TYPE(name, type)
+#define BPF_KCFI_CHECK(type)
+#endif
+
+/*
+ * The registers the call clobbers are outputs, so that none of them can be an
+ * operand of anything else. r0 is rax, on the way out and on the way to the pad.
+ * The return address stays the byte after the call through what
+ * apply_retpolines() and apply_fineibt() make of the call: they pad in front of
+ * it, never behind it.
+ */
+#define __bpf_call_cb(type, fn, a1, a2, a3, a4, a5, r0, unwound)		\
+do {										\
+	register u64 __fn asm("r11") = (u64)(long)(fn);				\
+	register u64 __a5 asm("r8") = (u64)(long)(a5);				\
+	register u64 __c5 asm("r8");						\
+	register u64 __c11 asm("r11");						\
+	u64 __c1, __c2, __c3, __c4;						\
+										\
+	asm_goto_output(BPF_KCFI_CHECK(type)					\
+		 "	" CALL_NOSPEC "\n"					\
+		 "1:\n"								\
+		 _ASM_EXTABLE_TYPE(1b, %l[unwound], EX_TYPE_BPF)		\
+		 : "=a"(r0), "=D"(__c1), "=S"(__c2), "=d"(__c3), "=c"(__c4),	\
+		   "=r"(__c5), "=r"(__c11)					\
+		 : [thunk_target] "r"(__fn), "D"((u64)(long)(a1)),		\
+		   "S"((u64)(long)(a2)), "d"((u64)(long)(a3)),			\
+		   "c"((u64)(long)(a4)), "r"(__a5)				\
+		 : "r9", "r10", "memory", "cc"					\
+		 : unwound);							\
+} while (0)
+
+/*
+ * The return address of the helper is the word below its canonical frame
+ * address. The pad is in the frame that BPF code called, see above, so there is
+ * a pad to go to: a miss is a bug, and the exception would be lost in a return.
+ */
+static __always_inline void bpf_unwind_leave(void)
+{
+	unsigned long *slot = (unsigned long *)__builtin_dwarf_cfa() - 1;
+	unsigned long to = bpf_unwind_target(*slot);
+
+	WARN_ON_ONCE(to == *slot);
+	*slot = to;
+}
+
+#elif defined(CONFIG_ARM64) && defined(CONFIG_CC_HAS_ASM_GOTO_OUTPUT)
+
+#include <linux/scs.h>
+#include <asm/asm-extable.h>
+#include <asm/brk-imm.h>
+#include <asm/compiler.h>
+
+#define BPF_HAS_CB_PADS 1
+
+#ifdef CONFIG_CFI
+/*
+ * The kCFI check of the compiler: the type id comes in w17, the target is in
+ * x15, and the brk says so. The hash of the callee is the word before it.
+ */
+#define BPF_CB_PAD_TYPE(name, type)						\
+	extern type name;							\
+	__ADDRESSABLE(name)
+
+#define BPF_KCFI_CHECK(type)							\
+	"	ldur w16, [x15, #-4]\n"						\
+	"	movz w17, #:abs_g0_nc:__kcfi_typeid_" #type "\n"		\
+	"	movk w17, #:abs_g1_nc:__kcfi_typeid_" #type "\n"		\
+	"	cmp w16, w17\n"							\
+	"	b.eq 2f\n"							\
+	"	brk #" __stringify((CFI_BRK_IMM_BASE | (17 << 5) | 15)) "\n"	\
+	"2:\n"
+#else
+#define BPF_CB_PAD_TYPE(name, type)
+#define BPF_KCFI_CHECK(type)
+#endif
+
+/*
+ * With a shadow call stack x18 holds it, the callee keeps it, and the compiler
+ * has it reserved. Without one it's a temporary like the rest.
+ */
+#ifdef CONFIG_SHADOW_CALL_STACK
+#define BPF_CB_CLOBBER_X18
+#else
+#define BPF_CB_CLOBBER_X18	"x18",
+#endif
+
+/*
+ * r0 is x0, which only a register variable of the macro can name. So the macro
+ * lands at a label of its own, assigns it there as on the way out, and goes on
+ * to the pad.
+ */
+#define __bpf_call_cb(type, fn, a1, a2, a3, a4, a5, r0, unwound)		\
+	___bpf_call_cb(__UNIQUE_ID(unwound), type, fn, a1, a2, a3, a4, a5, r0, unwound)
+
+#define ___bpf_call_cb(lbl, type, fn, a1, a2, a3, a4, a5, r0, unwound)		\
+do {										\
+	register u64 __fn asm("x15") = (u64)(long)(fn);				\
+	register u64 __a0 asm("x0") = (u64)(long)(a1);				\
+	register u64 __a1 asm("x1") = (u64)(long)(a2);				\
+	register u64 __a2 asm("x2") = (u64)(long)(a3);				\
+	register u64 __a3 asm("x3") = (u64)(long)(a4);				\
+	register u64 __a4 asm("x4") = (u64)(long)(a5);				\
+	register u64 __c0 asm("x0");						\
+	register u64 __c1 asm("x1");						\
+	register u64 __c2 asm("x2");						\
+	register u64 __c3 asm("x3");						\
+	register u64 __c4 asm("x4");						\
+	register u64 __c15 asm("x15");						\
+										\
+	asm_goto_output(BPF_KCFI_CHECK(type)					\
+		 "	blr x15\n"						\
+		 "1:\n"								\
+		 __ASM_EXTABLE_RAW("1b", "%l[" __stringify(lbl) "]",		\
+				   __stringify(EX_TYPE_BPF), "0")			\
+		 : "=r"(__c0), "=r"(__c1), "=r"(__c2), "=r"(__c3), "=r"(__c4),	\
+		   "=r"(__c15)							\
+		 : "r"(__fn), "r"(__a0), "r"(__a1), "r"(__a2), "r"(__a3),	\
+		   "r"(__a4)							\
+		 : "x5", "x6", "x7", "x8", "x9", "x10", "x11", "x12", "x13",	\
+		   "x14", "x16", "x17", BPF_CB_CLOBBER_X18 "x30", "memory", "cc"	\
+		 : lbl);							\
+	if (0) {								\
+lbl:		(r0) = __c0;							\
+		goto unwound;							\
+	}									\
+	(r0) = __c0;								\
+} while (0)
+
+/*
+ * The return address of the helper is the second word of its frame record. The
+ * pad is in the frame that BPF code called, see above, so there is a pad to go
+ * to: a miss is a bug, and the exception would be lost in a return.
+ *
+ * With pointer authentication the return address is signed with the stack
+ * pointer at entry, which is the canonical frame address, so the new one is
+ * signed the same way; pacia1716 is a hint, a nop without pointer
+ * authentication, like paciasp. clang has __builtin_dwarf_cfa() stand for the
+ * frame pointer on arm64 and __builtin_sponentry() for the stack pointer at
+ * entry. The shadow call stack holds a copy, which the epilogue prefers, when
+ * it is on: a dynamic one stays off where there is pointer authentication.
+ */
+static __always_inline void bpf_unwind_leave(void)
+{
+	unsigned long *slot = (unsigned long *)__builtin_frame_address(0) + 1;
+	unsigned long from = ptrauth_strip_kernel_insn_pac(*slot);
+	unsigned long to = bpf_unwind_target(from);
+
+	WARN_ON_ONCE(to == from);
+#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
+	{
+		register unsigned long x17 asm("x17") = to;
+#ifdef __clang__
+		register unsigned long x16 asm("x16") = (unsigned long)__builtin_sponentry();
+#else
+		register unsigned long x16 asm("x16") = (unsigned long)__builtin_dwarf_cfa();
+#endif
+
+		asm("pacia1716" : "+r"(x17) : "r"(x16));
+		to = x17;
+	}
+#endif
+	*slot = to;
+#ifdef CONFIG_SHADOW_CALL_STACK
+	if (scs_is_enabled()) {
+		unsigned long *scs;
+
+		asm("mov %0, x18" : "=r"(scs));
+		scs[-1] = to;
+	}
+#endif
+}
+
+#else
+
+#define BPF_HAS_CB_PADS 0
+
+#define BPF_CB_PAD_TYPE(name, type)
+
+#define __bpf_call_cb(type, fn, a1, a2, a3, a4, a5, r0, unwound)		\
+do {										\
+	(r0) = ((bpf_callback_t)(void *)(fn))((u64)(long)(a1), (u64)(long)(a2),	\
+					      (u64)(long)(a3), (u64)(long)(a4),	\
+					      (u64)(long)(a5));			\
+	if (0)									\
+		goto unwound;							\
+} while (0)
+
+static __always_inline void bpf_unwind_leave(void)
+{
+}
+
+#endif
+
+/* A callback: bpf_callback_t */
+BPF_CB_PAD_TYPE(__bpf_callback_fn, typeof(*(bpf_callback_t)0));
+
+#define bpf_call_cb(fn, a1, a2, a3, a4, a5, r0, unwound)			\
+	__bpf_call_cb(__bpf_callback_fn, fn, a1, a2, a3, a4, a5, r0, unwound)
+
+#endif /* _LINUX_BPF_UNWIND_H */
diff --git a/include/linux/filter.h b/include/linux/filter.h
index e0218e677a1d..9f670b3d8375 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1281,6 +1281,7 @@ bool bpf_jit_supports_kfunc_call(void);
 bool bpf_jit_supports_callx(void);
 bool bpf_jit_supports_kfunc_ret_reg_pair(void);
 bool bpf_jit_supports_unwind(void);
+u64 bpf_unwind_target(u64 ret_addr);
 bool bpf_jit_supports_stack_args(void);
 bool bpf_jit_supports_arena_args(void);
 bool bpf_jit_supports_far_kfunc_call(void);
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 3030beb531d9..1096b2adc695 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -3405,6 +3405,16 @@ bool __weak bpf_jit_supports_unwind(void)
 	return false;
 }
 
+/*
+ * Where unwind_exit returns: to the pad of the caller when the caller has one,
+ * and where exit would otherwise. The JITs that support unwinding have the real
+ * one. Without it no callback unwinds, so a helper never asks.
+ */
+u64 __weak bpf_unwind_target(u64 ret_addr)
+{
+	return ret_addr;
+}
+
 /*
  * How this arch places a by-value kfunc argument, or NULL for one that has
  * not opted in and so only takes an argument of a single eightbyte, which
-- 
2.53.0

[RFC PATCH bpf-next v1 02/11] bpf, arm64: Let the pad after a helper call have r0

[Kumar Kartikeya Dwivedi]

The JIT emits 'bl foo; mov x8, x0' for a call, and the extable entry of
the unwind_goto that follows sends unwind_exit of the callee past the
mov, into the pad. That is fine after the call of a subprog: r0 is x8 in
the subprog too, its epilogue copies x8 to x0 and leaves x8 as it is. A
helper that leaves to the pad from a pad of its own, see
bpf_unwind_leave(), returns r0 in x0 alone, and x8 is whatever it last
used it for.

So the pad after the call of a helper or a kfunc is entered through a
landing of its own, 'mov x8, x0; b pad', past the epilogue where nothing
falls into it, and the entry of the unwind_goto points there. The
landings go in after the body, when the pads are placed and the entries
are in, and each fills in the fixup of its entry. The normal path has
nothing in its way: unwind_goto emits nothing, as before. The pad after
the call of a subprog is as it was. x86-64 needs nothing: r0 is rax.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 arch/arm64/net/bpf_jit_comp.c | 97 +++++++++++++++++++++++++++++++----
 1 file changed, 86 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index b8fb49b00063..284c1dd791ad 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -81,6 +81,8 @@ struct jit_ctx {
 	int epilogue_offset;
 	int *offset;
 	int exentry_idx;
+	int *landing_ex;	/* the extable entry of each landing, see build_landings() */
+	int landing_idx;
 	int nr_used_callee_reg;
 	u8 used_callee_reg[8]; /* r6~r9, fp, arena_vm_start */
 	__le32 *image;
@@ -1227,19 +1229,24 @@ static int add_exception_handler(const struct bpf_insn *insn,
 	return 0;
 }
 
+/* unwind_goto after the call of a helper or a kfunc, which return r0 in x0 alone */
+static bool unwind_goto_after_helper(const struct bpf_insn *insn)
+{
+	const struct bpf_insn *call = insn - 1;
+
+	return call->code == (BPF_JMP | BPF_CALL) && call->src_reg != BPF_PSEUDO_CALL;
+}
+
 /*
  * There is no branch for unwind_goto, only an extable entry
- *   { .insn = ret_addr - 1, .fixup = pad - ret_addr }
- * where ret_addr is the return address of the call in front of it. unwind_exit
- * adds .fixup to its LR.
+ *   { .insn = ret_addr - 1, .fixup = target - ret_addr }
+ * where ret_addr is the return address of the call in front of it, and target
+ * the pad, or the way into it. unwind_exit adds .fixup to its LR.
  * ret_addr - 1, because what follows the call can have an entry of its own.
  * No insn starts there, so a fault cannot hit the entry.
- * 'bl foo; mov x8, x0' is in front of unwind_goto. The pad doesn't miss the mov:
- * r0 is x8 and the epilogue of the callee copies it to x0 and leaves x8 as it is.
  */
-static int add_unwind_exentry(struct jit_ctx *ctx, int pad)
+static int add_unwind_exentry(struct jit_ctx *ctx, int ret_addr, int target)
 {
-	int ret_addr = ctx->idx - 1;
 	struct exception_table_entry *ex;
 	off_t ins_offset;
 
@@ -1259,13 +1266,47 @@ static int add_unwind_exentry(struct jit_ctx *ctx, int pad)
 	/* switch ex to rw buffer for writing */
 	ex = (void *)ctx->image + ((void *)ex - (void *)ctx->ro_image);
 	ex->insn = ins_offset;
-	ex->fixup = (ctx->offset[pad] - ret_addr) * AARCH64_INSN_SIZE;
+	ex->fixup = (target - ret_addr) * AARCH64_INSN_SIZE;
 	ex->type = EX_TYPE_BPF;
 
 	ctx->exentry_idx++;
 	return 0;
 }
 
+/*
+ * The landings of the pads after the calls of helpers and kfuncs, past the
+ * epilogue, where nothing falls into them: 'mov x8, x0; b pad' each, see the
+ * unwind_goto case of build_insn(). They go in after the body, when the pads
+ * are placed and the entries are in, and each fills in the fixup of its entry.
+ */
+static int build_landings(struct jit_ctx *ctx)
+{
+	const struct bpf_prog *prog = ctx->prog;
+	int i, k = 0;
+
+	for (i = 1; i < prog->len; i++) {
+		const struct bpf_insn *insn = &prog->insnsi[i];
+		int ret_addr, jmp_offset;
+
+		if (!bpf_is_unwind_goto(insn) || !unwind_goto_after_helper(insn))
+			continue;
+		ret_addr = ctx->offset[i] - 1;
+		if (ctx->image) {
+			struct exception_table_entry *ex;
+
+			ex = &prog->aux->extable[ctx->landing_ex[k]];
+			ex = (void *)ctx->image + ((void *)ex - (void *)ctx->ro_image);
+			ex->fixup = (ctx->idx - ret_addr) * AARCH64_INSN_SIZE;
+		}
+		k++;
+		emit(A64_MOV(1, bpf2a64[BPF_REG_0], A64_R(0)), ctx);
+		jmp_offset = ctx->offset[i + 1 + insn->off] - ctx->idx;
+		check_imm26(jmp_offset);
+		emit(A64_B(jmp_offset), ctx);
+	}
+	return 0;
+}
+
 /*
  * The verifier made sure that unwind_goto follows a call in a prog. When the
  * kernel called an async callback, it's exit. A helper that calls a callback
@@ -1901,10 +1942,27 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
 		break;
 	/* unwind_goto. The verifier expanded may_goto. */
 	case BPF_JMP | BPF_JCOND:
-		ret = add_unwind_exentry(ctx, i + 1 + off);
+	{
+		int ret_addr = ctx->idx - 1;
+		int target = ctx->offset[i + 1 + off];
+
+		/*
+		 * 'bl foo; mov x8, x0' is in front. After the call of a subprog
+		 * the pad doesn't miss the mov: r0 is x8 in the subprog too, its
+		 * epilogue copies x8 to x0 and leaves x8 as it is. A helper that
+		 * leaves to the pad returns r0 in x0 alone, so the pad after its
+		 * call is entered through a landing past the epilogue, see
+		 * build_landings(), which fills in the fixup of the entry.
+		 */
+		if (unwind_goto_after_helper(insn)) {
+			ctx->landing_ex[ctx->landing_idx++] = ctx->exentry_idx;
+			target = ret_addr;
+		}
+		ret = add_unwind_exentry(ctx, ret_addr, target);
 		if (ret)
 			return ret;
 		break;
+	}
 	/* function return */
 	case BPF_JMP | BPF_EXIT:
 		if (bpf_is_unwind_exit(insn) && bpf_prog_can_unwind(ctx->prog)) {
@@ -2342,7 +2400,9 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 	u8 *ro_image_ptr;
 	int body_idx;
 	int exentry_idx;
+	int nr_landings = 0;
 	int out_cnt;
+	int i;
 
 	if (!prog->jit_requested)
 		return prog;
@@ -2387,6 +2447,16 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 	if (ctx.offset == NULL)
 		goto out_off;
 
+	for (i = 1; i < prog->len; i++)
+		if (bpf_is_unwind_goto(&prog->insnsi[i]) &&
+		    unwind_goto_after_helper(&prog->insnsi[i]))
+			nr_landings++;
+	if (nr_landings) {
+		ctx.landing_ex = kvzalloc_objs(int, nr_landings);
+		if (ctx.landing_ex == NULL)
+			goto out_off;
+	}
+
 	if (bpf_prog_uses_arena_insns(prog)) {
 		ctx.user_vm_start = bpf_arena_get_user_vm_start(prog->aux->arena);
 		ctx.arena_vm_start = bpf_arena_get_kern_vm_start(prog->aux->arena);
@@ -2416,6 +2486,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 
 	ctx.epilogue_offset = ctx.idx;
 	build_epilogue(&ctx, was_classic, false);
+	if (build_landings(&ctx))
+		goto out_off;
 	build_plt(&ctx);
 
 	extable_align = __alignof__(struct exception_table_entry);
@@ -2449,6 +2521,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 skip_init_ctx:
 	ctx.idx = 0;
 	ctx.exentry_idx = 0;
+	ctx.landing_idx = 0;
 	ctx.write = true;
 
 	build_prologue(&ctx, was_classic);
@@ -2464,6 +2537,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 
 	ctx.epilogue_offset = ctx.idx;
 	ctx.exentry_idx = exentry_idx;
+	ctx.landing_idx = 0;
 	ctx.idx = body_idx;
 	ctx.write = true;
 
@@ -2473,6 +2547,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 		goto out_free_hdr;
 
 	build_epilogue(&ctx, was_classic, false);
+	if (build_landings(&ctx))
+		goto out_free_hdr;
 	build_plt(&ctx);
 
 	/* Extra pass to validate JITed code. */
@@ -2514,8 +2590,6 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 	prog->jited_len = prog_size - cfi_get_offset();
 
 	if (!prog->is_func || extra_pass) {
-		int i;
-
 		/* offset[prog->len] is the size of program */
 		for (i = 0; i <= prog->len; i++)
 			ctx.offset[i] *= AARCH64_INSN_SIZE;
@@ -2533,6 +2607,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 			prog->aux->priv_stack_ptr = NULL;
 		}
 		kvfree(ctx.offset);
+		kvfree(ctx.landing_ex);
 out_priv_stack:
 		kfree(jit_data);
 		prog->aux->jit_data = NULL;
-- 
2.53.0

[RFC PATCH bpf-next v1 03/11] bpf: Let a callback unwind into the pad of its helper

[Kumar Kartikeya Dwivedi]

A helper that calls a callback is a C frame between the callback and the
BPF code that called the helper, and with bpf_call_cb() it has a pad: the
unwind_exit of the callback lands there, the helper drops what it holds
and leaves to the pad after its call with the r0 the callback unwound
with. To the verifier the helper is transparent on that path as it is on
the normal one: it models the call of a helper with a callback as the
callback running in a frame of its own, and a callback that unwinds now
leaves that frame the way a subprog does.

So the call of a helper or a kfunc with a callback that can unwind is a
call of something that can unwind: unwind_goto may follow it and has to
when the callback has unwind_exit, the same rule as for a subprog. At
unwind_exit of the callback the verifier goes back to the call once more,
as it does after a round of the callback, since the helper has effects of
its own on its way to the pad: bpf_rbtree_add() releases the node
whichever way it leaves. With the state marked unwound there is no further
round of the callback, the next call with a callback in the frame starts
from its first round, and the next insn is the unwind_goto. A callback
that returns on one path to the call and unwinds on another is fine: the
pad is there for whoever takes it.

The pad gets the state of the r0 of the callback, as the pad after a
subprog call does. The return of a callback is a scalar in a range because
the helper acts on it; what the callback unwinds with, the helper passes
on bit for bit and doesn't look at. So a known value stays known, a
pointer stays the pointer it was, with its type and its reference, and
what the pad does with it is checked there and in every pad above that
unwinds on with it, up to the main prog, whose return has to be a number.
A pointer into the stack of the callback is refused as at any return.
What the callback may leave in r0 is what it may store through
callback_ctx already. That includes pointers that don't outlive the helper:
the dynptr of bpf_user_ringbuf_drain() and the key of the array iterator
live in the frame of the helper, the vma of bpf_find_vma() is good under
the lock the pad drops. The verifier lets the store through today, and the
fix for that has to see to r0 too.

The effects of the helper on its way to the pad apply to that r0 like to
any register: on the revisit of the call, check_helper_call() and
check_kfunc_call() keep r0 across the scrub of the caller-saved registers
and skip the return. Two passes see r0 flow through the call. Liveness:
r0 is live before a call with a callback and a pad, since the revisit
passes it on, so that two unwound states at the call with a different r0
aren't one. Precision: the history entry of the revisit is marked
INSN_F_UNWOUND, so backtracking doesn't take r0 for the helper's there,
and unwind_exit of a callback hands the request on to the callback the way
exit of a subprog does.

No helper has a pad yet: "callback cannot unwind" stays until a helper is
opened, which the following patches do one by one, where BPF_HAS_CB_PADS.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf_verifier.h |   6 +-
 kernel/bpf/backtrack.c       |  21 +++++-
 kernel/bpf/liveness.c        |   7 ++
 kernel/bpf/verifier.c        | 138 ++++++++++++++++++++++++++++++-----
 4 files changed, 149 insertions(+), 23 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 5926f13ff43c..8b4d4d6981f3 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -434,6 +434,8 @@ enum {
 	INSN_F_SRC_REG_STACK = BIT(2), /* src_reg is PTR_TO_STACK */
 
 	INSN_F_STACK_ARG_ACCESS = BIT(3),
+
+	INSN_F_UNWOUND = BIT(4), /* the callback of this call unwound: r0 is the callback's */
 };
 
 /* Registers linked to one jump condition that a history entry can record */
@@ -444,8 +446,8 @@ struct bpf_jmp_history_entry {
 	u32 idx : 20;
 	u32 frame : 4;	/* stack access frame number */
 	/* special INSN_F_xxx flags */
-	u32 flags : 4;
-	u32 : 4;
+	u32 flags : 5;
+	u32 : 3;
 	u32 prev_idx : 20;
 	u32 spi : 12;	/* stack slot index */
 	/*
diff --git a/kernel/bpf/backtrack.c b/kernel/bpf/backtrack.c
index 43d5740034c0..ce171be5e754 100644
--- a/kernel/bpf/backtrack.c
+++ b/kernel/bpf/backtrack.c
@@ -530,8 +530,13 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 			 */
 			if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL && insn->imm == 0)
 				return -ENOTSUPP;
-			/* regular helper call sets R0 */
-			bt_clear_reg(bt, BPF_REG_0);
+			/*
+			 * A regular helper call sets R0. When its callback unwound,
+			 * R0 is the callback's, from the unwind_exit of the callback,
+			 * see the BPF_EXIT case.
+			 */
+			if (!hist || !(hist->flags & INSN_F_UNWOUND))
+				bt_clear_reg(bt, BPF_REG_0);
 			/* kfunc might also set R2 */
 			bt_clear_reg(bt, BPF_REG_2);
 			if (bt_reg_mask(bt) & BPF_REGMASK_ARGS) {
@@ -548,7 +553,7 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 					return -EFAULT;
 			}
 		} else if (opcode == BPF_EXIT) {
-			bool from_subprog_call, r0_precise, r2_precise;
+			bool from_subprog_call, cb_unwinds, r0_precise, r2_precise;
 
 			/* BPF_EXIT in subprog or callback always returns
 			 * right after the call instruction, so by checking
@@ -562,8 +567,16 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 			from_subprog_call = subseq_idx - 1 >= 0 &&
 					    (bpf_pseudo_call(&env->prog->insnsi[subseq_idx - 1]) ||
 					     bpf_is_callx(&env->prog->insnsi[subseq_idx - 1]));
+			/*
+			 * unwind_exit of a callback goes back to the call of the
+			 * helper, which passes R0 on to the pad after it: R0 is
+			 * the callback's, as it is the subprog's after a call.
+			 */
+			cb_unwinds = !from_subprog_call && bpf_is_unwind_exit(insn) &&
+				     subseq_idx >= 0 &&
+				     bpf_is_sync_callback_calling_insn(&env->prog->insnsi[subseq_idx]);
 
-			r0_precise = from_subprog_call && bt_is_reg_set(bt, BPF_REG_0);
+			r0_precise = (from_subprog_call || cb_unwinds) && bt_is_reg_set(bt, BPF_REG_0);
 			r2_precise = from_subprog_call && bt_is_reg_set(bt, BPF_REG_2);
 
 			/* Backtracking to a nested function call, 'idx' is a part of
diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c
index e657e6d95408..22225d1a0fe7 100644
--- a/kernel/bpf/liveness.c
+++ b/kernel/bpf/liveness.c
@@ -2637,6 +2637,13 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env,
 			/* callx reads the address of the callee from dst_reg */
 			if (bpf_is_callx(insn))
 				use |= dst;
+			/*
+			 * When the callback unwinds, the revisit of the call passes
+			 * the r0 of the callback on to the pad after the call, see
+			 * check_helper_call(): live before the call.
+			 */
+			if (bpf_is_sync_callback_calling_insn(insn) && bpf_is_unwind_goto(insn + 1))
+				use |= r0;
 			break;
 		default:
 			def = 0;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5601d2056218..ac9dbe082181 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10,6 +10,7 @@
 #include <linux/types.h>
 #include <linux/slab.h>
 #include <linux/bpf.h>
+#include <linux/bpf_unwind.h>
 #include <linux/btf.h>
 #include <linux/bpf_verifier.h>
 #include <linux/filter.h>
@@ -3435,6 +3436,8 @@ static int next_callx_callee(struct bpf_verifier_env *env, int caller, int from)
  * 'call foo; unwind_goto pad' goes to the pad when foo left with unwind_exit.
  * JITs don't emit a branch for it. unwind_exit finds the pad by its return
  * address. So unwind_goto has to follow the call and nothing may jump to it.
+ * The call may be of a helper or a kfunc that calls a callback: it leaves like
+ * the callback did, see check_cb_unwind().
  */
 static int check_unwind_insns(struct bpf_verifier_env *env)
 {
@@ -3450,8 +3453,10 @@ static int check_unwind_insns(struct bpf_verifier_env *env)
 		if (!bpf_is_unwind_goto(insn))
 			continue;
 		seen = true;
-		if (!i || !(bpf_pseudo_call(insn - 1) || bpf_is_callx(insn - 1))) {
-			verbose(env, "unwind_goto at insn %d doesn't follow a call of a subprog\n",
+		if (!i || !(bpf_pseudo_call(insn - 1) || bpf_is_callx(insn - 1) ||
+			    bpf_is_sync_callback_calling_insn(insn - 1))) {
+			verbose(env,
+				"unwind_goto at insn %d doesn't follow a call of a subprog or of a helper with a callback\n",
 				i);
 			return -EINVAL;
 		}
@@ -11397,6 +11402,55 @@ static int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog)
 	return err;
 }
 
+/* The helper calls its callback with a pad, see bpf_call_cb(). None yet. */
+static bool helper_has_cb_pad(enum bpf_func_id func_id)
+{
+	return false;
+}
+
+/* The kfunc calls its callback with a pad. None yet. */
+static bool kfunc_has_cb_pad(u32 btf_id)
+{
+	return false;
+}
+
+static bool insn_has_cb_pad(struct bpf_insn *insn)
+{
+	return (bpf_helper_call(insn) && helper_has_cb_pad(insn->imm)) ||
+	       (bpf_pseudo_kfunc_call(insn) && kfunc_has_cb_pad(insn->imm));
+}
+
+/*
+ * A sync callback is called by a helper or a kfunc, a C frame between the
+ * callback and the BPF code that called it. unwind_exit of the callback returns
+ * to the pad of its caller when there is one, and a helper or a kfunc that
+ * calls a callback has one where BPF_HAS_CB_PADS and insn_has_cb_pad(): it
+ * calls the callback with bpf_call_cb(), drops what it holds when the callback
+ * unwinds and leaves to the pad of its own caller with the r0 of the callback.
+ * So a call with such a callback that can unwind is a call of something that
+ * can unwind, and unwind_goto has to follow it.
+ */
+static int check_cb_unwind(struct bpf_verifier_env *env, struct bpf_insn *insn,
+			   int insn_idx, int subprog)
+{
+	if (!env->subprog_info[subprog].can_unwind)
+		return 0;
+	if (!BPF_HAS_CB_PADS || !insn_has_cb_pad(insn)) {
+		verbose(env, "callback cannot unwind\n");
+		return -EINVAL;
+	}
+	if (!bpf_is_unwind_goto(insn + 1)) {
+		if (bpf_helper_call(insn))
+			verbose(env, "no unwind_goto after the call of %s()\n",
+				func_id_name(insn->imm));
+		else
+			verbose(env, "no unwind_goto after the call of the kfunc at insn %d\n",
+				insn_idx);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			      int insn_idx, int subprog,
 			      set_callee_state_fn set_callee_state_cb)
@@ -11406,6 +11460,15 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins
 	int err;
 
 	caller = state->frame[state->curframe];
+	/*
+	 * The callback unwound, and the call is visited once more for the
+	 * effects of the helper, see prepare_func_exit(): no more rounds, and
+	 * the next call with a callback in this frame starts from its first.
+	 */
+	if (state->unwound) {
+		caller->callback_depth = 0;
+		return 0;
+	}
 	err = btf_check_subprog_call(env, subprog);
 	if (err == -EFAULT)
 		return err;
@@ -11459,6 +11522,10 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins
 	if (err)
 		return err;
 
+	err = check_cb_unwind(env, insn, insn_idx, subprog);
+	if (err)
+		return err;
+
 	/* for callback functions enqueue entry to callback and
 	 * proceed with next instruction within current frame.
 	 */
@@ -12090,7 +12157,7 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
 	struct bpf_verifier_state *state = env->cur_state, *prev_st;
 	struct bpf_func_state *caller, *callee;
 	struct bpf_reg_state *r0;
-	bool in_callback_fn;
+	bool in_callback_fn, unwinds;
 	u32 i, nregs;
 	int err;
 
@@ -12111,12 +12178,16 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
 	}
 
 	caller = state->frame[state->curframe - 1];
-	if (bpf_is_unwind_exit(&env->prog->insnsi[*insn_idx])) {
-		/* the helper or the kfunc that called it doesn't expect that */
-		if (callee->in_callback_fn) {
-			verbose(env, "callback cannot unwind\n");
-			return -EINVAL;
-		}
+	unwinds = bpf_is_unwind_exit(&env->prog->insnsi[*insn_idx]);
+	if (unwinds) {
+		/*
+		 * A callback returns into the helper that called it, and to the
+		 * pad of the helper when the helper has one. check_cb_unwind()
+		 * has seen to that, and to the unwind_goto after the call of
+		 * the helper, where the helper leaves to right away, with the
+		 * r0 of the callback as it is: the pad gets its state, as the
+		 * pad after a subprog call does.
+		 */
 		if (!bpf_is_unwind_goto(&env->prog->insnsi[callee->callsite + 1])) {
 			verbose(env, "unwind_exit, but no unwind_goto after the call at insn %d\n",
 				callee->callsite);
@@ -12124,7 +12195,7 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
 		}
 		state->unwound = true;
 	}
-	if (callee->in_callback_fn) {
+	if (callee->in_callback_fn && !unwinds) {
 		if (r0->type != SCALAR_VALUE) {
 			verbose(env, "R0 not a scalar value\n");
 			return -EACCES;
@@ -12171,6 +12242,14 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
 	 * there function call logic would reschedule callback visit. If iteration
 	 * converges is_state_visited() would prune that visit eventually.
 	 */
+	/*
+	 * A callback that unwound goes back to the call too: the helper has
+	 * effects of its own to apply, such as releasing what it consumed. With
+	 * the state marked unwound it is the last round, and r0 is still what
+	 * the callback unwound with after the call, with the effects of the
+	 * helper applied to it. See push_callback_call(), check_helper_call()
+	 * and check_kfunc_call().
+	 */
 	in_callback_fn = callee->in_callback_fn;
 	if (in_callback_fn)
 		*insn_idx = callee->callsite;
@@ -12201,7 +12280,7 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
 	 * This is similar to what is done in process_iter_next_call() for open
 	 * coded iterators.
 	 */
-	prev_st = in_callback_fn ? find_prev_entry(env, state, *insn_idx) : NULL;
+	prev_st = in_callback_fn && !unwinds ? find_prev_entry(env, state, *insn_idx) : NULL;
 	if (prev_st) {
 		err = widen_imprecise_scalars(env, prev_st, state);
 		if (err)
@@ -12614,7 +12693,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 	const struct bpf_func_proto *fn = NULL;
 	enum bpf_return_type ret_type;
 	enum bpf_type_flag ret_flag;
-	struct bpf_reg_state *regs;
+	struct bpf_reg_state *regs, unwound_r0;
 	struct bpf_call_arg_meta meta;
 	const char *operation;
 	int insn_idx = *insn_idx_p;
@@ -12870,12 +12949,25 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 
 	/* reset caller saved regs */
 	bpf_diag_record_caller_saved(env, regs);
-	bpf_diag_mod_begin(env, &regs[BPF_REG_0], NULL, BPF_DIAG_MOD_WRITE);
+	/*
+	 * The callback unwound: the helper leaves to the pad after the call
+	 * with the r0 of the callback, which prepare_func_exit() put here and
+	 * which the effects of the helper above apply to like to any register.
+	 * There is no return, and r0 isn't written.
+	 */
+	if (env->cur_state->unwound)
+		unwound_r0 = regs[BPF_REG_0];
+	else
+		bpf_diag_mod_begin(env, &regs[BPF_REG_0], NULL, BPF_DIAG_MOD_WRITE);
 	for (i = 0; i < CALLER_SAVED_REGS; i++) {
 		bpf_mark_reg_not_init(env, &regs[caller_saved[i]]);
 		check_reg_arg(env, caller_saved[i], DST_OP_NO_MARK);
 	}
 	invalidate_outgoing_stack_args(env, cur_func(env));
+	if (env->cur_state->unwound) {
+		regs[BPF_REG_0] = unwound_r0;
+		goto unwound;
+	}
 
 	/* update return register (already marked as written above) */
 	ret_type = fn->ret_type;
@@ -13063,7 +13155,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		return err;
 
 	bpf_diag_mod_end(env);
-
+unwound:
 	err = check_map_func_compatibility(env, meta.map.ptr, func_id);
 	if (err)
 		return err;
@@ -15398,7 +15490,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 {
 	bool sleepable, rcu_lock, rcu_unlock, preempt_disable, preempt_enable;
 	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
-	struct bpf_reg_state *regs = cur_regs(env);
+	struct bpf_reg_state *regs = cur_regs(env), unwound_r0;
 	const char *func_name, *ptr_type_name;
 	const struct btf_type *t, *ptr_type;
 	struct bpf_call_arg_meta meta;
@@ -15660,13 +15752,23 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	}
 
 	bpf_diag_record_caller_saved(env, regs);
-	bpf_diag_mod_begin(env, &regs[BPF_REG_0], NULL, BPF_DIAG_MOD_WRITE);
+	/* The callback unwound: r0 is the callback's, see check_helper_call() */
+	if (env->cur_state->unwound) {
+		unwound_r0 = regs[BPF_REG_0];
+		ret_nregs = 0;
+	} else {
+		bpf_diag_mod_begin(env, &regs[BPF_REG_0], NULL, BPF_DIAG_MOD_WRITE);
+	}
 	for (i = 0; i < CALLER_SAVED_REGS; i++) {
 		u32 regno = caller_saved[i];
 
 		bpf_mark_reg_not_init(env, &regs[regno]);
 	}
 	invalidate_outgoing_stack_args(env, cur_func(env));
+	if (env->cur_state->unwound) {
+		regs[BPF_REG_0] = unwound_r0;
+		goto unwound;
+	}
 
 	/* Check return type */
 	t = btf_type_skip_modifiers(desc_btf, meta.func_proto->type, NULL);
@@ -15851,6 +15953,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		}
 	}
 
+unwound:
 	if (is_kfunc_dynptr_write(&meta) ?
 	    dynptr_write_changes_pkt_data(meta.dynptr_write_type) :
 	    bpf_is_kfunc_pkt_changing(&meta))
@@ -20589,7 +20692,8 @@ static int do_check(struct bpf_verifier_env *env)
 		}
 
 		if (bpf_is_jmp_point(env, env->insn_idx)) {
-			err = bpf_push_jmp_history(env, state, 0, 0, 0, NULL, 0);
+			err = bpf_push_jmp_history(env, state, state->unwound ? INSN_F_UNWOUND : 0,
+						   0, 0, NULL, 0);
 			if (err)
 				return err;
 		}
-- 
2.53.0

[RFC PATCH bpf-next v1 04/11] bpf: Give the inlined bpf_loop a pad

[Kumar Kartikeya Dwivedi]

The verifier inlines bpf_loop() when the flags are zero and the callback
is the same at every visit. Once the loop is BPF insns the caller of the
callback is BPF code, and the pad is the ordinary one: unwind_goto after
the call of the callback, which costs nothing, and a pad that restores
r6-r8 and jumps to the pad after the call of bpf_loop with the r0 of the
callback, the one the verifier checked with the state of that r0. The
unwind_goto that followed the call of bpf_loop follows no call anymore and
becomes a no-op jump.

Only a callback with unwind_exit gets the pad, and only when an
unwind_goto follows the call: a loop of no rounds never runs its
callback, so the verifier asked for none, and the insn after the call is
some other insn. The inlined loop of every other program is the one it
was. Nothing takes the pad yet: the callback of bpf_loop() is let unwind
next.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/fixups.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index f062ef7dd943..94a379c94c67 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -2836,6 +2836,15 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
 	return 0;
 }
 
+/*
+ * When the callback can unwind and unwind_goto follows the call of bpf_loop,
+ * unwind_goto follows the call of the callback, and the pad restores r6-r8 and
+ * jumps to the pad after the call of bpf_loop with the r0 of the callback, which
+ * the verifier has checked with the state of that r0. The unwind_goto that
+ * followed the call of bpf_loop doesn't follow a call anymore and becomes a
+ * no-op jump. A loop of no rounds never runs its callback, so the verifier asked
+ * for no unwind_goto there, and the insn after the call is some other insn.
+ */
 static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
 					int position,
 					s32 stack_base,
@@ -2848,6 +2857,8 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
 	int reg_loop_max = BPF_REG_6;
 	int reg_loop_cnt = BPF_REG_7;
 	int reg_loop_ctx = BPF_REG_8;
+	bool pad = env->subprog_info[callback_subprogno].can_unwind &&
+		   bpf_is_unwind_goto(&env->prog->insnsi[position + 1]);
 
 	struct bpf_insn *insn_buf = env->insn_buf;
 	struct bpf_prog *new_prog;
@@ -2865,7 +2876,7 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
 	 */
 	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JLE, BPF_REG_1, BPF_MAX_LOOPS, 2);
 	insn_buf[cnt++] = BPF_MOV32_IMM(BPF_REG_0, -E2BIG);
-	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JA, 0, 0, 16);
+	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JA, 0, 0, pad ? 22 : 16);
 	/* spill R6, R7, R8 to use these as loop vars */
 	insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_6, r6_offset);
 	insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_7, r7_offset);
@@ -2877,17 +2888,20 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
 	/* loop header,
 	 * if reg_loop_cnt >= reg_loop_max skip the loop body
 	 */
-	insn_buf[cnt++] = BPF_JMP_REG(BPF_JGE, reg_loop_cnt, reg_loop_max, 5);
+	insn_buf[cnt++] = BPF_JMP_REG(BPF_JGE, reg_loop_cnt, reg_loop_max, pad ? 6 : 5);
 	/* callback call,
 	 * correct callback offset would be set after patching
 	 */
 	insn_buf[cnt++] = BPF_MOV64_REG(BPF_REG_1, reg_loop_cnt);
 	insn_buf[cnt++] = BPF_MOV64_REG(BPF_REG_2, reg_loop_ctx);
 	insn_buf[cnt++] = BPF_CALL_REL(0);
+	if (pad)
+		/* to the pad below when the callback left with unwind_exit */
+		insn_buf[cnt++] = BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, BPF_UNWIND_GOTO, 7, 0);
 	/* increment loop counter */
 	insn_buf[cnt++] = BPF_ALU64_IMM(BPF_ADD, reg_loop_cnt, 1);
 	/* jump to loop header if callback returned 0 */
-	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, -6);
+	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, pad ? -7 : -6);
 	/* return value of bpf_loop,
 	 * set R0 to the number of iterations
 	 */
@@ -2896,12 +2910,35 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
 	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_10, r6_offset);
 	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, r7_offset);
 	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_8, BPF_REG_10, r8_offset);
+	if (pad) {
+		/* over the pad */
+		insn_buf[cnt++] = BPF_JMP_IMM(BPF_JA, 0, 0, 4);
+		/* the pad: r0 is what the callback unwound with */
+		insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_10, r6_offset);
+		insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, r7_offset);
+		insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_8, BPF_REG_10, r8_offset);
+		/*
+		 * To the pad of the caller, where the unwind_goto that follows
+		 * the patch goes. Its offset is known once the patch is in and
+		 * the jumps across it are adjusted, see below.
+		 */
+		insn_buf[cnt++] = BPF_JMP32_A(0);
+	}
 
 	*total_cnt = cnt;
 	new_prog = bpf_patch_insn_data(env, position, insn_buf, cnt);
 	if (!new_prog)
 		return new_prog;
 
+	if (pad) {
+		struct bpf_insn *unwind_goto = &new_prog->insnsi[position + cnt];
+
+		/* the jump of the pad, one insn before, takes the target of the unwind_goto */
+		new_prog->insnsi[position + cnt - 1].imm = unwind_goto->off + 1;
+		/* and the unwind_goto after the call of bpf_loop follows no call anymore */
+		*unwind_goto = BPF_JMP_A(0);
+	}
+
 	/* callback start is known only after patching */
 	callback_start = env->subprog_info[callback_subprogno].start;
 	/* Note: insn_buf[12] is an offset of BPF_CALL_REL instruction */
-- 
2.53.0

[RFC PATCH bpf-next v1 05/11] bpf: Let the callback of bpf_loop unwind

[Kumar Kartikeya Dwivedi]

The loop holds nothing, so the pad only leaves, with the r0 of the
callback. The inlined loop has its pad already, so the verifier lets the
callback of bpf_loop() unwind from here on.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/bpf_iter.c | 10 ++++++++--
 kernel/bpf/verifier.c |  9 +++++++--
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index f0b29a170935..efd23c973115 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -5,6 +5,7 @@
 #include <linux/anon_inodes.h>
 #include <linux/filter.h>
 #include <linux/bpf.h>
+#include <linux/bpf_unwind.h>
 #include <linux/rcupdate_trace.h>
 
 struct bpf_iter_target_info {
@@ -749,7 +750,7 @@ const struct bpf_func_proto bpf_for_each_map_elem_proto = {
 	.arg4_type	= ARG_ANYTHING,
 };
 
-BPF_CALL_4(bpf_loop, u32, nr_loops, void *, callback_fn, void *, callback_ctx,
+BPF_CALL_x(4, noinline, bpf_loop, u32, nr_loops, void *, callback_fn, void *, callback_ctx,
 	   u64, flags)
 {
 	bpf_callback_t callback = (bpf_callback_t)callback_fn;
@@ -766,13 +767,18 @@ BPF_CALL_4(bpf_loop, u32, nr_loops, void *, callback_fn, void *, callback_ctx,
 		return -E2BIG;
 
 	for (i = 0; i < nr_loops; i++) {
-		ret = callback((u64)i, (u64)(long)callback_ctx, 0, 0, 0);
+		bpf_call_cb(callback, i, callback_ctx, 0, 0, 0, ret, unwound);
 		/* return value: 0 - continue, 1 - stop and return */
 		if (ret)
 			return i + 1;
 	}
 
 	return i;
+
+unwound:
+	/* The callback left with unwind_exit: the loop is over, and so is the helper */
+	bpf_unwind_leave();
+	return ret;
 }
 
 const struct bpf_func_proto bpf_loop_proto = {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ac9dbe082181..4b4d8479fa87 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11402,10 +11402,15 @@ static int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog)
 	return err;
 }
 
-/* The helper calls its callback with a pad, see bpf_call_cb(). None yet. */
+/* The helper calls its callback with a pad, see bpf_call_cb() */
 static bool helper_has_cb_pad(enum bpf_func_id func_id)
 {
-	return false;
+	switch (func_id) {
+	case BPF_FUNC_loop:
+		return true;
+	default:
+		return false;
+	}
 }
 
 /* The kfunc calls its callback with a pad. None yet. */
-- 
2.53.0

[RFC PATCH bpf-next v1 06/11] bpf: Let the callback of bpf_for_each_map_elem unwind

[Kumar Kartikeya Dwivedi]

The iterators of the array, the hash and the rhash maps get a pad. The
array holds nothing. The hash and the rhash run the callback in an RCU
section, which the pad drops the way the exit on a nonzero return does.

The verifier calls the op directly when it knows the map, and the pad of
the op leaves to the pad after that call. bpf_for_each_map_elem(), for a
map the verifier didn't know, calls the op the way it would call a
callback, with bpf_call_cb() and a pad of its own, so that the op can
leave to it, with the r0 of the callback either way. Without pads the
helper calls the op the plain way, with the type of the op: the fallback of
bpf_call_cb() has the type of a callback, which the op is not, and the
kCFI check of such a kernel would say so. With that the verifier lets the
callback of bpf_for_each_map_elem() unwind.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/arraymap.c | 11 ++++++++---
 kernel/bpf/bpf_iter.c | 22 +++++++++++++++++++++-
 kernel/bpf/hashtab.c  | 26 ++++++++++++++++++--------
 kernel/bpf/verifier.c |  1 +
 4 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 133f189bf6d2..4c81c7c76145 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2016,2017 Facebook
  */
 #include <linux/bpf.h>
+#include <linux/bpf_unwind.h>
 #include <linux/btf.h>
 #include <linux/err.h>
 #include <linux/slab.h>
@@ -800,7 +801,7 @@ static const struct bpf_iter_seq_info iter_seq_info = {
 	.seq_priv_size		= sizeof(struct bpf_iter_seq_array_map_info),
 };
 
-static long bpf_for_each_array_elem(struct bpf_map *map, bpf_callback_t callback_fn,
+static noinline long bpf_for_each_array_elem(struct bpf_map *map, bpf_callback_t callback_fn,
 				    void *callback_ctx, u64 flags)
 {
 	u32 i, key, num_elems = 0;
@@ -823,14 +824,18 @@ static long bpf_for_each_array_elem(struct bpf_map *map, bpf_callback_t callback
 			val = array_map_elem_ptr(array, i);
 		num_elems++;
 		key = i;
-		ret = callback_fn((u64)(long)map, (u64)(long)&key,
-				  (u64)(long)val, (u64)(long)callback_ctx, 0);
+		bpf_call_cb(callback_fn, map, &key, val, callback_ctx, 0, ret, unwound);
 		/* return value: 0 - continue, 1 - stop and return */
 		if (ret)
 			break;
 	}
 
 	return num_elems;
+
+unwound:
+	/* The callback left with unwind_exit: nothing to drop here */
+	bpf_unwind_leave();
+	return ret;
 }
 
 static u64 array_map_mem_usage(const struct bpf_map *map)
diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index efd23c973115..4a1bf5a1b8d9 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -734,10 +734,30 @@ int bpf_iter_run_prog(struct bpf_prog *prog, void *ctx)
 	return ret == 0 ? 0 : -EAGAIN;
 }
 
-BPF_CALL_4(bpf_for_each_map_elem, struct bpf_map *, map, void *, callback_fn,
+/*
+ * The op has a pad for the callback and leaves like a callback does when it is
+ * taken, so it is called like one, with the kCFI check for its own type. The
+ * verifier calls the op directly when it knows the map.
+ */
+BPF_CB_PAD_TYPE(__bpf_for_each_op, typeof(*((struct bpf_map_ops *)0)->map_for_each_callback));
+
+BPF_CALL_x(4, noinline, bpf_for_each_map_elem, struct bpf_map *, map, void *, callback_fn,
 	   void *, callback_ctx, u64, flags)
 {
+#if BPF_HAS_CB_PADS
+	u64 ret;
+
+	__bpf_call_cb(__bpf_for_each_op, map->ops->map_for_each_callback, map, callback_fn,
+		      callback_ctx, flags, 0, ret, unwound);
+	return ret;
+
+unwound:
+	bpf_unwind_leave();
+	return ret;
+#else
+	/* The plain call, with the type of the op: the fallback of bpf_call_cb() has a callback's */
 	return map->ops->map_for_each_callback(map, callback_fn, callback_ctx, flags);
+#endif
 }
 
 const struct bpf_func_proto bpf_for_each_map_elem_proto = {
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index bf78e09f4d51..3e995dacfaa0 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2016 Facebook
  */
 #include <linux/bpf.h>
+#include <linux/bpf_unwind.h>
 #include <linux/btf.h>
 #include <linux/jhash.h>
 #include <linux/filter.h>
@@ -2498,7 +2499,7 @@ static const struct bpf_iter_seq_info iter_seq_info = {
 	.seq_priv_size		= sizeof(struct bpf_iter_seq_hash_map_info),
 };
 
-static long bpf_for_each_hash_elem(struct bpf_map *map, bpf_callback_t callback_fn,
+static noinline long bpf_for_each_hash_elem(struct bpf_map *map, bpf_callback_t callback_fn,
 				   void *callback_ctx, u64 flags)
 {
 	struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
@@ -2537,8 +2538,7 @@ static long bpf_for_each_hash_elem(struct bpf_map *map, bpf_callback_t callback_
 				val = htab_elem_value(elem, map->key_size);
 			}
 			num_elems++;
-			ret = callback_fn((u64)(long)map, (u64)(long)key,
-					  (u64)(long)val, (u64)(long)callback_ctx, 0);
+			bpf_call_cb(callback_fn, map, key, val, callback_ctx, 0, ret, unwound);
 			/* return value: 0 - continue, 1 - stop and return */
 			if (ret) {
 				rcu_read_unlock();
@@ -2549,6 +2549,12 @@ static long bpf_for_each_hash_elem(struct bpf_map *map, bpf_callback_t callback_
 	}
 out:
 	return num_elems;
+
+unwound:
+	/* The callback left with unwind_exit, inside the RCU section of the bucket */
+	rcu_read_unlock();
+	bpf_unwind_leave();
+	return ret;
 }
 
 static u64 htab_map_mem_usage(const struct bpf_map *map)
@@ -3409,7 +3415,7 @@ static void rhtab_map_seq_show_elem(struct bpf_map *map, void *key, struct seq_f
 	seq_putc(m, '\n');
 }
 
-static long bpf_each_rhash_elem(struct bpf_map *map, bpf_callback_t callback_fn,
+static noinline long bpf_each_rhash_elem(struct bpf_map *map, bpf_callback_t callback_fn,
 				void *callback_ctx, u64 flags)
 {
 	struct bpf_rhtab *rhtab = container_of(map, struct bpf_rhtab, map);
@@ -3433,10 +3439,8 @@ static long bpf_each_rhash_elem(struct bpf_map *map, bpf_callback_t callback_fn,
 		if (IS_ERR(elem))
 			break;
 		num_elems++;
-		ret = callback_fn((u64)(long)map,
-				  (u64)(long)elem->data,
-				  (u64)(long)rhtab_elem_value(elem, map->key_size),
-				  (u64)(long)callback_ctx, 0);
+		bpf_call_cb(callback_fn, map, elem->data, rhtab_elem_value(elem, map->key_size),
+			    callback_ctx, 0, ret, unwound);
 		if (ret)
 			break;
 
@@ -3445,6 +3449,12 @@ static long bpf_each_rhash_elem(struct bpf_map *map, bpf_callback_t callback_fn,
 	rcu_read_unlock();
 
 	return num_elems;
+
+unwound:
+	/* The callback left with unwind_exit, under RCU */
+	rcu_read_unlock();
+	bpf_unwind_leave();
+	return ret;
 }
 
 static u64 rhtab_map_mem_usage(const struct bpf_map *map)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4b4d8479fa87..b2b72b3b32ac 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11407,6 +11407,7 @@ static bool helper_has_cb_pad(enum bpf_func_id func_id)
 {
 	switch (func_id) {
 	case BPF_FUNC_loop:
+	case BPF_FUNC_for_each_map_elem:
 		return true;
 	default:
 		return false;
-- 
2.53.0

[RFC PATCH bpf-next v1 07/11] bpf: Let the callback of bpf_find_vma unwind

[Kumar Kartikeya Dwivedi]

bpf_find_vma() runs the callback under the mmap lock and in an RCU section.
The pad drops both and the reference on a foreign mm, the same way out as
a return, and leaves with the r0 of the callback. With that the verifier
lets the callback of bpf_find_vma() unwind.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/task_iter.c | 21 ++++++++++++++++++---
 kernel/bpf/verifier.c  |  1 +
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 2ee7b3b3c10b..ae67c97fe845 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -6,6 +6,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/fs.h>
 #include <linux/filter.h>
+#include <linux/bpf_unwind.h>
 #include <linux/bpf_mem_alloc.h>
 #include <linux/btf_ids.h>
 #include <linux/mm_types.h>
@@ -754,7 +755,7 @@ static struct bpf_iter_reg task_vma_reg_info = {
 	.show_fdinfo		= bpf_iter_task_show_fdinfo,
 };
 
-BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
+BPF_CALL_x(5, noinline, bpf_find_vma, struct task_struct *, task, u64, start,
 	   bpf_callback_t, callback_fn, void *, callback_ctx, u64, flags)
 {
 	struct mmap_unlock_irq_work *work;
@@ -762,6 +763,7 @@ BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
 	bool __maybe_unused mmput_needed = false;
 	struct mm_struct *mm;
 	int ret = -ENOENT;
+	u64 r0;
 
 	if (flags)
 		return -EINVAL;
@@ -815,8 +817,7 @@ BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
 		 * verifier checks it as an RCU read-side critical section.
 		 */
 		rcu_read_lock();
-		callback_fn((u64)(long)task, (u64)(long)vma,
-			    (u64)(long)callback_ctx, 0, 0);
+		bpf_call_cb(callback_fn, task, vma, callback_ctx, 0, 0, r0, unwound);
 		rcu_read_unlock();
 		ret = 0;
 	}
@@ -827,6 +828,20 @@ BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
 		mmput_async(mm);
 #endif
 	return ret;
+
+unwound:
+	/*
+	 * The callback left with unwind_exit, under the mmap lock and in the
+	 * RCU section. The way out is the same as after a return.
+	 */
+	rcu_read_unlock();
+	bpf_mmap_unlock_mm(work, mm);
+#ifdef CONFIG_MMU
+	if (mmput_needed)
+		mmput_async(mm);
+#endif
+	bpf_unwind_leave();
+	return r0;
 }
 
 const struct bpf_func_proto bpf_find_vma_proto = {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b2b72b3b32ac..3c39cc0f358a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11408,6 +11408,7 @@ static bool helper_has_cb_pad(enum bpf_func_id func_id)
 	switch (func_id) {
 	case BPF_FUNC_loop:
 	case BPF_FUNC_for_each_map_elem:
+	case BPF_FUNC_find_vma:
 		return true;
 	default:
 		return false;
-- 
2.53.0

[RFC PATCH bpf-next v1 08/11] bpf: Let the callback of bpf_user_ringbuf_drain unwind

[Kumar Kartikeya Dwivedi]

bpf_user_ringbuf_drain() runs the callback with the busy bit set and a
sample peeked. The pad consumes the sample as a return does, and counts
it, since the wakeup at the end is sent for samples consumed. Then it
leaves the loop through the door the error paths use, which clears the
busy bit and sends the wakeup, and leaves with the r0 of the callback.
With that the verifier lets the callback of bpf_user_ringbuf_drain()
unwind.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/ringbuf.c  | 23 +++++++++++++++++++++--
 kernel/bpf/verifier.c |  1 +
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
index 447863ece26a..fdd997f36f73 100644
--- a/kernel/bpf/ringbuf.c
+++ b/kernel/bpf/ringbuf.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/bpf.h>
+#include <linux/bpf_unwind.h>
 #include <linux/btf.h>
 #include <linux/err.h>
 #include <linux/irq_work.h>
@@ -814,13 +815,14 @@ static void __bpf_user_ringbuf_sample_release(struct bpf_ringbuf *rb, size_t siz
 	smp_store_release(&rb->consumer_pos, consumer_pos + rounded_size);
 }
 
-BPF_CALL_4(bpf_user_ringbuf_drain, struct bpf_map *, map,
+BPF_CALL_x(4, noinline, bpf_user_ringbuf_drain, struct bpf_map *, map,
 	   void *, callback_fn, void *, callback_ctx, u64, flags)
 {
 	struct bpf_ringbuf *rb;
 	long samples, discarded_samples = 0, ret = 0;
 	bpf_callback_t callback = (bpf_callback_t)callback_fn;
 	u64 wakeup_flags = BPF_RB_NO_WAKEUP | BPF_RB_FORCE_WAKEUP;
+	bool unwound = false;
 	int busy = 0;
 
 	if (unlikely(flags & ~wakeup_flags))
@@ -852,8 +854,23 @@ BPF_CALL_4(bpf_user_ringbuf_drain, struct bpf_map *, map,
 		}
 
 		bpf_dynptr_init(&dynptr, sample, BPF_DYNPTR_TYPE_LOCAL, 0, size);
-		ret = callback((uintptr_t)&dynptr, (uintptr_t)callback_ctx, 0, 0, 0);
+		bpf_call_cb(callback, &dynptr, callback_ctx, 0, 0, 0, ret, unwound_at);
+		if (0) {
+unwound_at:
+			/*
+			 * The callback left with unwind_exit. The sample is
+			 * consumed and counted like after a return, so that
+			 * the wakeup below is sent for it, and the helper
+			 * leaves through the door below, as the error paths
+			 * do, with the r0 of the callback.
+			 */
+			unwound = true;
+		}
 		__bpf_user_ringbuf_sample_release(rb, size, flags);
+		if (unwound) {
+			samples++;
+			goto schedule_work_return;
+		}
 	}
 	ret = samples - discarded_samples;
 
@@ -867,6 +884,8 @@ BPF_CALL_4(bpf_user_ringbuf_drain, struct bpf_map *, map,
 		irq_work_queue(&rb->work);
 	else if (!(flags & BPF_RB_NO_WAKEUP) && samples > 0)
 		irq_work_queue(&rb->work);
+	if (unwound)
+		bpf_unwind_leave();
 	return ret;
 }
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3c39cc0f358a..fd63ff36eb90 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11409,6 +11409,7 @@ static bool helper_has_cb_pad(enum bpf_func_id func_id)
 	case BPF_FUNC_loop:
 	case BPF_FUNC_for_each_map_elem:
 	case BPF_FUNC_find_vma:
+	case BPF_FUNC_user_ringbuf_drain:
 		return true;
 	default:
 		return false;
-- 
2.53.0

[RFC PATCH bpf-next v1 09/11] bpf: Let the comparator of bpf_rbtree_add unwind

[Kumar Kartikeya Dwivedi]

When the comparator unwinds the node is not in the tree, and the caller
has let go of it: the kfunc releases the node whichever way it returns.
The pad drops it, as the error path for a node that is already in a tree
does, and leaves. The lock on the root is the caller's, and its pad is
checked for it like any exit. Both spellings of the kfunc get it, each
with its own copy of the body: the pad leaves from the frame that the BPF
code called, and bpf_rbtree_add_impl() calling bpf_rbtree_add() is a tail
jump on x86-64 but a call with a frame of its own on arm64, where the pad
would rewrite a return address into that frame and the exception would come
back as a return of 0.

The kfunc returns long in both spellings: the pad leaves with the r0 of
the comparator, and an int would cut it to 32 bits. The change shows in
BTF, and libbpf takes integers of any width as compatible, so a
declaration that says int keeps loading.

On the revisit of the call after the comparator unwound, the node is not
in the tree: the kfunc dropped it. So the verifier releases the reference,
and every register that held the node is gone with it, rather than turning
it into a non-owning reference to a node in the tree. The comparator was
handed the node too, as a non-owning reference, and may have unwound with
it in r0, so every non-owning reference goes, as it does at an unlock.
With that the verifier lets the comparator of bpf_rbtree_add() unwind, in
both spellings.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/helpers.c  | 50 +++++++++++++++++++++++++++++--------------
 kernel/bpf/verifier.c | 20 ++++++++++++++---
 2 files changed, 51 insertions(+), 19 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 648abe1ba63a..f8708b7181d7 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2,6 +2,7 @@
 /* Copyright (c) 2011-2014 PLUMgrid, http://plumgrid.com
  */
 #include <linux/bpf.h>
+#include <linux/bpf_unwind.h>
 #include <linux/btf.h>
 #include <linux/bpf-cgroup.h>
 #include <linux/cgroup.h>
@@ -2761,17 +2762,20 @@ __bpf_kfunc struct bpf_rb_node *bpf_rbtree_remove(struct bpf_rb_root *root,
 	return (struct bpf_rb_node *)n;
 }
 
-/* Need to copy rbtree_add_cached's logic here because our 'less' is a BPF
- * program
+/*
+ * Need to copy rbtree_add_cached's logic here because our 'less' is a BPF
+ * program. The pad leaves from the frame that the BPF code called, so the body
+ * is inlined into both spellings of the kfunc.
  */
-static int __bpf_rbtree_add(struct bpf_rb_root *root,
-			    struct bpf_rb_node_kern *node,
-			    void *less, struct btf_record *rec, u64 off)
+static __always_inline long __bpf_rbtree_add(struct bpf_rb_root *root,
+					     struct bpf_rb_node_kern *node,
+					     void *less, struct btf_record *rec, u64 off)
 {
 	struct rb_node **link = &((struct rb_root_cached *)root)->rb_root.rb_node;
 	struct rb_node *parent = NULL, *n = &node->rb_node;
 	bpf_callback_t cb = (bpf_callback_t)less;
 	bool leftmost = true;
+	u64 r0;
 
 	/* node->owner != NULL implies !RB_EMPTY_NODE(n), no need to separately
 	 * check the latter
@@ -2784,7 +2788,8 @@ static int __bpf_rbtree_add(struct bpf_rb_root *root,
 
 	while (*link) {
 		parent = *link;
-		if (cb((uintptr_t)node, (uintptr_t)parent, 0, 0, 0)) {
+		bpf_call_cb(cb, node, parent, 0, 0, 0, r0, unwound);
+		if (r0) {
 			link = &parent->rb_left;
 		} else {
 			link = &parent->rb_right;
@@ -2796,6 +2801,15 @@ static int __bpf_rbtree_add(struct bpf_rb_root *root,
 	rb_insert_color_cached(n, (struct rb_root_cached *)root, leftmost);
 	WRITE_ONCE(node->owner, root);
 	return 0;
+
+unwound:
+	/*
+	 * The comparator left with unwind_exit. The node is not in the tree, and
+	 * the caller has let go of it: drop it, as the error path above does.
+	 */
+	__bpf_obj_drop_impl((void *)n - off, rec, false);
+	bpf_unwind_leave();
+	return r0;
 }
 
 /**
@@ -2809,24 +2823,28 @@ static int __bpf_rbtree_add(struct bpf_rb_root *root,
  * Insert @node into @root using @less. The verifier rewrites @meta and @off;
  * BPF programs do not set them.
  *
- * Return: 0 on success, or %-EINVAL if @node is already linked in a tree.
+ * Return: 0 on success, or %-EINVAL if @node is already linked in a tree. When
+ * @less unwinds, what it unwound with, to the pad after the call.
  */
-__bpf_kfunc int bpf_rbtree_add(struct bpf_rb_root *root,
-			       struct bpf_rb_node *node,
-			       bool (less)(struct bpf_rb_node *a, const struct bpf_rb_node *b),
-			       struct btf_struct_meta *meta,
-			       u64 off)
+__bpf_kfunc long bpf_rbtree_add(struct bpf_rb_root *root,
+				struct bpf_rb_node *node,
+				bool (less)(struct bpf_rb_node *a, const struct bpf_rb_node *b),
+				struct btf_struct_meta *meta,
+				u64 off)
 {
 	struct bpf_rb_node_kern *n = (void *)node;
 
 	return __bpf_rbtree_add(root, n, (void *)less, meta ? meta->record : NULL, off);
 }
 
-__bpf_kfunc int bpf_rbtree_add_impl(struct bpf_rb_root *root, struct bpf_rb_node *node,
-				    bool (less)(struct bpf_rb_node *a, const struct bpf_rb_node *b),
-				    void *meta__ign, u64 off)
+__bpf_kfunc long bpf_rbtree_add_impl(struct bpf_rb_root *root, struct bpf_rb_node *node,
+				     bool (less)(struct bpf_rb_node *a, const struct bpf_rb_node *b),
+				     void *meta__ign, u64 off)
 {
-	return bpf_rbtree_add(root, node, less, meta__ign, off);
+	struct btf_struct_meta *meta = meta__ign;
+	struct bpf_rb_node_kern *n = (void *)node;
+
+	return __bpf_rbtree_add(root, n, (void *)less, meta ? meta->record : NULL, off);
 }
 
 __bpf_kfunc struct bpf_rb_node *bpf_rbtree_first(struct bpf_rb_root *root)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index fd63ff36eb90..50c3d44c37e1 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11416,10 +11416,10 @@ static bool helper_has_cb_pad(enum bpf_func_id func_id)
 	}
 }
 
-/* The kfunc calls its callback with a pad. None yet. */
+/* The kfunc calls its callback with a pad */
 static bool kfunc_has_cb_pad(u32 btf_id)
 {
-	return false;
+	return is_bpf_rbtree_add_kfunc(btf_id);
 }
 
 static bool insn_has_cb_pad(struct bpf_insn *insn)
@@ -15738,7 +15738,21 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		id = regs[BPF_REG_2].id;
 		insn_aux->insert_off = regs[BPF_REG_2].var_off.value;
 		insn_aux->kptr_struct_meta = btf_find_struct_meta(meta.arg_btf, meta.arg_btf_id);
-		ref_convert_owning_non_owning(env, id);
+		/*
+		 * The comparator unwound: the node never went in, and the
+		 * kfunc dropped it on its way to the pad. The comparator was
+		 * handed the node too, and may have unwound with it in r0, so
+		 * every non-owning reference goes. Otherwise the node is in
+		 * the tree now.
+		 */
+		if (env->cur_state->unwound) {
+			invalidate_non_owning_refs(env);
+			err = release_reference(env, id);
+			if (err)
+				return err;
+		} else {
+			ref_convert_owning_non_owning(env, id);
+		}
 	}
 
 	if (meta.func_id == special_kfunc_list[KF_bpf_throw]) {
-- 
2.53.0

[RFC PATCH bpf-next v1 10/11] selftests/bpf: Test callbacks that unwind

[Kumar Kartikeya Dwivedi]

One file for the callbacks of bpf_loop(), bpf_for_each_map_elem() over
an array, a hash and an rhash the prog fills first, bpf_find_vma() on
the stack of the current task, and the comparator of bpf_rbtree_add(),
in both spellings, meeting the one node that is in the tree. Each with
a callback that unwinds on its first run, where the pad after the call
returns the number of runs plus what the helper left in r0, which is
what the callback unwound with, plus 41. One callback of bpf_loop()
unwinds on its third run, one unwinds through its own pad, one call
sees a callback that unwinds on one path and one that returns on the
other, and the pad after a callback that returns is never taken.
Refused is the call without unwind_goto after it. A loop of no rounds
never runs its callback: it needs no unwind_goto, and the inlined loop
leaves the insn after the call alone. The normal path of the helpers is
what their own tests run.

The file includes vmlinux.h for the offsets the find_vma tests need,
which rules out BPF_RAW_INSN(), so unwind_goto and unwind_exit are raw
words. The rbtree test is in C with a naked subprog around the call of
the kfunc, since the unwind_goto has to follow the call.

The callback of bpf_user_ringbuf_drain() needs samples from user space,
so its test is a prog_tests one. The callback unwinds on the sample it
is given, with r0 = 5, and the pad after the call returns the number of
runs plus 5 plus 41. The drain before any sample returns 0, the one
after the unwind finds the sample consumed and the busy bit dropped,
and the one after the next sample unwinds again.

The existing "callback unwinds" test used bpf_loop() to get "callback
cannot unwind". bpf_loop() has a pad now, so what it lacks is the
unwind_goto after the call, and that is what the test expects.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/prog_tests/unwind_drain.c   |  71 +++
 .../selftests/bpf/prog_tests/verifier.c       |   2 +
 .../selftests/bpf/progs/unwind_drain.c        |  69 +++
 .../selftests/bpf/progs/verifier_unwind.c     |   3 +-
 .../selftests/bpf/progs/verifier_unwind_cb.c  | 569 ++++++++++++++++++
 5 files changed, 713 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/unwind_drain.c
 create mode 100644 tools/testing/selftests/bpf/progs/unwind_drain.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_unwind_cb.c

diff --git a/tools/testing/selftests/bpf/prog_tests/unwind_drain.c b/tools/testing/selftests/bpf/prog_tests/unwind_drain.c
new file mode 100644
index 000000000000..54b9cfbef440
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/unwind_drain.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include "unwind_drain.skel.h"
+
+static int run(struct bpf_program *prog)
+{
+	char data[64] = {};
+	LIBBPF_OPTS(bpf_test_run_opts, opts,
+		.data_in = data,
+		.data_size_in = sizeof(data),
+		.repeat = 1,
+	);
+
+	if (!ASSERT_OK(bpf_prog_test_run_opts(bpf_program__fd(prog), &opts), "test_run"))
+		return -1;
+	return opts.retval;
+}
+
+static int submit(struct user_ring_buffer *rb)
+{
+	void *sample = user_ring_buffer__reserve(rb, 8);
+
+	if (!ASSERT_OK_PTR(sample, "reserve"))
+		return -1;
+	user_ring_buffer__submit(rb, sample);
+	return 0;
+}
+
+/*
+ * The callback unwinds on the sample it is given, with r0 = 5. The pad after
+ * the call of bpf_user_ringbuf_drain() returns the number of runs plus 5 plus
+ * 41. The sample is consumed and the busy bit dropped on the way there: the
+ * next drain finds an empty ring buffer, and the one after that another
+ * sample.
+ */
+void test_unwind_drain(void)
+{
+	struct user_ring_buffer *rb = NULL;
+	struct unwind_drain *skel;
+	int err;
+
+	if (!is_jit_enabled()) {
+		printf("%s:SKIP:JIT is disabled\n", __func__);
+		test__skip();
+		return;
+	}
+	skel = unwind_drain__open_and_load();
+	err = libbpf_get_error(skel);
+	if (err == -ENOTSUPP) {
+		printf("%s:SKIP:JIT does not support unwinding\n", __func__);
+		test__skip();
+		return;
+	}
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		return;
+	rb = user_ring_buffer__new(bpf_map__fd(skel->maps.user_ringbuf), NULL);
+	if (!ASSERT_OK_PTR(rb, "user_ring_buffer__new"))
+		goto out;
+
+	ASSERT_EQ(run(skel->progs.drain), 0, "empty");
+	if (submit(rb))
+		goto out;
+	ASSERT_EQ(run(skel->progs.drain), 47, "unwinds");
+	ASSERT_EQ(run(skel->progs.drain), 0, "consumed");
+	if (submit(rb))
+		goto out;
+	ASSERT_EQ(run(skel->progs.drain), 48, "unwinds again");
+out:
+	user_ring_buffer__free(rb);
+	unwind_drain__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index 04740ba37e91..10beba6bed20 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -131,6 +131,7 @@
 #include "verifier_uninit.skel.h"
 #include "verifier_unpriv.skel.h"
 #include "verifier_unwind.skel.h"
+#include "verifier_unwind_cb.skel.h"
 #include "verifier_unpriv_perf.skel.h"
 #include "verifier_value_adj_spill.skel.h"
 #include "verifier_value.skel.h"
@@ -322,6 +323,7 @@ void test_verifier_typedef(void)              { RUN(verifier_typedef); }
 void test_verifier_uninit(void)               { RUN(verifier_uninit); }
 void test_verifier_unpriv(void)               { RUN(verifier_unpriv); }
 void test_verifier_unwind(void)               { RUN(verifier_unwind); }
+void test_verifier_unwind_cb(void)            { RUN(verifier_unwind_cb); }
 void test_verifier_unpriv_perf(void)          { RUN(verifier_unpriv_perf); }
 void test_verifier_value_adj_spill(void)      { RUN(verifier_value_adj_spill); }
 void test_verifier_value(void)                { RUN(verifier_value); }
diff --git a/tools/testing/selftests/bpf/progs/unwind_drain.c b/tools/testing/selftests/bpf/progs/unwind_drain.c
new file mode 100644
index 000000000000..1277d1a3e82e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/unwind_drain.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "../../../include/linux/filter.h"
+#include "bpf_misc.h"
+
+#define unwind_goto(off) ".8byte %[insn_unwind_goto_" #off "];"
+#define unwind_exit ".8byte %[insn_unwind_exit];"
+
+#define __imm_unwind_goto(off) \
+	__imm_insn(insn_unwind_goto_##off, \
+		   BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, BPF_UNWIND_GOTO, off, 0))
+#define __imm_unwind_exit \
+	__imm_insn(insn_unwind_exit, \
+		   BPF_RAW_INSN(BPF_JMP | BPF_EXIT, 0, BPF_UNWIND_EXIT, 0, 0))
+
+struct {
+	__uint(type, BPF_MAP_TYPE_USER_RINGBUF);
+	__uint(max_entries, 4096);
+} user_ringbuf SEC(".maps");
+
+/* How many times the callback ran */
+__u64 runs;
+
+/* Unwinds on every sample */
+__naked __noinline __used
+static unsigned long cb_unwinds(void)
+{
+	asm volatile (
+		"r1 = %[runs] ll;"
+		"r2 = *(u64 *)(r1 + 0);"
+		"r2 += 1;"
+		"*(u64 *)(r1 + 0) = r2;"
+		"r0 = 5;"
+		unwind_exit
+		:
+		: __imm_addr(runs),
+		  __imm_unwind_exit
+	);
+}
+
+/* The pad returns the number of runs plus what the helper left in r0, 5, plus 41 */
+SEC("tc")
+__naked int drain(void)
+{
+	asm volatile (
+		"r1 = %[user_ringbuf] ll;"
+		"r2 = %[cb_unwinds];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_user_ringbuf_drain];"
+		unwind_goto(1)
+		"exit;"
+		"r1 = %[runs] ll;"
+		"r1 = *(u64 *)(r1 + 0);"
+		"r0 += r1;"
+		"r0 += 41;"
+		"exit;"
+		:
+		: __imm_addr(user_ringbuf),
+		  __imm_ptr(cb_unwinds),
+		  __imm_addr(runs),
+		  __imm(bpf_user_ringbuf_drain),
+		  __imm_unwind_goto(1)
+		: __clobber_common
+	);
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/verifier_unwind.c b/tools/testing/selftests/bpf/progs/verifier_unwind.c
index 5a21345c34d4..3de145a2d065 100644
--- a/tools/testing/selftests/bpf/progs/verifier_unwind.c
+++ b/tools/testing/selftests/bpf/progs/verifier_unwind.c
@@ -896,12 +896,13 @@ static unsigned long cb_unwinds(void)
 	);
 }
 
+/* bpf_loop() has a pad for it, so what's missing is the one after the call */
 SEC("tc")
 __description("unwind: callback unwinds")
 __arch_x86_64
 __arch_arm64
 __load_if_JITed()
-__failure __msg("callback cannot unwind")
+__failure __msg("no unwind_goto after the call of bpf_loop()")
 __naked int callback_unwinds(void)
 {
 	asm volatile (
diff --git a/tools/testing/selftests/bpf/progs/verifier_unwind_cb.c b/tools/testing/selftests/bpf/progs/verifier_unwind_cb.c
new file mode 100644
index 000000000000..8a3dee4330ac
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_unwind_cb.c
@@ -0,0 +1,569 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
+#include "bpf_experimental.h"
+
+/*
+ * A callback that a helper or a kfunc calls unwinds into a pad in the helper,
+ * which leaves to the pad after the call of the helper. See verifier_unwind.c
+ * for the insns. vmlinux.h, for the offsets the find_vma tests need, leaves no
+ * BPF_RAW_INSN(), so here they are raw words: the code, src_reg = 1 in the
+ * high nibble of the second byte, off in the next two bytes.
+ */
+#define unwind_goto(off) ".8byte (" #off " << 16) | 0x10e5;"	/* BPF_JMP | BPF_JCOND */
+#define unwind_exit ".8byte 0x1095;"				/* BPF_JMP | BPF_EXIT */
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 3);
+	__type(key, int);
+	__type(value, int);
+} array SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__uint(max_entries, 3);
+	__type(key, int);
+	__type(value, int);
+} hash SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_RHASH);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__uint(max_entries, 3);
+	__type(key, int);
+	__type(value, int);
+} rhash SEC(".maps");
+
+struct node_data {
+	long key;
+	struct bpf_rb_node node;
+};
+
+#define private(name) SEC(".data." #name) __hidden __attribute__((aligned(8)))
+private(A) struct bpf_spin_lock glock;
+private(A) struct bpf_rb_root groot __contains(node_data, node);
+
+/* How many times the callback ran */
+__u64 runs;
+
+#define count_run()				\
+	"r1 = %[runs] ll;"			\
+	"r2 = *(u64 *)(r1 + 0);"		\
+	"r2 += 1;"				\
+	"*(u64 *)(r1 + 0) = r2;"
+
+/* Unwinds with r0 = 5 on its first run */
+__naked __noinline __used
+static unsigned long cb_unwinds(void)
+{
+	asm volatile (
+		count_run()
+		"r0 = 5;"
+		unwind_exit
+		:
+		: __imm_addr(runs)
+	);
+}
+
+/* Returns 0 twice, then unwinds */
+__naked __noinline __used
+static unsigned long cb_unwinds_third(void)
+{
+	asm volatile (
+		count_run()
+		"if r2 == 3 goto 1f;"
+		"r0 = 0;"
+		"exit;"
+	"1:"
+		"r0 = 5;"
+		unwind_exit
+		:
+		: __imm_addr(runs)
+	);
+}
+
+/* Returns 0: the loop runs to its end */
+__naked __noinline __used
+static unsigned long cb_returns(void)
+{
+	asm volatile (
+		count_run()
+		"r0 = 0;"
+		"exit;"
+		:
+		: __imm_addr(runs)
+	);
+}
+
+__naked __noinline __used
+static unsigned long always_unwinds(void)
+{
+	asm volatile (
+		"r0 = 5;"
+		unwind_exit
+	);
+}
+
+/* What it calls unwinds. Its pad counts the run and unwinds on. */
+__naked __noinline __used
+static unsigned long cb_passes_on(void)
+{
+	asm volatile (
+		"call always_unwinds;"
+		unwind_goto(2)
+		"r0 = 0;"
+		"exit;"
+		count_run()
+		"r0 = 7;"
+		unwind_exit
+		:
+		: __imm_addr(runs)
+	);
+}
+
+/*
+ * The pad after the call of the helper returns the number of runs plus what
+ * the helper left in r0, which is what the callback unwound with, plus 41.
+ */
+#define PAD					\
+	"r1 = %[runs] ll;"			\
+	"r1 = *(u64 *)(r1 + 0);"		\
+	"r0 += r1;"				\
+	"r0 += 41;"				\
+	"exit;"
+
+/* The loop is inlined, and so is its pad: unwind_goto after the call of the callback */
+SEC("tc")
+__description("unwind cb: the callback of bpf_loop unwinds on its first run")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(47)
+__xlated("unwind_goto pc+7")
+__naked int loop_cb_unwinds(void)
+{
+	asm volatile (
+		"r1 = 10;"
+		"r2 = %[cb_unwinds];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		PAD
+		:
+		: __imm_ptr(cb_unwinds),
+		  __imm_addr(runs),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+SEC("tc")
+__description("unwind cb: the callback of bpf_loop unwinds on its third run")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(49)
+__naked int loop_cb_unwinds_third(void)
+{
+	asm volatile (
+		"r1 = 10;"
+		"r2 = %[cb_unwinds_third];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		PAD
+		:
+		: __imm_ptr(cb_unwinds_third),
+		  __imm_addr(runs),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+/* unwind_goto after a helper whose callback returns: the pad is never taken */
+SEC("tc")
+__description("unwind cb: unwind_goto after bpf_loop whose callback returns")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(3)
+__naked int loop_cb_returns_pad(void)
+{
+	asm volatile (
+		"r1 = 3;"
+		"r2 = %[cb_returns];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		PAD
+		:
+		: __imm_ptr(cb_returns),
+		  __imm_addr(runs),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+SEC("tc")
+__description("unwind cb: the exception goes through the pad of the callback")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(49)
+__naked int loop_cb_passes_on(void)
+{
+	asm volatile (
+		"r1 = 10;"
+		"r2 = %[cb_passes_on];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		PAD
+		:
+		: __imm_ptr(cb_passes_on),
+		  __imm_addr(runs),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+SEC("tc")
+__description("unwind cb: no unwind_goto after bpf_loop whose callback unwinds")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__failure __msg("no unwind_goto after the call of bpf_loop()")
+__naked int loop_cb_unwinds_no_pad(void)
+{
+	asm volatile (
+		"r1 = 10;"
+		"r2 = %[cb_unwinds];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		"exit;"
+		:
+		: __imm_ptr(cb_unwinds),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+/*
+ * A loop of no rounds never runs its callback, so the call needs no unwind_goto,
+ * and the inlined loop gets no pad: the insn after the call is what it was, not
+ * the jump of a pad.
+ */
+SEC("tc")
+__description("unwind cb: a loop of no rounds needs no pad")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(7)
+__naked int loop_none_cb_unwinds(void)
+{
+	asm volatile (
+		"r1 = 0;"
+		"r2 = %[cb_unwinds];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		"r0 = 7;"
+		"exit;"
+		:
+		: __imm_ptr(cb_unwinds),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+/*
+ * One path passes a callback that unwinds, the other one that returns. The pad
+ * is there for whoever takes it. 64 bytes come in: the first one.
+ */
+SEC("tc")
+__description("unwind cb: the callbacks of one call may differ")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(47)
+__naked int loop_cbs_differ(void)
+{
+	asm volatile (
+		"r6 = *(u32 *)(r1 + %[len]);"
+		"r2 = %[cb_unwinds];"
+		"if r6 == 64 goto 1f;"
+		"r2 = %[cb_returns];"
+	"1:"
+		"r1 = 3;"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		PAD
+		:
+		: __imm_const(len, offsetof(struct __sk_buff, len)),
+		  __imm_ptr(cb_unwinds),
+		  __imm_ptr(cb_returns),
+		  __imm_addr(runs),
+		  __imm(bpf_loop)
+		: __clobber_common, "r6"
+	);
+}
+
+SEC("tc")
+__description("unwind cb: the callback of bpf_for_each_map_elem over an array unwinds")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(47)
+__naked int for_each_array_cb_unwinds(void)
+{
+	asm volatile (
+		"r1 = %[array] ll;"
+		"r2 = %[cb_unwinds];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_for_each_map_elem];"
+		unwind_goto(1)
+		"exit;"
+		PAD
+		:
+		: __imm_addr(array),
+		  __imm_ptr(cb_unwinds),
+		  __imm_addr(runs),
+		  __imm(bpf_for_each_map_elem)
+		: __clobber_common
+	);
+}
+
+/* Two elements go in first. The hash iterators run the callback under RCU. */
+#define FILL_MAP(map)				\
+	"r1 = 1;"				\
+	"*(u32 *)(r10 - 4) = r1;"		\
+	"*(u32 *)(r10 - 8) = r1;"		\
+	"r1 = %[" #map "] ll;"			\
+	"r2 = r10;"				\
+	"r2 += -4;"				\
+	"r3 = r10;"				\
+	"r3 += -8;"				\
+	"r4 = 0;"				\
+	"call %[bpf_map_update_elem];"		\
+	"r1 = 2;"				\
+	"*(u32 *)(r10 - 4) = r1;"		\
+	"r1 = %[" #map "] ll;"			\
+	"r2 = r10;"				\
+	"r2 += -4;"				\
+	"r3 = r10;"				\
+	"r3 += -8;"				\
+	"r4 = 0;"				\
+	"call %[bpf_map_update_elem];"
+
+SEC("tc")
+__description("unwind cb: the callback of bpf_for_each_map_elem over a hash unwinds")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(47)
+__naked int for_each_hash_cb_unwinds(void)
+{
+	asm volatile (
+		FILL_MAP(hash)
+		"r1 = %[hash] ll;"
+		"r2 = %[cb_unwinds];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_for_each_map_elem];"
+		unwind_goto(1)
+		"exit;"
+		PAD
+		:
+		: __imm_addr(hash),
+		  __imm_ptr(cb_unwinds),
+		  __imm_addr(runs),
+		  __imm(bpf_map_update_elem),
+		  __imm(bpf_for_each_map_elem)
+		: __clobber_common
+	);
+}
+
+SEC("tc")
+__description("unwind cb: the callback of bpf_for_each_map_elem over an rhash unwinds")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(47)
+__naked int for_each_rhash_cb_unwinds(void)
+{
+	asm volatile (
+		FILL_MAP(rhash)
+		"r1 = %[rhash] ll;"
+		"r2 = %[cb_unwinds];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_for_each_map_elem];"
+		unwind_goto(1)
+		"exit;"
+		PAD
+		:
+		: __imm_addr(rhash),
+		  __imm_ptr(cb_unwinds),
+		  __imm_addr(runs),
+		  __imm(bpf_map_update_elem),
+		  __imm(bpf_for_each_map_elem)
+		: __clobber_common
+	);
+}
+
+/*
+ * The vma of the stack of the current task. The callback runs under the mmap
+ * lock, which the helper drops on its way out whichever way the callback left.
+ */
+SEC("tc")
+__description("unwind cb: the callback of bpf_find_vma unwinds")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(47)
+__naked int find_vma_cb_unwinds(void)
+{
+	asm volatile (
+		"call %[bpf_get_current_task_btf];"
+		"r6 = r0;"
+		"r1 = *(u64 *)(r6 + %[mm]);"
+		"r2 = *(u64 *)(r1 + %[start_stack]);"
+		"r1 = r6;"
+		"r3 = %[cb_unwinds];"
+		"r4 = 0;"
+		"r5 = 0;"
+		"call %[bpf_find_vma];"
+		unwind_goto(1)
+		"exit;"
+		PAD
+		:
+		: __imm_ptr(cb_unwinds),
+		  __imm_addr(runs),
+		  __imm(bpf_get_current_task_btf),
+		  __imm(bpf_find_vma),
+		  __imm_const(mm, offsetof(struct task_struct, mm)),
+		  __imm_const(start_stack, offsetof(struct mm_struct, start_stack))
+		: __clobber_common, "r6"
+	);
+}
+
+static bool less_returns(struct bpf_rb_node *a, const struct bpf_rb_node *b)
+{
+	return true;
+}
+
+/* Not naked: the branch keeps the return reachable. Unwinds with r0 = 5. */
+static bool less_unwinds(struct bpf_rb_node *a, const struct bpf_rb_node *b)
+{
+	runs++;
+	if (a)
+		asm volatile ("r0 = 5;" unwind_exit ::: "r0");
+	return false;
+}
+
+/* The kfunc with a pad after it. The verifier fills r4 and r5. r0 + 42 from the pad. */
+__naked __noinline __used
+static int rbtree_add_with_pad(void *root, void *node, void *less)
+{
+	asm volatile (
+		"call %[bpf_rbtree_add];"
+		unwind_goto(1)
+		"exit;"
+		"r0 += 42;"
+		"exit;"
+		:
+		: __imm(bpf_rbtree_add)
+		: __clobber_all
+	);
+}
+
+/*
+ * The other spelling of it, with its own copy of the body and of the pad. It
+ * takes meta and off from the prog, and the verifier fills them in.
+ */
+__naked __noinline __used
+static int rbtree_add_impl_with_pad(void *root, void *node, void *less)
+{
+	asm volatile (
+		"r4 = 0;"
+		"r5 = 0;"
+		"call %[bpf_rbtree_add_impl];"
+		unwind_goto(1)
+		"exit;"
+		"r0 += 42;"
+		"exit;"
+		:
+		: __imm(bpf_rbtree_add_impl)
+		: __clobber_all
+	);
+}
+
+/*
+ * The first node goes in without a comparison. The second one meets it, the
+ * comparator unwinds, and the kfunc drops the node on its way to the pad. The
+ * lock is the caller's.
+ */
+static __always_inline int rbtree_cb_unwinds_via(bool impl)
+{
+	struct node_data *n, *m;
+	int ret;
+
+	n = bpf_obj_new(typeof(*n));
+	if (!n)
+		return 1;
+	m = bpf_obj_new(typeof(*m));
+	if (!m) {
+		bpf_obj_drop(n);
+		return 2;
+	}
+
+	bpf_spin_lock(&glock);
+	if (impl) {
+		bpf_rbtree_add_impl(&groot, &n->node, less_returns, NULL, 0);
+		ret = rbtree_add_impl_with_pad(&groot, &m->node, (void *)less_unwinds);
+	} else {
+		bpf_rbtree_add(&groot, &n->node, less_returns);
+		ret = rbtree_add_with_pad(&groot, &m->node, (void *)less_unwinds);
+	}
+	bpf_spin_unlock(&glock);
+	return ret + runs;
+}
+
+SEC("tc")
+__description("unwind cb: the comparator of bpf_rbtree_add unwinds")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(48)
+int rbtree_cb_unwinds(void *ctx)
+{
+	return rbtree_cb_unwinds_via(false);
+}
+
+SEC("tc")
+__description("unwind cb: the comparator of bpf_rbtree_add_impl unwinds")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(48)
+int rbtree_impl_cb_unwinds(void *ctx)
+{
+	return rbtree_cb_unwinds_via(true);
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.53.0

[RFC PATCH bpf-next v1 11/11] selftests/bpf: Test what a callback unwinds with

[Kumar Kartikeya Dwivedi]

What a callback unwinds with reaches the pad as it is. A callback that
unwinds with a pointer to an element of the array loads, and the pad reads
through it: 6, which the callback put there, plus 41. A callback that
unwinds with a reference it took hands it to the pad, which drops it, or
is refused for leaking it. A pointer into the stack of the callback is
refused as at any return.

The pad knows what the callback unwound with: it adds the 5 of cb_unwinds
to fp and reads the slot that is there, which an unknown r0 could not do,
and the index of the run, which the callback may unwind with, it does not
know. Two callbacks on two paths into one call unwind with 5 and with 100,
and the pad is checked with each: with 100 the read is out of the frame.
A loop of one unwinds on its only round. A helper in the pad, after a
callback unwound on the first of two rounds, starts its rounds afresh, so
its callback is checked and its call needs an unwind_goto of its own. The
pad before the call of an inlined bpf_loop() is reached from the inlined
loop.

The comparator of bpf_rbtree_add() that unwinds with the node being added
gets its pad refused for reading through it, and so is a read of the node
after the call: the kfunc dropped it. What the caller holds of the tree is
gone with the unwind as well, as at an unlock.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/progs/verifier_unwind_cb.c  | 497 +++++++++++++++++-
 1 file changed, 496 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/verifier_unwind_cb.c b/tools/testing/selftests/bpf/progs/verifier_unwind_cb.c
index 8a3dee4330ac..d9db8084b843 100644
--- a/tools/testing/selftests/bpf/progs/verifier_unwind_cb.c
+++ b/tools/testing/selftests/bpf/progs/verifier_unwind_cb.c
@@ -11,7 +11,7 @@
  * BPF_RAW_INSN(), so here they are raw words: the code, src_reg = 1 in the
  * high nibble of the second byte, off in the next two bytes.
  */
-#define unwind_goto(off) ".8byte (" #off " << 16) | 0x10e5;"	/* BPF_JMP | BPF_JCOND */
+#define unwind_goto(off) ".8byte ((" #off " & 0xffff) << 16) | 0x10e5;"	/* BPF_JMP | BPF_JCOND */
 #define unwind_exit ".8byte 0x1095;"				/* BPF_JMP | BPF_EXIT */
 
 struct {
@@ -259,6 +259,374 @@ __naked int loop_cb_unwinds_no_pad(void)
 	);
 }
 
+/* Unwinds with a pointer in r0: the element of the array, with 6 in it */
+__naked __noinline __used
+static unsigned long cb_unwinds_with_ptr(void)
+{
+	asm volatile (
+		"r1 = 0;"
+		"*(u32 *)(r10 - 4) = r1;"
+		"r1 = %[array] ll;"
+		"r2 = r10;"
+		"r2 += -4;"
+		"call %[bpf_map_lookup_elem];"
+		"if r0 == 0 goto 1f;"
+		"r1 = 6;"
+		"*(u32 *)(r0 + 0) = r1;"
+		unwind_exit
+	"1:"
+		"exit;"
+		:
+		: __imm_addr(array),
+		  __imm(bpf_map_lookup_elem)
+		: __clobber_all
+	);
+}
+
+/* The helper hands r0 on as it is: the pad reads through the pointer */
+SEC("tc")
+__description("unwind cb: the callback unwinds with a pointer in r0")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(47)
+__naked int loop_cb_unwinds_with_ptr(void)
+{
+	asm volatile (
+		"r1 = 10;"
+		"r2 = %[cb_unwinds_with_ptr];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		"r0 = *(u32 *)(r0 + 0);"
+		"r0 += 41;"
+		"exit;"
+		:
+		: __imm_ptr(cb_unwinds_with_ptr),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+/* Unwinds with a pointer into its own stack */
+__naked __noinline __used
+static unsigned long cb_unwinds_with_fp(void)
+{
+	asm volatile (
+		"r0 = r10;"
+		"r0 += -8;"
+		unwind_exit
+	);
+}
+
+SEC("tc")
+__description("unwind cb: the callback unwinds with a pointer into its stack")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__failure __msg("cannot return stack pointer to the caller")
+__naked int loop_cb_unwinds_with_fp(void)
+{
+	asm volatile (
+		"r1 = 10;"
+		"r2 = %[cb_unwinds_with_fp];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		"exit;"
+		:
+		: __imm_ptr(cb_unwinds_with_fp),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+/*
+ * The pad knows what the callback unwound with: 5, which cb_unwinds leaves in
+ * r0, and reads the slot through it. An unknown r0 would be no offset for fp.
+ */
+SEC("tc")
+__description("unwind cb: the pad knows what the callback unwound with")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(9)
+__naked int loop_cb_r0_known(void)
+{
+	asm volatile (
+		"r1 = 9;"
+		"*(u64 *)(r10 - 8) = r1;"
+		"r1 = 10;"
+		"r2 = %[cb_unwinds];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		"r1 = r10;"
+		"r1 += r0;"
+		"r0 = *(u8 *)(r1 - 13);"
+		"exit;"
+		:
+		: __imm_ptr(cb_unwinds),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+/* Allocates a node and unwinds with it: the reference comes back in r0 */
+static __noinline long cb_unwinds_with_obj(u64 i, void *ctx)
+{
+	struct node_data *n = bpf_obj_new(typeof(*n));
+
+	if (n)
+		asm volatile ("r0 = %[n];" unwind_exit :: [n] "r"(n) : "r0");
+	return 0;
+}
+
+/* The pad drops what came back */
+SEC("tc")
+__description("unwind cb: the callback unwinds with a reference, which the pad drops")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(7)
+__naked int loop_cb_obj_dropped(void)
+{
+	asm volatile (
+		"r1 = 10;"
+		"r2 = %[cb_unwinds_with_obj];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		"r1 = r0;"
+		"call %[bpf_obj_drop];"
+		"r0 = 7;"
+		"exit;"
+		:
+		: __imm_ptr(cb_unwinds_with_obj),
+		  __imm(bpf_loop),
+		  __imm(bpf_obj_drop)
+		: __clobber_common
+	);
+}
+
+/* The pad may not leave without dropping it: the reference is the prog's now */
+SEC("tc")
+__description("unwind cb: the callback unwinds with a reference, which the pad leaks")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__failure __msg("Unreleased reference")
+__naked int loop_cb_obj_leaked(void)
+{
+	asm volatile (
+		"r1 = 10;"
+		"r2 = %[cb_unwinds_with_obj];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		"r0 = 7;"
+		"exit;"
+		:
+		: __imm_ptr(cb_unwinds_with_obj),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+/* Unwinds with the index of the run, which the pad doesn't know */
+__naked __noinline __used
+static unsigned long cb_unwinds_with_index(void)
+{
+	asm volatile (
+		"r0 = r1;"
+		count_run()
+		unwind_exit
+		:
+		: __imm_addr(runs)
+	);
+}
+
+SEC("tc")
+__description("unwind cb: the pad doesn't know the index the callback unwound with")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__failure __msg("math between fp pointer and register with unbounded min value is not allowed")
+__naked int loop_cb_index_unknown(void)
+{
+	asm volatile (
+		"r1 = 9;"
+		"*(u64 *)(r10 - 8) = r1;"
+		"r1 = 10;"
+		"r2 = %[cb_unwinds_with_index];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		"r1 = r10;"
+		"r1 += r0;"
+		"r0 = *(u8 *)(r1 - 13);"
+		"exit;"
+		:
+		: __imm_ptr(cb_unwinds_with_index),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+/* Unwinds with r0 = 100, no offset for the slot */
+__naked __noinline __used
+static unsigned long cb_unwinds_100(void)
+{
+	asm volatile (
+		"r0 = 100;"
+		unwind_exit
+	);
+}
+
+/*
+ * Two callbacks on two paths into one call, unwinding with 5 and with 100. The
+ * pad is checked with each: the second one puts the read out of the frame.
+ */
+SEC("tc")
+__description("unwind cb: the pad is checked with what each callback unwound with")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__failure __msg("invalid read from stack")
+__naked int loop_cbs_differ_pad_checked(void)
+{
+	asm volatile (
+		"r6 = *(u32 *)(r1 + %[len]);"
+		"r2 = %[cb_unwinds];"
+		"if r6 == 64 goto 1f;"
+		"r2 = %[cb_unwinds_100];"
+	"1:"
+		"r1 = 9;"
+		"*(u64 *)(r10 - 8) = r1;"
+		"r1 = 10;"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		"r1 = r10;"
+		"r1 += r0;"
+		"r0 = *(u8 *)(r1 - 13);"
+		"exit;"
+		:
+		: __imm_const(len, offsetof(struct __sk_buff, len)),
+		  __imm_ptr(cb_unwinds),
+		  __imm_ptr(cb_unwinds_100),
+		  __imm(bpf_loop)
+		: __clobber_common, "r6"
+	);
+}
+
+/* A loop of one: the round that unwinds is the last the loop had */
+SEC("tc")
+__description("unwind cb: the callback of a loop of one unwinds")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(47)
+__naked int loop_once_cb_unwinds(void)
+{
+	asm volatile (
+		"r1 = 1;"
+		"r2 = %[cb_unwinds];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		PAD
+		:
+		: __imm_ptr(cb_unwinds),
+		  __imm_addr(runs),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+/*
+ * The callback unwinds on the first of two rounds. The helper in the pad starts
+ * its rounds afresh, so its callback is checked, and the call needs its own
+ * unwind_goto.
+ */
+SEC("tc")
+__description("unwind cb: a helper in the pad starts its rounds afresh")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__failure __msg("no unwind_goto after the call of bpf_loop()")
+__naked int loop_cb_unwinds_then_loops(void)
+{
+	asm volatile (
+		"r1 = 2;"
+		"r2 = %[cb_unwinds];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(1)
+		"exit;"
+		"r1 = 1;"
+		"r2 = %[cb_unwinds];"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		"exit;"
+		:
+		: __imm_ptr(cb_unwinds),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
+/*
+ * The pad before the call: the inlined loop jumps back to it. Insn 13 is the
+ * unwind_goto, insn 1 the pad: the callback is loaded in the asm, so that the
+ * compiler puts nothing in front of it.
+ */
+SEC("tc")
+__description("unwind cb: the pad before the call of the inlined bpf_loop")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__success __retval(47)
+__xlated("unwind_goto pc+7")
+__naked int loop_cb_pad_before_call(void)
+{
+	asm volatile (
+		"goto 1f;"
+		PAD
+	"1:"
+		"r1 = 10;"
+		"r2 = %[cb_unwinds] ll;"
+		"r3 = 0;"
+		"r4 = 0;"
+		"call %[bpf_loop];"
+		unwind_goto(-13)
+		"exit;"
+		:
+		: __imm_addr(cb_unwinds),
+		  __imm_addr(runs),
+		  __imm(bpf_loop)
+		: __clobber_common
+	);
+}
+
 /*
  * A loop of no rounds never runs its callback, so the call needs no unwind_goto,
  * and the inlined loop gets no pad: the insn after the call is what it was, not
@@ -566,4 +934,131 @@ int rbtree_impl_cb_unwinds(void *ctx)
 	return rbtree_cb_unwinds_via(true);
 }
 
+/* Unwinds with the node it is handed first: the one being added */
+__naked __noinline __used
+static bool less_unwinds_with_node(struct bpf_rb_node *a, const struct bpf_rb_node *b)
+{
+	asm volatile (
+		"r0 = r1;"
+		unwind_exit
+	);
+}
+
+/* The pad reads the key of the node that came back in r0 */
+__naked __noinline __used
+static int rbtree_add_pad_reads_node(void *root, void *node, void *less)
+{
+	asm volatile (
+		"call %[bpf_rbtree_add];"
+		unwind_goto(1)
+		"exit;"
+		"r0 = *(u64 *)(r0 - 8);"
+		"exit;"
+		:
+		: __imm(bpf_rbtree_add)
+		: __clobber_all
+	);
+}
+
+/*
+ * The comparator unwinds with the node being added, which the kfunc drops on
+ * its way to the pad: there is nothing left to read through it.
+ */
+SEC("tc")
+__description("unwind cb: the comparator unwinds with the node the kfunc dropped")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__failure __msg("invalid mem access")
+int rbtree_cb_unwinds_with_node(void *ctx)
+{
+	struct node_data *n, *m;
+	int ret;
+
+	n = bpf_obj_new(typeof(*n));
+	if (!n)
+		return 1;
+	m = bpf_obj_new(typeof(*m));
+	if (!m) {
+		bpf_obj_drop(n);
+		return 2;
+	}
+
+	bpf_spin_lock(&glock);
+	bpf_rbtree_add(&groot, &n->node, less_returns);
+	ret = rbtree_add_pad_reads_node(&groot, &m->node, (void *)less_unwinds_with_node);
+	bpf_spin_unlock(&glock);
+	return ret;
+}
+
+/*
+ * The node the comparator unwound on is not in the tree: the kfunc dropped it
+ * on its way to the pad, and the caller has nothing left to read.
+ */
+SEC("tc")
+__description("unwind cb: the node the comparator unwound on is gone")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__failure __msg("invalid mem access")
+int rbtree_cb_unwinds_node_gone(void *ctx)
+{
+	struct node_data *n, *m;
+	long key;
+	int ret;
+
+	n = bpf_obj_new(typeof(*n));
+	if (!n)
+		return 1;
+	m = bpf_obj_new(typeof(*m));
+	if (!m) {
+		bpf_obj_drop(n);
+		return 2;
+	}
+
+	bpf_spin_lock(&glock);
+	bpf_rbtree_add(&groot, &n->node, less_returns);
+	ret = rbtree_add_with_pad(&groot, &m->node, (void *)less_unwinds);
+	key = m->key;
+	bpf_spin_unlock(&glock);
+	return ret + key;
+}
+
+/*
+ * The comparator was handed the node in the tree as well, and could have
+ * unwound with it, so what the caller holds of the tree is gone too, as at an
+ * unlock.
+ */
+SEC("tc")
+__description("unwind cb: what the caller holds of the tree is gone when the comparator unwinds")
+__arch_x86_64
+__arch_arm64
+__load_if_JITed()
+__failure __msg("invalid mem access")
+int rbtree_cb_unwinds_first_gone(void *ctx)
+{
+	struct node_data *n, *m;
+	struct bpf_rb_node *f;
+	long key = 0;
+	int ret;
+
+	n = bpf_obj_new(typeof(*n));
+	if (!n)
+		return 1;
+	m = bpf_obj_new(typeof(*m));
+	if (!m) {
+		bpf_obj_drop(n);
+		return 2;
+	}
+
+	bpf_spin_lock(&glock);
+	bpf_rbtree_add(&groot, &n->node, less_returns);
+	f = bpf_rbtree_first(&groot);
+	ret = rbtree_add_with_pad(&groot, &m->node, (void *)less_unwinds);
+	if (f)
+		key = ((struct node_data *)((void *)f - offsetof(struct node_data, node)))->key;
+	bpf_spin_unlock(&glock);
+	return ret + key;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.53.0

Re: [RFC PATCH bpf-next v1 08/11] bpf: Let the callback of bpf_user_ringbuf_drain unwind

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf: missing callback dynptr invalidation on bpf_user_ringbuf_drain() unwind
--

commit 5e41f31a219e475ec387e971ce587d69bc948017
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Let the callback of bpf_user_ringbuf_drain unwind

This commit adds an unwind pad to bpf_user_ringbuf_drain() so that its
callback can exit via unwind_exit while releasing the sample, clearing the
busy bit, and queuing any pending wakeup. It also adds
BPF_FUNC_user_ringbuf_drain to helper_has_cb_pad() in the verifier.

> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index 447863ece26a3..fdd997f36f73d 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
[ ... ]
> @@ -852,8 +854,23 @@ BPF_CALL_4(bpf_user_ringbuf_drain, struct bpf_map *, map,
>  		}
>  
>  		bpf_dynptr_init(&dynptr, sample, BPF_DYNPTR_TYPE_LOCAL, 0, size);
> -		ret = callback((uintptr_t)&dynptr, (uintptr_t)callback_ctx, 0, 0, 0);
> +		bpf_call_cb(callback, &dynptr, callback_ctx, 0, 0, 0, ret, unwound_at);
> +		if (0) {
> +unwound_at:
> +			/*
> +			 * The callback left with unwind_exit. The sample is
> +			 * consumed and counted like after a return, so that
> +			 * the wakeup below is sent for it, and the helper
> +			 * leaves through the door below, as the error paths
> +			 * do, with the r0 of the callback.
> +			 */
> +			unwound = true;
> +		}
>  		__bpf_user_ringbuf_sample_release(rb, size, flags);
> +		if (unwound) {
> +			samples++;
> +			goto schedule_work_return;
> +		}
>  	}
>  	ret = samples - discarded_samples;
>  
> @@ -867,6 +884,8 @@ BPF_CALL_4(bpf_user_ringbuf_drain, struct bpf_map *, map,
>  		irq_work_queue(&rb->work);
>  	else if (!(flags & BPF_RB_NO_WAKEUP) && samples > 0)
>  		irq_work_queue(&rb->work);
> +	if (unwound)
> +		bpf_unwind_leave();
>  	return ret;
>  }
>  
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 3c39cc0f358a1..fd63ff36eb909 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11409,6 +11409,7 @@ static bool helper_has_cb_pad(enum bpf_func_id func_id)
>  	case BPF_FUNC_loop:
>  	case BPF_FUNC_for_each_map_elem:
>  	case BPF_FUNC_find_vma:
> +	case BPF_FUNC_user_ringbuf_drain:
>  		return true;

[Severity: Critical]
Can a callback for bpf_user_ringbuf_drain() smuggle its R1 dynptr (or a
slice derived from it via bpf_dynptr_data() or bpf_dynptr_slice()) out to
the caller's unwind pad in R0 when exiting via unwind_exit?

In bpf_user_ringbuf_drain(), struct bpf_dynptr_kern dynptr lives on the
C stack frame of bpf_user_ringbuf_drain(). When the callback exits via
unwind_exit and jumps to unwound_at, bpf_user_ringbuf_drain() releases the
sample with __bpf_user_ringbuf_sample_release(), clears rb->busy, and
returns ret (the callback's R0) to the caller's unwind pad after its stack
frame is popped.

In the verifier, set_user_ringbuf_callback_state() initializes the
callback's R1 using mark_dynptr_cb_reg(), which sets R1 to
CONST_PTR_TO_DYNPTR with a fresh id that is not backed by a STACK_DYNPTR
slot in the callee's BPF stack:

kernel/bpf/verifier.c:set_user_ringbuf_callback_state() {
	...
	bpf_mark_reg_not_init(env, &callee->regs[BPF_REG_0]);
	mark_dynptr_cb_reg(env, &callee->regs[BPF_REG_1], BPF_DYNPTR_TYPE_LOCAL);
	callee->regs[BPF_REG_2] = caller->regs[BPF_REG_3];
	...
}

When the callback executes unwind_exit, prepare_func_exit() only rejects
PTR_TO_STACK in R0, copies callee->regs[BPF_REG_0] into
caller->regs[BPF_REG_0], and calls destroy_dynptrs_in_stack_slots(), which
only invalidates STACK_DYNPTR slots in the callee's stack frame:

kernel/bpf/verifier.c:prepare_func_exit() {
	...
			caller->regs[regno] = callee->regs[regno];
			bpf_diag_mod_end(env);
		}
	}

	/* Invalidate callee-local dynptrs and their slices before the frame goes away. */
	err = destroy_dynptrs_in_stack_slots(env, callee, 0,
					     callee->allocated_stack / BPF_REG_SIZE - 1);
	...
}

On the revisit of BPF_FUNC_user_ringbuf_drain in check_helper_call(),
unwound_r0 is preserved across caller-saved register scrubbing without
invalidating the callback's dynptr id:

kernel/bpf/verifier.c:check_helper_call() {
	...
	if (env->cur_state->unwound) {
		regs[BPF_REG_0] = unwound_r0;
		goto unwound;
	}
	...
}

Wouldn't this leave the caller's unwind pad with a live CONST_PTR_TO_DYNPTR
pointing to the popped C stack frame of bpf_user_ringbuf_drain(), or a live
PTR_TO_MEM slice pointing to the already-released user ringbuf sample?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=8

Re: [RFC PATCH bpf-next v1 02/11] bpf, arm64: Let the pad after a helper call have r0

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf, arm64: memory leak of ctx.landing_ex in bpf_jit_free()
--

commit a9bbc452976e17e7771ffe7fb013d30ad6f17aba
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf, arm64: Let the pad after a helper call have r0

This commit adds landing pads after the epilogue for unwind_goto instructions
that follow helper or kfunc calls on arm64, copying the return value from x0
into x8 before branching to the pad. The extable fixup entries for these
calls are updated during a post-epilogue pass to point to the generated
landings.

> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index b8fb49b00063c..284c1dd791ad7 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c

[ ... ]

> @@ -2387,6 +2447,16 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
>  	if (ctx.offset == NULL)
>  		goto out_off;
>  
> +	for (i = 1; i < prog->len; i++)
> +		if (bpf_is_unwind_goto(&prog->insnsi[i]) &&
> +		    unwind_goto_after_helper(&prog->insnsi[i]))
> +			nr_landings++;
> +	if (nr_landings) {
> +		ctx.landing_ex = kvzalloc_objs(int, nr_landings);
> +		if (ctx.landing_ex == NULL)
> +			goto out_off;
> +	}

[ ... ]

> @@ -2533,6 +2607,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
>  			prog->aux->priv_stack_ptr = NULL;
>  		}
>  		kvfree(ctx.offset);
> +		kvfree(ctx.landing_ex);
>  out_priv_stack:
>  		kfree(jit_data);
>  		prog->aux->jit_data = NULL;

[Severity: Medium]
Can this leak ctx.landing_ex if multi-function JIT compilation fails between
passes?

During the first pass of jit_subprogs() -> bpf_int_jit_compile() for a
subprogram (where prog->is_func is true and extra_pass is false), ctx is saved
in prog->aux->jit_data and the cleanup block is skipped so that ctx.landing_ex
and ctx.offset remain available for the second pass:

arch/arm64/net/bpf_jit_comp.c:bpf_int_jit_compile() {
    ...
	if (!prog->is_func || extra_pass) {
		...
	} else {
		jit_data->ctx = ctx;
		jit_data->ro_image = ro_image_ptr;
		jit_data->header = header;
		jit_data->ro_header = ro_header;
	}

	prog->bpf_func = (void *)ctx.ro_image + cfi_get_offset();
	prog->jited = 1;
	prog->jited_len = prog_size - cfi_get_offset();

	if (!prog->is_func || extra_pass) {
		...
		kvfree(ctx.offset);
		kvfree(ctx.landing_ex);
out_priv_stack:
		kfree(jit_data);
		prog->aux->jit_data = NULL;
	}
    ...
}

If a subsequent subprogram fails its initial JIT pass in jit_subprogs(), or if
an earlier subprogram fails during the extra_pass loop, jit_subprogs() jumps
to out_free and calls bpf_jit_free() on each subprogram. However,
bpf_jit_free() frees jit_data without freeing jit_data->ctx.landing_ex (or the
pre-existing jit_data->ctx.offset):

arch/arm64/net/bpf_jit_comp.c:bpf_jit_free() {
	if (prog->jited) {
		struct arm64_jit_data *jit_data = prog->aux->jit_data;
		...
		if (jit_data) {
			bpf_jit_binary_pack_finalize(jit_data->ro_header, jit_data->header);
			kfree(jit_data);
		}
    ...
}

Should bpf_jit_free() also call kvfree(jit_data->ctx.landing_ex) and
kvfree(jit_data->ctx.offset) before freeing jit_data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=2

Re: [RFC PATCH bpf-next v1 08/11] bpf: Let the callback of bpf_user_ringbuf_drain unwind

[Kumar Kartikeya Dwivedi]

On Wed, 28 Oct 2026 at 02:01, <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] bpf: missing callback dynptr invalidation on bpf_user_ringbuf_drain() unwind
> --
>
> commit 5e41f31a219e475ec387e971ce587d69bc948017
> Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>
> bpf: Let the callback of bpf_user_ringbuf_drain unwind
>
> This commit adds an unwind pad to bpf_user_ringbuf_drain() so that its
> callback can exit via unwind_exit while releasing the sample, clearing the
> busy bit, and queuing any pending wakeup. It also adds
> BPF_FUNC_user_ringbuf_drain to helper_has_cb_pad() in the verifier.
>
> > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > index 447863ece26a3..fdd997f36f73d 100644
> > --- a/kernel/bpf/ringbuf.c
> > +++ b/kernel/bpf/ringbuf.c
> [ ... ]
> > @@ -852,8 +854,23 @@ BPF_CALL_4(bpf_user_ringbuf_drain, struct bpf_map *, map,
> >               }
> >
> >               bpf_dynptr_init(&dynptr, sample, BPF_DYNPTR_TYPE_LOCAL, 0, size);
> > -             ret = callback((uintptr_t)&dynptr, (uintptr_t)callback_ctx, 0, 0, 0);
> > +             bpf_call_cb(callback, &dynptr, callback_ctx, 0, 0, 0, ret, unwound_at);
> > +             if (0) {
> > +unwound_at:
> > +                     /*
> > +                      * The callback left with unwind_exit. The sample is
> > +                      * consumed and counted like after a return, so that
> > +                      * the wakeup below is sent for it, and the helper
> > +                      * leaves through the door below, as the error paths
> > +                      * do, with the r0 of the callback.
> > +                      */
> > +                     unwound = true;
> > +             }
> >               __bpf_user_ringbuf_sample_release(rb, size, flags);
> > +             if (unwound) {
> > +                     samples++;
> > +                     goto schedule_work_return;
> > +             }
> >       }
> >       ret = samples - discarded_samples;
> >
> > @@ -867,6 +884,8 @@ BPF_CALL_4(bpf_user_ringbuf_drain, struct bpf_map *, map,
> >               irq_work_queue(&rb->work);
> >       else if (!(flags & BPF_RB_NO_WAKEUP) && samples > 0)
> >               irq_work_queue(&rb->work);
> > +     if (unwound)
> > +             bpf_unwind_leave();
> >       return ret;
> >  }
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 3c39cc0f358a1..fd63ff36eb909 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -11409,6 +11409,7 @@ static bool helper_has_cb_pad(enum bpf_func_id func_id)
> >       case BPF_FUNC_loop:
> >       case BPF_FUNC_for_each_map_elem:
> >       case BPF_FUNC_find_vma:
> > +     case BPF_FUNC_user_ringbuf_drain:
> >               return true;
>
> [Severity: Critical]
> Can a callback for bpf_user_ringbuf_drain() smuggle its R1 dynptr (or a
> slice derived from it via bpf_dynptr_data() or bpf_dynptr_slice()) out to
> the caller's unwind pad in R0 when exiting via unwind_exit?
>
> In bpf_user_ringbuf_drain(), struct bpf_dynptr_kern dynptr lives on the
> C stack frame of bpf_user_ringbuf_drain(). When the callback exits via
> unwind_exit and jumps to unwound_at, bpf_user_ringbuf_drain() releases the
> sample with __bpf_user_ringbuf_sample_release(), clears rb->busy, and
> returns ret (the callback's R0) to the caller's unwind pad after its stack
> frame is popped.
>
> In the verifier, set_user_ringbuf_callback_state() initializes the
> callback's R1 using mark_dynptr_cb_reg(), which sets R1 to
> CONST_PTR_TO_DYNPTR with a fresh id that is not backed by a STACK_DYNPTR
> slot in the callee's BPF stack:
>
> kernel/bpf/verifier.c:set_user_ringbuf_callback_state() {
>         ...
>         bpf_mark_reg_not_init(env, &callee->regs[BPF_REG_0]);
>         mark_dynptr_cb_reg(env, &callee->regs[BPF_REG_1], BPF_DYNPTR_TYPE_LOCAL);
>         callee->regs[BPF_REG_2] = caller->regs[BPF_REG_3];
>         ...
> }
>
> When the callback executes unwind_exit, prepare_func_exit() only rejects
> PTR_TO_STACK in R0, copies callee->regs[BPF_REG_0] into
> caller->regs[BPF_REG_0], and calls destroy_dynptrs_in_stack_slots(), which
> only invalidates STACK_DYNPTR slots in the callee's stack frame:
>
> kernel/bpf/verifier.c:prepare_func_exit() {
>         ...
>                         caller->regs[regno] = callee->regs[regno];
>                         bpf_diag_mod_end(env);
>                 }
>         }
>
>         /* Invalidate callee-local dynptrs and their slices before the frame goes away. */
>         err = destroy_dynptrs_in_stack_slots(env, callee, 0,
>                                              callee->allocated_stack / BPF_REG_SIZE - 1);
>         ...
> }
>
> On the revisit of BPF_FUNC_user_ringbuf_drain in check_helper_call(),
> unwound_r0 is preserved across caller-saved register scrubbing without
> invalidating the callback's dynptr id:
>
> kernel/bpf/verifier.c:check_helper_call() {
>         ...
>         if (env->cur_state->unwound) {
>                 regs[BPF_REG_0] = unwound_r0;
>                 goto unwound;
>         }
>         ...
> }
>
> Wouldn't this leave the caller's unwind pad with a live CONST_PTR_TO_DYNPTR
> pointing to the popped C stack frame of bpf_user_ringbuf_drain(), or a live
> PTR_TO_MEM slice pointing to the already-released user ringbuf sample?

Known bug which Ihor is already fixing.

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=8

Re: [RFC PATCH bpf-next v1 07/11] bpf: Let the callback of bpf_find_vma unwind

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: bpf_find_vma() unwound pad unlocks mmap_lock with IRQs disabled
- [High] bpf: vma pointer escapes bpf_find_vma() lock scope via r0 on unwind_exit
--

commit d84887aac74ddde9372c2538e6e860df21d923db
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Let the callback of bpf_find_vma unwind

This commit adds an unwind landing pad to bpf_find_vma() that releases the RCU
read lock, unlocks mmap_lock, drops any foreign mm reference, and returns the
callback's r0 when the callback leaves via unwind_exit.

> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> index 2ee7b3b3c10be..ae67c97fe845e 100644
> --- a/kernel/bpf/task_iter.c
> +++ b/kernel/bpf/task_iter.c

[ ... ]

> @@ -827,6 +828,20 @@ BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
>  		mmput_async(mm);
>  #endif
>  	return ret;
> +
> +unwound:
> +	/*
> +	 * The callback left with unwind_exit, under the mmap lock and in the
> +	 * RCU section. The way out is the same as after a return.
> +	 */
> +	rcu_read_unlock();
> +	bpf_mmap_unlock_mm(work, mm);
> +#ifdef CONFIG_MMU
> +	if (mmput_needed)
> +		mmput_async(mm);
> +#endif

[Severity: High]
Can the unwound pad end up calling mmap_read_unlock() and mmput_async() with
hardware interrupts disabled?

When bpf_find_vma() is entered with interrupts enabled for a foreign task, the
irqs_disabled() check passes, mmput_needed is set to true, and
bpf_mmap_unlock_guard_get() returns NULL because !irqs_disabled():

kernel/bpf/mmap_unlock_work.h:bpf_mmap_unlock_guard_get() {
	struct mmap_unlock_irq_work *work;

	if (!irqs_disabled())
		return NULL;
    ...
}

Inside the callback, callback_ctx (R3) can point to a stack slot in the
caller's frame. If the callback calls bpf_local_irq_save() on that caller
stack slot and then executes unwind_exit, process_bpf_exit_full() calls
check_resource_leak() with check_lock set to false because curframe > 0:

kernel/bpf/verifier.c:process_bpf_exit_full() {
	int err = check_resource_leak(env, exception_exit,
				      exception_exit || !env->cur_state->curframe,
				      exception_exit ? "bpf_throw" :
				      "BPF_EXIT instruction in main prog");
    ...
}

Unlike a normal callback return, unwind_exit sets state->unwound = true in
prepare_func_exit(), so push_callback_call() returns 0 on revisit without
verifying a second callback round and jumps via unwind_goto to the caller's
pad where bpf_local_irq_restore() restores the caller's stack slot.

At runtime, execution enters the unwound label in bpf_find_vma() with hardware
interrupts disabled while work is NULL and mmput_needed is true.
bpf_mmap_unlock_mm(NULL, mm) then calls mmap_read_unlock(mm) directly with
IRQs disabled instead of deferring via irq_work, and mmput_async(mm) is also
called with IRQs disabled, risking a deadlock on rq_lock during reader wakeup
and triggering an invalid wait context under CONFIG_PREEMPT_RT.

> +	bpf_unwind_leave();
> +	return r0;
>  }

[Severity: High]
What prevents the callback from escaping the vma pointer (or pointers walked
from it, such as vma->vm_file, &vma->vm_file->f_path, or
vma->vm_file->f_inode) in r0 via unwind_exit after mmap_lock, rcu_read_lock(),
and the foreign mm reference have been dropped?

In set_find_vma_callback_state(), R2 (struct vm_area_struct *vma) is marked as
a plain PTR_TO_BTF_ID:

kernel/bpf/verifier.c:set_find_vma_callback_state() {
    ...
	callee->regs[BPF_REG_2].type = PTR_TO_BTF_ID;
	__mark_reg_known_zero(&callee->regs[BPF_REG_2]);
	callee->regs[BPF_REG_2].btf =  btf_vmlinux;
	callee->regs[BPF_REG_2].btf_id = btf_tracing_ids[BTF_TRACING_TYPE_VMA];
    ...
}

When the callback leaves via unwind_exit, prepare_func_exit() skips the
SCALAR_VALUE check on R0 and copies callee->regs[BPF_REG_0] into
caller->regs[BPF_REG_0]:

kernel/bpf/verifier.c:prepare_func_exit() {
    ...
	if (callee->in_callback_fn && !unwinds) {
		if (r0->type != SCALAR_VALUE) {
			verbose(env, "R0 not a scalar value\n");
			return -EACCES;
		}
    ...
	} else {
		for (i = 0; i < nregs; i++) {
			u32 regno = ret_regs[i];

			bpf_diag_mod_begin(env, &caller->regs[regno], &callee->regs[regno],
					   BPF_DIAG_MOD_WRITE);
			caller->regs[regno] = callee->regs[regno];
			bpf_diag_mod_end(env);
		}
	}
    ...
}

Once the unwound pad in bpf_find_vma() runs rcu_read_unlock(),
bpf_mmap_unlock_mm(work, mm), and mmput_async(mm), the caller's unwind pad
receives the plain PTR_TO_BTF_ID in R0 outside the lock and RCU scope. If the
VMA and its backing file or inode are concurrently unmapped and freed, passing
that dangling R0 pointer to helpers accepting ARG_PTR_TO_BTF_ID (such as
bpf_ima_file_hash(), bpf_ima_inode_hash(), bpf_d_path(), or
bpf_inode_storage_get()) can trigger a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=7

Re: [RFC PATCH bpf-next v1 04/11] bpf: Give the inlined bpf_loop a pad

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: inline_bpf_loop() emits pad to unseen target deleted by dead code removal
- [High] bpf: inline_bpf_loop() leaves orphaned unwind_goto after LDX_MEM when !pad
--

commit f87a8cc4d4bc638d4f9d4773f7657f7aec4d7868
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Give the inlined bpf_loop a pad

This commit updates inline_bpf_loop() to emit an unwind landing pad when the
callback subprogram can unwind and an unwind_goto instruction follows the
bpf_loop() call. The inlined pad restores callee-saved registers R6-R8, jumps
to the caller's landing pad, and rewrites the original unwind_goto after the
bpf_loop() call into a no-op jump.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index f062ef7dd943d..94a379c94c675 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -2836,6 +2836,15 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
>  	return 0;
>  }
>  
> +/*
> + * When the callback can unwind and unwind_goto follows the call of bpf_loop,
> + * unwind_goto follows the call of the callback, and the pad restores r6-r8 and
> + * jumps to the pad after the call of bpf_loop with the r0 of the callback, which
> + * the verifier has checked with the state of that r0. The unwind_goto that
> + * followed the call of bpf_loop doesn't follow a call anymore and becomes a
> + * no-op jump. A loop of no rounds never runs its callback, so the verifier asked
> + * for no unwind_goto there, and the insn after the call is some other insn.
> + */
>  static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
>  					int position,
>  					s32 stack_base,
> @@ -2848,6 +2857,8 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
>  	int reg_loop_max = BPF_REG_6;
>  	int reg_loop_cnt = BPF_REG_7;
>  	int reg_loop_ctx = BPF_REG_8;
> +	bool pad = env->subprog_info[callback_subprogno].can_unwind &&
> +		   bpf_is_unwind_goto(&env->prog->insnsi[position + 1]);

[Severity: High]
Can this leave a live BPF_JMP32_A instruction targeting a dead caller landing
pad that gets removed by bpf_opt_remove_dead_code(), leading to an
out-of-bounds read in addrs[] or ctx->offset[] during JIT compilation?

In inline_bpf_loop(), pad is enabled without checking whether the target of
the unwind_goto at position + 1 was actually visited during verification:

    caller_pad = position + 2 + env->prog->insnsi[position + 1].off;
    env->insn_aux_data[caller_pad].seen

The caller's landing pad can remain unseen while can_unwind and
bpf_is_unwind_goto(&env->prog->insnsi[position + 1]) are both true, for
example when bpf_loop() is called with 0 iterations (r1 == 0, which skips
push_callback_call() in check_helper_call() while still marking the call site
fit_for_inline in update_loop_inline_state()) or when unwind_exit in the
callback lies on a branch not taken for this call site.

In bpf_check(), bpf_optimize_bpf_loop() runs before
bpf_opt_hard_wire_dead_code_branches() and bpf_opt_remove_dead_code():

bpf_check()
  bpf_optimize_bpf_loop()
    inline_bpf_loop()
      bpf_patch_insn_data()
        adjust_insn_aux_data()
  bpf_opt_hard_wire_dead_code_branches()
  bpf_opt_remove_dead_code()
    verifier_remove_insns()
      bpf_remove_insns()
        bpf_adj_branches()
          bpf_adj_delta_to_off()

When bpf_patch_insn_data() patches the inlined loop, adjust_insn_aux_data()
marks all patched instructions in insn_buf as seen, including the inlined
pad and its trailing BPF_JMP32_A instruction, while the original unwind_goto
is overwritten with BPF_JMP_A(0).

When bpf_opt_hard_wire_dead_code_branches() runs next, it preserves the
inlined BPF_UNWIND_GOTO because its target (the inlined pad) is marked seen,
and ignores the BPF_JMP32_A instruction because it is an unconditional jump.

When bpf_opt_remove_dead_code() then removes the unseen caller landing pad
range [pos, end_old), bpf_adj_delta_to_off() only adjusts forward jumps when
curr + off + 1 >= end_old. Because BPF_JMP32_A targets caller_pad inside the
removed range [pos, end_old), its imm offset is not adjusted. If caller_pad
is at the end of a subprogram and preceded by other dead instructions (or
followed by a subprogram where bpf_do_misc_fixups() calls adjust_jmp_off()
for may_goto), BPF_JMP32_A jumps past the end of the subprogram, causing
do_jit() on x86-64 to read past addrs[] and bpf2a64_offset() on arm64 to read
past ctx->offset[].

Should pad also require env->insn_aux_data[position + 2 +
env->prog->insnsi[position + 1].off].seen to be true?

>  
>  	struct bpf_insn *insn_buf = env->insn_buf;
>  	struct bpf_prog *new_prog;
> @@ -2865,7 +2876,7 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
>  	 */
>  	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JLE, BPF_REG_1, BPF_MAX_LOOPS, 2);
>  	insn_buf[cnt++] = BPF_MOV32_IMM(BPF_REG_0, -E2BIG);
> -	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JA, 0, 0, 16);
> +	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JA, 0, 0, pad ? 22 : 16);
>  	/* spill R6, R7, R8 to use these as loop vars */
>  	insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_6, r6_offset);
>  	insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_7, r7_offset);
> @@ -2877,17 +2888,20 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
>  	/* loop header,
>  	 * if reg_loop_cnt >= reg_loop_max skip the loop body
>  	 */
> -	insn_buf[cnt++] = BPF_JMP_REG(BPF_JGE, reg_loop_cnt, reg_loop_max, 5);
> +	insn_buf[cnt++] = BPF_JMP_REG(BPF_JGE, reg_loop_cnt, reg_loop_max, pad ? 6 : 5);
>  	/* callback call,
>  	 * correct callback offset would be set after patching
>  	 */
>  	insn_buf[cnt++] = BPF_MOV64_REG(BPF_REG_1, reg_loop_cnt);
>  	insn_buf[cnt++] = BPF_MOV64_REG(BPF_REG_2, reg_loop_ctx);
>  	insn_buf[cnt++] = BPF_CALL_REL(0);
> +	if (pad)
> +		/* to the pad below when the callback left with unwind_exit */
> +		insn_buf[cnt++] = BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, BPF_UNWIND_GOTO, 7, 0);
>  	/* increment loop counter */
>  	insn_buf[cnt++] = BPF_ALU64_IMM(BPF_ADD, reg_loop_cnt, 1);
>  	/* jump to loop header if callback returned 0 */
> -	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, -6);
> +	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, pad ? -7 : -6);
>  	/* return value of bpf_loop,
>  	 * set R0 to the number of iterations
>  	 */
> @@ -2896,12 +2910,35 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
>  	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_10, r6_offset);
>  	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, r7_offset);
>  	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_8, BPF_REG_10, r8_offset);
> +	if (pad) {
> +		/* over the pad */
> +		insn_buf[cnt++] = BPF_JMP_IMM(BPF_JA, 0, 0, 4);
> +		/* the pad: r0 is what the callback unwound with */
> +		insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_10, r6_offset);
> +		insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, r7_offset);
> +		insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_8, BPF_REG_10, r8_offset);
> +		/*
> +		 * To the pad of the caller, where the unwind_goto that follows
> +		 * the patch goes. Its offset is known once the patch is in and
> +		 * the jumps across it are adjusted, see below.
> +		 */
> +		insn_buf[cnt++] = BPF_JMP32_A(0);
> +	}
>  
>  	*total_cnt = cnt;
>  	new_prog = bpf_patch_insn_data(env, position, insn_buf, cnt);
>  	if (!new_prog)
>  		return new_prog;
>  
> +	if (pad) {
> +		struct bpf_insn *unwind_goto = &new_prog->insnsi[position + cnt];
> +
> +		/* the jump of the pad, one insn before, takes the target of the unwind_goto */
> +		new_prog->insnsi[position + cnt - 1].imm = unwind_goto->off + 1;
> +		/* and the unwind_goto after the call of bpf_loop follows no call anymore */
> +		*unwind_goto = BPF_JMP_A(0);
> +	}

[Severity: High]
What happens if bpf_is_unwind_goto(&new_prog->insnsi[position + cnt]) is true,
but pad is false (for example, when
env->subprog_info[callback_subprogno].can_unwind is false, or when the
caller's landing pad was not reached from this call site)?

In check_unwind_insns() and check_cb_unwind(), an unwind_goto instruction is
permitted after a bpf_loop() call even when the callback subprogram has
can_unwind == false:

kernel/bpf/verifier.c:check_cb_unwind() {
    if (!env->subprog_info[subprog].can_unwind)
        return 0;
    ...
}

When inline_bpf_loop() runs with pad == false, it replaces the bpf_loop()
call with 19 instructions ending in BPF_LDX_MEM(BPF_DW, BPF_REG_8,
BPF_REG_10, r8_offset), and the error branch BPF_JMP_IMM(BPF_JA, 0, 0, 16)
jumps directly to new_prog->insnsi[position + cnt].

Because the rewrite of *unwind_goto = BPF_JMP_A(0) is guarded by if (pad),
an unwind_goto following the bpf_loop() call is left in place when !pad. If
the landing pad targeted by that unwind_goto was marked seen via another
unwinding call in the same subprogram, bpf_opt_hard_wire_dead_code_branches()
sees:

    if (bpf_is_unwind_goto(insn) && aux_data[i + 1 + insn->off].seen)

and preserves the orphaned unwind_goto instruction. That leaves
BPF_UNWIND_GOTO immediately following BPF_LDX_MEM instead of a call
instruction, causing the x86-64 and arm64 JITs (which assume insn[i - 1] is a
call instruction) to emit bogus EX_TYPE_BPF exception table entries and
unwind stubs keyed inside the BPF_LDX_MEM instruction.

Should new_prog->insnsi[position + cnt] be rewritten to BPF_JMP_A(0) whenever
bpf_is_unwind_goto(&new_prog->insnsi[position + cnt]) is true, regardless of
whether pad is true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=4

Re: [RFC PATCH bpf-next v1 05/11] bpf: Let the callback of bpf_loop unwind

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: inline_bpf_loop() emits dangling jump to dead unwind pad
- [Medium] bpf: clobbering of caller R2 in prepare_func_exit() on callback unwind
--

commit 342597e9c392b90ccb12e07eaf21a9e820d7f364
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Let the callback of bpf_loop unwind

This commit adds an unwind landing pad to bpf_loop() using bpf_call_cb() and
bpf_unwind_leave(), and updates helper_has_cb_pad() so the verifier permits
bpf_loop() callbacks to unwind.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ac9dbe0821811..4b4d8479fa87b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11402,10 +11402,15 @@ static int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog)
>  	return err;
>  }
>  
> -/* The helper calls its callback with a pad, see bpf_call_cb(). None yet. */
> +/* The helper calls its callback with a pad, see bpf_call_cb() */
>  static bool helper_has_cb_pad(enum bpf_func_id func_id)
>  {
> -	return false;
> +	switch (func_id) {
> +	case BPF_FUNC_loop:
> +		return true;
> +	default:
> +		return false;
> +	}
>  }

[Severity: Critical]
When helper_has_cb_pad(BPF_FUNC_loop) returns true, can inline_bpf_loop() in
kernel/bpf/fixups.c emit a live BPF_JMP32_A instruction that jumps to a dead
landing pad which is subsequently removed by dead code elimination, or leave a
stray BPF_UNWIND_GOTO after a load instruction?

In kernel/bpf/verifier.c:check_unwind_insns(), subprog_info[subprog].can_unwind
is set statically whenever the callback contains an unwind_exit instruction,
and check_cb_unwind() then requires an unwind_goto instruction at position + 1
immediately after the bpf_loop() call.

If a privileged program calls bpf_loop() with flags == 0 and a single callback
target (so fit_for_inline is true), and the callback does not dynamically take
its unwind_exit path during verification of that call (for example, when
guarded by callback_ctx or loop index, or when nr_loops == 0), state->unwound
is never set at position + 1. In check_cond_jmp_op(), the unwind_goto branch
to the caller's landing pad (caller_pad = position + 2 + unwind_goto->off) is
never taken, leaving env->insn_aux_data[caller_pad].seen == 0.

In kernel/bpf/verifier.c:bpf_check(), bpf_optimize_bpf_loop() runs before dead
code elimination:

	if (ret == 0)
		ret = bpf_optimize_bpf_loop(env);

	if (is_priv) {
		if (ret == 0)
			bpf_opt_hard_wire_dead_code_branches(env);
		if (ret == 0)
			ret = bpf_opt_remove_dead_code(env);

Inside kernel/bpf/fixups.c:inline_bpf_loop(), pad is computed without checking
whether the target of unwind_goto at position + 1 was actually visited:

	bool pad = env->subprog_info[callback_subprogno].can_unwind &&
		   bpf_is_unwind_goto(&env->prog->insnsi[position + 1]);
	...
	if (pad) {
		struct bpf_insn *unwind_goto = &new_prog->insnsi[position + cnt];

		/* the jump of the pad, one insn before, takes the target of the unwind_goto */
		new_prog->insnsi[position + cnt - 1].imm = unwind_goto->off + 1;
		/* and the unwind_goto after the call of bpf_loop follows no call anymore */
		*unwind_goto = BPF_JMP_A(0);
	}

Because adjust_insn_aux_data() copies the call instruction's non-zero seen
count to all patched instructions, the inlined pad and its trailing
BPF_JMP32_A are marked seen != 0. bpf_opt_hard_wire_dead_code_branches() does
not fold the inlined BPF_UNWIND_GOTO (its target is the inlined pad, which has
seen != 0) and ignores the unconditional BPF_JMP32_A.

When bpf_opt_remove_dead_code() then deletes the unvisited caller_pad via
verifier_remove_insns() -> bpf_remove_insns() -> bpf_adj_branches() ->
bpf_adj_delta_to_off() in kernel/bpf/core.c:

	if (curr < pos && curr + off + 1 >= end_old)
		off += delta;
	else if (curr >= end_new && curr + off + 1 < end_new)
		off -= delta;

BPF_JMP32_A's offset is not adjusted because its target falls inside the
removed range [pos, end_old). If caller_pad was at the end of the subprogram,
func[i]->len shrinks while BPF_JMP32_A still jumps past the end of the
subprogram, causing out-of-bounds reads of addrs[i + insn->imm] in do_jit() in
arch/x86/net/bpf_jit_comp.c and ctx->offset[bpf_insn + off] in
bpf2a64_offset() in arch/arm64/net/bpf_jit_comp.c, or jumping into post-return
int3 padding or subsequent instructions.

Additionally, if the callback has can_unwind == false and an unwind_goto
follows the bpf_loop() call (which check_unwind_insns() and check_cb_unwind()
allow), pad is false in inline_bpf_loop(), so *unwind_goto = BPF_JMP_A(0) is
skipped, leaving BPF_UNWIND_GOTO immediately after BPF_LDX_MEM instead of a
call instruction when the landing pad is shared (seen != 0) or !is_priv.

[Severity: Medium]
What happens in prepare_func_exit() in kernel/bpf/verifier.c when a bpf_loop()
callback subprogram with a 9 to 16 byte BTF return type (ret_reg_pair == true)
unwinds via unwind_exit?

In kernel/bpf/verifier.c:bpf_compute_subprog_ret_regs(), any static subprogram
whose BTF return type has size in [9, 16] bytes gets ret_reg_pair = true, and
neither check_func_args() nor btf_check_subprog_call() rejects such a
subprogram when passed as ARG_PTR_TO_FUNC.

With helper_has_cb_pad(BPF_FUNC_loop) now returning true, the
callee->in_callback_fn && unwinds path in prepare_func_exit() becomes
reachable:

	callee = state->frame[state->curframe];
	r0 = &callee->regs[BPF_REG_0];
	nregs = bpf_ret_reg_pair(env, callee->subprogno) ? 2 : 1;
	for (i = 0; i < nregs; i++) {
		if (callee->regs[ret_regs[i]].type != PTR_TO_STACK)
			continue;
		verbose(env, "cannot return stack pointer to the caller\n");
		return -EINVAL;
	}
	...
		for (i = 0; i < nregs; i++) {
			u32 regno = ret_regs[i];

			bpf_diag_mod_begin(env, &caller->regs[regno], &callee->regs[regno],
					   BPF_DIAG_MOD_WRITE);
			caller->regs[regno] = callee->regs[regno];
			bpf_diag_mod_end(env);
		}

Even though bpf_call_cb() in bpf_loop() only propagates R0 to the landing pad,
prepare_func_exit() uses nregs = 2 (ret_regs = { BPF_REG_0, BPF_REG_2 })
without checking !callee->in_callback_fn.

First, because set_loop_callback_state() initializes the callback's R2 to
caller->regs[BPF_REG_3] (callback_ctx), passing a stack pointer as
callback_ctx causes the PTR_TO_STACK check on callee->regs[BPF_REG_2] to
falsely reject the unwind if the callback did not overwrite R2.

Second, prepare_func_exit() copies callee->regs[BPF_REG_2] into
caller->regs[BPF_REG_2] before setting *insn_idx = callee->callsite to revisit
the bpf_loop() call instruction. When check_helper_call() re-runs with
env->cur_state->unwound == true, check_func_args() re-validates
caller->regs[BPF_REG_1..5] before caller-saved registers are scrubbed. Because
caller->regs[BPF_REG_2] (the callback_fn argument) was overwritten with the
callback's R2, could check_func_args() fail verification or update
meta.subprogno in update_loop_inline_state() to a different subprogram?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=5

Re: [RFC PATCH bpf-next v1 01/11] bpf: Give a helper a way to call its callback with a pad

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: clobber of local register variables during arg evaluation in __bpf_call_cb()
- [High] bpf: dead-store elimination of return address write in bpf_unwind_leave()
--

commit d437def6fc298113c802447cf3c5d5281b1f3dd3
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Give a helper a way to call its callback with a pad

This commit introduces bpf_call_cb() and bpf_unwind_leave() in
include/linux/bpf_unwind.h, along with bpf_unwind_target() in the x86-64
and arm64 JITs, so C helpers invoking BPF callbacks can catch callback
unwinds via an EX_TYPE_BPF exception table entry and redirect their own
return address to the caller's unwind pad.

> diff --git a/include/linux/bpf_unwind.h b/include/linux/bpf_unwind.h
> new file mode 100644
> index 0000000000000..190d6dbae8568
> --- /dev/null
> +++ b/include/linux/bpf_unwind.h
> @@ -0,0 +1,282 @@

[ ... ]

> +#define __bpf_call_cb(type, fn, a1, a2, a3, a4, a5, r0, unwound)		\
> +do {										\
> +	register u64 __fn asm("r11") = (u64)(long)(fn);				\
> +	register u64 __a5 asm("r8") = (u64)(long)(a5);				\
> +	register u64 __c5 asm("r8");						\
> +	register u64 __c11 asm("r11");						\
> +	u64 __c1, __c2, __c3, __c4;						\
> +										\
> +	asm_goto_output(BPF_KCFI_CHECK(type)					\
> +		 "	" CALL_NOSPEC "\n"					\
> +		 "1:\n"								\
> +		 _ASM_EXTABLE_TYPE(1b, %l[unwound], EX_TYPE_BPF)		\
> +		 : "=a"(r0), "=D"(__c1), "=S"(__c2), "=d"(__c3), "=c"(__c4),	\
> +		   "=r"(__c5), "=r"(__c11)					\
> +		 : [thunk_target] "r"(__fn), "D"((u64)(long)(a1)),		\
> +		   "S"((u64)(long)(a2)), "d"((u64)(long)(a3)),			\
> +		   "c"((u64)(long)(a4)), "r"(__a5)				\
> +		 : "r9", "r10", "memory", "cc"					\
> +		 : unwound);							\
> +} while (0)
> +
> +/*
> + * The return address of the helper is the word below its canonical frame
> + * address. The pad is in the frame that BPF code called, see above, so there is
> + * a pad to go to: a miss is a bug, and the exception would be lost in a return.
> + */
> +static __always_inline void bpf_unwind_leave(void)
> +{
> +	unsigned long *slot = (unsigned long *)__builtin_dwarf_cfa() - 1;
> +	unsigned long to = bpf_unwind_target(*slot);
> +
> +	WARN_ON_ONCE(to == *slot);
> +	*slot = to;
> +}

[ ... ]

> +#define __bpf_call_cb(type, fn, a1, a2, a3, a4, a5, r0, unwound)		\
> +	___bpf_call_cb(__UNIQUE_ID(unwound), type, fn, a1, a2, a3, a4, a5, r0, unwound)
> +
> +#define ___bpf_call_cb(lbl, type, fn, a1, a2, a3, a4, a5, r0, unwound)		\
> +do {										\
> +	register u64 __fn asm("x15") = (u64)(long)(fn);				\
> +	register u64 __a0 asm("x0") = (u64)(long)(a1);				\
> +	register u64 __a1 asm("x1") = (u64)(long)(a2);				\
> +	register u64 __a2 asm("x2") = (u64)(long)(a3);				\
> +	register u64 __a3 asm("x3") = (u64)(long)(a4);				\
> +	register u64 __a4 asm("x4") = (u64)(long)(a5);				\

[Severity: High]
Can evaluating the macro arguments in ___bpf_call_cb() on arm64 and
__bpf_call_cb() on x86-64 clobber the caller-saved local register
variables before the asm_goto_output() statement is reached?

On arm64, ___bpf_call_cb() in include/linux/bpf_unwind.h initializes x15
and x0..x4 directly from the macro expressions in sequence:

	register u64 __fn asm("x15") = (u64)(long)(fn);
	register u64 __a0 asm("x0") = (u64)(long)(a1);
	register u64 __a1 asm("x1") = (u64)(long)(a2);
	register u64 __a2 asm("x2") = (u64)(long)(a3);
	register u64 __a3 asm("x3") = (u64)(long)(a4);
	register u64 __a4 asm("x4") = (u64)(long)(a5);

And on x86-64, __bpf_call_cb() assigns __fn (r11) and __a5 (r8) before
evaluating a1..a4 in the asm_goto_output() input operands:

	register u64 __fn asm("r11") = (u64)(long)(fn);
	register u64 __a5 asm("r8") = (u64)(long)(a5);
	...
		 : [thunk_target] "r"(__fn), "D"((u64)(long)(a1)),
		   "S"((u64)(long)(a2)), "d"((u64)(long)(a3)),
		   "c"((u64)(long)(a4)), "r"(__a5)

Per GCC and Clang local register variable rules, local register variables
do not reserve caller-saved registers across function calls that occur
between their assignment and the extended asm statement.

Later in this series, commit "bpf: Let the callback of
bpf_for_each_map_elem unwind" updates bpf_each_rhash_elem() in
kernel/bpf/hashtab.c to pass a helper call and pointer dereferences as
macro arguments:

	bpf_call_cb(callback_fn, map, elem->data,
		    rhtab_elem_value(elem, map->key_size), callback_ctx, 0,
		    ret, unwound);

If evaluating a3 or another argument invokes a function call -- such as an
out-of-line call to rhtab_elem_value() (which is marked static inline
rather than __always_inline) or sanitizer instrumentation like
__asan_load4() under outline KASAN or __tsan_read4() under KCSAN -- that
call executes after __fn (x15 / r11) and earlier argument registers (x0,
x1 / r8) have already been populated, clobbering the callback target and
arguments prior to blr x15 or CALL_NOSPEC.

Should fn and a1..a5 be evaluated into ordinary C temporary variables
first before assigning them to the local register variables, similar to
how include/linux/arm-smccc.h stages register arguments?

> +	register u64 __c0 asm("x0");						\
> +	register u64 __c1 asm("x1");						\
> +	register u64 __c2 asm("x2");						\
> +	register u64 __c3 asm("x3");						\
> +	register u64 __c4 asm("x4");						\
> +	register u64 __c15 asm("x15");						\

[ ... ]

> +static __always_inline void bpf_unwind_leave(void)
> +{
> +	unsigned long *slot = (unsigned long *)__builtin_frame_address(0) + 1;
> +	unsigned long from = ptrauth_strip_kernel_insn_pac(*slot);
> +	unsigned long to = bpf_unwind_target(from);
> +
> +	WARN_ON_ONCE(to == from);
> +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
> +	{
> +		register unsigned long x17 asm("x17") = to;
> +#ifdef __clang__
> +		register unsigned long x16 asm("x16") = (unsigned long)__builtin_sponentry();
> +#else
> +		register unsigned long x16 asm("x16") = (unsigned long)__builtin_dwarf_cfa();
> +#endif
> +
> +		asm("pacia1716" : "+r"(x17) : "r"(x16));
> +		to = x17;
> +	}
> +#endif
> +	*slot = to;
> +#ifdef CONFIG_SHADOW_CALL_STACK
> +	if (scs_is_enabled()) {
> +		unsigned long *scs;
> +
> +		asm("mov %0, x18" : "=r"(scs));
> +		scs[-1] = to;
> +	}
> +#endif
> +}

[Severity: High]
Could the compiler eliminate the *slot = to store in bpf_unwind_leave()
(on both x86-64 and arm64) as a dead store to the current function's stack
frame right before returning?

In include/linux/bpf_unwind.h, both implementations of bpf_unwind_leave()
compute slot from __builtin_dwarf_cfa() or __builtin_frame_address(0) as a
non-volatile unsigned long * and perform a plain store:

	*slot = to;

Because bpf_unwind_leave() is __always_inline and runs at the end of the
helper's unwound path immediately before return, nothing in C reads *slot
before the function exits. In GCC's RTL dead-store elimination pass
(pass_rtl_dse1, which runs before prologue/epilogue threading),
non-volatile frame-related stores based on arg_pointer_rtx
(__builtin_dwarf_cfa()) or hard_frame_pointer_rtx
(__builtin_frame_address(0)) that are not read before EXIT_BLOCK are
candidates for elimination. If *slot = to is elided, the helper returns to
its normal call site instead of the caller's unwind pad, losing the BPF
exception after the helper has already released its resources.

Existing kernel code that overwrites the stack return address via
__builtin_frame_address(0) in drivers/misc/lkdtm/cfi.c:set_return_addr()
explicitly marks the store volatile for this reason:

	/* Use of volatile is to make sure final write isn't seen as a dead store. */
	unsigned long * volatile *ret_addr =
		(unsigned long **)__builtin_frame_address(0) + FRAME_RA_OFFSET;

	/* Make sure we've found the right place on the stack before writing it. */
	if (no_pac_addr(*ret_addr) == expected)
		*ret_addr = (addr);

Should bpf_unwind_leave() use WRITE_ONCE(*slot, to) (and
WRITE_ONCE(scs[-1], to) on arm64) or qualify slot as volatile so the
return-address update cannot be optimized away?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=1

Re: [RFC PATCH bpf-next v1 09/11] bpf: Let the comparator of bpf_rbtree_add unwind

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: clobbered R2 on bpf_rbtree_add() callback unwind causes UAF
- [High] bpf: __bpf_rbtree_add() leaves node->owner poisoned on comparator unwind
--

commit 55e1518aab2a62c50b8098afe20794d027a69689
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Let the comparator of bpf_rbtree_add unwind

This commit allows the comparator callback of bpf_rbtree_add() and
bpf_rbtree_add_impl() to unwind via unwind_exit. When the comparator
unwinds, the kfunc drops the uninserted node before leaving to the landing
pad, and the verifier invalidates non-owning references and releases the
caller's owning reference on revisit.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 648abe1ba63ae..f8708b7181d72 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c

[ ... ]

> @@ -2784,7 +2788,8 @@ static int __bpf_rbtree_add(struct bpf_rb_root *root,
>  
>  	while (*link) {
>  		parent = *link;
> -		if (cb((uintptr_t)node, (uintptr_t)parent, 0, 0, 0)) {
> +		bpf_call_cb(cb, node, parent, 0, 0, 0, r0, unwound);
> +		if (r0) {
>  			link = &parent->rb_left;
>  		} else {
>  			link = &parent->rb_right;
> @@ -2796,6 +2801,15 @@ static int __bpf_rbtree_add(struct bpf_rb_root *root,
>  	rb_insert_color_cached(n, (struct rb_root_cached *)root, leftmost);
>  	WRITE_ONCE(node->owner, root);
>  	return 0;
> +
> +unwound:
> +	/*
> +	 * The comparator left with unwind_exit. The node is not in the tree, and
> +	 * the caller has let go of it: drop it, as the error path above does.
> +	 */
> +	__bpf_obj_drop_impl((void *)n - off, rec, false);
> +	bpf_unwind_leave();
> +	return r0;
>  }

[Severity: High]
Does the unwound path in __bpf_rbtree_add() leave node->owner poisoned with
BPF_PTR_POISON when the object survives __bpf_obj_drop_impl()?

At the start of __bpf_rbtree_add() in kernel/bpf/helpers.c, node->owner is
transitioned from NULL to BPF_PTR_POISON before the traversal loop:

	if (cmpxchg(&node->owner, NULL, BPF_PTR_POISON)) {
		/* Only called from BPF prog, no need to migrate_disable */
		__bpf_obj_drop_impl((void *)n - off, rec, false);
		return -EINVAL;
	}

When the comparator callback cb exits via unwind_exit during the while (*link)
loop, control jumps to unwound after cmpxchg() has already set node->owner to
BPF_PTR_POISON, and before WRITE_ONCE(node->owner, root) is reached.

If the object contains a bpf_refcount field and another active reference to it
exists (for example via bpf_refcount_acquire() before calling bpf_rbtree_add()
or inside the comparator before unwinding), __bpf_obj_drop_impl() decrements
the refcount and returns without freeing the object:

	if (rec && rec->refcount_off >= 0 &&
	    !refcount_dec_and_test((refcount_t *)(p + rec->refcount_off))) {
		return;
	}

Unlike bpf_rb_root_free(), which clears node->owner back to NULL before
dropping the reference:

	/* Ensure __bpf_rbtree_add() sees the node as unlinked. */
	smp_store_release(&node->owner, NULL);
	__bpf_obj_drop_impl(obj, field->graph_root.value_rec, false);

the unwound path in __bpf_rbtree_add() leaves node->owner set to
BPF_PTR_POISON on the surviving unlinked object, causing any subsequent
bpf_rbtree_add() or bpf_rbtree_add_impl() call on that object to fail the
cmpxchg() check with -EINVAL. Should node->owner be reset to NULL before
calling __bpf_obj_drop_impl() in the unwound path?

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fd63ff36eb909..50c3d44c37e15 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -15738,7 +15738,21 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  		id = regs[BPF_REG_2].id;
>  		insn_aux->insert_off = regs[BPF_REG_2].var_off.value;
>  		insn_aux->kptr_struct_meta = btf_find_struct_meta(meta.arg_btf, meta.arg_btf_id);
> -		ref_convert_owning_non_owning(env, id);
> +		/*
> +		 * The comparator unwound: the node never went in, and the
> +		 * kfunc dropped it on its way to the pad. The comparator was
> +		 * handed the node too, and may have unwound with it in r0, so
> +		 * every non-owning reference goes. Otherwise the node is in
> +		 * the tree now.
> +		 */
> +		if (env->cur_state->unwound) {
> +			invalidate_non_owning_refs(env);
> +			err = release_reference(env, id);
> +			if (err)
> +				return err;
> +		} else {
> +			ref_convert_owning_non_owning(env, id);
> +		}
>  	}

[Severity: Critical]
Can an unwinding comparator callback with a two-register return type clobber
caller->regs[BPF_REG_2] in prepare_func_exit(), causing check_kfunc_call() to
release the wrong reference ID and leave a dangling owning pointer to the
dropped node?

During verifier setup, bpf_compute_subprog_ret_regs() sets ret_reg_pair = true
for any subprogram whose BTF return type is a 9-16 byte struct, and
btf_validate_return_type() allows static subprograms to return structs up to
16 bytes. Neither check_func_arg(), push_callback_call(), nor
check_cb_unwind() prevents the less callback passed to bpf_rbtree_add() or
bpf_rbtree_add_impl() from having ret_reg_pair == true.

When such a callback executes unwind_exit, prepare_func_exit() in
kernel/bpf/verifier.c computes nregs = 2 (where ret_regs[] = { BPF_REG_0,
BPF_REG_2 }) and copies both registers from the callee frame into the caller
frame before rewinding *insn_idx to callee->callsite:

	callee = state->frame[state->curframe];
	r0 = &callee->regs[BPF_REG_0];
	nregs = bpf_ret_reg_pair(env, callee->subprogno) ? 2 : 1;
	...
	if (callee->in_callback_fn && !unwinds) {
		...
	} else {
		for (i = 0; i < nregs; i++) {
			u32 regno = ret_regs[i];

			bpf_diag_mod_begin(env, &caller->regs[regno], &callee->regs[regno],
					   BPF_DIAG_MOD_WRITE);
			caller->regs[regno] = callee->regs[regno];
			bpf_diag_mod_end(env);
		}
	}

If the caller passed &n1->node (with id = n1_id) in BPF_REG_2 while keeping a
copy of n1 in a callee-saved register (R6-R9) or stack spill, and the
comparator callback acquires an owning reference to a second node n2 of the
same BTF type (for example via bpf_refcount_acquire() or
bpf_list_pop_front()), places &n2->node (with id = n2_id) into BPF_REG_2, and
executes unwind_exit:

1. prepare_func_exit() overwrites caller->regs[BPF_REG_2] with &n2->node
   (id = n2_id).

2. On the revisit of check_kfunc_call() at bpf_rbtree_add() with
   env->cur_state->unwound == true, check_func_args() succeeds because
   caller->regs[BPF_REG_2] holds a valid owning pointer of the expected
   node type, and release_reference(env, id) releases n2_id instead of n1_id.

3. At runtime, __bpf_rbtree_add() drops the caller's original node n1 via
   __bpf_obj_drop_impl(), while in the verifier state at the landing pad after
   unwind_goto, n1_id was never released and the caller's saved pointer to n1
   remains a valid owning PTR_TO_BTF_ID | MEM_ALLOC pointer to freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=9

Re: [RFC PATCH bpf-next v1 06/11] bpf: Let the callback of bpf_for_each_map_elem unwind

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 4415 bytes --]

> bpf: Let the callback of bpf_for_each_map_elem unwind

TL;DR:
- Array map callback may return a PTR_TO_MAP_KEY to dead kernel stack after
  unwind, possibly leaking stale stack
- bpf_unwind_leave() may misbehave (WARN, wrong return) if the helper is
  traced

> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 133f189bf6d2..4c81c7c76145 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c

[ ... ]

> @@ -823,14 +824,18 @@ static long bpf_for_each_array_elem(struct bpf_map *map, bpf_callback_t callback
>  			val = array_map_elem_ptr(array, i);
>  		num_elems++;
>  		key = i;
> -		ret = callback_fn((u64)(long)map, (u64)(long)&key,
> -				  (u64)(long)val, (u64)(long)callback_ctx, 0);
> +		bpf_call_cb(callback_fn, map, &key, val, callback_ctx, 0, ret, unwound);

Can the PTR_TO_MAP_KEY handed to the callback here outlive the frame it
points into?

&key is a local in the frame of bpf_for_each_array_elem(), and
map_set_for_each_callback_args() types it as PTR_TO_MAP_KEY in R2 of the
callback.  With BPF_FUNC_for_each_map_elem now in helper_has_cb_pad(), the
callback may leave with unwind_exit, and prepare_func_exit() from
b2f03cf6322f hands the callback's r0 to the pad unchanged, except for
PTR_TO_STACK, which it refuses.

A PTR_TO_MAP_KEY in r0 therefore reaches the pad after the call, where
check_mem_access() still allows reads of key_size bytes through it.  By then
the helper frame has been popped (and, on the indirect path, the frame of
bpf_for_each_map_elem() too), so the pointer refers to dead kernel stack that
any later call or interrupt can overwrite.  A program without
allow_ptr_leaks/allow_uninit_stack would be able to read stale kernel stack
through it.

The changelog of b2f03cf6322f mentions this case: "the key of the array
iterator live in the frame of the helper ... the fix for that has to see to
r0 too".  Nothing between b2f03cf6322f and the end of this series adds that
fix.  Hash and rhash keys point into map elements rather than the helper
frame, so only the array iterator is affected.

Should a PTR_TO_MAP_KEY r0 (or any pointer whose lifetime ends with the helper
frame) be refused or turned into a scalar at the callback's unwind_exit
before for_each_map_elem is opened up for array maps?  Alternatively, could
the key live somewhere that outlives the helper?

>  		/* return value: 0 - continue, 1 - stop and return */
>  		if (ret)
>  			break;
>  	}
>
>  	return num_elems;
> +
> +unwound:
> +	/* The callback left with unwind_exit: nothing to drop here */
> +	bpf_unwind_leave();
> +	return ret;

Does bpf_unwind_leave() work if the helper itself is being traced?

It reads the return-address slot of bpf_for_each_array_elem() and looks it up
with bpf_unwind_target(), which only works if the slot still holds the
address the BPF caller pushed.  The same leave path is added to
bpf_for_each_hash_elem(), bpf_each_rhash_elem() and bpf_for_each_map_elem().
All four are ordinary traceable functions: arraymap.o, hashtab.o and
bpf_iter.o are not in the CFLAGS_REMOVE list of $(CC_FLAGS_FTRACE) in
kernel/bpf/Makefile, none of the four is notrace, and none is in btf_id_deny.

If the function-graph tracer, a kretprobe/rethook, or a BPF fexit/fmod_ret
trampoline is attached to one of them, the slot would hold return_to_handler,
the rethook trampoline, or a trampoline address, and bpf_unwind_target()
would find no entry.  Then WARN_ON_ONCE(to == *slot) fires (fatal with
panic_on_warn), and the exception comes back as a plain return.  The BPF
program carries on past the unwind_goto on the normal path, in a state the
verifier only explored on the unwind edge.

bpf_check_attach_target() already guards the BPF-target version of this
("unwind_exit would return to the trampoline that called the target", "Only
fentry can attach to %s, which can unwind"), but kernel-function targets that
now carry pads get no equivalent check.  bpf_loop() from 4e61c40fe5c0 has the
same exposure.

Should these helpers be marked notrace and added to btf_id_deny, or should
bpf_unwind_leave() recover the original return address (as
ftrace_graph_ret_addr() and rethook do for unwinders) before the lookup?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40612543702

Re: [RFC PATCH bpf-next v1 05/11] bpf: Let the callback of bpf_loop unwind

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 5009 bytes --]

> bpf: Let the callback of bpf_loop unwind

TL;DR: If bpf_loop() is traced (fexit, kretprobe, function_graph),
bpf_unwind_leave() may not find the BPF return address, so it may WARN and
resume the program in a state the verifier never checked, possibly leaking
pointers or references.

> diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
> --- a/kernel/bpf/bpf_iter.c
> +++ b/kernel/bpf/bpf_iter.c

[ ... ]

> @@ -749,7 +750,7 @@ const struct bpf_func_proto bpf_for_each_map_elem_proto = {
>  	.arg4_type	= ARG_ANYTHING,
>  };
>
> -BPF_CALL_4(bpf_loop, u32, nr_loops, void *, callback_fn, void *, callback_ctx,
> +BPF_CALL_x(4, noinline, bpf_loop, u32, nr_loops, void *, callback_fn, void *, callback_ctx,
>  	   u64, flags)
>  {
>  	bpf_callback_t callback = (bpf_callback_t)callback_fn;

[ ... ]

> @@ -766,13 +767,18 @@ BPF_CALL_4(bpf_loop, u32, nr_loops, void *, callback_fn, void *, callback_ctx,
>  		return -E2BIG;
>
>  	for (i = 0; i < nr_loops; i++) {
> -		ret = callback((u64)i, (u64)(long)callback_ctx, 0, 0, 0);
> +		bpf_call_cb(callback, i, callback_ctx, 0, 0, 0, ret, unwound);
>  		/* return value: 0 - continue, 1 - stop and return */
>  		if (ret)
>  			return i + 1;
>  	}
>
>  	return i;
> +
> +unwound:
> +	/* The callback left with unwind_exit: the loop is over, and so is the helper */
> +	bpf_unwind_leave();
> +	return ret;
>  }
>
>  const struct bpf_func_proto bpf_loop_proto = {

Can bpf_unwind_leave() find the BPF caller's return address here when
bpf_loop() is being traced?

bpf_unwind_leave() looks for the return address of the BPF caller in
bpf_loop()'s own return-address slot (__builtin_dwarf_cfa() - 1 on x86,
the frame record LR on arm64).  But bpf_loop() is a regular traceable
kernel function.  bpf_iter.o is built with the ftrace flags (kernel/bpf/
Makefile only strips them for percpu_freelist, bpf_lru_list,
queue_stack_maps, lpm_trie, ringbuf and rqspinlock), bpf_loop() has no
NOKPROBE_SYMBOL, and it is not in btf_id_deny.  Several tracers change
that slot or add a frame:

- An fexit/fsession BPF trampoline calls the original function with
  emit_rsb_call(&prog, orig_call, ...) in arch/x86/net/bpf_jit_comp.c, so
  the return address of bpf_loop() points into the trampoline image.

- kretprobe/rethook: arch_rethook_prepare() does
  stack[0] = arch_rethook_trampoline.

- The function_graph tracer and fprobe exit handlers replace the slot with
  return_to_handler.

In each case bpf_unwind_target(*slot) does not find a BPF address.
search_bpf_extables() goes through bpf_prog_ksym_find(), which returns NULL
for a trampoline ksym (ksym->prog is false), and the kernel and module
tables have no EX_TYPE_BPF entry there, so the address comes back
unchanged.  Then WARN_ON_ONCE(to == *slot) fires, which panics with
panic_on_warn, and bpf_loop() returns ret through the tracer to the
instruction after "call bpf_loop".  On x86 and arm64 the JIT emits no code
for unwind_goto, so the program continues along the normal path.

The verifier checked that path only against callback states that returned
normally, where r0 is RET_INTEGER and the stack and references are as left
by a normal return.  It checked the unwind_exit state only against the
pad.  So the BPF program can run in a state the verifier never checked.
For example:

- r0 can be the pointer or reference the callback unwound with
  (b2f03cf6322f lets a pointer and its reference reach the pad), now seen
  as a scalar.  The pointer leaks and the reference is never released.

- Callback writes through callback_ctx on the unwind path only (for
  example a scalar over a spilled pointer) are seen by code that assumes
  the old slot type.

The trigger is a bpf_loop() call that the verifier does not inline (flags
not provably 0, or two callbacks at one call site) whose callback has
unwind_exit, while kretprobe:bpf_loop, an fexit program on bpf_loop(), or
the function_graph tracer is active.

bpf_check_attach_target() already treats this hazard as real for BPF
targets:

    /* unwind_exit would return to the trampoline that called the target */
    ...
    "Only fentry can attach to %s, which can unwind"

Nothing equivalent covers bpf_loop(), which this patch turns into a frame
that unwinds.  No later patch in the series addresses this (no notrace,
NOKPROBE_SYMBOL or btf_id_deny change, and no ftrace_graph_ret_addr() or
rethook lookup in bpf_unwind_leave()).  The helpers that get pads later in
the series (bpf_for_each_map_elem and its ops, bpf_find_vma,
bpf_user_ringbuf_drain, bpf_rbtree_add) look to have the same problem.

Could bpf_loop() and the other helpers with pads be made untraceable
(notrace plus NOKPROBE_SYMBOL, and a btf_id_deny entry for fexit/fmod_ret),
or could bpf_unwind_leave() resolve a hijacked return address?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40612543702

Re: [RFC PATCH bpf-next v1 10/11] selftests/bpf: Test callbacks that unwind

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 4684 bytes --]

> selftests/bpf: Test callbacks that unwind

TL;DR: Selftests may fail instead of skip on x86_64/arm64 kernels built with
older gcc (no asm goto output), where callback pads are absent: unwind_drain
load error and the changed callback_unwinds message expectation.

> diff --git a/tools/testing/selftests/bpf/prog_tests/unwind_drain.c b/tools/testing/selftests/bpf/prog_tests/unwind_drain.c
> new file mode 100644
> index 000000000000..54b9cfbef440
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/unwind_drain.c
> @@ -0,0 +1,71 @@

[ ... ]

> +void test_unwind_drain(void)
> +{
> +	struct user_ring_buffer *rb = NULL;
> +	struct unwind_drain *skel;
> +	int err;
> +
> +	if (!is_jit_enabled()) {
> +		printf("%s:SKIP:JIT is disabled\n", __func__);
> +		test__skip();
> +		return;
> +	}
> +	skel = unwind_drain__open_and_load();
> +	err = libbpf_get_error(skel);
> +	if (err == -ENOTSUPP) {
> +		printf("%s:SKIP:JIT does not support unwinding\n", __func__);
> +		test__skip();
> +		return;
> +	}
> +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +		return;

The test skips only on -ENOTSUPP, which is what check_unwind_insns() returns
when !bpf_jit_supports_unwind().  Is that enough on a kernel whose JIT
supports unwinding but whose helpers have no callback pads?

On x86_64 or arm64 without CONFIG_CC_HAS_ASM_GOTO_OUTPUT (gcc < 11.5,
12.0-12.3 and 13.0-13.2, see GCC_ASM_GOTO_OUTPUT_BROKEN in
scripts/Kconfig.toolchain), BPF_HAS_CB_PADS is 0 in
include/linux/bpf_unwind.h while bpf_jit_supports_unwind() still returns
true.

Loading drain then goes through:

push_callback_call()
  check_cb_unwind()
    verbose(env, "callback cannot unwind\n");
    return -EINVAL;

unwind_drain__open_and_load() returns NULL with errno = EINVAL, so the
-ENOTSUPP check is false and ASSERT_OK_PTR(skel, "open_and_load") fails the
test where it should skip, since the kernel does not offer the feature in
that configuration.

Should this also skip when the load fails with "callback cannot unwind" in
the verifier log (for example via bpf_program__set_log_buf()), or should
the kernel return -ENOTSUPP for that case so that this check catches it?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/verifier_unwind.c b/tools/testing/selftests/bpf/progs/verifier_unwind.c
> index 5a21345c34d4..3de145a2d065 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_unwind.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_unwind.c
> @@ -896,12 +896,13 @@ static unsigned long cb_unwinds(void)
>  	);
>  }
>
> +/* bpf_loop() has a pad for it, so what's missing is the one after the call */
>  SEC("tc")
>  __description("unwind: callback unwinds")
>  __arch_x86_64
>  __arch_arm64
>  __load_if_JITed()
> -__failure __msg("callback cannot unwind")
> +__failure __msg("no unwind_goto after the call of bpf_loop()")
>  __naked int callback_unwinds(void)
>  {
>  	asm volatile (

The new expected message is only what the verifier prints when
BPF_HAS_CB_PADS is 1.  In include/linux/bpf_unwind.h that is the case only
for defined(CONFIG_X86_64) && defined(CONFIG_CC_HAS_ASM_GOTO_OUTPUT) and
the arm64 equivalent; the #else branch sets it to 0 and calls the callback
the plain way.

check_cb_unwind() in kernel/bpf/verifier.c tests that first:

    if (!BPF_HAS_CB_PADS || !insn_has_cb_pad(insn)) {
        verbose(env, "callback cannot unwind\n");
        return -EINVAL;
    }

so on such a kernel the log for this program still says "callback cannot
unwind" and never reaches "no unwind_goto after the call of bpf_loop()".

CC_HAS_ASM_GOTO_OUTPUT depends on !GCC_ASM_GOTO_OUTPUT_BROKEN
(scripts/Kconfig.toolchain), which is y for GCC_VERSION < 11.5, 12.0-12.3
and 13.0-13.2, while the minimum gcc is 8.1 (scripts/min-tool-version.sh).
An x86_64 or arm64 kernel built with gcc 11.4 (Ubuntu 22.04), 12.2
(Debian 12) or 13.2 therefore still has bpf_jit_supports_unwind() == true.

check_unwind_insns() does not refuse the program there, so __arch_x86_64,
__arch_arm64 and __load_if_JITed() still let it load, and
verifier_unwind/callback_unwinds now fails.  Before this hunk the
expectation was correct on that configuration.

Nothing in the test can express the compiler dependency.  The new
verifier_unwind_cb.c tests in this commit depend on the compiler in the
same way.

Can the expectation be made to hold in either configuration, for example
with a regex that accepts either message, or can the test skip when the
helpers have no pads?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40612543702

Re: [RFC PATCH bpf-next v1 09/11] bpf: Let the comparator of bpf_rbtree_add unwind

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 6539 bytes --]

> bpf: Let the comparator of bpf_rbtree_add unwind

TL;DR:
- bpf_rbtree_add() unwind pad may be skipped under fexit/kretprobe/graph
  tracing, leaving a freed node usable by the program
- unwound path may leave node->owner poisoned, and may run kptr destructors in
  NMI-unsafe contexts

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 648abe1ba63a..f8708b7181d7 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c

[ ... ]

> @@ -2796,6 +2801,15 @@ static int __bpf_rbtree_add(struct bpf_rb_root *root,
>  	rb_insert_color_cached(n, (struct rb_root_cached *)root, leftmost);
>  	WRITE_ONCE(node->owner, root);
>  	return 0;
> +
> +unwound:
> +	/*
> +	 * The comparator left with unwind_exit. The node is not in the tree, and
> +	 * the caller has let go of it: drop it, as the error path above does.
> +	 */
> +	__bpf_obj_drop_impl((void *)n - off, rec, false);

Does this path leave node->owner set to BPF_PTR_POISON?  The function claims
the node before the loop:

	if (cmpxchg(&node->owner, NULL, BPF_PTR_POISON)) {

The normal exit replaces that with WRITE_ONCE(node->owner, root), but the
unwound exit never clears the claim.  For a non-refcounted node the drop
frees it, so that does not matter.

For a refcounted node with another live reference, __bpf_obj_drop_impl() only
drops the count and the object lives on with owner still set to
BPF_PTR_POISON.  Nothing resets it later, since bpf_obj_free_fields() does
nothing for BPF_RB_NODE and only bpf_rbtree_remove() or a successful add
writes owner.

The other reference can come from m = bpf_refcount_acquire(n) before the
add, or from the comparator calling bpf_refcount_acquire() on its argument
(the kfunc is KF_SPINLOCK_SAFE).  The verifier keeps that other reference as
owning after the unwind, but every later bpf_rbtree_add() of the object
fails at the cmpxchg, drops the caller's reference and returns -EINVAL.

The comment says "drop it, as the error path above does", but the error path
above never took the claim and this path did.  Would a
WRITE_ONCE(node->owner, NULL) before the __bpf_obj_drop_impl() call be
needed here?

Can this drop also run in a context where the verifier refuses
bpf_obj_drop()?  check_kfunc_call() rejects bpf_obj_drop() and
bpf_percpu_obj_drop() when btf_record_has_nmi_unsafe_fields() is true for
tracing program types, per commit 94c8d1c21be4 ("bpf: Reject bpf_obj_drop()
from tracing progs").  Every rbtree node type is covered, because
btf_field_is_nmi_safe() returns false for BPF_RB_NODE.

Before this patch, the only drop in bpf_rbtree_add() was the error path for a
node already in a collection, and that collection holds a reference of its
own.  Now a comparator that ends in unwind_exit makes the kfunc call
__bpf_obj_drop_impl() every time, and for a non-refcounted node that is the
last reference, so bpf_obj_free_fields() runs inline.

Nothing restricts this by program type: BPF_PROG_TYPE_TRACING gets
generic_kfunc_set, check_map_prog_compatibility() refuses BPF_SPIN_LOCK only
for is_tracing_prog_type() (which does not include BPF_PROG_TYPE_TRACING),
and neither check_cb_unwind() nor the new unwound branch in
check_kfunc_call() tests btf_record_has_nmi_unsafe_fields() or the program
type.

For example, a non-sleepable tp_btf or fentry program attached somewhere
NMI-reachable could allocate:

	struct { struct bpf_rb_node node; struct task_struct __kptr *t; }

store an acquired task in t, take the lock and call bpf_rbtree_add() with a
comparator that unwinds.  The verifier accepts this although it refuses
bpf_obj_drop() on the same object, and the pad then runs the task kptr
destructor (put_task_struct()) from that context.

Should the same btf_record_has_nmi_unsafe_fields() and program type check be
applied to a bpf_rbtree_add() whose comparator can unwind, for example in
check_cb_unwind() or in the unwound branch of check_kfunc_call()?

> +	bpf_unwind_leave();
> +	return r0;
>  }

Can this pad rely on the slot at the frame address of bpf_rbtree_add() or
bpf_rbtree_add_impl() still holding the BPF caller's return address?  Both
are ordinary traceable functions: helpers.o is built with ftrace flags and
btf_id_deny does not list them.

Several tracers that hook the return rewrite that slot:

  - an fexit, fmod_ret or fsession trampoline, which uses
    BPF_TRAMP_F_CALL_ORIG and calls orig_call + X86_PATCH_SIZE, so the body
    returns into the trampoline
  - a kretprobe or fprobe exit (the rethook trampoline)
  - the function_graph tracer, where ftrace_graph_func() sets
    *parent = return_to_handler

In each case bpf_unwind_leave() reads that address.  bpf_unwind_target()
finds no BPF extable entry for it and no EX_TYPE_BPF kernel entry, and
returns the address unchanged.  WARN_ON_ONCE(to == *slot) fires (a panic with
panic_on_warn), and the kfunc returns r0 the normal way.

The tracer then sends control to the BPF program's normal return address,
and the JIT emits nothing for unwind_goto, so the program carries on along
the path the verifier checked as "comparator returned".  On that path
ref_convert_owning_non_owning() has made the node a non-owning reference
that is valid in the tree until unlock, but the kfunc has already called
__bpf_obj_drop_impl() on it:

	__bpf_obj_drop_impl((void *)n - off, rec, false);
	bpf_unwind_leave();
	return r0;

Can the program then read and write a freed node?  For example, it could
bpf_kptr_xchg() an acquired reference into a kptr field of the dead object
(kptr_xchg_dest_types allows NON_OWN_REF), and that reference is never
released.

For helpers with pads, a missed unwind only loses the exception.
bpf_rbtree_add() is the first one whose two exits leave the object in
different states (in the tree, or freed), so for it a missed unwind leaves
the verifier's state wrong.

bpf_check_attach_target() already refuses everything but fentry on a subprog
that can unwind ("unwind_exit would return to the trampoline that called the
target"), but kernel functions with a pad get no such restriction, and the
rest of the series (through 5646f059f1bb) adds none.  Should return-hooking
tracers be kept off these kfuncs (notrace, a btf_id_deny-style list for
fexit/fmod_ret/fsession, and no kretprobe or graph tracing), or should the
pad tolerate a rewritten return address?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40612543702

Re: [RFC PATCH bpf-next v1 03/11] bpf: Let a callback unwind into the pad of its helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] bpf: prepare_func_exit() clobbers caller R2 on ret_reg_pair cb unwind
- [Medium] bpf: verifier_bug_if() in do_check() when callback unwinds with const R0
- [Medium] bpf: inline_bpf_loop() leaves unwind_goto after BPF_LDX_MEM when callback cannot unwind
--

commit 00033d1866979957f9c4c976952ca2e9b1a444c5
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Let a callback unwind into the pad of its helper

This commit extends the BPF verifier to allow synchronous callbacks invoked by
helpers or kfuncs to exit via unwind_exit into an unwind pad following the
helper or kfunc call. Upon unwind_exit, the verifier revisits the callsite
with the callback's R0 preserved so helper effects are applied before
proceeding to the unwind_goto instruction. It also updates register liveness
and precision backtracking to account for R0 flowing from the callback across
the call.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5601d20562186..ac9dbe0821811 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10,6 +10,7 @@
>  #include <linux/types.h>
>  #include <linux/slab.h>
>  #include <linux/bpf.h>
> +#include <linux/bpf_unwind.h>
>  #include <linux/btf.h>
>  #include <linux/bpf_verifier.h>
>  #include <linux/filter.h>
> @@ -3435,6 +3436,8 @@ static int next_callx_callee(struct bpf_verifier_env *env, int caller, int from)
>   * 'call foo; unwind_goto pad' goes to the pad when foo left with unwind_exit.
>   * JITs don't emit a branch for it. unwind_exit finds the pad by its return
>   * address. So unwind_goto has to follow the call and nothing may jump to it.
> + * The call may be of a helper or a kfunc that calls a callback: it leaves like
> + * the callback did, see check_cb_unwind().
>   */
>  static int check_unwind_insns(struct bpf_verifier_env *env)
>  {
> @@ -3450,8 +3453,10 @@ static int check_unwind_insns(struct bpf_verifier_env *env)
>  		if (!bpf_is_unwind_goto(insn))
>  			continue;
>  		seen = true;
> -		if (!i || !(bpf_pseudo_call(insn - 1) || bpf_is_callx(insn - 1))) {
> -			verbose(env, "unwind_goto at insn %d doesn't follow a call of a subprog\n",
> +		if (!i || !(bpf_pseudo_call(insn - 1) || bpf_is_callx(insn - 1) ||
> +			    bpf_is_sync_callback_calling_insn(insn - 1))) {
> +			verbose(env,
> +				"unwind_goto at insn %d doesn't follow a call of a subprog or of a helper with a callback\n",
>  				i);
>  			return -EINVAL;
>  		}

[Severity: Medium]
What happens when unwind_goto follows a call to bpf_loop() that gets inlined
by bpf_optimize_bpf_loop() -> inline_bpf_loop(), but the callback does not
unwind (!env->subprog_info[subprog].can_unwind)?

Here check_unwind_insns() permits unwind_goto after any
bpf_is_sync_callback_calling_insn(insn - 1), and check_cb_unwind() returns 0
when !env->subprog_info[subprog].can_unwind.

When bpf_loop() is inlined in kernel/bpf/fixups.c:inline_bpf_loop() (both here
and after the later commit "bpf: Give the inlined bpf_loop a pad" when
!can_unwind):

	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JLE, BPF_REG_1, BPF_MAX_LOOPS, 2);
	insn_buf[cnt++] = BPF_MOV32_IMM(BPF_REG_0, -E2BIG);
	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JA, 0, 0, 16);
	...
	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_8, BPF_REG_10, r8_offset);

the call instruction is replaced with 19 instructions ending in BPF_LDX_MEM
without converting the trailing unwind_goto into BPF_JMP_A(0), and the
BPF_JMP_IMM(BPF_JA, 0, 0, 16) instruction jumps directly to that unwind_goto.

If the target of that unwind_goto was marked seen during verification (for
example, a shared pad or unwind_goto with offset 0) or when !is_priv,
bpf_opt_hard_wire_dead_code_branches() leaves the unwind_goto instruction in
place:

	if (bpf_is_unwind_goto(insn) && aux_data[i + 1 + insn->off].seen) {
		if (!aux_data[i + 1].seen) {
			memcpy(insn + 1, &trap, sizeof(trap));
			aux_data[i + 1].seen = env->pass_cnt;
		}
		continue;
	}

Wouldn't this leave unwind_goto immediately after BPF_LDX_MEM instead of a
call instruction (and targeted by a jump), violating the JIT invariant in
arch/x86/net/bpf_jit_comp.c and arch/arm64/net/bpf_jit_comp.c (including the
x86-64 priv_frame_ptr path that assumes insn[i - 1] is a call ending in pop r9
and keys the exception table at addrs[i - 1] - 2)?

Conversely, in the later commit "bpf: Give the inlined bpf_loop a pad", if
can_unwind is true for the callback subprog, but R1 (nr_loops) is known to be
0 at the bpf_loop() callsite, check_helper_call() skips push_callback_call()
so the unwind pad is never verified. Yet inline_bpf_loop() sees
subprog_info[callback_subprogno].can_unwind && bpf_is_unwind_goto(insn + 1)
and emits an inlined pad jumping via BPF_JMP32_A to an unverified pad that
bpf_opt_remove_dead_code() then strips.

Should inline_bpf_loop() rewrite a trailing unwind_goto to BPF_JMP_A(0) when
the callback does not unwind during the call, or only emit the pad when the
unwind_goto target was actually verified?

[ ... ]

> @@ -12111,12 +12178,16 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
>  	}
>  
>  	caller = state->frame[state->curframe - 1];
> -	if (bpf_is_unwind_exit(&env->prog->insnsi[*insn_idx])) {
> -		/* the helper or the kfunc that called it doesn't expect that */
> -		if (callee->in_callback_fn) {
> -			verbose(env, "callback cannot unwind\n");
> -			return -EINVAL;
> -		}
> +	unwinds = bpf_is_unwind_exit(&env->prog->insnsi[*insn_idx]);
> +	if (unwinds) {
> +		/*
> +		 * A callback returns into the helper that called it, and to the
> +		 * pad of the helper when the helper has one. check_cb_unwind()
> +		 * has seen to that, and to the unwind_goto after the call of
> +		 * the helper, where the helper leaves to right away, with the
> +		 * r0 of the callback as it is: the pad gets its state, as the
> +		 * pad after a subprog call does.
> +		 */
>  		if (!bpf_is_unwind_goto(&env->prog->insnsi[callee->callsite + 1])) {
>  			verbose(env, "unwind_exit, but no unwind_goto after the call at insn %d\n",
>  				callee->callsite);
> @@ -12124,7 +12195,7 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
>  		}
>  		state->unwound = true;
>  	}
> -	if (callee->in_callback_fn) {
> +	if (callee->in_callback_fn && !unwinds) {
>  		if (r0->type != SCALAR_VALUE) {
>  			verbose(env, "R0 not a scalar value\n");
>  			return -EACCES;

[Severity: High]
Can a callback with a 9..16 byte BTF return type clobber the caller's R2
argument register when it exits via unwind_exit?

In bpf_compute_subprog_ret_regs(), ret_reg_pair is set for any static subprog
whose BTF return type is 9..16 bytes, and btf_check_subprog_call() in
push_callback_call() does not reject this for callbacks. At the top of
prepare_func_exit():

	callee = state->frame[state->curframe];
	r0 = &callee->regs[BPF_REG_0];
	nregs = bpf_ret_reg_pair(env, callee->subprogno) ? 2 : 1;

When callee->in_callback_fn && unwinds is true, prepare_func_exit() now takes
the else branch:

	} else {
		/*
		 * return to the caller whatever the callee had in the
		 * return register(s)
		 */
		for (i = 0; i < nregs; i++) {
			u32 regno = ret_regs[i];

			bpf_diag_mod_begin(env, &caller->regs[regno], &callee->regs[regno],
					   BPF_DIAG_MOD_WRITE);
			caller->regs[regno] = callee->regs[regno];
			bpf_diag_mod_end(env);
		}
	}

If nregs == 2, this copies callee->regs[BPF_REG_2] into
caller->regs[BPF_REG_2] before redirecting *insn_idx back to callee->callsite.

When the verifier revisits the call instruction at callee->callsite,
check_helper_call() or check_kfunc_call() re-runs argument validation and side
effects using the overwritten caller->regs[BPF_REG_2]. In particular, for
bpf_rbtree_add_impl(root, node, less, ...), R2 is the owning reference to the
node being added:

	if (is_bpf_list_push_kfunc(meta.func_id) || is_bpf_rbtree_add_kfunc(meta.func_id)) {
		id = regs[BPF_REG_2].id;
		insn_aux->insert_off = regs[BPF_REG_2].var_off.value;
		insn_aux->kptr_struct_meta = btf_find_struct_meta(meta.arg_btf, meta.arg_btf_id);
		ref_convert_owning_non_owning(env, id);
	}

If the callback places a different allocated node pointer in its R2 before
unwind_exit, wouldn't the revisit of check_kfunc_call() release the wrong
reference ID in the verifier while bpf_rbtree_add_impl() drops the original
node at runtime, leaving a dangling owning reference to the freed original
node in the caller's unwind pad?

Should nregs be forced to 1 when callee->in_callback_fn is true (or should
callbacks with ret_reg_pair be rejected)?

[ ... ]

> @@ -20589,7 +20692,8 @@ static int do_check(struct bpf_verifier_env *env)
>  		}
>  
>  		if (bpf_is_jmp_point(env, env->insn_idx)) {
> -			err = bpf_push_jmp_history(env, state, 0, 0, 0, NULL, 0);
> +			err = bpf_push_jmp_history(env, state, state->unwound ? INSN_F_UNWOUND : 0,
> +						   0, 0, NULL, 0);
>  			if (err)
>  				return err;
>  		}

[Severity: Medium]
Can revisiting the call instruction with state->unwound set trigger
verifier_bug_if() in do_check()'s precomputed constant register check?

Before do_check() runs, bpf_compute_const_regs() computes intraprocedural
constant register values at each instruction. If the caller holds a known
32-bit constant C1 in R0 before a callback-calling helper or kfunc instruction
(for instance, r0 = 0; call bpf_loop; unwind_goto +pad;),
bpf_compute_const_regs() sets BIT(BPF_REG_0) in
insn_aux->const_reg_mask and records C1 in
insn_aux->const_reg_vals[BPF_REG_0].

When the callback unwinds with a different constant scalar C2 in R0,
prepare_func_exit() sets state->unwound = true, copies C2 into
caller->regs[BPF_REG_0], and sets *insn_idx = callee->callsite. On the revisit
of the call instruction in do_check(), just below this hunk:

		/* Sanity check: precomputed constants must match verifier state */
		if (!state->speculative && insn_aux->const_reg_mask) {
			struct bpf_reg_state *regs = cur_regs(env);
			u16 mask = insn_aux->const_reg_mask;

			for (int r = 0; r < ARRAY_SIZE(insn_aux->const_reg_vals); r++) {
				u32 cval = insn_aux->const_reg_vals[r];

				if (!(mask & BIT(r)))
					continue;
				if (regs[r].type != SCALAR_VALUE)
					continue;
				if (!tnum_is_const(regs[r].var_off))
					continue;
				if (verifier_bug_if((u32)regs[r].var_off.value != cval,
						    env, "const R%d: %u != %llu",
						    r, cval, regs[r].var_off.value))
					return -EFAULT;
			}
		}

Because regs[BPF_REG_0] now holds the callback's unwound constant C2 rather
than the caller's pre-call constant C1, wouldn't this fire verifier_bug_if()
and fail verification with -EFAULT unless BPF_REG_0 is skipped when
state->unwound is true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=3

Re: [RFC PATCH bpf-next v1 03/11] bpf: Let a callback unwind into the pad of its helper

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 7084 bytes --]

> bpf: Let a callback unwind into the pad of its helper

TL;DR:
- Tracing (ftrace, kretprobe, fexit) may redirect a pad helper's return
  address, so unwind could fall through, possibly causing use-after-free or
  bad r0; not yet reachable
- Stale comment in backtrack.c (nit)

> diff --git a/kernel/bpf/backtrack.c b/kernel/bpf/backtrack.c
> index 43d5740034c0..ce171be5e754 100644
> --- a/kernel/bpf/backtrack.c
> +++ b/kernel/bpf/backtrack.c

[ ... ]

> @@ -562,8 +567,16 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
>  			from_subprog_call = subseq_idx - 1 >= 0 &&
>  					    (bpf_pseudo_call(&env->prog->insnsi[subseq_idx - 1]) ||
>  					     bpf_is_callx(&env->prog->insnsi[subseq_idx - 1]));
> +			/*
> +			 * unwind_exit of a callback goes back to the call of the
> +			 * helper, which passes R0 on to the pad after it: R0 is
> +			 * the callback's, as it is the subprog's after a call.
> +			 */
> +			cb_unwinds = !from_subprog_call && bpf_is_unwind_exit(insn) &&
> +				     subseq_idx >= 0 &&
> +				     bpf_is_sync_callback_calling_insn(&env->prog->insnsi[subseq_idx]);
>
> -			r0_precise = from_subprog_call && bt_is_reg_set(bt, BPF_REG_0);
> +			r0_precise = (from_subprog_call || cb_unwinds) && bt_is_reg_set(bt, BPF_REG_0);
>  			r2_precise = from_subprog_call && bt_is_reg_set(bt, BPF_REG_2);

This isn't a bug, but the comment at the top of the BPF_EXIT case says that
the precision of the return registers is never propagated for an exit from
a callback:

    * case, we need to propagate the precision of the
    * return registers, if necessary. In the latter we
    * never do that.

With this change, unwind_exit of a callback sets cb_unwinds, and
r0_precise = (from_subprog_call || cb_unwinds) && bt_is_reg_set(bt, BPF_REG_0)
does propagate r0 to the callback frame.

The new comment above cb_unwinds describes this, but the old sentence is
still there, so the same case has two comments that disagree.

Should the old comment say that the return registers of a callback are
dropped unless the callback left with unwind_exit?

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5601d2056218..ac9dbe082181 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11397,6 +11402,55 @@ static int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog)

[ ... ]

> +/*
> + * A sync callback is called by a helper or a kfunc, a C frame between the
> + * callback and the BPF code that called it. unwind_exit of the callback returns
> + * to the pad of its caller when there is one, and a helper or a kfunc that
> + * calls a callback has one where BPF_HAS_CB_PADS and insn_has_cb_pad(): it
> + * calls the callback with bpf_call_cb(), drops what it holds when the callback
> + * unwinds and leaves to the pad of its own caller with the r0 of the callback.
> + * So a call with such a callback that can unwind is a call of something that
> + * can unwind, and unwind_goto has to follow it.
> + */
> +static int check_cb_unwind(struct bpf_verifier_env *env, struct bpf_insn *insn,
> +			   int insn_idx, int subprog)
> +{
> +	if (!env->subprog_info[subprog].can_unwind)
> +		return 0;
> +	if (!BPF_HAS_CB_PADS || !insn_has_cb_pad(insn)) {
> +		verbose(env, "callback cannot unwind\n");
> +		return -EINVAL;
> +	}

Can a callback that unwinds through one of these helpers end up on the
fall-through path of the call instead of at the pad?

check_cb_unwind() accepts the unwind based on BPF_HAS_CB_PADS and
insn_has_cb_pad(), which are static properties. At run time the pad is only
reached if the return address slot of the helper still holds the address in
the BPF program. bpf_unwind_leave() does this:

    unsigned long *slot = (unsigned long *)__builtin_dwarf_cfa() - 1;
    unsigned long to = bpf_unwind_target(*slot);

    WARN_ON_ONCE(to == *slot);
    *slot = to;

bpf_unwind_target() looks up *slot in the BPF and kernel extables, and
several kinds of tracing replace or move that return address:

  - function_graph: prepare_ftrace_return() does *parent = return_hooker
  - kretprobe/rethook and fprobe exit handlers (kprobe.multi return, session)
  - a BPF fexit/fmod_ret trampoline: with BPF_TRAMP_F_CALL_ORIG it calls
    orig_call from the trampoline, so the helper returns into the trampoline

None of the helpers that get a pad later in the series is protected against
this. bpf_loop, bpf_for_each_map_elem and bpf_find_vma become plain noinline
functions through BPF_CALL_x, and bpf_for_each_array_elem, bpf_rbtree_add_impl
and the others are ordinary traceable functions.  None of them is in
btf_id_deny and none is notrace.

If the slot was redirected, bpf_unwind_target() finds no entry and returns the
address unchanged.  WARN_ON_ONCE() fires, the helper does a normal return,
and the tracer then returns to the instruction after the BPF call.

That instruction is unwind_goto, for which the arch backends emit no branch.
Execution continues on the fall-through path, which the verifier modelled as
a normal return of the helper.

On that path r0 holds what the callback unwound with, and the helper has
already done its pad-side work:

  - bpf_loop: the verifier thinks r0 is the scalar returned by bpf_loop(),
    but it can be a kernel pointer, which could be stored to a map, or a
    referenced pointer whose reference is never released.

  - bpf_rbtree_add: on the unwind path the pad has already freed the node
    with __bpf_obj_drop_impl().  On the fall-through path the verifier ran
    ref_convert_owning_non_owning() and treats the node as a live non-owning
    reference, so the program can read and write freed memory.

bpf_check_attach_target() already refuses anything but fentry on a subprog
that can unwind ("unwind_exit would return to the trampoline that called the
target"). Is there an equivalent for the kernel helpers with pads?  The
verifier has no say over function_graph or kretprobes at all.

This commit only adds the gate, since helper_has_cb_pad() and
kfunc_has_cb_pad() return false for now.  The path is opened later in the
series by:

  4e61c40fe5c0 ("bpf: Let the callback of bpf_loop unwind")
  7a687425732b ("bpf: Let the callback of bpf_for_each_map_elem unwind")
  59e3a600668d ("bpf: Let the callback of bpf_find_vma unwind")
  bee41eec3c39 ("bpf: Let the callback of bpf_user_ringbuf_drain unwind")
  ecd4c98df2ee ("bpf: Let the comparator of bpf_rbtree_add unwind")

No commit up to 5646f059f1bb adds a fix: no notrace or NOKPROBE marking, no
attach restriction, and no return_to_handler or trampoline handling in
bpf_unwind_target().

Would it work to make the pad helpers untraceable (notrace plus NOKPROBE, and
deny them in btf_id_deny for fexit/fmod_ret), or to have bpf_unwind_leave()
resolve the real return address through the ftrace/rethook shadow stacks?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40612543702

Re: [RFC PATCH bpf-next v1 01/11] bpf: Give a helper a way to call its callback with a pad

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 6631 bytes --]

> bpf: Give a helper a way to call its callback with a pad

TL;DR:
- bpf_unwind_leave() may misbehave if the helper is ftrace/kretprobe/fexit
  traced: unwind lost, pointer or reference treated as scalar
- With gcc, bpf_call_cb() register variables may be clobbered by KASAN/KCSAN
  calls, possibly jumping to a bad address

> diff --git a/include/linux/bpf_unwind.h b/include/linux/bpf_unwind.h
> new file mode 100644
> index 000000000000..190d6dbae856
> --- /dev/null
> +++ b/include/linux/bpf_unwind.h
> @@ -0,0 +1,282 @@

[ ... ]

> +/*
> + * The registers the call clobbers are outputs, so that none of them can be an
> + * operand of anything else. r0 is rax, on the way out and on the way to the pad.
> + * The return address stays the byte after the call through what
> + * apply_retpolines() and apply_fineibt() make of the call: they pad in front of
> + * it, never behind it.
> + */
> +#define __bpf_call_cb(type, fn, a1, a2, a3, a4, a5, r0, unwound)		\
> +do {										\
> +	register u64 __fn asm("r11") = (u64)(long)(fn);				\
> +	register u64 __a5 asm("r8") = (u64)(long)(a5);				\
> +	register u64 __c5 asm("r8");						\
> +	register u64 __c11 asm("r11");						\
> +	u64 __c1, __c2, __c3, __c4;						\
> +										\
> +	asm_goto_output(BPF_KCFI_CHECK(type)					\
> +		 "	" CALL_NOSPEC "\n"					\
> +		 "1:\n"								\
> +		 _ASM_EXTABLE_TYPE(1b, %l[unwound], EX_TYPE_BPF)		\
> +		 : "=a"(r0), "=D"(__c1), "=S"(__c2), "=d"(__c3), "=c"(__c4),	\
> +		   "=r"(__c5), "=r"(__c11)					\
> +		 : [thunk_target] "r"(__fn), "D"((u64)(long)(a1)),		\
> +		   "S"((u64)(long)(a2)), "d"((u64)(long)(a3)),			\
> +		   "c"((u64)(long)(a4)), "r"(__a5)				\
> +		 : "r9", "r10", "memory", "cc"					\
> +		 : unwound);							\
> +} while (0)

Can the argument evaluation here clobber the register variables?

__fn is bound to r11 and __a5 to r8 before a1 through a4 are evaluated as
asm inputs.  The arm64 version has the same shape, binding x15 and x0
through x4 one after another before later arguments are evaluated.

With gcc, a register variable is only guaranteed to hold its register at the
asm statement itself.  If evaluating a later macro argument involves a call,
the earlier binding is not kept across it.

A later patch in this series (7a687425732b) calls:

    bpf_call_cb(callback_fn, map, elem->data,
                rhtab_elem_value(elem, map->key_size), callback_ctx, 0,
                ret, unwound);

The map->key_size load becomes a call to __tsan_read4 with CONFIG_KCSAN, or
to __asan_load4_noabort with CONFIG_KASAN_OUTLINE, and kernel/bpf is not
excluded from either.  That call is placed after the register variables
have been set.

On arm64 the call's own argument goes in x0, so the callback could get a
garbage map pointer, and x15 (the call target) can be clobbered as well.  On
x86 r11 and r8 are call-clobbered, so the call __x86_indirect_thunk_r11 that
follows could jump to a garbage address.

clang ties register variables only at the asm itself, so this would only
affect gcc builds.

Would it be safer to evaluate fn and a1 through a5 into ordinary
temporaries first, for example typeof(a) __t = a;, and only then declare the
register variables, the way the arm-smccc.h macros do?

> +
> +/*
> + * The return address of the helper is the word below its canonical frame
> + * address. The pad is in the frame that BPF code called, see above, so there is
> + * a pad to go to: a miss is a bug, and the exception would be lost in a return.
> + */
> +static __always_inline void bpf_unwind_leave(void)
> +{
> +	unsigned long *slot = (unsigned long *)__builtin_dwarf_cfa() - 1;
> +	unsigned long to = bpf_unwind_target(*slot);
> +
> +	WARN_ON_ONCE(to == *slot);
> +	*slot = to;
> +}

Can *slot hold something other than the address in the bpf program that
called the helper?

bpf_unwind_leave() assumes that it does, and the comment says a miss is a
bug.  While the helper is traced, though, the slot can be replaced:

  - function_graph: prepare_ftrace_return()/ftrace_graph_func() put
    return_to_handler in *parent
  - kretprobe/rethook: arch_rethook_trampoline is put there
  - a bpf fexit/fmod_ret trampoline calls the original function, so the
    return address is inside the trampoline

On arm64 the same replacement is made to the link register at entry, and the
prologue then saves it into the frame record that bpf_unwind_leave() reads.

kernel/bpf/Makefile removes CC_FLAGS_FTRACE only from percpu_freelist.o,
bpf_lru_list.o, queue_stack_maps.o, lpm_trie.o, ringbuf.o and rqspinlock.o.
So bpf_loop() (bpf_iter.c), the map iterators (hashtab.c, arraymap.c),
bpf_find_vma() (task_iter.c) and bpf_rbtree_add (helpers.c) can all be
graph-traced.  They can also be kretprobed, and so can
bpf_user_ringbuf_drain().

Later commits in this series give these helpers pads (4e61c40fe5c0,
7a687425732b, 59e3a600668d, bee41eec3c39, ecd4c98df2ee) and add no notrace,
NOKPROBE or attach restriction.

If the callback unwinds in that state, bpf_unwind_target(return_to_handler)
misses in the bpf table and in the kernel table, and returns its argument:

    unsigned long to = bpf_unwind_target(*slot);   /* to == *slot */

    WARN_ON_ONCE(to == *slot);                     /* fires */
    *slot = to;                                    /* unchanged */

The helper then returns through the tracer to the normal continuation after
the call instead of to the pad, and the exception is lost after the helper
has already stopped and dropped what it holds.

There is also a typing consequence.  According to b2f03cf6322f the pad gets
the callback's r0 bit for bit, "a pointer stays the pointer it was, with its
type and its reference".  On the fallthrough path the verifier instead types
r0 as the helper's own return (RET_INTEGER, for example).  So a pointer or a
referenced object the callback unwound with reaches code that treats it as a
scalar, and the pad that would have consumed or released it never runs.

bpf_check_attach_target() already refuses the analogous case for bpf
targets: "Only fentry can attach to %s, which can unwind".  Is there
something equivalent for the C helpers that have a pad?

Would translating the slot with ftrace_graph_ret_addr() and the rethook
equivalent (and refusing fexit/fmod_ret on these helpers) cover this?
Alternatively, could the helpers with pads be made notrace and NOKPROBE, with
tracing attachment to them denied?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40612543702

Re: [RFC PATCH bpf-next v1 01/11] bpf: Give a helper a way to call its callback with a pad

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:20 AM CET, sashiko-bot wrote:
> [...]
> [Severity: High]
> Can evaluating the macro arguments in ___bpf_call_cb() on arm64 and
> __bpf_call_cb() on x86-64 clobber the caller-saved local register
> variables before the asm_goto_output() statement is reached?

Right: the compiler keeps no local register variable across a call, and
a call in the evaluation of a later argument clobbers the ones set
before it. The next version evaluates every argument into a temporary
first and sets the register variables from those.

> [...]
> [Severity: High]
> Could the compiler eliminate the *slot = to store in bpf_unwind_leave()
> (on both x86-64 and arm64) as a dead store to the current function's stack
> frame right before returning?

Right, and gcc 15 does drop it, on arm64 and x86-64 alike: that is why
the gcc runs of BPF CI returned from every helper but the array iterator
instead of unwinding. The next version stores with WRITE_ONCE(), on the
shadow call stack too.

> [...]

Re: [RFC PATCH bpf-next v1 02/11] bpf, arm64: Let the pad after a helper call have r0

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:02 AM CET, sashiko-bot wrote:
> [...]
> [Severity: Medium]
> Can this leak ctx.landing_ex if multi-function JIT compilation fails between
> passes?

Right. ctx.offset leaked the same way before, where x86-64 frees its
addrs; the next version frees both in bpf_jit_free().

> [...]

Re: [RFC PATCH bpf-next v1 03/11] bpf: Let a callback unwind into the pad of its helper

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:31 AM CET, sashiko-bot wrote:
> [...]
> [Severity: Medium]
> What happens when unwind_goto follows a call to bpf_loop() that gets inlined
> by bpf_optimize_bpf_loop() -> inline_bpf_loop(), but the callback does not
> unwind (!env->subprog_info[subprog].can_unwind)?

The next version turns the unwind_goto after the call of bpf_loop() into
a no-op jump whether or not the inlined loop gets a pad, so it never
follows the last insn of the loop, and the loop gets a pad only when the
verifier saw the pad of the caller: with a loop of no rounds that pad is
dead and goes with the dead code.

> [...]
> [Severity: High]
> Can a callback with a 9..16 byte BTF return type clobber the caller's R2
> argument register when it exits via unwind_exit?

Right. The pad of a helper carries r0 alone, so a callback hands over r0
alone in the next version, whatever its prototype says.

> [...]
> [Severity: Medium]
> Can revisiting the call instruction with state->unwound set trigger
> verifier_bug_if() in do_check()'s precomputed constant register check?

Right. The revisit carries the r0 of the callback, so r0 is no constant
of such an insn: bpf_compute_const_regs() skips it there in the next
version, as liveness has it live there already.

> [...]

Re: [RFC PATCH bpf-next v1 04/11] bpf: Give the inlined bpf_loop a pad

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:10 AM CET, sashiko-bot wrote:
> [...]
> [Severity: High]
> Can this leave a live BPF_JMP32_A instruction targeting a dead caller landing
> pad that gets removed by bpf_opt_remove_dead_code(), leading to an
> out-of-bounds read in addrs[] or ctx->offset[] during JIT compilation?

Right: the pad of the caller is dead with a loop of no rounds, or with
an unwind_exit on a path the verifier never took, and the jump of the
inlined pad was left pointing into what the removal of dead code took
away. KASAN sees do_jit() read addrs[] past the end. The next version
gives the inlined loop a pad only when the verifier saw the pad of the
caller, with a test for the loop of no rounds.

> [...]
> [Severity: High]
> What happens if bpf_is_unwind_goto(&new_prog->insnsi[position + cnt]) is true,
> but pad is false (for example, when
> env->subprog_info[callback_subprogno].can_unwind is false, or when the
> caller's landing pad was not reached from this call site)?

The unwind_goto after the call becomes a no-op jump either way in the
next version, so it never follows the last insn of the loop.

> [...]

Re: [RFC PATCH bpf-next v1 05/11] bpf: Let the callback of bpf_loop unwind

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:19 AM CET, sashiko-bot wrote:
> [...]
> [Severity: Critical]
> When helper_has_cb_pad(BPF_FUNC_loop) returns true, can inline_bpf_loop() in
> kernel/bpf/fixups.c emit a live BPF_JMP32_A instruction that jumps to a dead
> landing pad which is subsequently removed by dead code elimination, or leave a
> stray BPF_UNWIND_GOTO after a load instruction?

Same as on 04/11, fixed in the next version: the inlined loop gets a pad
only when the verifier saw the pad of the caller, and the unwind_goto
after the call becomes a no-op jump either way.

> [...]
> [Severity: Medium]
> What happens in prepare_func_exit() in kernel/bpf/verifier.c when a bpf_loop()
> callback subprogram with a 9 to 16 byte BTF return type (ret_reg_pair == true)
> unwinds via unwind_exit?

Same as on 03/11: a callback hands over r0 alone in the next version,
whatever its prototype says.

> [...]

Re: [RFC PATCH bpf-next v1 07/11] bpf: Let the callback of bpf_find_vma unwind

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:08 AM CET, sashiko-bot wrote:
> [...]
> [Severity: High]
> Can the unwound pad end up calling mmap_read_unlock() and mmput_async() with
> hardware interrupts disabled?

A callback can return into the helper with IRQs disabled today, with the
flags saved through callback_ctx into the frame of the caller: the exit
of a callback does not check the IRQ state, and the helper unlocks the
same way on that path. The pad changes nothing there.

> [...]
> [Severity: High]
> What prevents the callback from escaping the vma pointer (or pointers walked
> from it, such as vma->vm_file, &vma->vm_file->f_path, or
> vma->vm_file->f_inode) in r0 via unwind_exit after mmap_lock, rcu_read_lock(),
> and the foreign mm reference have been dropped?

Known class: the callback can store the vma through callback_ctx today
just the same. Ihor is fixing that for the arguments of a callback, and
the fix has to see to r0 too.

> [...]

Re: [RFC PATCH bpf-next v1 09/11] bpf: Let the comparator of bpf_rbtree_add unwind

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:21 AM CET, sashiko-bot wrote:
> [...]
> [Severity: High]
> Does the unwound path in __bpf_rbtree_add() leave node->owner poisoned with
> BPF_PTR_POISON when the object survives __bpf_obj_drop_impl()?

Right: with another reference holding the node, the drop left it with
its owner poisoned, and no add would take it again. The next version
unlinks it first, as bpf_rb_root_free() does.

> [...]
> [Severity: Critical]
> Can an unwinding comparator callback with a two-register return type clobber
> caller->regs[BPF_REG_2] in prepare_func_exit(), causing check_kfunc_call() to
> release the wrong reference ID and leave a dangling owning pointer to the
> dropped node?

Same as on 03/11: a callback hands over r0 alone in the next version, so
r2 stays the node the revisit releases.

> [...]

Re: [RFC PATCH bpf-next v1 01/11] bpf: Give a helper a way to call its callback with a pad

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:39 AM CET, bot+bpf-ci wrote:
> [...]
> TL;DR:
> - bpf_unwind_leave() may misbehave if the helper is ftrace/kretprobe/fexit
>   traced: unwind lost, pointer or reference treated as scalar

Right. Every function with a pad is notrace and no kprobe target in the
next version, as the verifier lets only fentry attach to a subprog that
unwinds, with a test that fexit on bpf_loop() is refused.

> - With gcc, bpf_call_cb() register variables may be clobbered by KASAN/KCSAN
>   calls, possibly jumping to a bad address

Right, for a call in the evaluation of a later argument, which the
compiler does not keep the register variables across. The next version
evaluates every argument into a temporary first.

> [...]

Re: [RFC PATCH bpf-next v1 03/11] bpf: Let a callback unwind into the pad of its helper

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:39 AM CET, bot+bpf-ci wrote:
> [...]
> TL;DR:
> - Tracing (ftrace, kretprobe, fexit) may redirect a pad helper's return
>   address, so unwind could fall through, possibly causing use-after-free or
>   bad r0; not yet reachable

Right. Every function with a pad is notrace and no kprobe target in the
next version, as the verifier lets only fentry attach to a subprog that
unwinds, with a test that fexit on bpf_loop() is refused.

> - Stale comment in backtrack.c (nit)

Fixed in the next version.

> [...]

Re: [RFC PATCH bpf-next v1 05/11] bpf: Let the callback of bpf_loop unwind

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:25 AM CET, bot+bpf-ci wrote:
> [...]
> TL;DR: If bpf_loop() is traced (fexit, kretprobe, function_graph),
> bpf_unwind_leave() may not find the BPF return address, so it may WARN and
> resume the program in a state the verifier never checked, possibly leaking
> pointers or references.

Right. Every function with a pad is notrace and no kprobe target in the
next version, as the verifier lets only fentry attach to a subprog that
unwinds, with a test that fexit on bpf_loop() is refused.

> [...]

Re: [RFC PATCH bpf-next v1 06/11] bpf: Let the callback of bpf_for_each_map_elem unwind

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:25 AM CET, bot+bpf-ci wrote:
> [...]
> TL;DR:
> - Array map callback may return a PTR_TO_MAP_KEY to dead kernel stack after
>   unwind, possibly leaking stale stack

Known class: the callback can store the key through callback_ctx today
just the same. Ihor is fixing that for the arguments of a callback, and
the fix has to see to r0 too.

> - bpf_unwind_leave() may misbehave (WARN, wrong return) if the helper is
>   traced

Right. Every function with a pad is notrace and no kprobe target in the
next version, as the verifier lets only fentry attach to a subprog that
unwinds, with a test that fexit on bpf_loop() is refused.

> [...]

Re: [RFC PATCH bpf-next v1 09/11] bpf: Let the comparator of bpf_rbtree_add unwind

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:25 AM CET, bot+bpf-ci wrote:
> [...]
> TL;DR:
> - bpf_rbtree_add() unwind pad may be skipped under fexit/kretprobe/graph
>   tracing, leaving a freed node usable by the program

Right. Every function with a pad is notrace and no kprobe target in the
next version, as the verifier lets only fentry attach to a subprog that
unwinds, with a test that fexit on bpf_loop() is refused.

> - unwound path may leave node->owner poisoned, and may run kptr destructors in
>   NMI-unsafe contexts

Right on both. The next version unlinks the node before dropping it, as
bpf_rb_root_free() does, and refuses bpf_rbtree_add() in tracing
programs on types with NMI unsafe fields when its comparator can unwind,
as bpf_obj_drop() is refused there, with tests for both.

> [...]

Re: [RFC PATCH bpf-next v1 10/11] selftests/bpf: Test callbacks that unwind

[Kumar Kartikeya Dwivedi]

On Wed Oct 28, 2026 at 2:25 AM CET, bot+bpf-ci wrote:
> [...]
> TL;DR: Selftests may fail instead of skip on x86_64/arm64 kernels built with
> older gcc (no asm goto output), where callback pads are absent: unwind_drain
> load error and the changed callback_unwinds message expectation.

Right: without asm goto outputs the helpers have no pads while the JIT
unwinds. In the next version check_cb_unwind() says -ENOTSUPP for that,
as for a JIT that does not unwind, and the tests skip on it; the check
comes after the one for the unwind_goto, so the message for a missing
unwind_goto is the same everywhere.

> [...]

Re: [RFC PATCH bpf-next v1 00/11] Unwinding for synchronous callbacks

[Alexei Starovoitov]

On Tue, Oct 27, 2026 at 05:44 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> Code that unwinds due to panics or exceptions doesn't often control
> where they can be invoked, and reusing the same code in callbacks would
> lead to verifier rejections since unwinding across such synchronous
> callbacks is not supported.

I feel that you're too attached to bpf_throw concept.
The code was quite intrusive earlier and it is still today.
I told my agents that bpf_throw is a dead end. sorry.
I really think it's not a kernel job to solve it.

Re: [RFC PATCH bpf-next v1 00/11] Unwinding for synchronous callbacks

[Kumar Kartikeya Dwivedi]

On Wed, 28 Oct 2026 at 06:06, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Oct 27, 2026 at 05:44 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > Code that unwinds due to panics or exceptions doesn't often control
> > where they can be invoked, and reusing the same code in callbacks would
> > lead to verifier rejections since unwinding across such synchronous
> > callbacks is not supported.
>
> I feel that you're too attached to bpf_throw concept.
> The code was quite intrusive earlier and it is still today.
> I told my agents that bpf_throw is a dead end. sorry.
> I really think it's not a kernel job to solve it.

Huh? This set is about making unwind_goto / unwind_exit work.

Re: [RFC PATCH bpf-next v1 00/11] Unwinding for synchronous callbacks

[Alexei Starovoitov]

On Wed, Oct 28, 2026 at 03:20 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> On Wed, 28 Oct 2026 at 06:06, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> I feel that you're too attached to bpf_throw concept.
>> The code was quite intrusive earlier and it is still today.
>> I told my agents that bpf_throw is a dead end. sorry.
>> I really think it's not a kernel job to solve it.
>
> Huh? This set is about making unwind_goto / unwind_exit work.

Sorry. Mixed it up.
I meant it in the "Eliminate spill overhead for bpf_throw()" thread.
I don't see the point doing any work for bpf_throw().
imo it's mainly in the way of any verifier work.
I'd rather delete all the bpf_throw related code and make it nop.
It didn't get any traction.
Originally it was a mechanism to enable bpf_assert() which in turn
was an idea to stop fighting the verifier.
That didn't materialize.
So when I see the title "Eliminate spill overhead for bpf_throw"
it makes me react the way I did... in the wrong thread :(

Re: [RFC PATCH bpf-next v1 00/11] Unwinding for synchronous callbacks

[Kumar Kartikeya Dwivedi]

On Wed, 28 Oct 2026 at 21:17, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Oct 28, 2026 at 03:20 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > On Wed, 28 Oct 2026 at 06:06, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> >> I feel that you're too attached to bpf_throw concept.
> >> The code was quite intrusive earlier and it is still today.
> >> I told my agents that bpf_throw is a dead end. sorry.
> >> I really think it's not a kernel job to solve it.
> >
> > Huh? This set is about making unwind_goto / unwind_exit work.
>
> Sorry. Mixed it up.
> I meant it in the "Eliminate spill overhead for bpf_throw()" thread.
> I don't see the point doing any work for bpf_throw().
> imo it's mainly in the way of any verifier work.
> I'd rather delete all the bpf_throw related code and make it nop.
> It didn't get any traction.
> Originally it was a mechanism to enable bpf_assert() which in turn
> was an idea to stop fighting the verifier.
> That didn't materialize.
> So when I see the title "Eliminate spill overhead for bpf_throw"
> it makes me react the way I did... in the wrong thread :(

I've already expressed in the past that I'm fine with it, feel free to
go ahead with it (alternatively, I can also send the series dropping
it). In its current state upstream, it is useless.

I mostly sent the one you are reacting to out to clear my queue, and
wasn't tied to my own stuff, but without any expectations (same for
this one as well). The less I need to rebase the better, etc.

That said, on the topic, this one seemed useful to me, but you are
free to disagree and not take it either.

-------------8<-------------

(Mostly for documentation and public record; I don't have the energy
to argue about the merits / demerits).

Since we plan to sunset bpf_throw(), here is some information for other readers:

I carry out-of-tree patches since ~2024 to complete the support, and
it works well whenever I need it in any academic project. All of the
concerns for the original approach + spilling as a fallback turned out
to be mostly theoretical. Across all programs in selftests, scx,
cilium, there are a handful of spills in total where the merging logic
is unable to reconcile differing states; naturally, in practice, the
compiler already keeps resources the program releases itself in known
stack locations or registers since the code gen is better.

So in practice, no program gets rejected; the worst case is having
extra spills for contrived examples. I personally don't buy the LoC
argument either, the whole thing is contained in 2k LoC on the kernel
side, including comments and various boilerplate. Given the rate at
which we add code today, it's a non-sequitur.

In return, we get various appealing properties:
- Do the equivalent of bail!() or assert!() to prove a property, or
just not handle a given condition, regardless of the language used
(and for BPF objects linking across languages).
- A building block to insert runtime conditions from the verifier that
the program needs at a given point (bounds check, etc.), but cannot be
proven due to the limitations of the static analysis, or losing
precision. It does not depend on whether the program properly releases
resources at that point, which we cannot assume to be the case.
- On arena faults, or may_goto timeouts, stopping execution of the
program and ejecting it from the machine. This is done manually by scx
schedulers scraping streams and unloading the scheduler today.
- Programs that hog CPU using very long loops get killed
asynchronously at the next cancellation point (loop backedge) using
NMI watchdogs, and get ejected from the system. The counting
implementation of may_goto can be replaced with a load of a bit in
prog->aux for non-NMI programs (majority).

There are other various other misc. cases where it is more appropriate
or necessary:
- Software CFI-checked indirect calls to subprogs (basically, emitting
a check on an indirect call (say with pointer from arena) and aborting
the program when the CFI-like check fails).
- If runtime stack probing or recursion limits are hit, the program is
aborted, instead of return 0 (as done in rust-bpf).
- Making bpf_spin_lock() just work and aborting on deadlocks.
- When writing actual programs in C++ or Rust, inevitably, panic() may
be seen (but may not be exercised) in code that is unwinding. The
verifier will just reject such cases. In both C++ or Rust the behavior
is to panic=abort or std::terminate(), which is a good analogue for
simply aborting the program.

In general, it serves many purposes, has analogue in every other
virtual machine runtime, thus is a well understood idea, and
encompasses what various alternative proposals for some of the covered
use cases do (fast-execute, etc.) but with better semantics.

Re: [RFC PATCH bpf-next v1 00/11] Unwinding for synchronous callbacks

[Eduard Zingerman]

On Wed, 2026-10-28 at 22:42 +0100, Kumar Kartikeya Dwivedi wrote:

...

> There are other various other misc. cases where it is more appropriate
> or necessary:
> - Software CFI-checked indirect calls to subprogs (basically, emitting
> a check on an indirect call (say with pointer from arena) and aborting
> the program when the CFI-like check fails).
> - If runtime stack probing or recursion limits are hit, the program is
> aborted, instead of return 0 (as done in rust-bpf).
> - Making bpf_spin_lock() just work and aborting on deadlocks.
> - When writing actual programs in C++ or Rust, inevitably, panic() may
> be seen (but may not be exercised) in code that is unwinding. The
> verifier will just reject such cases. In both C++ or Rust the behavior
> is to panic=abort or std::terminate(), which is a good analogue for
> simply aborting the program.
> 
> In general, it serves many purposes, has analogue in every other
> virtual machine runtime, thus is a well understood idea, and
> encompasses what various alternative proposals for some of the covered
> use cases do (fast-execute, etc.) but with better semantics.

I agree with Kartikeya on these points.
Unwinding and terminating execution is a well understood and common concept.
It is "in distribution", so to speak.

The new verifier series would benefit from it instead of taking quite
unusual and unexpected actions, like inserting additional control flow exits,
or returning 0 from a subprogram that doesn't have stack space to recur.
"If loop runs for too long the program is terminated" is a much simpler
concept to work with compared to "if loop runs for too long the program
is taking some exit from a loop".

2K LOC is basically nothing, if one assumes that code is maintained by LLM.
There are much more invasive verification concepts atm, like freplace
for parts of the program, or tail calls.

...

Re: [RFC PATCH bpf-next v1 00/11] Unwinding for synchronous callbacks

[Alexei Starovoitov]

On Wed Oct 28, 2026 at 11:12 PM UTC, Eduard Zingerman wrote:
> On Wed, 2026-10-28 at 22:42 +0100, Kumar Kartikeya Dwivedi wrote:
>
> ...
>
>> There are other various other misc. cases where it is more appropriate
>> or necessary:
>> - Software CFI-checked indirect calls to subprogs (basically, emitting
>> a check on an indirect call (say with pointer from arena) and aborting
>> the program when the CFI-like check fails).
>> - If runtime stack probing or recursion limits are hit, the program is
>> aborted, instead of return 0 (as done in rust-bpf).
>> - Making bpf_spin_lock() just work and aborting on deadlocks.
>> - When writing actual programs in C++ or Rust, inevitably, panic() may
>> be seen (but may not be exercised) in code that is unwinding. The
>> verifier will just reject such cases. In both C++ or Rust the behavior
>> is to panic=abort or std::terminate(), which is a good analogue for
>> simply aborting the program.
>> 
>> In general, it serves many purposes, has analogue in every other
>> virtual machine runtime, thus is a well understood idea, and
>> encompasses what various alternative proposals for some of the covered
>> use cases do (fast-execute, etc.) but with better semantics.
>
> I agree with Kartikeya on these points.
> Unwinding and terminating execution is a well understood and common concept.

I agree with it as well.
My objection was to 'Eliminate spill overhead for bpf_throw()'
as a pointless step _today_. If we can make bpf_throw() into works-everywhere
mechanism then let's do it, but it has to be that first step.
Polishing 'spill oveheard' is very much optional follow up.

> It is "in distribution", so to speak.

'in distribution' is so early AI lingvo. I hope it's obvious now
that 'in distribution' is the opposite of what is needed to
build secure sw stack.

> The new verifier series would benefit from it instead of taking quite
> unusual and unexpected actions, like inserting additional control flow exits,
> or returning 0 from a subprogram that doesn't have stack space to recur.
> "If loop runs for too long the program is terminated" is a much simpler
> concept to work with compared to "if loop runs for too long the program
> is taking some exit from a loop".

Yes, but the base line is different now. goal posts have moved a lot.

> 2K LOC is basically nothing, if one assumes that code is maintained by LLM.
> There are much more invasive verification concepts atm, like freplace
> for parts of the program, or tail calls.

it's fine if it's 2k->20k lines too.

Before we do the happy dance pls read section 8
https://lore.kernel.org/all/20261026025943.404450-70-alexei.starovoitov@gmail.com/
It needs to support all 3:

1.
  do { p = lookup(hash map); } while (!p);
  return *p;

2.
  lock(&v->l);
  do { v->cnt++; } while (v->flag);
  unlock(&v->l);
  return 1;

3.
  rcu_read_lock();
  do { p = lookup(hash map); } while (!p);
  x = *p;
  rcu_read_unlock();

The patches I sent don't support 3rd yet. I left it for the follow up.

I'm more than happy to drop may_goto approach,
but I want to see the patches for bpf_throw-that-works-everywhere,
including 3rd case above.
And bpf_throw shouldn't skip rust landing pads.
That's a bar.

Also time is an essence.
We cannot sit and debate things like we did in the past.
If there is code that satisfies the goals that we set, we should be landing it.
How it achieves the gaols is a minor detail at this point.