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

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

Daniel Borkmann wrote:
> On 05/18/2018 02:50 PM, Sandipan Das wrote:
>> 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 <sandipan@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/net/bpf_jit_comp64.c | 79 ++++++++++++++++++++++++++++++++=
++-----
>>  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
>> 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
>>  /* 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,
>>  			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
>> +				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,
>>  	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)
>>  	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)
>>  	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)
>>  		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
>> @@ -904,10 +947,10 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_pr=
og *fp)
>>  	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)
>>  			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)
>>  		/* 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)
>>  	((u64 *)image)[1] =3D local_paca->kernel_toc;
>>  #endif
>> =20
>> +	bpf_flush_icache(bpf_hdr, (u8 *)bpf_hdr + (bpf_hdr->pages * PAGE_SIZE)=
);
>> +
>> +	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

=