netdev - Re: [PATCH v2 bpf-next 03/17] bpf: Introduce BPF trampoline

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <FABEB3EB-2AC4-43F8-984B-EFD1DA621A3E@fb.com>
Date:   Thu, 7 Nov 2019 23:07:21 +0000
From:   Song Liu <songliubraving@...com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     Alexei Starovoitov <ast@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "x86@...nel.org" <x86@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "Kernel Team" <Kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 03/17] bpf: Introduce BPF trampoline



> On Nov 7, 2019, at 2:55 PM, Alexei Starovoitov <alexei.starovoitov@...il.com> 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@...nel.org> wrote:
>>> 
>> 
>> [...]
>> 
>>> 
>>> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
>>> Acked-by: Andrii Nakryiko <andriin@...com>
>>> ---
>>> 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? 
> 
>>> +{
>>> +	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?