| Message ID | 20210605111034.1810858-11-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | x86/ftrace/bpf: Add batch support for direct/tracing attach | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 6/5/21 4:10 AM, Jiri Olsa wrote: > When we will have multiple functions attached to trampoline > we need to propagate the function's address to the bpf program. > > Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline > function that will store origin caller's address before function's > arguments. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++---- > include/linux/bpf.h | 5 +++++ > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > index b77e6bd78354..d2425c18272a 100644 > --- a/arch/x86/net/bpf_jit_comp.c > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -1951,7 +1951,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > void *orig_call) > { > int ret, i, cnt = 0, nr_args = m->nr_args; > - int stack_size = nr_args * 8; > + int stack_size = nr_args * 8, ip_arg = 0; > struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY]; > struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT]; > struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN]; > @@ -1975,6 +1975,9 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > */ > orig_call += X86_PATCH_SIZE; > > + if (flags & BPF_TRAMP_F_IP_ARG) > + stack_size += 8; > + > prog = image; > > EMIT1(0x55); /* push rbp */ > @@ -1982,7 +1985,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > EMIT4(0x48, 0x83, 0xEC, stack_size); /* sub rsp, stack_size */ > EMIT1(0x53); /* push rbx */ > > - save_regs(m, &prog, nr_args, stack_size); > + if (flags & BPF_TRAMP_F_IP_ARG) { > + emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); > + EMIT4(0x48, 0x83, 0xe8, X86_PATCH_SIZE); /* sub $X86_PATCH_SIZE,%rax*/ Could you explain what the above EMIT4 is for? I am not quite familiar with this piece of code and hence the question. Some comments here should help too. > + emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -stack_size); > + ip_arg = 8; > + } > + > + save_regs(m, &prog, nr_args, stack_size - ip_arg); > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > /* arg1: mov rdi, im */ > @@ -2011,7 +2021,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > } > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > - restore_regs(m, &prog, nr_args, stack_size); > + restore_regs(m, &prog, nr_args, stack_size - ip_arg); > > if (flags & BPF_TRAMP_F_ORIG_STACK) { > emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); > @@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > } > > if (flags & BPF_TRAMP_F_RESTORE_REGS) > - restore_regs(m, &prog, nr_args, stack_size); > + restore_regs(m, &prog, nr_args, stack_size - ip_arg); > > /* This needs to be done regardless. If there were fmod_ret programs, > * the return value is only updated on the stack and still needs to be [...]
On Sun, Jun 06, 2021 at 08:21:51PM -0700, Yonghong Song wrote: > > > On 6/5/21 4:10 AM, Jiri Olsa wrote: > > When we will have multiple functions attached to trampoline > > we need to propagate the function's address to the bpf program. > > > > Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline > > function that will store origin caller's address before function's > > arguments. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++---- > > include/linux/bpf.h | 5 +++++ > > 2 files changed, 19 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > > index b77e6bd78354..d2425c18272a 100644 > > --- a/arch/x86/net/bpf_jit_comp.c > > +++ b/arch/x86/net/bpf_jit_comp.c > > @@ -1951,7 +1951,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > void *orig_call) > > { > > int ret, i, cnt = 0, nr_args = m->nr_args; > > - int stack_size = nr_args * 8; > > + int stack_size = nr_args * 8, ip_arg = 0; > > struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY]; > > struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT]; > > struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN]; > > @@ -1975,6 +1975,9 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > */ > > orig_call += X86_PATCH_SIZE; > > + if (flags & BPF_TRAMP_F_IP_ARG) > > + stack_size += 8; > > + > > prog = image; > > EMIT1(0x55); /* push rbp */ > > @@ -1982,7 +1985,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > EMIT4(0x48, 0x83, 0xEC, stack_size); /* sub rsp, stack_size */ > > EMIT1(0x53); /* push rbx */ > > - save_regs(m, &prog, nr_args, stack_size); > > + if (flags & BPF_TRAMP_F_IP_ARG) { > > + emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); > > + EMIT4(0x48, 0x83, 0xe8, X86_PATCH_SIZE); /* sub $X86_PATCH_SIZE,%rax*/ > > Could you explain what the above EMIT4 is for? I am not quite familiar with > this piece of code and hence the question. Some comments here > should help too. it's there to generate the 'sub $X86_PATCH_SIZE,%rax' instruction to get the real IP address of the traced function, and it's stored to stack on the next line I'll put more comments in there jirka > > > + emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -stack_size); > > + ip_arg = 8; > > + } > > + > > + save_regs(m, &prog, nr_args, stack_size - ip_arg); > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > > /* arg1: mov rdi, im */ > > @@ -2011,7 +2021,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > } > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > > - restore_regs(m, &prog, nr_args, stack_size); > > + restore_regs(m, &prog, nr_args, stack_size - ip_arg); > > if (flags & BPF_TRAMP_F_ORIG_STACK) { > > emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); > > @@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > } > > if (flags & BPF_TRAMP_F_RESTORE_REGS) > > - restore_regs(m, &prog, nr_args, stack_size); > > + restore_regs(m, &prog, nr_args, stack_size - ip_arg); > > /* This needs to be done regardless. If there were fmod_ret programs, > > * the return value is only updated on the stack and still needs to be > [...] >
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa <jolsa@kernel.org> wrote: > > When we will have multiple functions attached to trampoline > we need to propagate the function's address to the bpf program. > > Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline > function that will store origin caller's address before function's > arguments. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++---- > include/linux/bpf.h | 5 +++++ > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > index b77e6bd78354..d2425c18272a 100644 > --- a/arch/x86/net/bpf_jit_comp.c > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -1951,7 +1951,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > void *orig_call) > { > int ret, i, cnt = 0, nr_args = m->nr_args; > - int stack_size = nr_args * 8; > + int stack_size = nr_args * 8, ip_arg = 0; > struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY]; > struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT]; > struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN]; > @@ -1975,6 +1975,9 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > */ > orig_call += X86_PATCH_SIZE; > > + if (flags & BPF_TRAMP_F_IP_ARG) > + stack_size += 8; > + nit: move it a bit up where we adjust stack_size for BPF_TRAMP_F_CALL_ORIG flag? > prog = image; > > EMIT1(0x55); /* push rbp */ > @@ -1982,7 +1985,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > EMIT4(0x48, 0x83, 0xEC, stack_size); /* sub rsp, stack_size */ > EMIT1(0x53); /* push rbx */ > > - save_regs(m, &prog, nr_args, stack_size); > + if (flags & BPF_TRAMP_F_IP_ARG) { > + emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); > + EMIT4(0x48, 0x83, 0xe8, X86_PATCH_SIZE); /* sub $X86_PATCH_SIZE,%rax*/ > + emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -stack_size); > + ip_arg = 8; > + } why not pass flags into save_regs and let it handle this case without this extra ip_arg adjustment? > + > + save_regs(m, &prog, nr_args, stack_size - ip_arg); > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > /* arg1: mov rdi, im */ > @@ -2011,7 +2021,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > } > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > - restore_regs(m, &prog, nr_args, stack_size); > + restore_regs(m, &prog, nr_args, stack_size - ip_arg); > similarly (and symmetrically), pass flags into restore_regs() to handle that ip_arg transparently? > if (flags & BPF_TRAMP_F_ORIG_STACK) { > emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); > @@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > } > > if (flags & BPF_TRAMP_F_RESTORE_REGS) > - restore_regs(m, &prog, nr_args, stack_size); > + restore_regs(m, &prog, nr_args, stack_size - ip_arg); > > /* This needs to be done regardless. If there were fmod_ret programs, > * the return value is only updated on the stack and still needs to be > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 16fc600503fb..6cbf3c81c650 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -559,6 +559,11 @@ struct btf_func_model { > */ > #define BPF_TRAMP_F_ORIG_STACK BIT(3) > > +/* First argument is IP address of the caller. Makes sense for fentry/fexit > + * programs only. > + */ > +#define BPF_TRAMP_F_IP_ARG BIT(4) > + > /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50 > * bytes on x86. Pick a number to fit into BPF_IMAGE_SIZE / 2 > */ > -- > 2.31.1 >
On Tue, Jun 08, 2021 at 11:49:31AM -0700, Andrii Nakryiko wrote: > On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > When we will have multiple functions attached to trampoline > > we need to propagate the function's address to the bpf program. > > > > Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline > > function that will store origin caller's address before function's > > arguments. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++---- > > include/linux/bpf.h | 5 +++++ > > 2 files changed, 19 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > > index b77e6bd78354..d2425c18272a 100644 > > --- a/arch/x86/net/bpf_jit_comp.c > > +++ b/arch/x86/net/bpf_jit_comp.c > > @@ -1951,7 +1951,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > void *orig_call) > > { > > int ret, i, cnt = 0, nr_args = m->nr_args; > > - int stack_size = nr_args * 8; > > + int stack_size = nr_args * 8, ip_arg = 0; > > struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY]; > > struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT]; > > struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN]; > > @@ -1975,6 +1975,9 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > */ > > orig_call += X86_PATCH_SIZE; > > > > + if (flags & BPF_TRAMP_F_IP_ARG) > > + stack_size += 8; > > + > > nit: move it a bit up where we adjust stack_size for BPF_TRAMP_F_CALL_ORIG flag? ok > > > prog = image; > > > > EMIT1(0x55); /* push rbp */ > > @@ -1982,7 +1985,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > EMIT4(0x48, 0x83, 0xEC, stack_size); /* sub rsp, stack_size */ > > EMIT1(0x53); /* push rbx */ > > > > - save_regs(m, &prog, nr_args, stack_size); > > + if (flags & BPF_TRAMP_F_IP_ARG) { > > + emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); > > + EMIT4(0x48, 0x83, 0xe8, X86_PATCH_SIZE); /* sub $X86_PATCH_SIZE,%rax*/ > > + emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -stack_size); > > + ip_arg = 8; > > + } > > why not pass flags into save_regs and let it handle this case without > this extra ip_arg adjustment? > > > + > > + save_regs(m, &prog, nr_args, stack_size - ip_arg); > > > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > > /* arg1: mov rdi, im */ > > @@ -2011,7 +2021,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > } > > > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > > - restore_regs(m, &prog, nr_args, stack_size); > > + restore_regs(m, &prog, nr_args, stack_size - ip_arg); > > > > similarly (and symmetrically), pass flags into restore_regs() to > handle that ip_arg transparently? so you mean something like: if (flags & BPF_TRAMP_F_IP_ARG) stack_size -= 8; in both save_regs and restore_regs function, right? jirka > > > if (flags & BPF_TRAMP_F_ORIG_STACK) { > > emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); > > @@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > } > > > > if (flags & BPF_TRAMP_F_RESTORE_REGS) > > - restore_regs(m, &prog, nr_args, stack_size); > > + restore_regs(m, &prog, nr_args, stack_size - ip_arg); > > > > /* This needs to be done regardless. If there were fmod_ret programs, > > * the return value is only updated on the stack and still needs to be > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 16fc600503fb..6cbf3c81c650 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -559,6 +559,11 @@ struct btf_func_model { > > */ > > #define BPF_TRAMP_F_ORIG_STACK BIT(3) > > > > +/* First argument is IP address of the caller. Makes sense for fentry/fexit > > + * programs only. > > + */ > > +#define BPF_TRAMP_F_IP_ARG BIT(4) > > + > > /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50 > > * bytes on x86. Pick a number to fit into BPF_IMAGE_SIZE / 2 > > */ > > -- > > 2.31.1 > > >
On Tue, Jun 8, 2021 at 1:58 PM Jiri Olsa <jolsa@redhat.com> wrote: > > On Tue, Jun 08, 2021 at 11:49:31AM -0700, Andrii Nakryiko wrote: > > On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > > > When we will have multiple functions attached to trampoline > > > we need to propagate the function's address to the bpf program. > > > > > > Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline > > > function that will store origin caller's address before function's > > > arguments. > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > --- > > > arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++---- > > > include/linux/bpf.h | 5 +++++ > > > 2 files changed, 19 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > > > index b77e6bd78354..d2425c18272a 100644 > > > --- a/arch/x86/net/bpf_jit_comp.c > > > +++ b/arch/x86/net/bpf_jit_comp.c > > > @@ -1951,7 +1951,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > > void *orig_call) > > > { > > > int ret, i, cnt = 0, nr_args = m->nr_args; > > > - int stack_size = nr_args * 8; > > > + int stack_size = nr_args * 8, ip_arg = 0; > > > struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY]; > > > struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT]; > > > struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN]; > > > @@ -1975,6 +1975,9 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > > */ > > > orig_call += X86_PATCH_SIZE; > > > > > > + if (flags & BPF_TRAMP_F_IP_ARG) > > > + stack_size += 8; > > > + > > > > nit: move it a bit up where we adjust stack_size for BPF_TRAMP_F_CALL_ORIG flag? > > ok > > > > > > prog = image; > > > > > > EMIT1(0x55); /* push rbp */ > > > @@ -1982,7 +1985,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > > EMIT4(0x48, 0x83, 0xEC, stack_size); /* sub rsp, stack_size */ > > > EMIT1(0x53); /* push rbx */ > > > > > > - save_regs(m, &prog, nr_args, stack_size); > > > + if (flags & BPF_TRAMP_F_IP_ARG) { > > > + emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); > > > + EMIT4(0x48, 0x83, 0xe8, X86_PATCH_SIZE); /* sub $X86_PATCH_SIZE,%rax*/ > > > + emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -stack_size); > > > + ip_arg = 8; > > > + } > > > > why not pass flags into save_regs and let it handle this case without > > this extra ip_arg adjustment? > > > > > + > > > + save_regs(m, &prog, nr_args, stack_size - ip_arg); > > > > > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > > > /* arg1: mov rdi, im */ > > > @@ -2011,7 +2021,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > > } > > > > > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > > > - restore_regs(m, &prog, nr_args, stack_size); > > > + restore_regs(m, &prog, nr_args, stack_size - ip_arg); > > > > > > > similarly (and symmetrically), pass flags into restore_regs() to > > handle that ip_arg transparently? > > so you mean something like: > > if (flags & BPF_TRAMP_F_IP_ARG) > stack_size -= 8; > > in both save_regs and restore_regs function, right? yes, but for save_regs it will do more (emit_ldx and stuff) > > jirka > > > > > > if (flags & BPF_TRAMP_F_ORIG_STACK) { > > > emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); > > > @@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > > } > > > > > > if (flags & BPF_TRAMP_F_RESTORE_REGS) > > > - restore_regs(m, &prog, nr_args, stack_size); > > > + restore_regs(m, &prog, nr_args, stack_size - ip_arg); > > > > > > /* This needs to be done regardless. If there were fmod_ret programs, > > > * the return value is only updated on the stack and still needs to be > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > index 16fc600503fb..6cbf3c81c650 100644 > > > --- a/include/linux/bpf.h > > > +++ b/include/linux/bpf.h > > > @@ -559,6 +559,11 @@ struct btf_func_model { > > > */ > > > #define BPF_TRAMP_F_ORIG_STACK BIT(3) > > > > > > +/* First argument is IP address of the caller. Makes sense for fentry/fexit > > > + * programs only. > > > + */ > > > +#define BPF_TRAMP_F_IP_ARG BIT(4) > > > + > > > /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50 > > > * bytes on x86. Pick a number to fit into BPF_IMAGE_SIZE / 2 > > > */ > > > -- > > > 2.31.1 > > > > > >
On Tue, Jun 08, 2021 at 02:02:56PM -0700, Andrii Nakryiko wrote: > On Tue, Jun 8, 2021 at 1:58 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Tue, Jun 08, 2021 at 11:49:31AM -0700, Andrii Nakryiko wrote: > > > On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > > > > > When we will have multiple functions attached to trampoline > > > > we need to propagate the function's address to the bpf program. > > > > > > > > Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline > > > > function that will store origin caller's address before function's > > > > arguments. > > > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > > --- > > > > arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++---- > > > > include/linux/bpf.h | 5 +++++ > > > > 2 files changed, 19 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > > > > index b77e6bd78354..d2425c18272a 100644 > > > > --- a/arch/x86/net/bpf_jit_comp.c > > > > +++ b/arch/x86/net/bpf_jit_comp.c > > > > @@ -1951,7 +1951,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > > > void *orig_call) > > > > { > > > > int ret, i, cnt = 0, nr_args = m->nr_args; > > > > - int stack_size = nr_args * 8; > > > > + int stack_size = nr_args * 8, ip_arg = 0; > > > > struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY]; > > > > struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT]; > > > > struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN]; > > > > @@ -1975,6 +1975,9 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > > > */ > > > > orig_call += X86_PATCH_SIZE; > > > > > > > > + if (flags & BPF_TRAMP_F_IP_ARG) > > > > + stack_size += 8; > > > > + > > > > > > nit: move it a bit up where we adjust stack_size for BPF_TRAMP_F_CALL_ORIG flag? > > > > ok > > > > > > > > > prog = image; > > > > > > > > EMIT1(0x55); /* push rbp */ > > > > @@ -1982,7 +1985,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > > > EMIT4(0x48, 0x83, 0xEC, stack_size); /* sub rsp, stack_size */ > > > > EMIT1(0x53); /* push rbx */ > > > > > > > > - save_regs(m, &prog, nr_args, stack_size); > > > > + if (flags & BPF_TRAMP_F_IP_ARG) { > > > > + emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); > > > > + EMIT4(0x48, 0x83, 0xe8, X86_PATCH_SIZE); /* sub $X86_PATCH_SIZE,%rax*/ > > > > + emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -stack_size); > > > > + ip_arg = 8; > > > > + } > > > > > > why not pass flags into save_regs and let it handle this case without > > > this extra ip_arg adjustment? > > > > > > > + > > > > + save_regs(m, &prog, nr_args, stack_size - ip_arg); > > > > > > > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > > > > /* arg1: mov rdi, im */ > > > > @@ -2011,7 +2021,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i > > > > } > > > > > > > > if (flags & BPF_TRAMP_F_CALL_ORIG) { > > > > - restore_regs(m, &prog, nr_args, stack_size); > > > > + restore_regs(m, &prog, nr_args, stack_size - ip_arg); > > > > > > > > > > similarly (and symmetrically), pass flags into restore_regs() to > > > handle that ip_arg transparently? > > > > so you mean something like: > > > > if (flags & BPF_TRAMP_F_IP_ARG) > > stack_size -= 8; > > > > in both save_regs and restore_regs function, right? > > yes, but for save_regs it will do more (emit_ldx and stuff) so the whole stuff then, ok jirka
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c index b77e6bd78354..d2425c18272a 100644 --- a/arch/x86/net/bpf_jit_comp.c +++ b/arch/x86/net/bpf_jit_comp.c @@ -1951,7 +1951,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i void *orig_call) { int ret, i, cnt = 0, nr_args = m->nr_args; - int stack_size = nr_args * 8; + int stack_size = nr_args * 8, ip_arg = 0; struct bpf_tramp_progs *fentry = &tprogs[BPF_TRAMP_FENTRY]; struct bpf_tramp_progs *fexit = &tprogs[BPF_TRAMP_FEXIT]; struct bpf_tramp_progs *fmod_ret = &tprogs[BPF_TRAMP_MODIFY_RETURN]; @@ -1975,6 +1975,9 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i */ orig_call += X86_PATCH_SIZE; + if (flags & BPF_TRAMP_F_IP_ARG) + stack_size += 8; + prog = image; EMIT1(0x55); /* push rbp */ @@ -1982,7 +1985,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i EMIT4(0x48, 0x83, 0xEC, stack_size); /* sub rsp, stack_size */ EMIT1(0x53); /* push rbx */ - save_regs(m, &prog, nr_args, stack_size); + if (flags & BPF_TRAMP_F_IP_ARG) { + emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); + EMIT4(0x48, 0x83, 0xe8, X86_PATCH_SIZE); /* sub $X86_PATCH_SIZE,%rax*/ + emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -stack_size); + ip_arg = 8; + } + + save_regs(m, &prog, nr_args, stack_size - ip_arg); if (flags & BPF_TRAMP_F_CALL_ORIG) { /* arg1: mov rdi, im */ @@ -2011,7 +2021,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i } if (flags & BPF_TRAMP_F_CALL_ORIG) { - restore_regs(m, &prog, nr_args, stack_size); + restore_regs(m, &prog, nr_args, stack_size - ip_arg); if (flags & BPF_TRAMP_F_ORIG_STACK) { emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8); @@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i } if (flags & BPF_TRAMP_F_RESTORE_REGS) - restore_regs(m, &prog, nr_args, stack_size); + restore_regs(m, &prog, nr_args, stack_size - ip_arg); /* This needs to be done regardless. If there were fmod_ret programs, * the return value is only updated on the stack and still needs to be diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 16fc600503fb..6cbf3c81c650 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -559,6 +559,11 @@ struct btf_func_model { */ #define BPF_TRAMP_F_ORIG_STACK BIT(3) +/* First argument is IP address of the caller. Makes sense for fentry/fexit + * programs only. + */ +#define BPF_TRAMP_F_IP_ARG BIT(4) + /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50 * bytes on x86. Pick a number to fit into BPF_IMAGE_SIZE / 2 */
When we will have multiple functions attached to trampoline we need to propagate the function's address to the bpf program. Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline function that will store origin caller's address before function's arguments. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++---- include/linux/bpf.h | 5 +++++ 2 files changed, 19 insertions(+), 4 deletions(-)