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@xxxxxx>
> > > ---
> > > 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.
>
> ...
> 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);
> ...
>
