Re: [PATCH bpf-next 1/2] bpf: Call the missed btf_record_free() when map creation fails

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Sep 12, 2024 at 09:28:44AM +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> When security_bpf_map_create() in map_create() fails, map_create() will
> call btf_put() and ->map_free() callback to free the map. It doesn't
> free the btf_record of map value, so add the missed btf_record_free()
> when map creation fails.
> 
> However btf_record_free() needs to be called after ->map_free() just
> like bpf_map_free_deferred() did, because ->map_free() may use the
> btf_record to free the special fields in preallocated map value. So
> factor out bpf_map_free() helper to free the map, btf_record, and btf
> orderly and use the helper in both map_create() and
> bpf_map_free_deferred().

lgtm

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  kernel/bpf/syscall.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6988e432fc3d..1a0dc1d81118 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -735,15 +735,11 @@ void bpf_obj_free_fields(const struct btf_record *rec, void *obj)
>  	}
>  }
>  
> -/* called from workqueue */
> -static void bpf_map_free_deferred(struct work_struct *work)
> +static void bpf_map_free(struct bpf_map *map)
>  {
> -	struct bpf_map *map = container_of(work, struct bpf_map, work);
>  	struct btf_record *rec = map->record;
>  	struct btf *btf = map->btf;
>  
> -	security_bpf_map_free(map);
> -	bpf_map_release_memcg(map);
>  	/* implementation dependent freeing */
>  	map->ops->map_free(map);
>  	/* Delay freeing of btf_record for maps, as map_free
> @@ -762,6 +758,16 @@ static void bpf_map_free_deferred(struct work_struct *work)
>  	btf_put(btf);
>  }
>  
> +/* called from workqueue */
> +static void bpf_map_free_deferred(struct work_struct *work)
> +{
> +	struct bpf_map *map = container_of(work, struct bpf_map, work);
> +
> +	security_bpf_map_free(map);
> +	bpf_map_release_memcg(map);
> +	bpf_map_free(map);
> +}
> +
>  static void bpf_map_put_uref(struct bpf_map *map)
>  {
>  	if (atomic64_dec_and_test(&map->usercnt)) {
> @@ -1413,8 +1419,7 @@ static int map_create(union bpf_attr *attr)
>  free_map_sec:
>  	security_bpf_map_free(map);
>  free_map:
> -	btf_put(map->btf);
> -	map->ops->map_free(map);
> +	bpf_map_free(map);
>  put_token:
>  	bpf_token_put(token);
>  	return err;
> -- 
> 2.29.2
>