Re: CORE feature request: support checking field type directly

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Jan 13, 2023 at 5:06 PM Hao Luo <haoluo@google.com> wrote:
>
> On Fri, Jan 13, 2023 at 3:41 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Jan 12, 2023 at 2:18 PM Hao Luo <haoluo@google.com> wrote:
> > >
> > > Hi all,
> > >
> > > Feature request:
> > >
> > > To support checking the type of a specific field directly.
> > >
> > > Background:
> > >
> > > Currently, As far as I know, CORE is able to check a field’s
> > > existence, offset, size and signedness, but not the field’s type
> > > directly.
> > >
> > > There are changes that convert a field from a scalar type to a struct
> > > type, without changing the field’s name, offset or size. In that case,
> > > it is currently difficult to use CORE to check such changes. For a
> > > concrete example,
> > >
> > > Commit 94a9717b3c (“locking/rwsem: Make rwsem->owner an atomic_long_t”)
> > >
> > > Changed the type of rw_semaphore::owner from tast_struct * to
> > > atomic_long_t. In that change, the field name, offset and size remain
> > > the same. But the BPF program code used to extract the value is
> > > different. For the kernel where the field is a pointer, we can write:
> > >
> > > sem->owner
> > >
> > > While in the kernel where the field is an atomic, we need to write:
> > >
> > > sem->owner.counter.
> > >
> > > It would be great to be able to check a field’s type directly.
> > > Probably something like:
> > >
> > > #include “vmlinux.h”
> > >
> > > struct rw_semaphore__old {
> > >         struct task_struct *owner;
> > > };
> > >
> > > struct rw_semaphore__new {
> > >         atomic_long_t owner;
> > > };
> > >
> > > u64 owner;
> > > if (bpf_core_field_type_is(sem->owner, struct task_struct *)) {
> > >         struct rw_semaphore__old *old = (struct rw_semaphore__old *)sem;
> > >         owner = (u64)sem->owner;
> > > } else if (bpf_core_field_type_is(sem->owner, atomic_long_t)) {
> > >         struct rw_semaphore__new *new = (struct rw_semaphore__new *)sem;
> > >         owner = new->owner.counter;
> > > }
> > >
> >
> > Have you tried bpf_core_type_matches()? It seems like exactly what you
> > are looking for? See [0] for logic of what constitutes "a match".
> >
>
> It seems bpf_core_type_matches() is for the userspace code. I'm

It's in the same family as bpf_type_{exists,size}() and
bpf_field_{exists,size,offset}(). It's purely BPF-side. Please grep
for bpf_core_type_matches() in selftests/bpf.

> looking for type checking in the BPF code. We probably don't need to
> check type equivalence, just comparing the btf_id of the field's type
> and the btf_id of a target type may be sufficient.

With the example above something like below should work:

struct rw_semaphore__old {
        struct task_struct *owner;
};

struct rw_semaphore__new {
        atomic_long_t owner;
};

u64 owner;
if (bpf_core_type_matches(struct rw_semaphore__old) /* owner is
task_struct pointer */) {
        struct rw_semaphore__old *old = (struct rw_semaphore__old *)sem;
        owner = (u64)sem->owner;
} else if (bpf_core_type_matches(struct rw_semaphore__old) /* owner
field is atomic_long_t */) {
        struct rw_semaphore__new *new = (struct rw_semaphore__new *)sem;
        owner = new->owner.counter;
}

>
> The commit 94a9717b3c (“locking/rwsem: Make rwsem->owner an
> atomic_long_t”) is rare, but the 'owner' field is useful for tracking
> the owner of a kernel lock.

We implemented bpf_core_type_matches() to detect tracepoint changes,
which is equivalent (if not harder) use case. Give it a try.

>
> >   [0] https://github.com/libbpf/libbpf/blob/master/src/relo_core.c#L1517-L1543
> >
> > > Hao