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

[Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>]

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.