[PATCH bpf-next v3 0/2] bpf, arm64: Support Exceptions

[PATCH bpf-next v3 0/2] bpf, arm64: Support Exceptions

[Puranjay Mohan]

Changes in V2->V3:
V2: https://lore.kernel.org/all/20230917000045.56377-1-puranjay12@gmail.com/
- Use unwinder from stacktrace.c rather than open coding the unwind logic.
- Fix a bug in the prologue related to BPF_FP (Xu Kuohai)

Changes in V1->V2:
V1: https://lore.kernel.org/all/20230912233942.6734-1-puranjay12@gmail.com/
- Remove exceptions from DENYLIST.aarch64 as they are supported now.

The base support for exceptions was merged with [1] and it was enabled for
x86-64.

This patch set enables the support on ARM64, all sefltests are passing:

# ./test_progs -a exceptions
#74/1    exceptions/exception_throw_always_1:OK
#74/2    exceptions/exception_throw_always_2:OK
#74/3    exceptions/exception_throw_unwind_1:OK
#74/4    exceptions/exception_throw_unwind_2:OK
#74/5    exceptions/exception_throw_default:OK
#74/6    exceptions/exception_throw_default_value:OK
#74/7    exceptions/exception_tail_call:OK
#74/8    exceptions/exception_ext:OK
#74/9    exceptions/exception_ext_mod_cb_runtime:OK
#74/10   exceptions/exception_throw_subprog:OK
#74/11   exceptions/exception_assert_nz_gfunc:OK
#74/12   exceptions/exception_assert_zero_gfunc:OK
#74/13   exceptions/exception_assert_neg_gfunc:OK
#74/14   exceptions/exception_assert_pos_gfunc:OK
#74/15   exceptions/exception_assert_negeq_gfunc:OK
#74/16   exceptions/exception_assert_poseq_gfunc:OK
#74/17   exceptions/exception_assert_nz_gfunc_with:OK
#74/18   exceptions/exception_assert_zero_gfunc_with:OK
#74/19   exceptions/exception_assert_neg_gfunc_with:OK
#74/20   exceptions/exception_assert_pos_gfunc_with:OK
#74/21   exceptions/exception_assert_negeq_gfunc_with:OK
#74/22   exceptions/exception_assert_poseq_gfunc_with:OK
#74/23   exceptions/exception_bad_assert_nz_gfunc:OK
#74/24   exceptions/exception_bad_assert_zero_gfunc:OK
#74/25   exceptions/exception_bad_assert_neg_gfunc:OK
#74/26   exceptions/exception_bad_assert_pos_gfunc:OK
#74/27   exceptions/exception_bad_assert_negeq_gfunc:OK
#74/28   exceptions/exception_bad_assert_poseq_gfunc:OK
#74/29   exceptions/exception_bad_assert_nz_gfunc_with:OK
#74/30   exceptions/exception_bad_assert_zero_gfunc_with:OK
#74/31   exceptions/exception_bad_assert_neg_gfunc_with:OK
#74/32   exceptions/exception_bad_assert_pos_gfunc_with:OK
#74/33   exceptions/exception_bad_assert_negeq_gfunc_with:OK
#74/34   exceptions/exception_bad_assert_poseq_gfunc_with:OK
#74/35   exceptions/exception_assert_range:OK
#74/36   exceptions/exception_assert_range_with:OK
#74/37   exceptions/exception_bad_assert_range:OK
#74/38   exceptions/exception_bad_assert_range_with:OK
#74/39   exceptions/non-throwing fentry -> exception_cb:OK
#74/40   exceptions/throwing fentry -> exception_cb:OK
#74/41   exceptions/non-throwing fexit -> exception_cb:OK
#74/42   exceptions/throwing fexit -> exception_cb:OK
#74/43   exceptions/throwing extension (with custom cb) -> exception_cb:OK
#74/44   exceptions/throwing extension -> global func in exception_cb:OK
#74/45   exceptions/exception_ext_mod_cb_runtime:OK
#74/46   exceptions/throwing extension (with custom cb) -> global func in exception_cb:OK
#74/47   exceptions/exception_ext:OK
#74/48   exceptions/non-throwing fentry -> non-throwing subprog:OK
#74/49   exceptions/throwing fentry -> non-throwing subprog:OK
#74/50   exceptions/non-throwing fentry -> throwing subprog:OK
#74/51   exceptions/throwing fentry -> throwing subprog:OK
#74/52   exceptions/non-throwing fexit -> non-throwing subprog:OK
#74/53   exceptions/throwing fexit -> non-throwing subprog:OK
#74/54   exceptions/non-throwing fexit -> throwing subprog:OK
#74/55   exceptions/throwing fexit -> throwing subprog:OK
#74/56   exceptions/non-throwing fmod_ret -> non-throwing subprog:OK
#74/57   exceptions/non-throwing fmod_ret -> non-throwing global subprog:OK
#74/58   exceptions/non-throwing extension -> non-throwing subprog:OK
#74/59   exceptions/non-throwing extension -> throwing subprog:OK
#74/60   exceptions/non-throwing extension -> non-throwing subprog:OK
#74/61   exceptions/non-throwing extension -> throwing global subprog:OK
#74/62   exceptions/throwing extension -> throwing global subprog:OK
#74/63   exceptions/throwing extension -> non-throwing global subprog:OK
#74/64   exceptions/non-throwing extension -> main subprog:OK
#74/65   exceptions/throwing extension -> main subprog:OK
#74/66   exceptions/reject_exception_cb_type_1:OK
#74/67   exceptions/reject_exception_cb_type_2:OK
#74/68   exceptions/reject_exception_cb_type_3:OK
#74/69   exceptions/reject_exception_cb_type_4:OK
#74/70   exceptions/reject_async_callback_throw:OK
#74/71   exceptions/reject_with_lock:OK
#74/72   exceptions/reject_subprog_with_lock:OK
#74/73   exceptions/reject_with_rcu_read_lock:OK
#74/74   exceptions/reject_subprog_with_rcu_read_lock:OK
#74/75   exceptions/reject_with_rbtree_add_throw:OK
#74/76   exceptions/reject_with_reference:OK
#74/77   exceptions/reject_with_cb_reference:OK
#74/78   exceptions/reject_with_cb:OK
#74/79   exceptions/reject_with_subprog_reference:OK
#74/80   exceptions/reject_throwing_exception_cb:OK
#74/81   exceptions/reject_exception_cb_call_global_func:OK
#74/82   exceptions/reject_exception_cb_call_static_func:OK
#74/83   exceptions/reject_multiple_exception_cb:OK
#74/84   exceptions/reject_exception_throw_cb:OK
#74/85   exceptions/reject_exception_throw_cb_diff:OK
#74/86   exceptions/reject_set_exception_cb_bad_ret1:OK
#74/87   exceptions/reject_set_exception_cb_bad_ret2:OK
#74/88   exceptions/check_assert_eq_int_min:OK
#74/89   exceptions/check_assert_eq_int_max:OK
#74/90   exceptions/check_assert_eq_zero:OK
#74/91   exceptions/check_assert_eq_llong_min:OK
#74/92   exceptions/check_assert_eq_llong_max:OK
#74/93   exceptions/check_assert_lt_pos:OK
#74/94   exceptions/check_assert_lt_zero:OK
#74/95   exceptions/check_assert_lt_neg:OK
#74/96   exceptions/check_assert_le_pos:OK
#74/97   exceptions/check_assert_le_zero:OK
#74/98   exceptions/check_assert_le_neg:OK
#74/99   exceptions/check_assert_gt_pos:OK
#74/100  exceptions/check_assert_gt_zero:OK
#74/101  exceptions/check_assert_gt_neg:OK
#74/102  exceptions/check_assert_ge_pos:OK
#74/103  exceptions/check_assert_ge_zero:OK
#74/104  exceptions/check_assert_ge_neg:OK
#74/105  exceptions/check_assert_range_s64:OK
#74/106  exceptions/check_assert_range_u64:OK
#74/107  exceptions/check_assert_single_range_s64:OK
#74/108  exceptions/check_assert_single_range_u64:OK
#74/109  exceptions/check_assert_generic:OK
#74/110  exceptions/check_assert_with_return:OK
#74      exceptions:OK
Summary: 1/110 PASSED, 0 SKIPPED, 0 FAILED

