Re: [PATCH bpf v2 2/6] bpf: powerpc64: add JIT support for multi-function programs

Daniel Borkmann wrote:

On 05/18/2018 02:50 PM, Sandipan Das wrote:

quoted

This adds support for bpf-to-bpf function calls in the powerpc64
JIT compiler. The JIT compiler converts the bpf call instructions
to native branch instructions. After a round of the usual passes,
the start addresses of the JITed images for the callee functions
are known. Finally, to fixup the branch target addresses, we need
to perform an extra pass.
=20
Because of the address range in which JITed images are allocated
on powerpc64, the offsets of the start addresses of these images
from __bpf_call_base are as large as 64 bits. So, for a function
call, we cannot use the imm field of the instruction to determine
the callee's address. Instead, we use the alternative method of
getting it from the list of function addresses in the auxillary
data of the caller by using the off field as an index.
=20
Signed-off-by: Sandipan Das <redacted>
---
 arch/powerpc/net/bpf_jit_comp64.c | 79 ++++++++++++++++++++++++++++++++=

++-----

quoted

 1 file changed, 69 insertions(+), 10 deletions(-)
=20

diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_ji=

t_comp64.c

quoted

index 1bdb1aff0619..25939892d8f7 100644

--- a/arch/powerpc/net/bpf_jit_comp64.c
+++ b/arch/powerpc/net/bpf_jit_comp64.c

@@ -256,7 +256,7 @@ static void bpf_jit_emit_tail_call(u32 *image, struc=

t codegen_context *ctx, u32

quoted

 /* Assemble the body code between the prologue & epilogue */
 static int bpf_jit_build_body(struct bpf_prog *fp, u32 *image,
 			      struct codegen_context *ctx,
-			      u32 *addrs)
+			      u32 *addrs, bool extra_pass)
 {
 	const struct bpf_insn *insn =3D fp->insnsi;
 	int flen =3D fp->len;

@@ -712,11 +712,23 @@ static int bpf_jit_build_body(struct bpf_prog *fp,=

 u32 *image,

quoted

 			break;
=20
 		/*
-		 * Call kernel helper
+		 * Call kernel helper or bpf function
 		 */
 		case BPF_JMP | BPF_CALL:
 			ctx->seen |=3D SEEN_FUNC;
-			func =3D (u8 *) __bpf_call_base + imm;
+
+			/* bpf function call */
+			if (insn[i].src_reg =3D=3D BPF_PSEUDO_CALL && extra_pass)

=20
Perhaps it might make sense here for !extra_pass to set func to some dumm=

y

address as otherwise the 'kernel helper call' branch used for this is a b=

it

misleading in that sense. The PPC_LI64() used in bpf_jit_emit_func_call()
optimizes the immediate addr, I presume the JIT can handle situations whe=

re

in the final extra_pass the image needs to grow/shrink again (due to diff=

erent

final address for the call)?

That's a good catch. We don't handle that -- we expect to get the size=20
right on first pass. We could probably have PPC_FUNC_ADDR() pad the=20
result with nops to make it a constant 5-instruction sequence.

=20

quoted

+				if (fp->aux->func && off < fp->aux->func_cnt)
+					/* use the subprog id from the off
+					 * field to lookup the callee address
+					 */
+					func =3D (u8 *) fp->aux->func[off]->bpf_func;
+				else
+					return -EINVAL;
+			/* kernel helper call */
+			else
+				func =3D (u8 *) __bpf_call_base + imm;
=20
 			bpf_jit_emit_func_call(image, ctx, (u64)func);
=20

@@ -864,6 +876,14 @@ static int bpf_jit_build_body(struct bpf_prog *fp, =

u32 *image,

quoted

 	return 0;
 }
=20
+struct powerpc64_jit_data {
+	struct bpf_binary_header *header;
+	u32 *addrs;
+	u8 *image;
+	u32 proglen;
+	struct codegen_context ctx;
+};
+
 struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
 {
 	u32 proglen;

@@ -871,6 +891,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog=

 *fp)

quoted

 	u8 *image =3D NULL;
 	u32 *code_base;
 	u32 *addrs;
+	struct powerpc64_jit_data *jit_data;
 	struct codegen_context cgctx;
 	int pass;
 	int flen;

@@ -878,6 +899,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog=

 *fp)

quoted

 	struct bpf_prog *org_fp =3D fp;
 	struct bpf_prog *tmp_fp;
 	bool bpf_blinded =3D false;
+	bool extra_pass =3D false;
=20
 	if (!fp->jit_requested)
 		return org_fp;

@@ -891,7 +913,28 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_pro=

g *fp)

