Re: [PATCH bpf-next 2/2] bpf, x86: set header->size properly before freeing it

[Yonghong Song <yhs@fb.com>]

On 3/1/22 4:43 PM, Song Liu wrote:
> On do_jit failure path, the header is freed by bpf_jit_binary_pack_free.
> While bpf_jit_binary_pack_free doesn't require proper ro_header->size,
> bpf_prog_pack_free still uses it. Set header->size in bpf_int_jit_compile
> before calling bpf_jit_binary_pack_free.
> 
> Fixes: 1022a5498f6f ("bpf, x86_64: Use bpf_jit_binary_pack_alloc")
> Fixes: 33c9805860e5 ("bpf: Introduce bpf_jit_binary_pack_[alloc|finalize|free]")
> Reported-by: Kui-Feng Lee <kuifeng@fb.com>
> Signed-off-by: Song Liu <song@kernel.org>

LGTM with a nit below related to comments.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   arch/x86/net/bpf_jit_comp.c | 6 +++++-
>   kernel/bpf/core.c           | 7 ++++---
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index c7db0fe4de2f..b923d81ff6f9 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -2330,8 +2330,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>   		if (proglen <= 0) {
>   out_image:
>   			image = NULL;
> -			if (header)
> +			if (header) {
> +				/* set header->size for bpf_arch_text_copy */

This comment is confusing. Setting header->size is not for 
bpf_arch_text_copy. Probably you mean 'by bpf_arch_text_copy?
I think this comment is not necessary.

> +				bpf_arch_text_copy(&header->size, &rw_header->size,
> +						   sizeof(rw_header->size));
>   				bpf_jit_binary_pack_free(header, rw_header);
> +			}
>   			prog = orig_prog;
>   			goto out_addrs;
>   		}
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index ebb0193d07f0..da587e4619e0 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1112,13 +1112,14 @@ int bpf_jit_binary_pack_finalize(struct bpf_prog *prog,
>    *   1) when the program is freed after;
>    *   2) when the JIT engine fails (before bpf_jit_binary_pack_finalize).
>    * For case 2), we need to free both the RO memory and the RW buffer.
> - * Also, ro_header->size in 2) is not properly set yet, so rw_header->size
> - * is used for uncharge.
> + *
> + * If bpf_jit_binary_pack_free is called before _finalize (jit error),

Do you mean bpf_jit_binary_pack_free() is called before calling
bpf_jit_binary_pack_finalize()? This seems not the case.

> + * it is necessary to set ro_header->size properly before freeing it.
>    */
>   void bpf_jit_binary_pack_free(struct bpf_binary_header *ro_header,
>   			      struct bpf_binary_header *rw_header)
>   {
> -	u32 size = rw_header ? rw_header->size : ro_header->size;
> +	u32 size = ro_header->size;
>   
>   	bpf_prog_pack_free(ro_header);
>   	kvfree(rw_header);