[1] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?h=for-next&id=ec6f1b4db95b7eedb3fe85f4f14e08fa0e9281c3

Puranjay Mohan (2):
  arm64: stacktrace: Implement arch_bpf_stack_walk() for the BPF JIT
  bpf, arm64: support exceptions

 arch/arm64/kernel/stacktrace.c               | 26 ++++++
 arch/arm64/net/bpf_jit_comp.c                | 87 +++++++++++++++-----
 tools/testing/selftests/bpf/DENYLIST.aarch64 |  1 -
 3 files changed, 94 insertions(+), 20 deletions(-)

-- 
2.40.1

[PATCH bpf-next v3 1/2] arm64: stacktrace: Implement arch_bpf_stack_walk() for the BPF JIT

[Puranjay Mohan]

This will be used by bpf_throw() to unwind till the program marked as
exception boundary and run the callback with the stack of the main
program.

This is required for supporting BPF exceptions on ARM64.

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 arch/arm64/kernel/stacktrace.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 7f88028a00c0..66cffc5fc0be 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -7,6 +7,7 @@
 #include <linux/kernel.h>
 #include <linux/efi.h>
 #include <linux/export.h>
+#include <linux/filter.h>
 #include <linux/ftrace.h>
 #include <linux/kprobes.h>
 #include <linux/sched.h>
