On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
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
>
