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

[Yonghong Song <yonghong.song@linux.dev>]

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.