@@ -266,6 +267,31 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
 	kunwind_stack_walk(arch_kunwind_consume_entry, &data, task, regs);
 }
 
+struct bpf_unwind_consume_entry_data {
+	bool (*consume_entry)(void *cookie, u64 ip, u64 sp, u64 fp);
+	void *cookie;
+};
+
+static bool
+arch_bpf_unwind_consume_entry(const struct kunwind_state *state, void *cookie)
+{
+	struct bpf_unwind_consume_entry_data *data = cookie;
+
+	return data->consume_entry(data->cookie, state->common.pc, 0,
+				   state->common.fp);
+}
+
+noinline noinstr void arch_bpf_stack_walk(bool (*consume_entry)(void *cookie, u64 ip, u64 sp,
+								u64 fp), void *cookie)
+{
+	struct bpf_unwind_consume_entry_data data = {
+		.consume_entry = consume_entry,
+		.cookie = cookie,
+	};
+
+	kunwind_stack_walk(arch_bpf_unwind_consume_entry, &data, current, NULL);
+}
+
 static bool dump_backtrace_entry(void *arg, unsigned long where)
 {
 	char *loglvl = arg;
-- 
2.40.1

[PATCH bpf-next v3 2/2] bpf, arm64: support exceptions

[Puranjay Mohan]

The prologue generation code has been modified to make the callback
program use the stack of the program marked as exception boundary where
callee-saved registers are already pushed.

As the bpf_throw function never returns, if it clobbers any callee-saved
registers, they would remain clobbered. So, the prologue of the
exception-boundary program is modified to push R23 and R24 as well,
which the callback will then recover in its epilogue.

The Procedure Call Standard for the Arm 64-bit Architecture[1] states
that registers r19 to r28 should be saved by the callee. BPF programs on
ARM64 already save all callee-saved registers except r23 and r24. This
patch adds an instruction in prologue of the  program to save these
two registers and another instruction in the epilogue to recover them.

These extra instructions are only added if bpf_throw() is used. Otherwise
the emitted prologue/epilogue remains unchanged.

[1] https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 arch/arm64/net/bpf_jit_comp.c                | 87 +++++++++++++++-----
 tools/testing/selftests/bpf/DENYLIST.aarch64 |  1 -
 2 files changed, 68 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index cfd5434de483..20720ec346b8 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -285,7 +285,8 @@ static bool is_lsi_offset(int offset, int scale)
 /* Tail call offset to jump into */
 #define PROLOGUE_OFFSET (BTI_INSNS + 2 + PAC_INSNS + 8)
 
-static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf)
+static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf,
+			  bool is_exception_cb)
 {
 	const struct bpf_prog *prog = ctx->prog;
 	const bool is_main_prog = !bpf_is_subprog(prog);
@@ -333,19 +334,34 @@ static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf)
 	emit(A64_MOV(1, A64_R(9), A64_LR), ctx);
 	emit(A64_NOP, ctx);
 
