Re: [PATCH bpf-next] bpf, arm64: Call bpf_jit_binary_pack_finalize() in bpf_jit_free()

[Puranjay Mohan <puranjay@kernel.org>]

Hengqi Chen <hengqi.chen@gmail.com> writes:

> The current implementation seems incorrect and does NOT match the
> comment above, use bpf_jit_binary_pack_finalize() instead.
>
> Fixes: 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory management")
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
>  arch/arm64/net/bpf_jit_comp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 52ffe115a8c4..4ef9b7b8fb40 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -3064,8 +3064,7 @@ void bpf_jit_free(struct bpf_prog *prog)
>  		 * before freeing it.
>  		 */
>  		if (jit_data) {
> -			bpf_arch_text_copy(&jit_data->ro_header->size, &jit_data->header->size,
> -					   sizeof(jit_data->header->size));
> +			bpf_jit_binary_pack_finalize(jit_data->ro_header, jit_data->header);
>  			kfree(jit_data);

Thanks for this patch!

So, this is fixing a bug because bpf_jit_binary_pack_finalize() will do
kvfree(rw_header); but without it currently, jit_data->header is never
freed.

But I think we shouldn't use bpf_jit_binary_pack_finalize() here as it
copies the whole rw_header to ro_header using  bpf_arch_text_copy()
which is an expensive operation (patch_map/unmap in loop +
flush_icache_range()) and not needed here because we are going
to free ro_header anyway.

We only need to copy jit_data->header->size to jit_data->ro_header->size
because this size is later used by bpf_jit_binary_pack_free(), see
comment above bpf_jit_binary_pack_free().

How I suggest we should fix the code and the comment:

-- >8 --

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 5083886d6e66b..cb4c50eeada13 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -3093,12 +3093,14 @@ void bpf_jit_free(struct bpf_prog *prog)

                /*
                 * If we fail the final pass of JIT (from jit_subprogs),
-                * the program may not be finalized yet. Call finalize here
-                * before freeing it.
+                * the program may not be finalized yet. Copy the header size
+                * from rw_header to ro_header before freeing the ro_header
+                * with bpf_jit_binary_pack_free().
                 */
                if (jit_data) {
                        bpf_arch_text_copy(&jit_data->ro_header->size, &jit_data->header->size,
                                           sizeof(jit_data->header->size));
+                       kvfree(jit_data->header);
                        kfree(jit_data);
                }
                prog->bpf_func -= cfi_get_offset();

-- 8< --

Song,

Do you think this optimization is worth it or should we just call
bpf_jit_binary_pack_finalize() here like this patch is doing?

Thanks,
Puranjay