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

Commit Message

Puranjay Mohan Feb. 1, 2024, 12:52 p.m. UTC

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 <[e-mail]>
---
 arch/arm64/kernel/stacktrace.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Catalin Marinas Feb. 22, 2024, 7:28 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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 <[e-mail]>

Alexei Starovoitov Feb. 23, 2024, 2:04 a.m. UTC | #2

On Thu, Feb 22, 2024 at 11:28 AM Catalin Marinas
<[e-mail]> 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 <[e-mail]>
> > ---
> >  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 <[e-mail]>

Thank you for the review!

Catalin Marinas Feb. 27, 2024, 6:01 p.m. UTC | #3

On Thu, Feb 22, 2024 at 06:04:35PM -0800, Alexei Starovoitov wrote:
> On Thu, Feb 22, 2024 at 11:28 AM Catalin Marinas
> <[e-mail]> 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 <[e-mail]>
> > > ---
> > >  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 Marinas Feb. 27, 2024, 6:10 p.m. UTC | #4

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
> > <[e-mail]> 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 <[e-mail]>
> > > > ---
> > > >  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.

Alexei Starovoitov Feb. 27, 2024, 9:57 p.m. UTC | #5

On Tue, Feb 27, 2024 at 10:10 AM Catalin Marinas
<[e-mail]> 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
> > > <[e-mail]> 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 <[e-mail]>
> > > > > ---
> > > > >  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/[e-mail]/

It's a pretty nice performance improvement.

Patch

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;