-	/* Sign lr */
-	if (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL))
-		emit(A64_PACIASP, ctx);
-
-	/* Save FP and LR registers to stay align with ARM64 AAPCS */
-	emit(A64_PUSH(A64_FP, A64_LR, A64_SP), ctx);
-	emit(A64_MOV(1, A64_FP, A64_SP), ctx);
-
-	/* Save callee-saved registers */
-	emit(A64_PUSH(r6, r7, A64_SP), ctx);
-	emit(A64_PUSH(r8, r9, A64_SP), ctx);
-	emit(A64_PUSH(fp, tcc, A64_SP), ctx);
-	emit(A64_PUSH(fpb, A64_R(28), A64_SP), ctx);
+	if (!is_exception_cb) {
+		/* Sign lr */
+		if (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL))
+			emit(A64_PACIASP, ctx);
+		/* Save FP and LR registers to stay align with ARM64 AAPCS */
+		emit(A64_PUSH(A64_FP, A64_LR, A64_SP), ctx);
+		emit(A64_MOV(1, A64_FP, A64_SP), ctx);
+
+		/* Save callee-saved registers */
+		emit(A64_PUSH(r6, r7, A64_SP), ctx);
+		emit(A64_PUSH(r8, r9, A64_SP), ctx);
+		emit(A64_PUSH(fp, tcc, A64_SP), ctx);
+		emit(A64_PUSH(fpb, A64_R(28), A64_SP), ctx);
+	} else {
+		/*
+		 * Exception callback receives FP of Main Program as third
+		 * parameter
+		 */
+		emit(A64_MOV(1, A64_FP, A64_R(2)), ctx);
+		/*
+		 * Main Program already pushed the frame record and the
+		 * callee-saved registers. The exception callback will not push
+		 * anything and re-use the main program's stack.
+		 *
+		 * 10 registers are on the stack
+		 */
+		emit(A64_SUB_I(1, A64_SP, A64_FP, 80), ctx);
+	}
 
 	/* Set up BPF prog stack base register */
 	emit(A64_MOV(1, fp, A64_SP), ctx);
@@ -365,6 +381,20 @@ static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf)
 		emit_bti(A64_BTI_J, ctx);
 	}
 
+	/*
+	 * Program acting as exception boundary should save all ARM64
+	 * Callee-saved registers as the exception callback needs to recover
+	 * all ARM64 Callee-saved registers in its epilogue.
+	 */
+	if (prog->aux->exception_boundary) {
+		/*
+		 * As we are pushing two more registers, BPF_FP should be moved
+		 * 16 bytes
+		 */
+		emit(A64_SUB_I(1, fp, fp, 16), ctx);
+		emit(A64_PUSH(A64_R(23), A64_R(24), A64_SP), ctx);
+	}
+
 	emit(A64_SUB_I(1, fpb, fp, ctx->fpb_offset), ctx);
 
 	/* Stack must be multiples of 16B */
@@ -653,7 +683,7 @@ static void build_plt(struct jit_ctx *ctx)
 		plt->target = (u64)&dummy_tramp;
 }
 
