Re: [PATCH bpf-next 03/15] bpf: Add alloc/xchg/direct_access support for local percpu kptr

[Yonghong Song <yonghong.song@xxxxxxxxx>]

On 8/18/23 5:29 PM, Alexei Starovoitov wrote:
> On Mon, Aug 14, 2023 at 10:28:25AM -0700, Yonghong Song wrote:
> > @@ -4997,13 +4997,20 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
> >   	if (kptr_field->type == BPF_KPTR_UNREF)
> >   		perm_flags |= PTR_UNTRUSTED;
> >   
> > +	if (kptr_field->type == BPF_KPTR_PERCPU_REF)
> > +		perm_flags |= MEM_PERCPU | MEM_ALLOC;
>
> this bit doesn't look right and ...
>
> > +
> >   	if (base_type(reg->type) != PTR_TO_BTF_ID || (type_flag(reg->type) & ~perm_flags))
> >   		goto bad_type;
> >   
> > -	if (!btf_is_kernel(reg->btf)) {
> > +	if (kptr_field->type != BPF_KPTR_PERCPU_REF && !btf_is_kernel(reg->btf)) {
> >   		verbose(env, "R%d must point to kernel BTF\n", regno);
> >   		return -EINVAL;
> >   	}
> > +	if (kptr_field->type == BPF_KPTR_PERCPU_REF && btf_is_kernel(reg->btf)) {
> > +		verbose(env, "R%d must point to prog BTF\n", regno);
> > +		return -EINVAL;
> > +	}
>
> .. here it really doesn't look right.
> The map_kptr_match_type() should have been used for kptrs pointing to kernel objects only.
> But you're calling it for MEM_ALLOC object with prog's BTF...
>
> > +	case PTR_TO_BTF_ID | MEM_PERCPU | MEM_ALLOC:
> > +		if (meta->func_id != BPF_FUNC_kptr_xchg) {
> > +			verbose(env, "verifier internal error: unimplemented handling of MEM_PERCPU | MEM_ALLOC\n");
> > +			return -EFAULT;
> > +		}
>
> this part should be handling it, but ...
>
> > +		if (map_kptr_match_type(env, meta->kptr_field, reg, regno))
> > +			return -EACCES;
>
> why call this here?
>
> Existing:
>          case PTR_TO_BTF_ID | MEM_ALLOC:
>                  if (meta->func_id != BPF_FUNC_spin_lock && meta->func_id != BPF_FUNC_spin_unlock &&
>                      meta->func_id != BPF_FUNC_kptr_xchg) {
>                          verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n");
>                          return -EFAULT;
>                  }
> doesn't call map_kptr_match_type().
> Where do we check that btf of arg1 and arg2 matches for kptr_xchg of MEM_ALLOC objs? Do we have a bug?
>
> Yep. We do :(
>
> diff --git a/tools/testing/selftests/bpf/progs/local_kptr_stash.c b/tools/testing/selftests/bpf/progs/local_kptr_stash.c
> index 06838083079c..a6f546f4da9a 100644
> --- a/tools/testing/selftests/bpf/progs/local_kptr_stash.c
> +++ b/tools/testing/selftests/bpf/progs/local_kptr_stash.c
> @@ -14,10 +14,12 @@ struct node_data {
>          struct bpf_rb_node node;
>   };
>
> +struct node_data2 { long foo[4];};
> +
>   struct map_value {
>          struct prog_test_ref_kfunc *not_kptr;
>          struct prog_test_ref_kfunc __kptr *val;
> -       struct node_data __kptr *node;
> +       struct node_data2 __kptr *node;
>   };
>
>   /* This is necessary so that LLVM generates BTF for node_data struct
> @@ -32,6 +34,7 @@ struct map_value {
>    * Had to do the same w/ bpf_kfunc_call_test_release below
>    */
>   struct node_data *just_here_because_btf_bug;
> +struct node_data2 *just_here_because_btf_bug2;
>
> passes the verifier and runs into kernel WARN_ONCE.
>
> Let's fix this issue first before proceeding with this series.

Sounds good. I will investigate and fix this issue before sending
out v2.