Re: [PATCH v8 bpf-next 06/13] bpf: Factor btf_struct_access function

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