From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>, Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
Martin KaFai Lau <kafai@fb.com>, David Miller <davem@redhat.com>,
John Fastabend <john.fastabend@gmail.com>,
Wenbo Zhang <ethercflow@gmail.com>,
KP Singh <kpsingh@chromium.org>, Andrii Nakryiko <andriin@fb.com>,
Brendan Gregg <bgregg@netflix.com>,
Florent Revest <revest@chromium.org>,
Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH v4 bpf-next 07/14] bpf: Allow nested BTF object to be refferenced by BTF object + offset
Date: Mon, 6 Jul 2020 16:15:47 -0700 [thread overview]
Message-ID: <CAEf4BzYR59T5_kjFRSQ4pUxzmA5i02nCt_V5YqY0dXQwMRSjtA@mail.gmail.com> (raw)
In-Reply-To: <20200702100859.GC3144378@krava>
On Thu, Jul 2, 2020 at 3:09 AM Jiri Olsa <jolsa@redhat.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@kernel.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@kernel.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
>
next prev parent reply other threads:[~2020-07-06 23:16 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-25 22:12 [PATCH v4 bpf-next 00/14] bpf: Add d_path helper Jiri Olsa
2020-06-25 22:12 ` [PATCH v4 bpf-next 01/14] bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object Jiri Olsa
2020-06-26 20:53 ` Andrii Nakryiko
2020-06-26 21:09 ` Yonghong Song
2020-06-28 19:09 ` Alexei Starovoitov
2020-06-28 19:35 ` Jiri Olsa
2020-06-28 20:53 ` Yonghong Song
2020-06-25 22:12 ` [PATCH v4 bpf-next 02/14] bpf: Compile resolve_btfids tool at kernel compilation start Jiri Olsa
2020-06-26 21:28 ` Andrii Nakryiko
2020-06-28 19:48 ` Jiri Olsa
2020-06-25 22:12 ` [PATCH v4 bpf-next 03/14] bpf: Add BTF_ID_LIST/BTF_ID macros Jiri Olsa
2020-06-26 21:32 ` Andrii Nakryiko
2020-06-28 19:50 ` Jiri Olsa
2020-06-25 22:12 ` [PATCH v4 bpf-next 04/14] bpf: Resolve BTF IDs in vmlinux image Jiri Olsa
2020-06-26 21:34 ` Andrii Nakryiko
2020-06-25 22:12 ` [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving Jiri Olsa
2020-06-26 21:36 ` Yonghong Song
2020-06-26 21:40 ` Andrii Nakryiko
2020-06-26 23:29 ` Yonghong Song
2020-06-28 18:50 ` Alexei Starovoitov
2020-06-28 20:00 ` Andrii Nakryiko
2020-06-28 20:16 ` Jiri Olsa
2020-06-28 20:59 ` Yonghong Song
2020-06-28 21:20 ` Jiri Olsa
2020-06-25 22:12 ` [PATCH v4 bpf-next 06/14] bpf: Use BTF_ID to resolve bpf_ctx_convert struct Jiri Olsa
2020-06-26 21:44 ` Andrii Nakryiko
2020-06-26 21:44 ` Yonghong Song
2020-06-28 19:52 ` Jiri Olsa
2020-06-25 22:12 ` [PATCH v4 bpf-next 07/14] bpf: Allow nested BTF object to be refferenced by BTF object + offset Jiri Olsa
2020-06-30 1:52 ` Andrii Nakryiko
2020-06-30 13:54 ` Jiri Olsa
2020-06-30 20:05 ` Andrii Nakryiko
2020-06-30 20:07 ` Andrii Nakryiko
2020-07-02 10:08 ` Jiri Olsa
2020-07-06 23:15 ` Andrii Nakryiko [this message]
2020-06-25 22:12 ` [PATCH v4 bpf-next 08/14] bpf: Add BTF_SET_START/END macros Jiri Olsa
2020-06-26 21:49 ` Andrii Nakryiko
2020-06-25 22:12 ` [PATCH v4 bpf-next 09/14] bpf: Add info about .BTF.ids section to btf.rst Jiri Olsa
2020-06-25 22:13 ` [PATCH v4 bpf-next 10/14] bpf: Add d_path helper Jiri Olsa
2020-06-26 20:38 ` Andrii Nakryiko
2020-06-28 19:42 ` Jiri Olsa
2020-07-16 23:13 ` KP Singh
2020-07-17 8:28 ` Jiri Olsa
2020-06-25 22:13 ` [PATCH v4 bpf-next 11/14] tools headers: Adopt verbatim copy of btf_ids.h from kernel sources Jiri Olsa
2020-06-26 21:51 ` Andrii Nakryiko
2020-06-25 22:13 ` [PATCH v4 bpf-next 12/14] selftests/bpf: Add verifier test for d_path helper Jiri Olsa
2020-06-30 1:30 ` Andrii Nakryiko
2020-06-25 22:13 ` [PATCH v4 bpf-next 13/14] selftests/bpf: Add " Jiri Olsa
2020-06-26 21:55 ` Andrii Nakryiko
2020-06-28 19:55 ` Jiri Olsa
2020-06-25 22:13 ` [PATCH v4 bpf-next 14/14] selftests/bpf: Add test for resolve_btfids Jiri Olsa
2020-06-30 1:43 ` Andrii Nakryiko
2020-06-30 14:27 ` Jiri Olsa
2020-06-30 18:13 ` Andrii Nakryiko
2020-06-30 1:54 ` [PATCH v4 bpf-next 00/14] bpf: Add d_path helper Andrii Nakryiko
2020-06-30 13:55 ` Jiri Olsa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEf4BzYR59T5_kjFRSQ4pUxzmA5i02nCt_V5YqY0dXQwMRSjtA@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=andriin@fb.com \
--cc=ast@kernel.org \
--cc=bgregg@netflix.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@redhat.com \
--cc=ethercflow@gmail.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=jolsa@redhat.com \
--cc=kafai@fb.com \
--cc=kpsingh@chromium.org \
--cc=netdev@vger.kernel.org \
--cc=revest@chromium.org \
--cc=songliubraving@fb.com \
--cc=viro@zeniv.linux.org.uk \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).