-static void build_epilogue(struct jit_ctx *ctx)
+static void build_epilogue(struct jit_ctx *ctx, bool is_exception_cb)
 {
 	const u8 r0 = bpf2a64[BPF_REG_0];
 	const u8 r6 = bpf2a64[BPF_REG_6];
@@ -666,6 +696,15 @@ static void build_epilogue(struct jit_ctx *ctx)
 	/* We're done with BPF stack */
 	emit(A64_ADD_I(1, A64_SP, A64_SP, ctx->stack_size), ctx);
 
+	/*
+	 * Program acting as exception boundary pushes R23 and R24 in addition
+	 * to BPF callee-saved registers. Exception callback uses the boundary
+	 * program's stack frame, so recover these extra registers in the above
+	 * two cases.
+	 */
+	if (ctx->prog->aux->exception_boundary || is_exception_cb)
+		emit(A64_POP(A64_R(23), A64_R(24), A64_SP), ctx);
+
 	/* Restore x27 and x28 */
 	emit(A64_POP(fpb, A64_R(28), A64_SP), ctx);
 	/* Restore fs (x25) and x26 */
@@ -1575,7 +1614,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	 * BPF line info needs ctx->offset[i] to be the offset of
 	 * instruction[i] in jited image, so build prologue first.
 	 */
-	if (build_prologue(&ctx, was_classic)) {
+	if (build_prologue(&ctx, was_classic, prog->aux->exception_cb)) {
 		prog = orig_prog;
 		goto out_off;
 	}
@@ -1586,7 +1625,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	}
 
 	ctx.epilogue_offset = ctx.idx;
-	build_epilogue(&ctx);
+	build_epilogue(&ctx, prog->aux->exception_cb);
 	build_plt(&ctx);
 
 	extable_align = __alignof__(struct exception_table_entry);
@@ -1614,7 +1653,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	ctx.idx = 0;
 	ctx.exentry_idx = 0;
 
-	build_prologue(&ctx, was_classic);
+	build_prologue(&ctx, was_classic, prog->aux->exception_cb);
 
 	if (build_body(&ctx, extra_pass)) {
 		bpf_jit_binary_free(header);
@@ -1622,7 +1661,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 		goto out_off;
 	}
 
-	build_epilogue(&ctx);
+	build_epilogue(&ctx, prog->aux->exception_cb);
 	build_plt(&ctx);
 
 	/* 3. Extra pass to validate JITed code. */
@@ -2310,3 +2349,13 @@ bool bpf_jit_supports_ptr_xchg(void)
 {
 	return true;
 }
+
+bool bpf_jit_supports_exceptions(void)
+{
+	/* We unwind through both kernel frames starting from within bpf_throw
+	 * call and BPF frames. Therefore we require FP unwinder to be enabled
+	 * to walk kernel frames and reach BPF frames in the stack trace.
+	 * ARM64 kernel is aways compiled with CONFIG_FRAME_POINTER=y
+	 */
+	return true;
+}
diff --git a/tools/testing/selftests/bpf/DENYLIST.aarch64 b/tools/testing/selftests/bpf/DENYLIST.aarch64
index 5c2cc7e8c5d0..0445ac38bc07 100644
--- a/tools/testing/selftests/bpf/DENYLIST.aarch64
+++ b/tools/testing/selftests/bpf/DENYLIST.aarch64
@@ -1,6 +1,5 @@
 bpf_cookie/multi_kprobe_attach_api               # kprobe_multi_link_api_subtest:FAIL:fentry_raw_skel_load unexpected error: -3
 bpf_cookie/multi_kprobe_link_api                 # kprobe_multi_link_api_subtest:FAIL:fentry_raw_skel_load unexpected error: -3
-exceptions					 # JIT does not support calling kfunc bpf_throw: -524
 fexit_sleep                                      # The test never returns. The remaining tests cannot start.
 kprobe_multi_bench_attach                        # needs CONFIG_FPROBE
 kprobe_multi_test                                # needs CONFIG_FPROBE
-- 
2.40.1

Re: [PATCH bpf-next v3 0/2] bpf, arm64: Support Exceptions

[Kumar Kartikeya Dwivedi]

On Thu, 1 Feb 2024 at 13:52, Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> Changes in V2->V3:
> V2: https://lore.kernel.org/all/20230917000045.56377-1-puranjay12@gmail.com/
> - Use unwinder from stacktrace.c rather than open coding the unwind logic.
> - Fix a bug in the prologue related to BPF_FP (Xu Kuohai)
>
> Changes in V1->V2:
> V1: https://lore.kernel.org/all/20230912233942.6734-1-puranjay12@gmail.com/
> - Remove exceptions from DENYLIST.aarch64 as they are supported now.
>
> The base support for exceptions was merged with [1] and it was enabled for
> x86-64.
>
> This patch set enables the support on ARM64, all sefltests are passing:
>
> # ./test_progs -a exceptions
> #74/1    exceptions/exception_throw_always_1:OK
> [...]

I think this looks ok, would be nice if it received acks from arm64 experts.

If you have cycles to spare, please also look into
https://lore.kernel.org/bpf/20240201042109.1150490-1-memxor@gmail.com
and let me know how architecture independent the cleanup code right
now in the x86 JIT should be made so that we can do the
same for arm64 as well later. That would be required to complete the
support for cleanups.

I guess we just need bpf_frame_spilled_caller_reg_off to be arch
specific and lift the rest out into the BPF core.
I will make that change in v2 in any case.

Just a note but based on our off-list discussion for supporting this
stuff on riscv as well (where a lot of registers would have to be
saved on entry),
the hidden subprog trampoline could be a way to avoid that. They can
be pushed by this subprog before entry into bpf_throw, and since the
BPF program does not touch the extra callee-saved registers, they
should be what the kernel had originally upon entry into the BPF
program itself. The same could be done on arm64 and x86, but the
returns would be diminishing. It would be nice to quantify how much
this saves in terms of costs of pushing extra registers, before doing
this.

Re: [PATCH bpf-next v3 1/2] arm64: stacktrace: Implement arch_bpf_stack_walk() for the BPF JIT

[Catalin Marinas]

On Thu, Feb 01, 2024 at 12:52:24PM +0000, Puranjay Mohan wrote:
> This will be used by bpf_throw() to unwind till the program marked as
> exception boundary and run the callback with the stack of the main
> program.
> 
> This is required for supporting BPF exceptions on ARM64.
> 
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
>  arch/arm64/kernel/stacktrace.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 7f88028a00c0..66cffc5fc0be 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -7,6 +7,7 @@
>  #include <linux/kernel.h>
>  #include <linux/efi.h>
>  #include <linux/export.h>
> +#include <linux/filter.h>
>  #include <linux/ftrace.h>
>  #include <linux/kprobes.h>
>  #include <linux/sched.h>
> @@ -266,6 +267,31 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
>  	kunwind_stack_walk(arch_kunwind_consume_entry, &data, task, regs);
>  }
>  
> +struct bpf_unwind_consume_entry_data {
> +	bool (*consume_entry)(void *cookie, u64 ip, u64 sp, u64 fp);
> +	void *cookie;
> +};
> +
> +static bool
> +arch_bpf_unwind_consume_entry(const struct kunwind_state *state, void *cookie)
> +{
> +	struct bpf_unwind_consume_entry_data *data = cookie;
> +
> +	return data->consume_entry(data->cookie, state->common.pc, 0,
> +				   state->common.fp);
> +}
> +
> +noinline noinstr void arch_bpf_stack_walk(bool (*consume_entry)(void *cookie, u64 ip, u64 sp,
> +								u64 fp), void *cookie)
> +{
> +	struct bpf_unwind_consume_entry_data data = {
> +		.consume_entry = consume_entry,
> +		.cookie = cookie,
> +	};
> +
> +	kunwind_stack_walk(arch_bpf_unwind_consume_entry, &data, current, NULL);
> +}

