Re: [PATCH bpf-next v1] bpf: Tidy up verifier check_func_arg()

[sdf@google.com]

On 07/12, Joanne Koong wrote:
> This patch does two things:

> 1. For matching against the arg type, the match should be against the
> base type of the arg type, since the arg type can have different
> bpf_type_flags set on it.

Does this need a fixes tag? Something around the following maybe:

Fixes: d639b9d13a39 ("bpf: Introduce composable reg, ret and arg types.")

?

> 2. Uses switch casing to improve readability + efficiency.

> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>   kernel/bpf/verifier.c | 66 +++++++++++++++++++++++++------------------
>   1 file changed, 38 insertions(+), 28 deletions(-)

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 328cfab3af60..26e7e787c20a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5533,17 +5533,6 @@ static bool arg_type_is_mem_size(enum bpf_arg_type  
> type)
>   	       type == ARG_CONST_SIZE_OR_ZERO;
>   }

> -static bool arg_type_is_alloc_size(enum bpf_arg_type type)
> -{
> -	return type == ARG_CONST_ALLOC_SIZE_OR_ZERO;
> -}
> -
> -static bool arg_type_is_int_ptr(enum bpf_arg_type type)
> -{
> -	return type == ARG_PTR_TO_INT ||
> -	       type == ARG_PTR_TO_LONG;
> -}
> -
>   static bool arg_type_is_release(enum bpf_arg_type type)
>   {
>   	return type & OBJ_RELEASE;
> @@ -5929,7 +5918,8 @@ static int check_func_arg(struct bpf_verifier_env  
> *env, u32 arg,
>   		meta->ref_obj_id = reg->ref_obj_id;
>   	}

> -	if (arg_type == ARG_CONST_MAP_PTR) {
> +	switch (base_type(arg_type)) {
> +	case ARG_CONST_MAP_PTR:
>   		/* bpf_map_xxx(map_ptr) call: remember that map_ptr */
>   		if (meta->map_ptr) {
>   			/* Use map_uid (which is unique id of inner map) to reject:
> @@ -5954,7 +5944,8 @@ static int check_func_arg(struct bpf_verifier_env  
> *env, u32 arg,
>   		}
>   		meta->map_ptr = reg->map_ptr;
>   		meta->map_uid = reg->map_uid;
> -	} else if (arg_type == ARG_PTR_TO_MAP_KEY) {
> +		break;
> +	case ARG_PTR_TO_MAP_KEY:
>   		/* bpf_map_xxx(..., map_ptr, ..., key) call:
>   		 * check that [key, key + map->key_size) are within
>   		 * stack limits and initialized
> @@ -5971,7 +5962,8 @@ static int check_func_arg(struct bpf_verifier_env  
> *env, u32 arg,
>   		err = check_helper_mem_access(env, regno,
>   					      meta->map_ptr->key_size, false,
>   					      NULL);
> -	} else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) {
> +		break;
> +	case ARG_PTR_TO_MAP_VALUE:
>   		if (type_may_be_null(arg_type) && register_is_null(reg))
>   			return 0;

> @@ -5987,14 +5979,16 @@ static int check_func_arg(struct bpf_verifier_env  
> *env, u32 arg,
>   		err = check_helper_mem_access(env, regno,
>   					      meta->map_ptr->value_size, false,
>   					      meta);
> -	} else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
> +		break;
> +	case ARG_PTR_TO_PERCPU_BTF_ID:
>   		if (!reg->btf_id) {
>   			verbose(env, "Helper has invalid btf_id in R%d\n", regno);
>   			return -EACCES;
>   		}
>   		meta->ret_btf = reg->btf;
>   		meta->ret_btf_id = reg->btf_id;
> -	} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
> +		break;
> +	case ARG_PTR_TO_SPIN_LOCK:
>   		if (meta->func_id == BPF_FUNC_spin_lock) {
>   			if (process_spin_lock(env, regno, true))
>   				return -EACCES;
> @@ -6005,12 +5999,15 @@ static int check_func_arg(struct bpf_verifier_env  
> *env, u32 arg,
>   			verbose(env, "verifier internal error\n");
>   			return -EFAULT;
>   		}
> -	} else if (arg_type == ARG_PTR_TO_TIMER) {
> +		break;
> +	case ARG_PTR_TO_TIMER:
>   		if (process_timer_func(env, regno, meta))
>   			return -EACCES;
> -	} else if (arg_type == ARG_PTR_TO_FUNC) {
> +		break;
> +	case ARG_PTR_TO_FUNC:
>   		meta->subprogno = reg->subprogno;
> -	} else if (base_type(arg_type) == ARG_PTR_TO_MEM) {
> +		break;
> +	case ARG_PTR_TO_MEM:
>   		/* The access to this pointer is only checked when we hit the
>   		 * next is_mem_size argument below.
>   		 */
> @@ -6020,11 +6017,14 @@ static int check_func_arg(struct bpf_verifier_env  
> *env, u32 arg,
>   						      fn->arg_size[arg], false,
>   						      meta);
>   		}
> -	} else if (arg_type_is_mem_size(arg_type)) {
> -		bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
> -
> -		err = check_mem_size_reg(env, reg, regno, zero_size_allowed, meta);
> -	} else if (arg_type_is_dynptr(arg_type)) {
> +		break;
> +	case ARG_CONST_SIZE:
> +		err = check_mem_size_reg(env, reg, regno, false, meta);
> +		break;
> +	case ARG_CONST_SIZE_OR_ZERO:
> +		err = check_mem_size_reg(env, reg, regno, true, meta);
> +		break;
> +	case ARG_PTR_TO_DYNPTR:
>   		if (arg_type & MEM_UNINIT) {
>   			if (!is_dynptr_reg_valid_uninit(env, reg)) {
>   				verbose(env, "Dynptr has to be an uninitialized dynptr\n");
> @@ -6058,21 +6058,28 @@ static int check_func_arg(struct bpf_verifier_env  
> *env, u32 arg,
>   				err_extra, arg + 1);
>   			return -EINVAL;
>   		}
> -	} else if (arg_type_is_alloc_size(arg_type)) {
> +		break;
> +	case ARG_CONST_ALLOC_SIZE_OR_ZERO:
>   		if (!tnum_is_const(reg->var_off)) {
>   			verbose(env, "R%d is not a known constant'\n",
>   				regno);
>   			return -EACCES;
>   		}
>   		meta->mem_size = reg->var_off.value;
> -	} else if (arg_type_is_int_ptr(arg_type)) {
> +		break;
> +	case ARG_PTR_TO_INT:
> +	case ARG_PTR_TO_LONG:
> +	{
>   		int size = int_ptr_type_to_size(arg_type);

>   		err = check_helper_mem_access(env, regno, size, false, meta);
>   		if (err)
>   			return err;
>   		err = check_ptr_alignment(env, reg, 0, size, true);
> -	} else if (arg_type == ARG_PTR_TO_CONST_STR) {
> +		break;
> +	}
> +	case ARG_PTR_TO_CONST_STR:
> +	{
>   		struct bpf_map *map = reg->map_ptr;
>   		int map_off;
>   		u64 map_addr;
> @@ -6111,9 +6118,12 @@ static int check_func_arg(struct bpf_verifier_env  
> *env, u32 arg,
>   			verbose(env, "string is not zero-terminated\n");
>   			return -EINVAL;
>   		}
> -	} else if (arg_type == ARG_PTR_TO_KPTR) {
> +		break;
> +	}
> +	case ARG_PTR_TO_KPTR:
>   		if (process_kptr_func(env, regno, meta))
>   			return -EACCES;
> +		break;
>   	}

>   	return err;
> --
> 2.30.2