Re: [PATCH v3 bpf-next 2/8] bpf: Let bpf_prog_pack_free handle any pointer

From: "Björn Töpel" <bjorn@kernel.org>
To: Song Liu <song@kernel.org>, bpf@vger.kernel.org
Cc: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
	martin.lau@kernel.org, kernel-team@meta.com, iii@linux.ibm.com,
	Song Liu <song@kernel.org>
Subject: Re: [PATCH v3 bpf-next 2/8] bpf: Let bpf_prog_pack_free handle any pointer
Date: Wed, 27 Sep 2023 20:29:01 +0200	[thread overview]
Message-ID: <87pm23cvwy.fsf@all.your.base.are.belong.to.us> (raw)
In-Reply-To: <20230926190020.1111575-3-song@kernel.org>

Song Liu <song@kernel.org> writes:

> Currently, bpf_prog_pack_free only can only free pointer to struct
> bpf_binary_header, which is not flexible. Add a size argument to
> bpf_prog_pack_free so that it can handle any pointer.
>
> Signed-off-by: Song Liu <song@kernel.org>
> Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Tested-by: Ilya Leoshkevich <iii@linux.ibm.com>  # on s390x
> ---
>  include/linux/filter.h  |  2 +-
>  kernel/bpf/core.c       | 21 ++++++++++-----------
>  kernel/bpf/dispatcher.c |  5 +----
>  3 files changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 27406aee2d40..eda9efe20026 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1073,7 +1073,7 @@ struct bpf_binary_header *
>  bpf_jit_binary_pack_hdr(const struct bpf_prog *fp);
>  
>  void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns);
> -void bpf_prog_pack_free(struct bpf_binary_header *hdr);
> +void bpf_prog_pack_free(void *ptr, u32 size);
>  
>  static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp)
>  {
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 08626b519ce2..fcdf710e6a32 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -928,20 +928,20 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
>  	return ptr;
>  }
>  
> -void bpf_prog_pack_free(struct bpf_binary_header *hdr)
> +void bpf_prog_pack_free(void *ptr, u32 size)
>  {
>  	struct bpf_prog_pack *pack = NULL, *tmp;
>  	unsigned int nbits;
>  	unsigned long pos;
>  
>  	mutex_lock(&pack_mutex);
> -	if (hdr->size > BPF_PROG_PACK_SIZE) {
> -		bpf_jit_free_exec(hdr);
> +	if (size > BPF_PROG_PACK_SIZE) {
> +		bpf_jit_free_exec(ptr);
>  		goto out;
>  	}
>  
>  	list_for_each_entry(tmp, &pack_list, list) {
> -		if ((void *)hdr >= tmp->ptr && (tmp->ptr + BPF_PROG_PACK_SIZE) > (void *)hdr) {
> +		if (ptr >= tmp->ptr && (tmp->ptr + BPF_PROG_PACK_SIZE) > ptr) {
>  			pack = tmp;
>  			break;
>  		}
> @@ -950,10 +950,10 @@ void bpf_prog_pack_free(struct bpf_binary_header *hdr)
>  	if (WARN_ONCE(!pack, "bpf_prog_pack bug\n"))
>  		goto out;
>  
> -	nbits = BPF_PROG_SIZE_TO_NBITS(hdr->size);
> -	pos = ((unsigned long)hdr - (unsigned long)pack->ptr) >> BPF_PROG_CHUNK_SHIFT;
> +	nbits = BPF_PROG_SIZE_TO_NBITS(size);
> +	pos = ((unsigned long)ptr - (unsigned long)pack->ptr) >> BPF_PROG_CHUNK_SHIFT;
>  
> -	WARN_ONCE(bpf_arch_text_invalidate(hdr, hdr->size),
> +	WARN_ONCE(bpf_arch_text_invalidate(ptr, size),
>  		  "bpf_prog_pack bug: missing bpf_arch_text_invalidate?\n");
>  
>  	bitmap_clear(pack->bitmap, pos, nbits);
> @@ -1100,8 +1100,7 @@ bpf_jit_binary_pack_alloc(unsigned int proglen, u8 **image_ptr,
>  
>  	*rw_header = kvmalloc(size, GFP_KERNEL);
>  	if (!*rw_header) {
> -		bpf_arch_text_copy(&ro_header->size, &size, sizeof(size));
> -		bpf_prog_pack_free(ro_header);
> +		bpf_prog_pack_free(ro_header, size);
>  		bpf_jit_uncharge_modmem(size);
>  		return NULL;
>  	}
> @@ -1132,7 +1131,7 @@ int bpf_jit_binary_pack_finalize(struct bpf_prog *prog,
>  	kvfree(rw_header);
>  
>  	if (IS_ERR(ptr)) {
> -		bpf_prog_pack_free(ro_header);
> +		bpf_prog_pack_free(ro_header, ro_header->size);
>  		return PTR_ERR(ptr);
>  	}
>  	return 0;
> @@ -1153,7 +1152,7 @@ void bpf_jit_binary_pack_free(struct bpf_binary_header *ro_header,
>  {
>  	u32 size = ro_header->size;
>  
> -	bpf_prog_pack_free(ro_header);
> +	bpf_prog_pack_free(ro_header, size);
>  	kvfree(rw_header);
>  	bpf_jit_uncharge_modmem(size);
>  }
> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
> index fa3e9225aedc..56760fc10e78 100644
> --- a/kernel/bpf/dispatcher.c
> +++ b/kernel/bpf/dispatcher.c
> @@ -150,10 +150,7 @@ void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
>  			goto out;
>  		d->rw_image = bpf_jit_alloc_exec(PAGE_SIZE);
>  		if (!d->rw_image) {
> -			u32 size = PAGE_SIZE;
> -
> -			bpf_arch_text_copy(d->image, &size, sizeof(size));
> -			bpf_prog_pack_free((struct bpf_binary_header *)d->image);
> +			bpf_prog_pack_free(d->image, PAGE_SIZE);

Nice to get rid of that ugliness!

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>