Too many "cookies", I found reading this confusing. If you ever respin,
please use some different "cookie" names.

I guess you want this to be merged via the bpf tree?

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH bpf-next v3 1/2] arm64: stacktrace: Implement arch_bpf_stack_walk() for the BPF JIT

[Alexei Starovoitov]

On Thu, Feb 22, 2024 at 11:28 AM Catalin Marinas
<catalin.marinas@arm.com> wrote:
>
> On Thu, Feb 01, 2024 at 12:52:24PM +0000, Puranjay Mohan wrote:
> > This will be used by bpf_throw() to unwind till the program marked as
> > exception boundary and run the callback with the stack of the main
> > program.
> >
> > This is required for supporting BPF exceptions on ARM64.
> >
> > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> > ---
> >  arch/arm64/kernel/stacktrace.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > index 7f88028a00c0..66cffc5fc0be 100644
> > --- a/arch/arm64/kernel/stacktrace.c
> > +++ b/arch/arm64/kernel/stacktrace.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/efi.h>
> >  #include <linux/export.h>
> > +#include <linux/filter.h>
> >  #include <linux/ftrace.h>
> >  #include <linux/kprobes.h>
> >  #include <linux/sched.h>
> > @@ -266,6 +267,31 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
> >       kunwind_stack_walk(arch_kunwind_consume_entry, &data, task, regs);
> >  }
> >
> > +struct bpf_unwind_consume_entry_data {
> > +     bool (*consume_entry)(void *cookie, u64 ip, u64 sp, u64 fp);
> > +     void *cookie;
> > +};
> > +
> > +static bool
> > +arch_bpf_unwind_consume_entry(const struct kunwind_state *state, void *cookie)
> > +{
> > +     struct bpf_unwind_consume_entry_data *data = cookie;
> > +
> > +     return data->consume_entry(data->cookie, state->common.pc, 0,
> > +                                state->common.fp);
> > +}
> > +
> > +noinline noinstr void arch_bpf_stack_walk(bool (*consume_entry)(void *cookie, u64 ip, u64 sp,
> > +                                                             u64 fp), void *cookie)
> > +{
> > +     struct bpf_unwind_consume_entry_data data = {
> > +             .consume_entry = consume_entry,
> > +             .cookie = cookie,
> > +     };
> > +
> > +     kunwind_stack_walk(arch_bpf_unwind_consume_entry, &data, current, NULL);
> > +}
>
> Too many "cookies", I found reading this confusing. If you ever respin,
> please use some different "cookie" names.
>
> I guess you want this to be merged via the bpf tree?

