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

On Thu, Nov 07, 2019 at 11:07:21PM +0000, Song Liu wrote:

quoted

On Nov 7, 2019, at 2:55 PM, Alexei Starovoitov [off-list ref] wrote:

On Thu, Nov 07, 2019 at 10:37:19PM +0000, Song Liu wrote:

quoted

quoted

On Nov 6, 2019, at 9:46 PM, Alexei Starovoitov [off-list ref] wrote:

[...]

quoted

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Andrii Nakryiko <redacted>
---
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.

quoted

quoted

+	/* 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.

quoted

quoted

+
+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.

quoted

quoted

quoted

+{
+	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.