Re: [PATCH bpf-next 2/8] bpf: inline map creation logic in map_create() function

From: Kees Cook <keescook@chromium.org>
To: Andrii Nakryiko <andrii@kernel.org>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	kpsingh@kernel.org, paul@paul-moore.com,
	linux-security-module@vger.kernel.org
Subject: Re: [PATCH bpf-next 2/8] bpf: inline map creation logic in map_create() function
Date: Wed, 12 Apr 2023 10:53:31 -0700	[thread overview]
Message-ID: <6436f01e.a70a0220.e12e2.828e@mx.google.com> (raw)
In-Reply-To: <20230412043300.360803-3-andrii@kernel.org>

On Tue, Apr 11, 2023 at 09:32:54PM -0700, Andrii Nakryiko wrote:
> Keep all the relevant generic sanity checks, permission checks, and
> creation and initialization logic in one linear piece of code. Currently
> helper function that handles memory allocation and partial
> initialization is split apart and is about 1000 lines higher in the
> file, hurting readability.

At first glance, this seems like a step in the wrong direction: having a
single-purpose function pulled out of a larger one seems like a good
thing for stuff like unit testing, etc. Unless there's a reason later in
the series for this inlining (which should be called out in the
changelog here), I would say if it is only readability, just move the
function down 1000 lines but leave it a separate function.

-Kees

> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  kernel/bpf/syscall.c | 54 ++++++++++++++++++--------------------------
>  1 file changed, 22 insertions(+), 32 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index c1d268025985..a090737f98ea 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -108,37 +108,6 @@ const struct bpf_map_ops bpf_map_offload_ops = {
>  	.map_mem_usage = bpf_map_offload_map_mem_usage,
>  };
>  
> -static struct bpf_map *find_and_alloc_map(union bpf_attr *attr)
> -{
> -	const struct bpf_map_ops *ops;
> -	u32 type = attr->map_type;
> -	struct bpf_map *map;
> -	int err;
> -
> -	if (type >= ARRAY_SIZE(bpf_map_types))
> -		return ERR_PTR(-EINVAL);
> -	type = array_index_nospec(type, ARRAY_SIZE(bpf_map_types));
> -	ops = bpf_map_types[type];
> -	if (!ops)
> -		return ERR_PTR(-EINVAL);
> -
> -	if (ops->map_alloc_check) {
> -		err = ops->map_alloc_check(attr);
> -		if (err)
> -			return ERR_PTR(err);
> -	}
> -	if (attr->map_ifindex)
> -		ops = &bpf_map_offload_ops;
> -	if (!ops->map_mem_usage)
> -		return ERR_PTR(-EINVAL);
> -	map = ops->map_alloc(attr);
> -	if (IS_ERR(map))
> -		return map;
> -	map->ops = ops;
> -	map->map_type = type;
> -	return map;
> -}
> -
>  static void bpf_map_write_active_inc(struct bpf_map *map)
>  {
>  	atomic64_inc(&map->writecnt);
> @@ -1124,7 +1093,9 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
>  /* called via syscall */
>  static int map_create(union bpf_attr *attr)
>  {
> +	const struct bpf_map_ops *ops;
>  	int numa_node = bpf_map_attr_numa_node(attr);
> +	u32 map_type = attr->map_type;
>  	struct btf_field_offs *foffs;
>  	struct bpf_map *map;
>  	int f_flags;
> @@ -1167,9 +1138,28 @@ static int map_create(union bpf_attr *attr)
>  		return -EINVAL;
>  
>  	/* find map type and init map: hashtable vs rbtree vs bloom vs ... */
> -	map = find_and_alloc_map(attr);
> +	map_type = attr->map_type;
> +	if (map_type >= ARRAY_SIZE(bpf_map_types))
> +		return -EINVAL;
> +	map_type = array_index_nospec(map_type, ARRAY_SIZE(bpf_map_types));
> +	ops = bpf_map_types[map_type];
> +	if (!ops)
> +		return -EINVAL;
> +
> +	if (ops->map_alloc_check) {
> +		err = ops->map_alloc_check(attr);
> +		if (err)
> +			return err;
> +	}
> +	if (attr->map_ifindex)
> +		ops = &bpf_map_offload_ops;
> +	if (!ops->map_mem_usage)
> +		return -EINVAL;
> +	map = ops->map_alloc(attr);
>  	if (IS_ERR(map))
>  		return PTR_ERR(map);
> +	map->ops = ops;
> +	map->map_type = map_type;
>  
>  	err = bpf_obj_name_cpy(map->name, attr->map_name,
>  			       sizeof(attr->map_name));
> -- 
> 2.34.1
> 

-- 
Kees Cook