We typically take bpf jit patches through bpf-next, since
we do cross arch jits refactoring from time to time,
but nothing like this is pending for this merge window,
so if you want it to go through arm64 tree that's fine with us.

> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Thank you for the review!

Re: [PATCH bpf-next v3 1/2] arm64: stacktrace: Implement arch_bpf_stack_walk() for the BPF JIT

[Catalin Marinas]

On Thu, Feb 22, 2024 at 06:04:35PM -0800, Alexei Starovoitov wrote:
> On Thu, Feb 22, 2024 at 11:28 AM Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Thu, Feb 01, 2024 at 12:52:24PM +0000, Puranjay Mohan wrote:
> > > This will be used by bpf_throw() to unwind till the program marked as
> > > exception boundary and run the callback with the stack of the main
> > > program.
> > >
> > > This is required for supporting BPF exceptions on ARM64.
> > >
> > > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> > > ---
> > >  arch/arm64/kernel/stacktrace.c | 26 ++++++++++++++++++++++++++
> > >  1 file changed, 26 insertions(+)
[...]
> > I guess you want this to be merged via the bpf tree?
> 
> We typically take bpf jit patches through bpf-next, since
> we do cross arch jits refactoring from time to time,
> but nothing like this is pending for this merge window,
> so if you want it to go through arm64 tree that's fine with us.

