On Tue, Feb 13, 2024 at 8:40 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2024-02-12 at 15:32 -0800, Andrii Nakryiko wrote:
> > Expected canonical argument type for global function arguments
> > representing PTR_TO_CTX is `bpf_user_pt_regs_t *ctx`. This currently
> > works on s390x by accident because kernel resolves such typedef to
> > underlying struct (which is anonymous on s390x), and erroneously
> > accepting it as expected context type. We are fixing this problem next,
> > which would break s390x arch, so we need to handle `bpf_user_pt_regs_t`
> > case explicitly for KPROBE programs.
> >
> > Fixes: 91cc1a99740e ("bpf: Annotate context types")
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
>
> Nit: same could be achieved w/o special casing kprobes by looking
> if typedef's type is named before skipping, e.g. as below.
> But I do not insist, probably good as it is as well.
>
> ---
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index f0ce384aa73e..830635b37fa1 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -907,11 +907,9 @@ bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
> }
>
> /* Similar to btf_type_skip_modifiers() but does not skip typedefs. */
> -static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
> - u32 id)
> +static const struct btf_type *__btf_type_skip_qualifiers(const struct btf *btf,
> + const struct btf_type *t)
> {
> - const struct btf_type *t = btf_type_by_id(btf, id);
> -
> while (btf_type_is_modifier(t) &&
> BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) {
> t = btf_type_by_id(btf, t->type);
> @@ -920,6 +918,12 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
> return t;
> }
>
> +static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
> + u32 id)
> +{
> + return __btf_type_skip_qualifiers(btf, btf_type_by_id(btf, id));
> +}
> +
> #define BTF_SHOW_MAX_ITER 10
>
> #define BTF_KIND_BIT(kind) (1ULL << kind)
> @@ -5695,9 +5699,25 @@ bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
> const char *tname, *ctx_tname;
>
> t = btf_type_by_id(btf, t->type);
> - while (btf_type_is_modifier(t))
> - t = btf_type_by_id(btf, t->type);
> - if (!btf_type_is_struct(t)) {
> +
> + /* Skip modifiers, but stop if skipping of typedef would
> + * lead an anonymous type, e.g. like for s390:
> + *
> + * typedef struct { ... } user_pt_regs;
> + * typedef user_pt_regs bpf_user_pt_regs_t;
> + */
> + t = __btf_type_skip_qualifiers(btf, t);
> + while (btf_type_is_typedef(t)) {
> + const struct btf_type *t1;
> +
> + t1 = btf_type_by_id(btf, t->type);
> + t1 = __btf_type_skip_qualifiers(btf, t1);
> + tname = btf_name_by_offset(btf, t1->name_off);
> + if (!tname || tname[0] == '\0')
> + break;
> + t = t1;
> + }
> + if (!btf_type_is_struct(t) && !btf_type_is_typedef(t)) {
and now we potentially are intermixing structs and typedefs and don't
really distinguish them later (but struct abc is not the same thing as
typedef abc), which is probably not what we want.
Also, we resolve typedef to its underlying type *or* typedef, right?
So if I have typedef A -> typedef B -> struct C, we won't get to
struct C, even if struct C is the expected correct context type for a
given program type (and it should work).
So in general, yes, I think this code could be changed to not ignore
typedefs and do the right thing, but we'd need to be very careful to
not allow unexpected things for all program types. Given only kprobes
define context as typedef, it's simpler and more reliable to special
case kprobe, IMO.
> /* Only pointer to struct is supported for now.
> * That means that BPF_PROG_TYPE_TRACEPOINT with BTF
> * is not supported yet.
