From: Jiri Olsa <jolsa@redhat.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>, Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andriin@fb.com>,
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>,
Brendan Gregg <bgregg@netflix.com>,
Florent Revest <revest@chromium.org>,
Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH v8 bpf-next 06/13] bpf: Factor btf_struct_access function
Date: Wed, 29 Jul 2020 17:59:37 +0200 [thread overview]
Message-ID: <20200729155937.GL1319041@krava> (raw)
In-Reply-To: <CAEf4BzbS_JFW70Z_68hDtN4VTkLfohkR0PV0d9jCJRjZEhc01Q@mail.gmail.com>
On Tue, Jul 28, 2020 at 04:27:21PM -0700, Andrii Nakryiko wrote:
SNIP
>
> > kernel/bpf/btf.c | 73 +++++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 60 insertions(+), 13 deletions(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 841be6c49f11..1ab5fd5bf992 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -3873,16 +3873,22 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > return true;
> > }
> >
> > -int btf_struct_access(struct bpf_verifier_log *log,
> > - const struct btf_type *t, int off, int size,
> > - enum bpf_access_type atype,
> > - u32 *next_btf_id)
> > +enum walk_return {
> > + /* < 0 error */
> > + walk_scalar = 0,
> > + walk_ptr,
> > + walk_struct,
> > +};
>
> let's keep enum values in ALL_CAPS? walk_return is also a bit generic,
> maybe something like bpf_struct_walk_result?
ok
>
> > +
> > +static int btf_struct_walk(struct bpf_verifier_log *log,
> > + const struct btf_type *t, int off, int size,
> > + u32 *rid)
> > {
> > u32 i, moff, mtrue_end, msize = 0, total_nelems = 0;
> > const struct btf_type *mtype, *elem_type = NULL;
> > const struct btf_member *member;
> > const char *tname, *mname;
> > - u32 vlen;
> > + u32 vlen, elem_id, mid;
> >
> > again:
> > tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
> > @@ -3924,8 +3930,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
> > goto error;
> >
> > off = (off - moff) % elem_type->size;
> > - return btf_struct_access(log, elem_type, off, size, atype,
> > - next_btf_id);
> > + return btf_struct_walk(log, elem_type, off, size, rid);
>
> oh, btw, this is a recursion in the kernel, let's fix that? I think it
> could easily be just `goto again` here?
probably, I'll put it into separate change then
SNIP
>
> > @@ -4066,11 +4080,10 @@ int btf_struct_access(struct bpf_verifier_log *log,
> > mname, moff, tname, off, size);
> > return -EACCES;
> > }
> > -
> > stype = btf_type_skip_modifiers(btf_vmlinux, mtype->type, &id);
> > if (btf_type_is_struct(stype)) {
> > - *next_btf_id = id;
> > - return PTR_TO_BTF_ID;
> > + *rid = id;
>
> nit: rid is a very opaque name, I find next_btf_id more appropriate
> (even if it's meaning changes depending on walk_ptr vs walk_struct.
ok, will change
SNIP
> > +int btf_struct_access(struct bpf_verifier_log *log,
> > + const struct btf_type *t, int off, int size,
> > + enum bpf_access_type atype __maybe_unused,
> > + u32 *next_btf_id)
> > +{
> > + int err;
> > + u32 id;
> > +
> > + do {
> > + err = btf_struct_walk(log, t, off, size, &id);
> > + if (err < 0)
> > + return err;
> > +
> > + /* We found the pointer or scalar on t+off,
> > + * we're done.
> > + */
> > + if (err == walk_ptr) {
> > + *next_btf_id = id;
> > + return PTR_TO_BTF_ID;
> > + }
> > + if (err == walk_scalar)
> > + return SCALAR_VALUE;
> > +
> > + /* We found nested struct, so continue the search
> > + * by diving in it. At this point the offset is
> > + * aligned with the new type, so set it to 0.
> > + */
> > + t = btf_type_by_id(btf_vmlinux, id);
> > + off = 0;
>
> It's very easy to miss that this case corresponds to walk_struct here.
> If someone in the future adds a 4th special value, it will be too easy
> to forget to update this piece of logic. So when dealing with enums, I
> generally prefer this approach:
>
> switch (err) {
> case walk_ptr:
> ...
> case walk_scalar:
> ...
> case walk_struct:
> ...
> default: /* complain loudly here */
> }
>
> WDYT?
right, I like it, make sense for future.. will change
thanks,
jirka
next prev parent reply other threads:[~2020-07-29 15:59 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-22 21:12 [PATCH v8 bpf-next 00/13] bpf: Add d_path helper Jiri Olsa
2020-07-22 21:12 ` [PATCH v8 bpf-next 01/13] selftests/bpf: Fix resolve_btfids test Jiri Olsa
2020-07-28 20:27 ` Andrii Nakryiko
2020-07-29 20:06 ` Jiri Olsa
2020-07-29 21:38 ` Andrii Nakryiko
2020-07-22 21:12 ` [PATCH v8 bpf-next 02/13] tools resolve_btfids: Add support for set symbols Jiri Olsa
2020-07-28 0:53 ` Andrii Nakryiko
2020-07-28 9:35 ` Jiri Olsa
2020-07-22 21:12 ` [PATCH v8 bpf-next 03/13] bpf: Move btf_resolve_size into __btf_resolve_size Jiri Olsa
2020-07-28 20:29 ` Andrii Nakryiko
2020-07-22 21:12 ` [PATCH v8 bpf-next 04/13] bpf: Add elem_id pointer as argument to __btf_resolve_size Jiri Olsa
2020-07-28 20:30 ` Andrii Nakryiko
2020-07-22 21:12 ` [PATCH v8 bpf-next 05/13] bpf: Add type_id " Jiri Olsa
2020-07-22 21:12 ` [PATCH v8 bpf-next 06/13] bpf: Factor btf_struct_access function Jiri Olsa
2020-07-28 23:27 ` Andrii Nakryiko
2020-07-29 15:59 ` Jiri Olsa [this message]
2020-07-22 21:12 ` [PATCH v8 bpf-next 07/13] bpf: Add btf_struct_ids_match function Jiri Olsa
2020-07-28 23:35 ` Andrii Nakryiko
2020-07-29 16:04 ` Jiri Olsa
2020-07-29 17:51 ` Andrii Nakryiko
2020-07-29 18:55 ` Jiri Olsa
2020-07-22 21:12 ` [PATCH v8 bpf-next 08/13] bpf: Add BTF_SET_START/END macros Jiri Olsa
2020-07-28 19:39 ` Andrii Nakryiko
2020-07-29 11:54 ` Jiri Olsa
2020-07-22 21:12 ` [PATCH v8 bpf-next 09/13] bpf: Add d_path helper Jiri Olsa
2020-07-28 19:47 ` Andrii Nakryiko
2020-07-29 15:54 ` Jiri Olsa
2020-07-29 20:11 ` Al Viro
2020-07-30 10:22 ` Jiri Olsa
2020-07-29 20:17 ` Al Viro
2020-07-30 10:09 ` Jiri Olsa
2020-07-22 21:12 ` [PATCH v8 bpf-next 10/13] bpf: Update .BTF_ids section in btf.rst with sets info Jiri Olsa
2020-07-22 21:12 ` [PATCH v8 bpf-next 11/13] selftests/bpf: Add verifier test for d_path helper Jiri Olsa
2020-07-28 19:49 ` Andrii Nakryiko
2020-07-22 21:12 ` [PATCH v8 bpf-next 12/13] selftests/bpf: Add " Jiri Olsa
2020-07-28 19:53 ` Andrii Nakryiko
2020-07-29 11:25 ` Jiri Olsa
2020-07-22 21:12 ` [PATCH v8 bpf-next 13/13] selftests/bpf: Add set test to resolve_btfids Jiri Olsa
2020-07-28 19:56 ` Andrii Nakryiko
2020-07-29 15:34 ` 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=20200729155937.GL1319041@krava \
--to=jolsa@redhat.com \
--cc=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=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).