| Message ID | 20200722211223.1055107-7-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Add d_path helper | expand |
On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote: > > Adding btf_struct_walk function that walks through the > struct type + given offset and returns following values: > > enum walk_return { > /* < 0 error */ > walk_scalar = 0, > walk_ptr, > walk_struct, > }; > > walk_scalar - when SCALAR_VALUE is found > walk_ptr - when pointer value is found, its ID is stored > in 'rid' output param > walk_struct - when nested struct object is found, its ID is stored > in 'rid' output param > > It will be used in following patches to get all nested > struct objects for given type and offset. > > The btf_struct_access now calls btf_struct_walk function, > as long as it gets nested structs as return value. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- It actually worked out to a pretty minimal changes to btf_struct_access, I'm pleasantly surprised :)) Logic seems correct, just have few nits about naming and a bit more safe handling in btf_struct_access, see below. > 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? > + > +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? > > error: > bpf_log(log, "access beyond struct %s at off %u size %u\n", > @@ -3954,7 +3959,7 @@ int btf_struct_access(struct bpf_verifier_log *log, > */ > if (off <= moff && > BITS_ROUNDUP_BYTES(end_bit) <= off + size) > - return SCALAR_VALUE; > + return walk_scalar; > > /* off may be accessing a following member > * [...] > @@ -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. > + return walk_ptr; > } > } > > @@ -4087,12 +4100,46 @@ int btf_struct_access(struct bpf_verifier_log *log, > return -EACCES; > } > > - return SCALAR_VALUE; > + return walk_scalar; > } > bpf_log(log, "struct %s doesn't have field at offset %d\n", tname, off); > return -EINVAL; > } > > +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? > + } while (t); > + > + return -EINVAL; > +} > + > int btf_resolve_helper_id(struct bpf_verifier_log *log, > const struct bpf_func_proto *fn, int arg) > { > -- > 2.25.4 >
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
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, +}; + +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); error: bpf_log(log, "access beyond struct %s at off %u size %u\n", @@ -3954,7 +3959,7 @@ int btf_struct_access(struct bpf_verifier_log *log, */ if (off <= moff && BITS_ROUNDUP_BYTES(end_bit) <= off + size) - return SCALAR_VALUE; + return walk_scalar; /* off may be accessing a following member * @@ -3976,11 +3981,13 @@ int btf_struct_access(struct bpf_verifier_log *log, break; /* type of the field */ + mid = member->type; mtype = btf_type_by_id(btf_vmlinux, member->type); mname = __btf_name_by_offset(btf_vmlinux, member->name_off); mtype = __btf_resolve_size(btf_vmlinux, mtype, &msize, - &elem_type, NULL, &total_nelems, NULL); + &elem_type, &elem_id, &total_nelems, + &mid); if (IS_ERR(mtype)) { bpf_log(log, "field %s doesn't have size\n", mname); return -EFAULT; @@ -4042,6 +4049,7 @@ int btf_struct_access(struct bpf_verifier_log *log, elem_idx = (off - moff) / msize; moff += elem_idx * msize; mtype = elem_type; + mid = elem_id; } /* the 'off' we're looking for is either equal to start @@ -4051,6 +4059,12 @@ int btf_struct_access(struct bpf_verifier_log *log, /* our field must be inside that union or struct */ t = mtype; + /* return if the offset matches the member offset */ + if (off == moff) { + *rid = mid; + return walk_struct; + } + /* adjust offset we're looking for */ off -= moff; goto again; @@ -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; + return walk_ptr; } } @@ -4087,12 +4100,46 @@ int btf_struct_access(struct bpf_verifier_log *log, return -EACCES; } - return SCALAR_VALUE; + return walk_scalar; } bpf_log(log, "struct %s doesn't have field at offset %d\n", tname, off); return -EINVAL; } +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; + } while (t); + + return -EINVAL; +} + int btf_resolve_helper_id(struct bpf_verifier_log *log, const struct bpf_func_proto *fn, int arg) {
Adding btf_struct_walk function that walks through the struct type + given offset and returns following values: enum walk_return { /* < 0 error */ walk_scalar = 0, walk_ptr, walk_struct, }; walk_scalar - when SCALAR_VALUE is found walk_ptr - when pointer value is found, its ID is stored in 'rid' output param walk_struct - when nested struct object is found, its ID is stored in 'rid' output param It will be used in following patches to get all nested struct objects for given type and offset. The btf_struct_access now calls btf_struct_walk function, as long as it gets nested structs as return value. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- kernel/bpf/btf.c | 73 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 13 deletions(-)