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

[Puranjay Mohan <puranjay@kernel.org>]

Song Liu <song@kernel.org> writes:

> Sorry for the late reply.
>
> On Thu, Aug 28, 2025 at 5:10 AM Puranjay Mohan <puranjay@kernel.org> wrote:
> [...]
>> 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?
>
> This is a good optimization. However, given this is not a hot path,
> I don't have a strong preference either way. At the moment, most
> other architectures use bpf_jit_binary_pack_finalize(), so it is good
> to just use bpf_jit_binary_pack_finalize and keep the logic
> consistent.

So, in that case we can merge this patch.

Acked-by: Puranjay Mohan <puranjay@kernel.org>

Thanks,
Puranjay