On Thu, Nov 07, 2019 at 11:07:21PM +0000, Song Liu wrote:
>
>
> > On Nov 7, 2019, at 2:55 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Thu, Nov 07, 2019 at 10:37:19PM +0000, Song Liu wrote:
> >>
> >>
> >>> On Nov 6, 2019, at 9:46 PM, Alexei Starovoitov <ast@xxxxxxxxxx> wrote:
> >>>
> >>
> >> [...]
> >>
> >>>
> >>> Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> >>> Acked-by: Andrii Nakryiko <andriin@xxxxxx>
> >>> ---
> >>> arch/x86/net/bpf_jit_comp.c | 227 ++++++++++++++++++++++++++++++--
> >>> include/linux/bpf.h | 98 ++++++++++++++
> >>> include/uapi/linux/bpf.h | 2 +
> >>> kernel/bpf/Makefile | 1 +
> >>> kernel/bpf/btf.c | 77 ++++++++++-
> >>> kernel/bpf/core.c | 1 +
> >>> kernel/bpf/syscall.c | 53 +++++++-
> >>> kernel/bpf/trampoline.c | 252 ++++++++++++++++++++++++++++++++++++
> >>> kernel/bpf/verifier.c | 39 ++++++
> >>> 9 files changed, 732 insertions(+), 18 deletions(-)
> >>> create mode 100644 kernel/bpf/trampoline.c
> >>>
> >>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> >>> index 8631d3bd637f..44169e8bffc0 100644
> >>> --- a/arch/x86/net/bpf_jit_comp.c
> >>> +++ b/arch/x86/net/bpf_jit_comp.c
> >>> @@ -98,6 +98,7 @@ static int bpf_size_to_x86_bytes(int bpf_size)
> >>>
> >>> /* Pick a register outside of BPF range for JIT internal work */
> >>> #define AUX_REG (MAX_BPF_JIT_REG + 1)
> >>> +#define X86_REG_R9 (MAX_BPF_JIT_REG + 2)
> >>>
> >>> /*
> >>> * The following table maps BPF registers to x86-64 registers.
> >>> @@ -123,6 +124,7 @@ static const int reg2hex[] = {
> >>> [BPF_REG_FP] = 5, /* RBP readonly */
> >>> [BPF_REG_AX] = 2, /* R10 temp register */
> >>> [AUX_REG] = 3, /* R11 temp register */
> >>> + [X86_REG_R9] = 1, /* R9 register, 6th function argument */
> >>
> >> We should update the comment above this:
> >>
> >> * Also x86-64 register R9 is unused. ...
> >
> > good point. fixed.
> >
> >>> + /* One half of the page has active running trampoline.
> >>> + * Another half is an area for next trampoline.
> >>> + * Make sure the trampoline generation logic doesn't overflow.
> >>> + */
> >>> + if (WARN_ON_ONCE(prog - (u8 *)image > PAGE_SIZE / 2 - BPF_INSN_SAFETY))
> >>> + return -EFAULT;
> >>
> >> Given max number of args, can we catch this error at compile time?
> >
> > I don't see how to do that. I was thinking about having fake __init function
> > that would call it with flags that can generate the longest trampoline, but
> > it's not fool proof either.
> > So I've added a test for it instead. See patch 10.
> >
> >>> +
> >>> +static int bpf_trampoline_update(struct bpf_prog *prog)
> >>
> >> Seems argument "prog" is not used at all?
> >
> > like one below ? ;)
> e... I was really dumb... sorry..
>
> Maybe we should just pass the tr in?
that would be imbalanced.
> >
> >>> +{
> >>> + struct bpf_trampoline *tr = prog->aux->trampoline;
> >>> + void *old_image = tr->image + ((tr->selector + 1) & 1) * PAGE_SIZE/2;
> >>> + void *new_image = tr->image + (tr->selector & 1) * PAGE_SIZE/2;
> >>> + if (err)
> >>> + goto out;
> >>> + tr->selector++;
> >>
> >> Shall we do selector-- for unlink?
> >
> > It's a bit flip. I think it would be more confusing with --
>
> Right.. Maybe should use int instead of u64 for selector?
No, since int can overflow.