I don't have any preference. I can add it on top of the other arm64
patches if there are no dependencies on it from your side.

-- 
Catalin

Re: [PATCH bpf-next v3 1/2] arm64: stacktrace: Implement arch_bpf_stack_walk() for the BPF JIT

[Catalin Marinas]

On Tue, Feb 27, 2024 at 06:01:56PM +0000, Catalin Marinas wrote:
> On Thu, Feb 22, 2024 at 06:04:35PM -0800, Alexei Starovoitov wrote:
> > On Thu, Feb 22, 2024 at 11:28 AM Catalin Marinas
> > <catalin.marinas@arm.com> wrote:
> > > On Thu, Feb 01, 2024 at 12:52:24PM +0000, Puranjay Mohan wrote:
> > > > This will be used by bpf_throw() to unwind till the program marked as
> > > > exception boundary and run the callback with the stack of the main
> > > > program.
> > > >
> > > > This is required for supporting BPF exceptions on ARM64.
> > > >
> > > > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> > > > ---
> > > >  arch/arm64/kernel/stacktrace.c | 26 ++++++++++++++++++++++++++
> > > >  1 file changed, 26 insertions(+)
> [...]
> > > I guess you want this to be merged via the bpf tree?
> > 
> > We typically take bpf jit patches through bpf-next, since
> > we do cross arch jits refactoring from time to time,
> > but nothing like this is pending for this merge window,
> > so if you want it to go through arm64 tree that's fine with us.
> 
> I don't have any preference. I can add it on top of the other arm64
> patches if there are no dependencies on it from your side.

Actually, it depends on patches in bpf-next AFAICT (it doesn't apply
cleanly on top of vanilla -rc3). So please take the patches through the
bpf tree.

Thanks.

-- 
Catalin

Re: [PATCH bpf-next v3 1/2] arm64: stacktrace: Implement arch_bpf_stack_walk() for the BPF JIT

[Alexei Starovoitov]

On Tue, Feb 27, 2024 at 10:10 AM Catalin Marinas
<catalin.marinas@arm.com> wrote:
>
> On Tue, Feb 27, 2024 at 06:01:56PM +0000, Catalin Marinas wrote:
> > On Thu, Feb 22, 2024 at 06:04:35PM -0800, Alexei Starovoitov wrote:
> > > On Thu, Feb 22, 2024 at 11:28 AM Catalin Marinas
> > > <catalin.marinas@arm.com> wrote:
> > > > On Thu, Feb 01, 2024 at 12:52:24PM +0000, Puranjay Mohan wrote:
> > > > > This will be used by bpf_throw() to unwind till the program marked as
> > > > > exception boundary and run the callback with the stack of the main
> > > > > program.
> > > > >
> > > > > This is required for supporting BPF exceptions on ARM64.
> > > > >
> > > > > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> > > > > ---
> > > > >  arch/arm64/kernel/stacktrace.c | 26 ++++++++++++++++++++++++++
> > > > >  1 file changed, 26 insertions(+)
> > [...]
> > > > I guess you want this to be merged via the bpf tree?
> > >
> > > We typically take bpf jit patches through bpf-next, since
> > > we do cross arch jits refactoring from time to time,
> > > but nothing like this is pending for this merge window,
> > > so if you want it to go through arm64 tree that's fine with us.
> >
> > I don't have any preference. I can add it on top of the other arm64
> > patches if there are no dependencies on it from your side.
>
> Actually, it depends on patches in bpf-next AFAICT (it doesn't apply
> cleanly on top of vanilla -rc3). So please take the patches through the
> bpf tree.

Ok. Took it into bpf-next.

Please take a look at these Puranjay's patchset:

https://patchwork.kernel.org/project/netdevbpf/cover/20240221145106.105995-1-puranjay12@gmail.com/

It's a pretty nice performance improvement.