Re: [PATCH bpf-next v3 1/6] bpf: Add MEM_UNINIT as a bpf_type_flag

[David Vernet <void@manifault.com>]

Hi Joanne,

On Thu, Apr 28, 2022 at 02:10:54PM -0700, Joanne Koong wrote:
> @@ -5523,7 +5517,6 @@ static const struct bpf_reg_types kptr_types = { .types = { PTR_TO_MAP_VALUE } }
>  static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
>  	[ARG_PTR_TO_MAP_KEY]		= &map_key_value_types,
>  	[ARG_PTR_TO_MAP_VALUE]		= &map_key_value_types,
> -	[ARG_PTR_TO_UNINIT_MAP_VALUE]	= &map_key_value_types,
>  	[ARG_CONST_SIZE]		= &scalar_types,
>  	[ARG_CONST_SIZE_OR_ZERO]	= &scalar_types,
>  	[ARG_CONST_ALLOC_SIZE_OR_ZERO]	= &scalar_types,
> @@ -5537,7 +5530,6 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
>  	[ARG_PTR_TO_BTF_ID]		= &btf_ptr_types,
>  	[ARG_PTR_TO_SPIN_LOCK]		= &spin_lock_types,
>  	[ARG_PTR_TO_MEM]		= &mem_types,
> -	[ARG_PTR_TO_UNINIT_MEM]		= &mem_types,
>  	[ARG_PTR_TO_ALLOC_MEM]		= &alloc_mem_types,
>  	[ARG_PTR_TO_INT]		= &int_ptr_types,
>  	[ARG_PTR_TO_LONG]		= &int_ptr_types,
> @@ -5711,8 +5703,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  		return -EACCES;
>  	}

Could (or should) this go in a separate patch? Even with removing
ARG_PTR_TO_UNINIT_MAP_VALUE, it seems like this could stand on its own. Not
sure what the norm is for how granular to split patches for bpf, so even if
it could go in its own patch, feel free to disregard if you think splitting
this off is excessive / unnecessary.

> -	} else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE ||
> -		   base_type(arg_type) == ARG_PTR_TO_UNINIT_MAP_VALUE) {
> +	} else if (base_type(arg_type) == ARG_PTR_TO_MAP_VALUE) {
>  		if (type_may_be_null(arg_type) && register_is_null(reg))
>  			return 0;
>  
> @@ -5811,7 +5801,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  			verbose(env, "invalid map_ptr to access map->value\n");
>  			return -EACCES;
>  		}
> -		meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE);
> +		meta->raw_mode = arg_type & MEM_UNINIT;

Given that we're stashing in a bool here, should this be:

	meta->raw_mode = (arg_type & MEM_UNINIT) != 0;

>  		err = check_helper_mem_access(env, regno,
>  					      meta->map_ptr->value_size, false,
>  					      meta);
> @@ -5838,11 +5828,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  			return -EACCES;
>  	} else if (arg_type == ARG_PTR_TO_FUNC) {
>  		meta->subprogno = reg->subprogno;
> -	} else if (arg_type_is_mem_ptr(arg_type)) {
> +	} else if (base_type(arg_type) == ARG_PTR_TO_MEM) {
>  		/* The access to this pointer is only checked when we hit the
>  		 * next is_mem_size argument below.
>  		 */
> -		meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM);
> +		meta->raw_mode = arg_type & MEM_UNINIT;

Same here.

>  	} else if (arg_type_is_mem_size(arg_type)) {
>  		bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
>  
> @@ -6189,9 +6179,9 @@ static bool check_raw_mode_ok(const struct bpf_func_proto *fn)
>  static bool check_args_pair_invalid(enum bpf_arg_type arg_curr,
>  				    enum bpf_arg_type arg_next)
>  {
> -	return (arg_type_is_mem_ptr(arg_curr) &&
> +	return (base_type(arg_curr) == ARG_PTR_TO_MEM &&
>  	        !arg_type_is_mem_size(arg_next)) ||
> -	       (!arg_type_is_mem_ptr(arg_curr) &&
> +	       (base_type(arg_curr) != ARG_PTR_TO_MEM &&
>  		arg_type_is_mem_size(arg_next));
>  }

What do you think about this as a possibly more concise way to express that
the curr and next args differ?

	return (base_type(arg_curr) == ARG_PTR_TO_MEM) !=
		arg_type_is_mem_size(arg_next);


Looks great overall!

Thanks,
David