On Mon, 30 Sept 2024 at 17:03, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Sep 26, 2024 at 4:45 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> >
> > Add jit support for private stack. For a particular subtree, e.g.,
> > subtree_root <== stack depth 120
> > subprog1 <== stack depth 80
> > subprog2 <== stack depth 40
> > subprog3 <== stack depth 160
> >
> > Let us say that private_stack_ptr is the memory address allocated for
> > private stack. The frame pointer for each above is calculated like below:
> > subtree_root <== subtree_root_fp = private_stack_ptr + 120
> > subprog1 <== subtree_subprog1_fp = subtree_root_fp + 80
> > subprog2 <== subtree_subprog2_fp = subtree_subprog1_fp + 40
> > subprog3 <== subtree_subprog1_fp = subtree_root_fp + 160
> >
> > For any function call to helper/kfunc, push/pop prog frame pointer
> > is needed in order to preserve frame pointer value.
> >
> > To deal with exception handling, push/pop frame pointer is also used
> > surrounding call to subsequent subprog. For example,
> > subtree_root
> > subprog1
> > ...
> > insn: call bpf_throw
> > ...
> >
> > After jit, we will have
> > subtree_root
> > insn: push r9
> > subprog1
> > ...
> > insn: push r9
> > insn: call bpf_throw
> > insn: pop r9
> > ...
> > insn: pop r9
> >
> > exception_handler
> > pop r9
> > ...
> > where r9 represents the fp for each subprog.
>
> Kumar,
> please review the interaction of priv_stack with exceptions.
>
Hm, I think it works fine (because of push_r9 around subprog calls),
but I think it's not needed (unless I missed something).
The kernel won't care about r9's value, so the only reason pop_r9 is
being done in ex_handler is to restore the private stack value?
In that case you can just push_r9 once for
prog->aux->exception_boundary after emit_private_frame_ptr, since only
this main subprog's stack will be reused to run the exception
callback. Then keep the pop_r9 as is. And then you don't need to
push_r9 and pop_r9 around subprog calls.
But do you need to do it? It seems later on it will just be overwritten.
Since exception_cb is a PSTACK_TREE_ROOT, you will set its r9 using mov.
So it seems all of this may be unnecessary.
Let me know if I misunderstood something.
...
Another issue that I came across when reading through patches is how
this interacts with extension progs.
Say right now I have this call chain:
main_subprog
global_subprog (overriden by ext prog)
subprog1
call helper
subprog2
global_subprog (which is a separate ext prog attached there using
freplace) is not using private stack, but main prog is.
So when main_subprog calls into it, it is possible r9 gets overwritten
since JIT didn't preserve it for helper calls from the ext prog.
When subprog2 is called, this will mean r9 equals garbage?
There was a vaguely similar decision to make for exceptions: if we
keep unwinding beyond an extension prog frame, we might end up in the
main subprog frame that did not push the necessary registers to
restore kernel state from exception_cb (because it did not have
bpf_throw during verification). So I ended up making the ext prog
itself an exception boundary, so unwinding stops there and it would
return a result to its caller prog which may or may not use
exceptions.
In my case, I cannot go and re-JIT old progs to make them exception
aware (short of always pushing r12 and all callee regs).
But in your case the changes would be contained to ext_prog, so you
could just figure that out from the tgt_prog seen in
bpf_check_attach_target (i.e. whether it is using priv_stack or not,
and if so, extra JIT instrumentation to reuse and push/pop r9 around
helper calls is necessary for this ext_prog).
> >
> > [...]
