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
