Re: [PATCH bpf-next v2 10/25] bpf: Introduce local kptrs

[Dave Marchevsky <davemarchevsky@meta.com>]

On 10/13/22 2:22 AM, Kumar Kartikeya Dwivedi wrote:
> Introduce the idea of local kptrs, i.e. PTR_TO_BTF_ID that point to a
> type in program BTF. This is indicated by the presence of MEM_TYPE_LOCAL
> type tag in reg->type to avoid having to check btf_is_kernel when trying
> to match argument types in helpers.
> 
> For now, these local kptrs will always be referenced in verifier
> context, hence ref_obj_id == 0 for them is a bug. It is allowed to write
> to such objects, as long fields that are special are not touched
> (support for which will be added in subsequent patches).
> 
> No PROBE_MEM handling is hence done since they can never be in an
> undefined state, and their lifetime will always be valid.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

One nit unrelated to the other thread we have going for this patch.

[...]

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 066984d73a8b..65f444405d9c 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6019,11 +6019,13 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
>  	return -EINVAL;
>  }
>  
> -int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
> +int btf_struct_access(struct bpf_verifier_log *log,
> +		      const struct bpf_reg_state *reg, const struct btf *btf,
>  		      const struct btf_type *t, int off, int size,
>  		      enum bpf_access_type atype __maybe_unused,
>  		      u32 *next_btf_id, enum bpf_type_flag *flag)
>  {
> +	bool local_type = reg && (type_flag(reg->type) & MEM_TYPE_LOCAL);

Can you add a type_is_local_kptr helper (or similar name) to reduce this
type_flag(reg->type) & MEM_TYPE_LOCAL repetition here and elsewhere in the patch?
Some examples of repetition in verifier.c below.

>  	enum bpf_type_flag tmp_flag = 0;
>  	int err;
>  	u32 id;
> @@ -6033,6 +6035,11 @@ int btf_struct_access(struct bpf_verifier_log *log, const struct btf *btf,
>  
>  		switch (err) {
>  		case WALK_PTR:
> +			/* For local types, the destination register cannot
> +			 * become a pointer again.
> +			 */
> +			if (local_type)
> +				return SCALAR_VALUE;
>  			/* If we found the pointer or scalar on t+off,
>  			 * we're done.
>  			 */
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 3c47cecda302..6ee8c06c2080 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4522,16 +4522,20 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
>  		return -EACCES;
>  	}
>  
> -	if (env->ops->btf_struct_access) {
> -		ret = env->ops->btf_struct_access(&env->log, reg->btf, t,
> +	if (env->ops->btf_struct_access && !(type_flag(reg->type) & MEM_TYPE_LOCAL)) {
> +		WARN_ON_ONCE(!btf_is_kernel(reg->btf));
> +		ret = env->ops->btf_struct_access(&env->log, reg, reg->btf, t,
>  						  off, size, atype, &btf_id, &flag);
>  	} else {
> -		if (atype != BPF_READ) {
> +		if (atype != BPF_READ && !(type_flag(reg->type) & MEM_TYPE_LOCAL)) {
>  			verbose(env, "only read is supported\n");
>  			return -EACCES;
>  		}
>  
> -		ret = btf_struct_access(&env->log, reg->btf, t, off, size,
> +		if (reg->type & MEM_TYPE_LOCAL)
> +			WARN_ON_ONCE(!reg->ref_obj_id);
> +
> +		ret = btf_struct_access(&env->log, reg, reg->btf, t, off, size,
>  					atype, &btf_id, &flag);
>  	}
>  
> @@ -4596,7 +4600,7 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,
>  		return -EACCES;
>  	}
>  
> -	ret = btf_struct_access(&env->log, btf_vmlinux, t, off, size, atype, &btf_id, &flag);
> +	ret = btf_struct_access(&env->log, NULL, btf_vmlinux, t, off, size, atype, &btf_id, &flag);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -5816,6 +5820,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
>  	 * fixed offset.
>  	 */
>  	case PTR_TO_BTF_ID:
> +	case PTR_TO_BTF_ID | MEM_TYPE_LOCAL:
>  		/* When referenced PTR_TO_BTF_ID is passed to release function,
>  		 * it's fixed offset must be 0.	In the other cases, fixed offset
>  		 * can be non-zero.

[...]