Re: [PATCH v8 bpf-next 06/13] bpf: Factor btf_struct_access function
From: Andrii Nakryiko <hidden>
Date: 2020-07-28 23:27:35
Also in:
bpf
On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa [off-list ref] 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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
+ +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?
quoted hunk ↗ jump to hunk
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 *
[...]
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
+ 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