Re: [RFC PATCH 3/3] powerpc/bpf: use bpf_jit_binary_pack_[alloc|finalize|free]

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 10/11/2022 à 19:43, Hari Bathini a écrit :
> Use bpf_jit_binary_pack_alloc in powerpc jit. The jit engine first
> writes the program to the rw buffer. When the jit is done, the program
> is copied to the final location with bpf_jit_binary_pack_finalize.
> With multiple jit_subprogs, bpf_jit_free is called on some subprograms
> that haven't got bpf_jit_binary_pack_finalize() yet. Implement custom
> bpf_jit_free() like in commit 1d5f82d9dd47 ("bpf, x86: fix freeing of
> not-finalized bpf_prog_pack") to call bpf_jit_binary_pack_finalize(),
> if necessary. While here, correct the misnomer powerpc64_jit_data to
> powerpc_jit_data as it is meant for both ppc32 and ppc64.

This patch looks heavy compared to x86 commit 1022a5498f6f.

I didn't look into details, is there really a need to carry that 
rw_image over all functions you changed ?

As far as I can see, ok you need it for EMIT macro. But then some of the 
function that use EMIT will now use rw_image instead of image, so why do 
they need both image and rw_image ?

Maybe you'd have less churn if you leave image, and add a ro_image 
wherever necessary but not everywhere.


> 
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> ---
>   arch/powerpc/net/bpf_jit.h        |  18 +++--
>   arch/powerpc/net/bpf_jit_comp.c   | 123 +++++++++++++++++++++---------
>   arch/powerpc/net/bpf_jit_comp32.c |  26 +++----
>   arch/powerpc/net/bpf_jit_comp64.c |  32 ++++----
>   4 files changed, 128 insertions(+), 71 deletions(-)
> 
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index f925755cd249..c4c1f7a21d89 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -181,22 +183,25 @@ bool bpf_jit_needs_zext(void)
>   
>   struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>   {
> -	u32 proglen;
> -	u32 alloclen;
> -	u8 *image = NULL;
> -	u32 *code_base;
> -	u32 *addrs;
> -	struct powerpc64_jit_data *jit_data;
> -	struct codegen_context cgctx;
> -	int pass;
> -	int flen;
> +	struct bpf_binary_header *rw_header = NULL;
> +	struct powerpc_jit_data *jit_data;
>   	struct bpf_binary_header *bpf_hdr;
> +	struct codegen_context cgctx;
>   	struct bpf_prog *org_fp = fp;
>   	struct bpf_prog *tmp_fp;
>   	bool bpf_blinded = false;
>   	bool extra_pass = false;
> +	u8 *rw_image = NULL;
> +	u32 *rw_code_base;
> +	u8 *image = NULL;
>   	u32 extable_len;
> +	u32 *code_base;
>   	u32 fixup_len;
> +	u32 alloclen;
> +	u32 proglen;
> +	u32 *addrs;
> +	int pass;
> +	int flen;

Why so many changes here, a lot of items seems to only have moved 
without any modification. Why that churn ?

>   
>   	if (!fp->jit_requested)
>   		return org_fp;
> @@ -227,6 +232,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>   		image = jit_data->image;
>   		bpf_hdr = jit_data->header;
>   		proglen = jit_data->proglen;
> +		rw_header = jit_data->rw_header;
> +		rw_image = (void *)rw_header + ((void *)image - (void *)bpf_hdr);
>   		extra_pass = true;
>   		goto skip_init_ctx;
>   	}
> @@ -244,7 +251,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>   	cgctx.stack_size = round_up(fp->aux->stack_depth, 16);
>   
>   	/* Scouting faux-generate pass 0 */
> -	if (bpf_jit_build_body(fp, 0, &cgctx, addrs, 0)) {
> +	if (bpf_jit_build_body(fp, 0, 0, &cgctx, addrs, 0)) {

Some of the 0s in this call are pointers. You should use NULL instead.
This comment applies to several other lines you have changed.

>   		/* We hit something illegal or unsupported. */
>   		fp = org_fp;
>   		goto out_addrs;
> @@ -259,7 +266,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>   	 */
>   	if (cgctx.seen & SEEN_TAILCALL || !is_offset_in_branch_range((long)cgctx.idx * 4)) {
>   		cgctx.idx = 0;
> -		if (bpf_jit_build_body(fp, 0, &cgctx, addrs, 0)) {
> +		if (bpf_jit_build_body(fp, 0, 0, &cgctx, addrs, 0)) {

0 ==> NULL

>   			fp = org_fp;
>   			goto out_addrs;
>   		}
> @@ -271,9 +278,9 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>   	 * update ctgtx.idx as it pretends to output instructions, then we can
>   	 * calculate total size from idx.
>   	 */
> -	bpf_jit_build_prologue(0, &cgctx);
> +	bpf_jit_build_prologue(0, 0, &cgctx);
>   	addrs[fp->len] = cgctx.idx * 4;
> -	bpf_jit_build_epilogue(0, &cgctx);
> +	bpf_jit_build_epilogue(0, 0, &cgctx);

0 ==> NULL

>   
>   	fixup_len = fp->aux->num_exentries * BPF_FIXUP_LEN * 4;
>   	extable_len = fp->aux->num_exentries * sizeof(struct exception_table_entry);
> @@ -337,17 +348,26 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>   
>   #ifdef CONFIG_PPC64_ELF_ABI_V1
>   	/* Function descriptor nastiness: Address + TOC */
> -	((u64 *)image)[0] = (u64)code_base;
> -	((u64 *)image)[1] = local_paca->kernel_toc;
> +	((u64 *)rw_image)[0] = (u64)code_base;
> +	((u64 *)rw_image)[1] = local_paca->kernel_toc;

Would be better to use 'struct func_desc'

And the #ifdef is not necessary, IS_ENABLED() would be enough.

>   #endif
>   
>   	fp->bpf_func = (void *)image;
>   	fp->jited = 1;
>   	fp->jited_len = proglen + FUNCTION_DESCR_SIZE;
>   
> -	bpf_flush_icache(bpf_hdr, (u8 *)bpf_hdr + bpf_hdr->size);
>   	if (!fp->is_func || extra_pass) {
> -		bpf_jit_binary_lock_ro(bpf_hdr);
> +		/*
> +		 * bpf_jit_binary_pack_finalize fails in two scenarios:
> +		 *   1) header is not pointing to proper module memory;

Can that really happen ?

> +		 *   2) the arch doesn't support bpf_arch_text_copy().

The above cannot happen, you added support bpf_arch_text_copy() in patch 
1. So why this comment ?

> +		 *
> +		 * Both cases are serious bugs that justify WARN_ON.
> +		 */

Case 2 would mean a bug in the compiler, if you can't trust your 
compiler for that you can't trust it for anything else. That's odd.

> +		if (WARN_ON(bpf_jit_binary_pack_finalize(fp, bpf_hdr, rw_header))) {
> +			fp = org_fp;
> +			goto out_addrs;
> +		}
>   		bpf_prog_fill_jited_linfo(fp, addrs);
>   out_addrs:
>   		kfree(addrs);