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

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

[Hengqi Chen]

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);
 		}
 		prog->bpf_func -= cfi_get_offset();
-- 
2.27.0

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

[Puranjay Mohan]

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

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

[Song Liu]

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.

In the longer term, we can consider refactoring bpf_jit_free so that
multiple architectures can share code. After this patch, bpf_jit_free
for x86_64 and arm64 are very similar to each other. There are
likely some opportunities to reduce code duplications.

Thanks,
Song

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

[Puranjay Mohan]

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

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

[Alexei Starovoitov]

On Tue, Sep 16, 2025 at 5:51 AM Puranjay Mohan <puranjay@kernel.org> wrote:
>
> 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>

It's out of patchwork.
Hengqi,
pls repost.

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

[Hengqi Chen]

On Tue, Sep 16, 2025 at 10:59 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Sep 16, 2025 at 5:51 AM Puranjay Mohan <puranjay@kernel.org> wrote:
> >
> > 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>
>
> It's out of patchwork.
> Hengqi,
> pls repost.

OK, just resend.

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

[Hengqi Chen]

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")
Acked-by: Puranjay Mohan <puranjay@kernel.org>
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);
 		}
 		prog->bpf_func -= cfi_get_offset();
-- 
2.27.0

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

[Song Liu]

On Tue, Sep 16, 2025 at 6:26 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> 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")
> Acked-by: Puranjay Mohan <puranjay@kernel.org>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>

Acked-by: Song Liu <song@kernel.org>

Thanks for the fix!

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

[Puranjay Mohan]

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")
> Acked-by: Puranjay Mohan <puranjay@kernel.org>
> 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);
>  		}
>  		prog->bpf_func -= cfi_get_offset();
> -- 
> 2.27.0

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

Thanks,
Puranjay

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue, 16 Sep 2025 23:26:53 +0000 you wrote:
> 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")
> Acked-by: Puranjay Mohan <puranjay@kernel.org>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf, arm64: Call bpf_jit_binary_pack_finalize() in bpf_jit_free()
    https://git.kernel.org/bpf/bpf-next/c/6ff4a0fa3e1b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html