Date:   Wed, 29 Jul 2020 17:59:37 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        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>,
        Brendan Gregg <bgregg@...flix.com>,
        Florent Revest <revest@...omium.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v8 bpf-next 06/13] bpf: Factor btf_struct_access function

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

Powered by blists - more mailing lists