On Mon, Jul 17, 2023 at 9:29 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Sat, 15 Jul 2023 at 03:35, Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Thu, Jul 13, 2023 at 08:02:27AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > The plumbing for offline unwinding when we throw an exception in
> > > programs would require walking the stack, hence introduce a new
> > > arch_bpf_stack_walk function. This is provided when the JIT supports
> > > exceptions, i.e. bpf_jit_supports_exceptions is true. The arch-specific
> > > code is really minimal, hence it should straightforward to extend this
> > > support to other architectures as well, as it reuses the logic of
> > > arch_stack_walk, but allowing access to unwind_state data.
> > >
> > > Once the stack pointer and frame pointer are known for the main subprog
> > > during the unwinding, we know the stack layout and location of any
> > > callee-saved registers which must be restored before we return back to
> > > the kernel.
> > >
> > > This handling will be added in the next patch.
> > >
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > > ---
> > > arch/x86/net/bpf_jit_comp.c | 21 +++++++++++++++++++++
> > > include/linux/filter.h | 2 ++
> > > kernel/bpf/core.c | 9 +++++++++
> > > 3 files changed, 32 insertions(+)
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index 438adb695daa..d326503ce242 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -16,6 +16,7 @@
> > > #include <asm/set_memory.h>
> > > #include <asm/nospec-branch.h>
> > > #include <asm/text-patching.h>
> > > +#include <asm/unwind.h>
> > >
> > > static u8 *emit_code(u8 *ptr, u32 bytes, unsigned int len)
> > > {
> > > @@ -2660,3 +2661,23 @@ void bpf_jit_free(struct bpf_prog *prog)
> > >
> > > bpf_prog_unlock_free(prog);
> > > }
> > > +
> > > +bool bpf_jit_supports_exceptions(void)
> > > +{
> > > + return IS_ENABLED(CONFIG_UNWINDER_ORC) || IS_ENABLED(CONFIG_UNWINDER_FRAME_POINTER);
> > > +}
> > > +
> > > +void arch_bpf_stack_walk(bool (*consume_fn)(void *cookie, u64 ip, u64 sp, u64 bp), void *cookie)
> > > +{
> > > +#if defined(CONFIG_UNWINDER_ORC) || defined(CONFIG_UNWINDER_FRAME_POINTER)
> > > + struct unwind_state state;
> > > + unsigned long addr;
> > > +
> > > + for (unwind_start(&state, current, NULL, NULL); !unwind_done(&state);
> > > + unwind_next_frame(&state)) {
> > > + addr = unwind_get_return_address(&state);
> >
> > I think these steps will work even with UNWINDER_GUESS.
> > What is the reason for #ifdef ?
>
> I think we require both unwind_state::sp and unwind_state::bp, but
> arch/x86/include/asm/unwind.h does not include unwind_state::bp when
> both UNWINDER_ORC and UNWINDER_FRAME_POINTER are unset.
>
> Although it might be possible to calculate and save bp offset during
> JIT in bpf_prog_aux (by adding roundup(stack_depth) + 8 (push rax if
> tail call reachable) + callee_regs_saved) for the subprog
> corresponding to a frame. Then we can make it work everywhere.
> The JIT will abstract get_prog_bp(sp) using an arch specific helper.
>
> Let me know if I misunderstood something.
JITed progs always have frames. So we're effectively doing
unconditional UNWINDER_FRAME_POINTER.
I think the intended usage of arch_bpf_stack_walk() is to only walk
bpf frames _in this patch set_, if so the extra #ifdefs are misleading.
If in follow-ups we're going to unwind through JITed progs _and_
through kfunc/helpers then this ifdef will be necessary.
I suspect we might want something like this in the future.
So the ifdef is ok to have from the start, but the comment is necessary
to describe what it is for.
