Re: [PATCH bpf-next v3 1/8] bpf: Add check_func_arg_reg_off function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 04, 2022 at 05:35:01AM +0530, Kumar Kartikeya Dwivedi wrote:
> Lift the list of register types allowed for having fixed and variable
> offsets when passed as helper function arguments into a common helper,
> so that they can be reused for kfunc checks in later commits. Keeping a
> common helper aids maintainability and allows us to follow the same
> consistent rules across helpers and kfuncs. Also, convert check_func_arg
> to use this function.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
>  include/linux/bpf_verifier.h |  3 ++
>  kernel/bpf/verifier.c        | 69 +++++++++++++++++++++---------------
>  2 files changed, 44 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 7a7be8c057f2..38b24ee8d8c2 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -521,6 +521,9 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt);
>  
>  int check_ptr_off_reg(struct bpf_verifier_env *env,
>  		      const struct bpf_reg_state *reg, int regno);
> +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> +			   const struct bpf_reg_state *reg, int regno,
> +			   enum bpf_arg_type arg_type);
>  int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
>  			     u32 regno);
>  int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a57db4b2803c..c85f4b2458f4 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5359,6 +5359,44 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
>  	return 0;
>  }
>  
> +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> +			   const struct bpf_reg_state *reg, int regno,
> +			   enum bpf_arg_type arg_type)
> +{
> +	enum bpf_reg_type type = reg->type;
> +	int err;
> +
> +	switch ((u32)type) {
> +	case SCALAR_VALUE:
> +	/* Pointer types where reg offset is explicitly allowed: */
> +	case PTR_TO_PACKET:
> +	case PTR_TO_PACKET_META:
> +	case PTR_TO_MAP_KEY:
> +	case PTR_TO_MAP_VALUE:
> +	case PTR_TO_MEM:
> +	case PTR_TO_MEM | MEM_RDONLY:
> +	case PTR_TO_MEM | MEM_ALLOC:
> +	case PTR_TO_BUF:
> +	case PTR_TO_BUF | MEM_RDONLY:
> +	case PTR_TO_STACK:
> +		/* Some of the argument types nevertheless require a
> +		 * zero register offset.
> +		 */
> +		if (arg_type == ARG_PTR_TO_ALLOC_MEM)
> +			goto force_off_check;
> +		break;
> +	/* All the rest must be rejected: */
> +	default:
> +force_off_check:
> +		err = __check_ptr_off_reg(env, reg, regno,
> +					  type == PTR_TO_BTF_ID);
> +		if (err < 0)
> +			return err;
Nit. Since it is refactoring to a new function now,
it can directly return __check_ptr_off_reg().

		
> +		break;
> +	}
> +	return 0;
> +}
May as well flip the arg_type check and leave calling
the __check_ptr_off_reg at the end.

Uncompiled code:

int check_func_arg_reg_off(struct bpf_verifier_env *env,
			   const struct bpf_reg_state *reg, int regno,
			   enum bpf_arg_type arg_type)
{
	enum bpf_reg_type type = reg->type;
	bool fixed_off_ok = false;

	switch ((u32)type) {
	case SCALAR_VALUE:
	/* Pointer types where reg offset is explicitly allowed: */
	case PTR_TO_PACKET:
	case PTR_TO_PACKET_META:
	case PTR_TO_MAP_KEY:
	case PTR_TO_MAP_VALUE:
	case PTR_TO_MEM:
	case PTR_TO_MEM | MEM_RDONLY:
	case PTR_TO_MEM | MEM_ALLOC:
	case PTR_TO_BUF:
	case PTR_TO_BUF | MEM_RDONLY:
	case PTR_TO_STACK:
		/* Some of the argument types nevertheless require a
		 * zero register offset.
		 */
		if (arg_type != ARG_PTR_TO_ALLOC_MEM)
			return 0;
		break;
	case PTR_TO_BTF_ID:
		fixed_off_ok = true;
		break;
	}

	return __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
}

Then 'case PTR_TO_BTF_ID' can then be reused in patch 4 instead
of adding a special 'if (type == PTR_TO_BTF_ID)' just
before the 'switch ((u32)type)'

Both are nits. can be ignored.

> +
>  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  			  struct bpf_call_arg_meta *meta,
>  			  const struct bpf_func_proto *fn)
> @@ -5408,34 +5446,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  	if (err)
>  		return err;
>  
> -	switch ((u32)type) {
> -	case SCALAR_VALUE:
> -	/* Pointer types where reg offset is explicitly allowed: */
> -	case PTR_TO_PACKET:
> -	case PTR_TO_PACKET_META:
> -	case PTR_TO_MAP_KEY:
> -	case PTR_TO_MAP_VALUE:
> -	case PTR_TO_MEM:
> -	case PTR_TO_MEM | MEM_RDONLY:
> -	case PTR_TO_MEM | MEM_ALLOC:
> -	case PTR_TO_BUF:
> -	case PTR_TO_BUF | MEM_RDONLY:
> -	case PTR_TO_STACK:
> -		/* Some of the argument types nevertheless require a
> -		 * zero register offset.
> -		 */
> -		if (arg_type == ARG_PTR_TO_ALLOC_MEM)
> -			goto force_off_check;
> -		break;
> -	/* All the rest must be rejected: */
> -	default:
> -force_off_check:
> -		err = __check_ptr_off_reg(env, reg, regno,
> -					  type == PTR_TO_BTF_ID);
> -		if (err < 0)
> -			return err;
> -		break;
> -	}
> +	err = check_func_arg_reg_off(env, reg, regno, arg_type);
> +	if (err)
> +		return err;
>  
>  skip_type_check:
>  	if (reg->ref_obj_id) {
> -- 
> 2.35.1
> 



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux