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

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

[Joanne Koong]

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.

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

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

[sdf]

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

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

[Joanne Koong]

On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote:
>
> 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.")
>
> ?
I will add that tag. Thanks!
>
> > 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
>

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

[Hao Luo]

On Tue, Jul 12, 2022 at 6:10 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote:
> >
> > 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.")
> >
> > ?
> I will add that tag. Thanks!

Joanne and Stan, IMO this is not necessary. I think this change is a
cleanup rather than a fix.

> >
> > > 2. Uses switch casing to improve readability + efficiency.
> >
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > ---
[...]
> >

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

[Alexei Starovoitov]

On Tue, Jul 12, 2022 at 6:20 PM Hao Luo <haoluo@google.com> wrote:
>
> On Tue, Jul 12, 2022 at 6:10 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote:
> > >
> > > 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.")
> > >
> > > ?
> > I will add that tag. Thanks!
>
> Joanne and Stan, IMO this is not necessary. I think this change is a
> cleanup rather than a fix.

I don't see the bug easier.
The helper types that are compared directly as arg_type
instead of base_type(arg_type) were all without flags so far.
So I don't think the patch changes behavior or fixes anything today.
It looks like a good future proofing change though.
Am I missing something?

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

[Hao Luo]

On Tue, Jul 12, 2022 at 6:26 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Jul 12, 2022 at 6:20 PM Hao Luo <haoluo@google.com> wrote:
> >
> > On Tue, Jul 12, 2022 at 6:10 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote:
> > > >
> > > > 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.")
> > > >
> > > > ?
> > > I will add that tag. Thanks!
> >
> > Joanne and Stan, IMO this is not necessary. I think this change is a
> > cleanup rather than a fix.
>
> I don't see the bug easier.
> The helper types that are compared directly as arg_type
> instead of base_type(arg_type) were all without flags so far.
> So I don't think the patch changes behavior or fixes anything today.
> It looks like a good future proofing change though.
> Am I missing something?

Agree, I mean adding a fixes tag isn't necessary and the patch is
toward a good direction. As long as the selftests pass on this patch,
it looks good to me.

Acked-by: Hao Luo <haoluo@google.com>

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

[Stanislav Fomichev]

On Tue, Jul 12, 2022 at 6:39 PM Hao Luo <haoluo@google.com> wrote:
>
> On Tue, Jul 12, 2022 at 6:26 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Jul 12, 2022 at 6:20 PM Hao Luo <haoluo@google.com> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 6:10 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > > >
> > > > On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote:
> > > > >
> > > > > 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.")
> > > > >
> > > > > ?
> > > > I will add that tag. Thanks!
> > >
> > > Joanne and Stan, IMO this is not necessary. I think this change is a
> > > cleanup rather than a fix.
> >
> > I don't see the bug easier.
> > The helper types that are compared directly as arg_type
> > instead of base_type(arg_type) were all without flags so far.
> > So I don't think the patch changes behavior or fixes anything today.
> > It looks like a good future proofing change though.
> > Am I missing something?
>
> Agree, I mean adding a fixes tag isn't necessary and the patch is
> toward a good direction. As long as the selftests pass on this patch,
> it looks good to me.
>
> Acked-by: Hao Luo <haoluo@google.com>

Perfect, thanks for clarifying! It wasn't clear from the description
so I started looking for where that base_type() came from.

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

[Joanne Koong]

On Tue, Jul 12, 2022 at 7:10 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Tue, Jul 12, 2022 at 6:39 PM Hao Luo <haoluo@google.com> wrote:
> >
> > On Tue, Jul 12, 2022 at 6:26 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 6:20 PM Hao Luo <haoluo@google.com> wrote:
> > > >
> > > > On Tue, Jul 12, 2022 at 6:10 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jul 12, 2022 at 3:44 PM <sdf@google.com> wrote:
> > > > > >
> > > > > > 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.")
> > > > > >
> > > > > > ?
> > > > > I will add that tag. Thanks!
> > > >
> > > > Joanne and Stan, IMO this is not necessary. I think this change is a
> > > > cleanup rather than a fix.
> > >
> > > I don't see the bug easier.
> > > The helper types that are compared directly as arg_type
> > > instead of base_type(arg_type) were all without flags so far.
> > > So I don't think the patch changes behavior or fixes anything today.
> > > It looks like a good future proofing change though.
> > > Am I missing something?
> >
> > Agree, I mean adding a fixes tag isn't necessary and the patch is
> > toward a good direction. As long as the selftests pass on this patch,
> > it looks good to me.
> >
> > Acked-by: Hao Luo <haoluo@google.com>
>
> Perfect, thanks for clarifying! It wasn't clear from the description
> so I started looking for where that base_type() came from.
Great, I will leave it as is then. Thanks for the feedback.

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue, 12 Jul 2022 14:06:03 -0700 you 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.
> 
> 2. Uses switch casing to improve readability + efficiency.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v1] bpf: Tidy up verifier check_func_arg()
    https://git.kernel.org/bpf/bpf-next/c/8ab4cdcf03d0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html