Re: [PATCH bpf-next v2 2/4] bpf: Add a kfunc to type cast from bpf uapi ctx to kernel ctx

[Yonghong Song <yhs@meta.com>]

On 11/20/22 11:10 AM, Alexei Starovoitov wrote:
> On Sun, Nov 20, 2022 at 10:55:54AM -0800, Yonghong Song wrote:
>>
>>
>> On 11/20/22 10:33 AM, Alexei Starovoitov wrote:
>>> On Sun, Nov 20, 2022 at 08:15:22AM -0800, Yonghong Song wrote:
>>>> Implement bpf_cast_to_kern_ctx() kfunc which does a type cast
>>>> of a uapi ctx object to the corresponding kernel ctx. Previously
>>>> if users want to access some data available in kctx but not
>>>> in uapi ctx, bpf_probe_read_kernel() helper is needed.
>>>> The introduction of bpf_cast_to_kern_ctx() allows direct
>>>> memory access which makes code simpler and easier to understand.
>>>>
>>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>>> ---
>>>>    include/linux/btf.h   |  5 +++++
>>>>    kernel/bpf/btf.c      | 25 +++++++++++++++++++++++++
>>>>    kernel/bpf/helpers.c  |  6 ++++++
>>>>    kernel/bpf/verifier.c | 21 +++++++++++++++++++++
>>>>    4 files changed, 57 insertions(+)
>>>>
>>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>>> index d5b26380a60f..4b5d799f5d02 100644
>>>> --- a/include/linux/btf.h
>>>> +++ b/include/linux/btf.h
>>>> @@ -470,6 +470,7 @@ const struct btf_member *
>>>>    btf_get_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
>>>>    		      const struct btf_type *t, enum bpf_prog_type prog_type,
>>>>    		      int arg);
>>>> +int get_kern_ctx_btf_id(struct bpf_verifier_log *log, enum bpf_prog_type prog_type);
>>>>    bool btf_types_are_same(const struct btf *btf1, u32 id1,
>>>>    			const struct btf *btf2, u32 id2);
>>>>    #else
>>>> @@ -514,6 +515,10 @@ btf_get_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
>>>>    {
>>>>    	return NULL;
>>>>    }
>>>> +static inline int get_kern_ctx_btf_id(struct bpf_verifier_log *log,
>>>> +				      enum bpf_prog_type prog_type) {
>>>> +	return -EINVAL;
>>>> +}
>>>>    static inline bool btf_types_are_same(const struct btf *btf1, u32 id1,
>>>>    				      const struct btf *btf2, u32 id2)
>>>>    {
>>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>>> index 0a3abbe56c5d..bef1b6cfe6b8 100644
>>>> --- a/kernel/bpf/btf.c
>>>> +++ b/kernel/bpf/btf.c
>>>> @@ -5603,6 +5603,31 @@ static int btf_translate_to_vmlinux(struct bpf_verifier_log *log,
>>>>    	return kern_ctx_type->type;
>>>>    }
>>>> +int get_kern_ctx_btf_id(struct bpf_verifier_log *log, enum bpf_prog_type prog_type)
>>>> +{
>>>> +	const struct btf_member *kctx_member;
>>>> +	const struct btf_type *conv_struct;
>>>> +	const struct btf_type *kctx_type;
>>>> +	u32 kctx_type_id;
>>>> +
>>>> +	conv_struct = bpf_ctx_convert.t;
>>>> +	if (!conv_struct) {
>>>> +		bpf_log(log, "btf_vmlinux is malformed\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>> If we get to this point this internal pointer would be already checked.
>>> No need to check it again. Just use it.
>>
>> This is probably not true.
>>
>> Currently, conv_struct is tested in function btf_get_prog_ctx_type() which
>> is called by get_kfunc_ptr_arg_type().
>>
>> const struct btf_member *
>> btf_get_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
>>                        const struct btf_type *t, enum bpf_prog_type
>> prog_type,
>>                        int arg)
>> {
>>          const struct btf_type *conv_struct;
>>          const struct btf_type *ctx_struct;
>>          const struct btf_member *ctx_type;
>>          const char *tname, *ctx_tname;
>>
>>          conv_struct = bpf_ctx_convert.t;
>>          if (!conv_struct) {
>>                  bpf_log(log, "btf_vmlinux is malformed\n");
>>                  return NULL;
>>          }
>> 	...
>> }
>>
>> In get_kfunc_ptr_arg_type(),
>>
>> ...
>>
>>          /* In this function, we verify the kfunc's BTF as per the argument
>> type,
>>           * leaving the rest of the verification with respect to the register
>>           * type to our caller. When a set of conditions hold in the BTF type
>> of
>>           * arguments, we resolve it to a known kfunc_ptr_arg_type.
>>           */
>>          if (btf_get_prog_ctx_type(&env->log, meta->btf, t,
>> resolve_prog_type(env->prog), argno))
>>                  return KF_ARG_PTR_TO_CTX;
>>
>> Note that if bpf_ctx_convert.t is NULL, btf_get_prog_ctx_type() simply
>> returns NULL and the logic simply follows through.
> 
> Right. It will return NULL and the code further won't see KF_ARG_PTR_TO_CTX
> and will not call get_kern_ctx_btf_id().
> So it still looks to me that the check can be dropped.
> 
>> Should we actually add a NULL checking for bpf_ctx_convert.t in
>> bpf_parse_vmlinux?
> 
> Ideally yes, but right now CONFIG_DEBUG_INFO_BTF can be enabled
> independently and I'm afraid btf_get_prog_ctx_type() can be called
> via btf_translate_to_vmlinux() even when btf_vmlinux == NULL.
> So bpf_ctx_convert.t == NULL at that point
> because btf_parse_vmlinux wasn't called.

Okay, I see. btf_get_prog_ctx_type() could be called even when vmlinux 
btf is not parsed yet. But get_kfunc_ptr_arg_type() should already
have vmlinux parsed properly. So for bpf_cast_to_kern_ctx handling,
bpf_ctx_convert.t should not be NULL.

Will drop the error checking in the next revision.

> 
>>
>> ...
>>          err = btf_check_type_tags(env, btf, 1);
>>          if (err)
>>                  goto errout;
>>
>>          /* btf_parse_vmlinux() runs under bpf_verifier_lock */
>>          bpf_ctx_convert.t = btf_type_by_id(btf, bpf_ctx_convert_btf_id[0]);
>>
>>          bpf_struct_ops_init(btf, log);
>> ...
>>