Message-ID: <CAEf4BzYR59T5_kjFRSQ4pUxzmA5i02nCt_V5YqY0dXQwMRSjtA@mail.gmail.com>
Date:   Mon, 6 Jul 2020 16:15:47 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Martin KaFai Lau <kafai@...com>,
        David Miller <davem@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Wenbo Zhang <ethercflow@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Andrii Nakryiko <andriin@...com>,
        Brendan Gregg <bgregg@...flix.com>,
        Florent Revest <revest@...omium.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v4 bpf-next 07/14] bpf: Allow nested BTF object to be
 refferenced by BTF object + offset

On Thu, Jul 2, 2020 at 3:09 AM Jiri Olsa <jolsa@...hat.com> wrote:
>
> On Tue, Jun 30, 2020 at 01:05:52PM -0700, Andrii Nakryiko wrote:
> > On Thu, Jun 25, 2020 at 4:49 PM Jiri Olsa <jolsa@...nel.org> wrote:
> > >
> > > Adding btf_struct_address function that takes 2 BTF objects
> > > and offset as arguments and checks whether object A is nested
> > > in object B on given offset.
> > >
> > > This function will be used when checking the helper function
> > > PTR_TO_BTF_ID arguments. If the argument has an offset value,
> > > the btf_struct_address will check if the final address is
> > > the expected BTF ID.
> > >
> > > This way we can access nested BTF objects under PTR_TO_BTF_ID
> > > pointer type and pass them to helpers, while they still point
> > > to valid kernel BTF objects.
> > >
> > > Using btf_struct_access to implement new btf_struct_address
> > > function, because it already walks down the given BTF object.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > > ---

[...]

> >
> > Ok, I think I'm grasping this a bit more. How about we actually don't
> > have two different cases (btf_struct_access and btf_struct_address),
> > but instead make unified btf_struct_access that will return the
> > earliest field that register points to (so it sort of iterates deeper
> > and deeper with each invocation). So e.g., let's assume we have this
> > type:
> >
> >
> > struct A {
> >   struct B {
> >     struct C {
> >       int x;
> >     } c;
> >   } b;
> >   struct D { int y; } d;
> > };
> >
> > Now consider the extreme case of a BPF helper that expects a pointer
> > to the struct C or D (so uses a custom btf_id check function to say if
> > a passed argument is acceptable or not), ok?
> >
> > Now you write BPF program as such, r1 has pointer to struct A,
> > originally (so verifier knows btf_id points to struct A):
> >
> > int prog(struct A *a) {
> >    return fancy_helper(&a->b.c);
> > }
> >
> > Now, when verifier checks fancy_helper first time, its btf_id check
> > will say "nope". But before giving up, verifier calls
> > btf_struct_access, it goes into struct A field, finds field b with
> > offset 0, it matches register's offset (always zero in this scenario),
> > sees that that field is a struct B, so returns that register now
> > points to struct B. Verifier passed that updated BTF ID to
> > fancy_helper's check, it still says no. Again, don't give up,
> > btf_struct_access again, but now register assumes it starts in struct
> > B. It finds field c of type struct C, so returns successfully. Again,
> > we are checking with fancy_helper's check_btf_id() check, now it
> > succeeds, so we keep register's BTF_ID as struct C and carry on.
> >
> > Now assume fancy_helper only accepts struct D. So once we pass struct
> > C, it rejects. Again, btf_struct_access() is called, this time find
> > field x, which is int (and thus register is SCALAR now).
> > check_btf_id() rejects it, we call btf_struct_access() again, but this
> > time we can't really go deeper into type int, so we give up at this
> > point and return error.
> >
> > Now, when register's offset is non-zero, the process is exactly the
> > same, we just need to keep locally adjusted offset, so that when we
> > find inner struct, we start with the offset within that struct, not
> > origin struct A's offset.
> >
> > It's quite verbose explanation, but hopefully you get the idea. I
> > think implementation shouldn't be too hard, we might need to extend
> > register's state to have this extra local offset to get to the start
> > of a type we believe register points to (something like inner_offset,
> > or something). Then btf_struct_access can take into account both
> > register's off and inner_off to maintain this offset to inner type.
> >
> > It should nicely work in all cases, not just partially as it is right now. WDYT?
>
> I think above should work nicely for my case, but we need
> to keep the current btf_struct_access usage, which is to
> check if we point to a pointer type and return the ID it
> points to
>
> I think it's doable with the functionality you described,
> we'll just check of the returned type is pointer and get
> the ID it points to.. which makes me think we still need
> functions like below (again bad names probably ;-) )
>
>   btf_struct_walk
>     - implements the walk through the type as you described
>       above.. returns the type we point to and we can call
>       it again to resolve the next type at the same offset
>
>   btf_struct_address
>     - calls btf_struct_walk and checks if the returned type ID
>       matches the requested BTF ID of the helper argument if not
>       and the returned type is struct, call btf_struct_walk again
>       to get the next layer and repeat..
>
>   btf_struct_access
>     - calls btf_struct_walk repeatedly until the returned type is
>       a pointer and then it returns the BTF ID it points to
>

Sure, as long as all the BTF walking is contained in one place (in
btf_struct_walk), which was my main objection on your very first
version. All the other wrapper functions can have their own extra
restrictions, but still use the same struct-walking primitive
operation. Ok, glad we figured it out :)

> thanks,
> jirka
>

Powered by blists - more mailing lists