On Mon, 17 Jul 2023 at 22:59, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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.
I think we actually do unwind through bpf_throw at the very least, so
we are going through both kernel and BPF frames.
> So the ifdef is ok to have from the start, but the comment is necessary
> to describe what it is for.
I'll add the comment in v2.
