Re: [PATCH bpf-next v2 2/4] bpf, x86: Create bpf_trace_run_ctx on the caller thread's stack

[Kui-Feng Lee <kuifeng@fb.com>]

On Sun, 2022-03-20 at 13:08 -0700, Alexei Starovoitov wrote:
> On Sun, Mar 20, 2022 at 2:31 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > 
> > On Fri, 2022-03-18 at 12:09 -0700, Alexei Starovoitov wrote:
> > > On Tue, Mar 15, 2022 at 05:42:29PM -0700, Kui-Feng Lee wrote:
> > > > BPF trampolines will create a bpf_trace_run_ctx on their
> > > > stacks,
> > > > and
> > > > set/reset the current bpf_run_ctx whenever calling/returning
> > > > from a
> > > > bpf_prog.
> > > > 
> > > > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > > > ---
> > > >  arch/x86/net/bpf_jit_comp.c | 32
> > > > ++++++++++++++++++++++++++++++++
> > > >  include/linux/bpf.h         | 12 ++++++++----
> > > >  kernel/bpf/syscall.c        |  4 ++--
> > > >  kernel/bpf/trampoline.c     | 21 +++++++++++++++++----
> > > >  4 files changed, 59 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/net/bpf_jit_comp.c
> > > > b/arch/x86/net/bpf_jit_comp.c
> > > > index 1228e6e6a420..29775a475513 100644
> > > > --- a/arch/x86/net/bpf_jit_comp.c
> > > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > > @@ -1748,10 +1748,33 @@ static int invoke_bpf_prog(const struct
> > > > btf_func_model *m, u8 **pprog,
> > > >  {
> > > >         u8 *prog = *pprog;
> > > >         u8 *jmp_insn;
> > > > +       int ctx_cookie_off = offsetof(struct bpf_trace_run_ctx,
> > > > bpf_cookie);
> > > >         struct bpf_prog *p = l->prog;
> > > > 
> > > > +       EMIT1(0x52);             /* push rdx */
> > > 
> > > Why save/restore rdx?
> > 
> > > 
> > > > +
> > > > +       /* mov rdi, 0 */
> > > > +       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> > > > +
> > > > +       /* Prepare struct bpf_trace_run_ctx.
> > > > +        * sub rsp, sizeof(struct bpf_trace_run_ctx)
> > > > +        * mov rax, rsp
> > > > +        * mov QWORD PTR [rax + ctx_cookie_off], rdi
> > > > +        */
> > > 
> > > How about the following instead:
> > > sub rsp, sizeof(struct bpf_trace_run_ctx)
> > > mov qword ptr [rsp + ctx_cookie_off], 0
> > > ?
> > 
> > AFAIK, rsp can not be used with the base + displacement addressing
> > mode.  Although, it can be used with base + index + displacement
> > addressing mode.
> 
> Where did you find this?

I use the following document to figure out opcodes.

https://ref.x86asm.net/coder64.html#modrm_byte_32_64

It lists available addressing modes and codes.

By the way, I found I had missed SIB byte, that provides extra
features.  It seems working for this case.  I will try it.


> 
> 0:  48 c7 44 24 08 00 00    mov    QWORD PTR [rsp+0x8],0x0
> 7:  00 00
> 
> > > 
> > > > +       EMIT4(0x48, 0x83, 0xEC, sizeof(struct
> > > > bpf_trace_run_ctx));
> > > > +       EMIT3(0x48, 0x89, 0xE0);
> > > > +       EMIT4(0x48, 0x89, 0x78, ctx_cookie_off);
> > > > +
> > > > +       /* mov rdi, rsp */
> > > > +       EMIT3(0x48, 0x89, 0xE7);
> > > > +       /* mov QWORD PTR [rdi + sizeof(struct
> > > > bpf_trace_run_ctx)],
> > > > rax */
> > > > +       emit_stx(&prog, BPF_DW, BPF_REG_1, BPF_REG_0,
> > > > sizeof(struct
> > > > bpf_trace_run_ctx));
> > > 
> > > why not to do:
> > > mov qword ptr[rsp + sizeof(struct bpf_trace_run_ctx)], rsp
> > > ?
> > 
> > The same reason as above.
> 
> 0:  48 89 64 24 08          mov    QWORD PTR [rsp+0x8],rsp