Re: [PATCH bpf-next] bpf: Charge modmem for struct_ops trampoline

[Martin KaFai Lau <martin.lau@linux.dev>]

On 9/13/23 3:26 PM, Song Liu wrote:
> Current code charges modmem for regular trampoline, but not for struct_ops
> trampoline. Add bpf_jit_[charge|uncharge]_modmem() to struct_ops so the
> trampoline is charged in both cases.
> 
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>   kernel/bpf/bpf_struct_ops.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index fdc3e8705a3c..ea6ca87a2ed9 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -615,7 +615,10 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
>   	if (st_map->links)
>   		bpf_struct_ops_map_put_progs(st_map);
>   	bpf_map_area_free(st_map->links);
> -	bpf_jit_free_exec(st_map->image);
> +	if (st_map->image) {
> +		bpf_jit_free_exec(st_map->image);
> +		bpf_jit_uncharge_modmem(PAGE_SIZE);
> +	}
>   	bpf_map_area_free(st_map->uvalue);
>   	bpf_map_area_free(st_map);
>   }
> @@ -657,6 +660,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   	struct bpf_struct_ops_map *st_map;
>   	const struct btf_type *t, *vt;
>   	struct bpf_map *map;
> +	int ret;
>   
>   	st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
>   	if (!st_ops)
> @@ -681,6 +685,12 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   	st_map->st_ops = st_ops;
>   	map = &st_map->map;
>   
> +	ret = bpf_jit_charge_modmem(PAGE_SIZE);
> +	if (ret) {
> +		__bpf_struct_ops_map_free(map);
> +		return ERR_PTR(ret);
> +	}


This just came to my mind when reading it again.

It will miss a bpf_jit_uncharge_modmem() if the bpf_jit_alloc_exec() at a few 
lines below did fail (meaning st_map->image is NULL). It is because the 
__bpf_struct_ops_map_free() only uncharge if st_map->image is not NULL.

How above moving the bpf_jit_alloc_exec() to here (immediately after 
bpf_jit_charge_modem succeeded). Like,

	st_map->image = bpf_jit_alloc_exec(PAGE_SIZE);
	if (!st_map->image) {
		bpf_jit_uncharge_modmem(PAGE_SIZE);
		__bpf_struct_ops_map_free(map);
		return ERR_PTR(-ENOMEM);
	}

Then there is also no need to test 'if (st_map->image)' in 
__bpf_struct_ops_map_free().

> +
>   	st_map->uvalue = bpf_map_area_alloc(vt->size, NUMA_NO_NODE);
>   	st_map->links =
>   		bpf_map_area_alloc(btf_type_vlen(t) * sizeof(struct bpf_links *),
> @@ -907,4 +917,3 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
>   	kfree(link);
>   	return err;
>   }
> -