quoted

 		fp =3D tmp_fp;
 	}
=20
+	jit_data =3D fp->aux->jit_data;
+	if (!jit_data) {
+		jit_data =3D kzalloc(sizeof(*jit_data), GFP_KERNEL);
+		if (!jit_data) {
+			fp =3D org_fp;
+			goto out;
+		}
+		fp->aux->jit_data =3D jit_data;
+	}
+
 	flen =3D fp->len;
+	addrs =3D jit_data->addrs;
+	if (addrs) {
+		cgctx =3D jit_data->ctx;
+		image =3D jit_data->image;
+		bpf_hdr =3D jit_data->header;
+		proglen =3D jit_data->proglen;
+		alloclen =3D proglen + FUNCTION_DESCR_SIZE;
+		extra_pass =3D true;
+		goto skip_init_ctx;
+	}
+
 	addrs =3D kzalloc((flen+1) * sizeof(*addrs), GFP_KERNEL);
 	if (addrs =3D=3D NULL) {
 		fp =3D org_fp;

=20
In this case of !addrs, we leak the just allocated jit_data here!
=20

quoted

@@ -904,10 +947,10 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_pr=

og *fp)

quoted

 	cgctx.stack_size =3D round_up(fp->aux->stack_depth, 16);
=20
 	/* Scouting faux-generate pass 0 */
-	if (bpf_jit_build_body(fp, 0, &cgctx, addrs)) {
+	if (bpf_jit_build_body(fp, 0, &cgctx, addrs, false)) {
 		/* We hit something illegal or unsupported. */
 		fp =3D org_fp;
-		goto out;
+		goto out_addrs;
 	}
=20
 	/*

@@ -925,9 +968,10 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_pro=

g *fp)

quoted

 			bpf_jit_fill_ill_insns);
 	if (!bpf_hdr) {
 		fp =3D org_fp;
-		goto out;
+		goto out_addrs;
 	}
=20
+skip_init_ctx:
 	code_base =3D (u32 *)(image + FUNCTION_DESCR_SIZE);
=20
 	/* Code generation passes 1-2 */

@@ -935,7 +979,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog=

 *fp)

quoted

 		/* Now build the prologue, body code & epilogue for real. */
 		cgctx.idx =3D 0;
 		bpf_jit_build_prologue(code_base, &cgctx);
-		bpf_jit_build_body(fp, code_base, &cgctx, addrs);
+		bpf_jit_build_body(fp, code_base, &cgctx, addrs, extra_pass);
 		bpf_jit_build_epilogue(code_base, &cgctx);
=20
 		if (bpf_jit_enable > 1)

@@ -956,15 +1000,30 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_p=

rog *fp)

quoted

 	((u64 *)image)[1] =3D local_paca->kernel_toc;
 #endif
=20
+	bpf_flush_icache(bpf_hdr, (u8 *)bpf_hdr + (bpf_hdr->pages * PAGE_SIZE)=

);

quoted

+
+	if (!fp->is_func || extra_pass) {
+		bpf_jit_binary_lock_ro(bpf_hdr);

=20
powerpc doesn't implement set_memory_ro(). Generally this is not a proble=

m since

set_memory_ro() defaults to 'return 0' in this case, but since the bpf_ji=

t_free()

destructor is overridden here, there's no bpf_jit_binary_unlock_ro() and =

in case

powerpc would get set_memory_*() support one day this will then crash in =

random

places once the mem gets back to the allocator, thus hard to debug. Two o=

ptions:

either you remove the bpf_jit_free() override or you remove the bpf_jit_b=

inary_lock_ro().

Yeah, we shouldn't be using the lock here.

Thanks,
Naveen

=