* [PATCH v1 bpf-next 0/4] Descend into struct, array types when searching for fields
@ 2023-10-23 22:00 Dave Marchevsky
2023-10-23 22:00 ` [PATCH v1 bpf-next 1/4] bpf: Fix btf_get_field_type to fail for multiple bpf_refcount fields Dave Marchevsky
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Dave Marchevsky @ 2023-10-23 22:00 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team, Dave Marchevsky
Consider the following BPF program:
struct val {
int d;
struct some_kptr __kptr *ref_ptr;
};
struct array_map {
__uint(type, BPF_MAP_TYPE_ARRAY);
__type(key, int);
__type(value, struct val);
__uint(max_entries, 10);
} array_map SEC(".maps");
__hidden struct val global_array[25];
void bpf_prog(void *ctx)
{
struct val *map_val;
struct some_kptr *p;
int idx = 0;
map_val = bpf_map_lookup_elem(&array_map, &idx);
if (!map_val) { /* omitted */ }
p = acquire_some_kptr();
if (!p) { /* omitted */ }
bpf_kptr_xchg(&map_val->ref_ptr, p);
bpf_kptr_xchg(&global_array[20].ref_ptr, p);
}
One would expect both bpf_kptr_xchg's to be possible, but currently
only the first one works. From BPF program writer's perspective this
is unexpected - the array map is an array with struct val elements, so
is global_array, so why the difference in behavior? The confusion is
not hypothetical - we stumbled onto this confusing situation while
developing scheduling BPF programs for the sched_ext project [0].
The key difference is that libbpf-style global vars like global_array
are implemented as single-elem MAP_TYPE_ARRAYs with the value type
holding the vars. e.g. from the verifier's perspective, our global
vars have a value type like
struct bss_map_value {
struct var global_array[25];
};
Note: In BTF it's actually a DATASEC with VARs instead of a STRUCT
with some members. For brevity's sake, when this series refers to the
toplevel type being searched for fields, 'struct' is shorthand for
{struct, datasec} and similarly 'struct members' for {struct members,
datasec vars}.
When creating a map, the verifier looks for special fields in the
value type. Currently, only the type's direct members are examined.
For example, struct val above has ref_ptr field with a special tag,
while bss_map_value's global_array field is has the uninteresting
BTF_KIND_ARRAY type. The verifier doesn't currently examine the
array's elem type - struct var - to look for special fields. The same
issue occurs when examining struct fields as well, e.g. if struct val
were instead:
struct val2 {
int d;
struct some_kptr __kptr *ref_ptr;
};
struct val {
int x;
struct val2 v;
};
then xchg'ing with array_map would look like
bpf_kptr_xchg(&mapval->v.ref_ptr, p);
and would fail for the same reason: as far as verifier is concerned,
struct val doesn't have a kptr field at that offset.
This series adds logic to descend into aggregate types while searching
for special fields. The meat of said search logic occurs in
btf_find_field and its helpers. btf_find_field returns a list of
struct btf_field_info's with information about each field's properties
and location. If the search finds btf_field_infos while descending
into an aggregate type, they're "flattened" into the btf_field_info
list. "Flattening" here means that these two types will have the same
btf_field_info list:
struct x {
int a;
struct some_kptr __kptr *inner[100];
};
struct y {
int a;
struct some_kptr __kptr *inner_1;
struct some_kptr __kptr *inner_2;
/* inner 3-99 snipped */
struct some_kptr __kptr *inner_100;
};
Namely, struct x's btf_find_field search will produce btf_field_info
list as if it were struct y. There's no indication of hierarchy or
nestedness in struct x's btf_field_info list, just (offset, type)
pairs as if struct x directly contained 100 kptrs.
Multi-dimensional arrays are supported as well e.g.
struct z {
int a;
struct some_kptr __kptr *inner[10][10];
}
will have the same btf_field_infos as struct y above.
This flattening approach allows us to find fields in nested aggregate
types without recreating the BTF type graph, which is something we
want to avoid. If we instead modified btf_find_field to return a
hierarchical representation of field info, we'd also have to add
functionality similar to btf_struct_walk to the query path for
btf_records. The goal of btf_record is to answer questions about
special fields more efficiently than walking BTF type graph, so
keeping things simple and flat is preferable.
Some field types expect to be in the same struct as another field,
which doesn't play well with a flattened btf_field_info list. Consider
this example:
struct root_and_lock {
struct bpf_rb_root r;
struct bpf_spin_lock l;
};
__hidden struct root_and_lock global_array[10];
global_array's btf_field_infos and btf_record will indicate that it
has 10 bpf_spin_lock fields, which will cause btf_parse_fields to fail
and no btf_record to be associated with global_array. In the future we
can modify struct btf_field_info to explicitly tie a specific
bpf_spin_lock field to a bpf_rb_root.
For fields without such expectations, though, this series' approach
works. Our current usecase for this is to find nested kptrs only, so
nested struct digging is only enabled to look for kptr fields in this
series. We can also consider enabling digging for bpf_{rb,list}_node
fields.
[0]: https://lore.kernel.org/bpf/20230711011412.100319-1-tj@kernel.org/
Dave Marchevsky (4):
bpf: Fix btf_get_field_type to fail for multiple bpf_refcount fields
bpf: Refactor btf_find_field with btf_field_info_search
btf: Descend into structs and arrays during special field search
selftests/bpf: Add tests exercising aggregate type BTF field search
include/linux/bpf.h | 4 +-
kernel/bpf/btf.c | 483 +++++++++++++-----
.../selftests/bpf/prog_tests/array_kptr.c | 12 +
.../testing/selftests/bpf/progs/array_kptr.c | 179 +++++++
4 files changed, 550 insertions(+), 128 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/array_kptr.c
create mode 100644 tools/testing/selftests/bpf/progs/array_kptr.c
--
2.34.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v1 bpf-next 1/4] bpf: Fix btf_get_field_type to fail for multiple bpf_refcount fields
2023-10-23 22:00 [PATCH v1 bpf-next 0/4] Descend into struct, array types when searching for fields Dave Marchevsky
@ 2023-10-23 22:00 ` Dave Marchevsky
2023-10-30 17:56 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
2023-10-23 22:00 ` [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search Dave Marchevsky
` (2 subsequent siblings)
3 siblings, 2 replies; 18+ messages in thread
From: Dave Marchevsky @ 2023-10-23 22:00 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team, Dave Marchevsky
If a struct has a bpf_refcount field, the refcount controls lifetime of
the entire struct. Currently there's no usecase or support for multiple
bpf_refcount fields in a struct.
bpf_spin_lock and bpf_timer fields don't support multiples either, but
with better error behavior. Parsing BTF w/ a struct containing multiple
{bpf_spin_lock, bpf_timer} fields fails in btf_get_field_type, while
multiple bpf_refcount fields doesn't fail BTF parsing at all, instead
triggering a WARN_ON_ONCE in btf_parse_fields, with the verifier using
the last bpf_refcount field to actually do refcounting.
This patch changes bpf_refcount handling in btf_get_field_type to use
same error logic as bpf_spin_lock and bpf_timer. Since it's being used
3x and is boilerplatey, the logic is shoved into
field_mask_test_name_check_seen helper macro.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Fixes: d54730b50bae ("bpf: Introduce opaque bpf_refcount struct and add btf_record plumbing")
---
kernel/bpf/btf.c | 37 ++++++++++++++++---------------------
1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 15d71d2986d3..975ef8e73393 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3374,8 +3374,17 @@ btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
return BTF_FIELD_FOUND;
}
-#define field_mask_test_name(field_type, field_type_str) \
- if (field_mask & field_type && !strcmp(name, field_type_str)) { \
+#define field_mask_test_name(field_type, field_type_str) \
+ if (field_mask & field_type && !strcmp(name, field_type_str)) { \
+ type = field_type; \
+ goto end; \
+ }
+
+#define field_mask_test_name_check_seen(field_type, field_type_str) \
+ if (field_mask & field_type && !strcmp(name, field_type_str)) { \
+ if (*seen_mask & field_type) \
+ return -E2BIG; \
+ *seen_mask |= field_type; \
type = field_type; \
goto end; \
}
@@ -3385,29 +3394,14 @@ static int btf_get_field_type(const char *name, u32 field_mask, u32 *seen_mask,
{
int type = 0;
- if (field_mask & BPF_SPIN_LOCK) {
- if (!strcmp(name, "bpf_spin_lock")) {
- if (*seen_mask & BPF_SPIN_LOCK)
- return -E2BIG;
- *seen_mask |= BPF_SPIN_LOCK;
- type = BPF_SPIN_LOCK;
- goto end;
- }
- }
- if (field_mask & BPF_TIMER) {
- if (!strcmp(name, "bpf_timer")) {
- if (*seen_mask & BPF_TIMER)
- return -E2BIG;
- *seen_mask |= BPF_TIMER;
- type = BPF_TIMER;
- goto end;
- }
- }
+ field_mask_test_name_check_seen(BPF_SPIN_LOCK, "bpf_spin_lock");
+ field_mask_test_name_check_seen(BPF_TIMER, "bpf_timer");
+ field_mask_test_name_check_seen(BPF_REFCOUNT, "bpf_refcount");
+
field_mask_test_name(BPF_LIST_HEAD, "bpf_list_head");
field_mask_test_name(BPF_LIST_NODE, "bpf_list_node");
field_mask_test_name(BPF_RB_ROOT, "bpf_rb_root");
field_mask_test_name(BPF_RB_NODE, "bpf_rb_node");
- field_mask_test_name(BPF_REFCOUNT, "bpf_refcount");
/* Only return BPF_KPTR when all other types with matchable names fail */
if (field_mask & BPF_KPTR) {
@@ -3421,6 +3415,7 @@ static int btf_get_field_type(const char *name, u32 field_mask, u32 *seen_mask,
return type;
}
+#undef field_mask_test_name_check_seen
#undef field_mask_test_name
static int btf_find_struct_field(const struct btf *btf,
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search
2023-10-23 22:00 [PATCH v1 bpf-next 0/4] Descend into struct, array types when searching for fields Dave Marchevsky
2023-10-23 22:00 ` [PATCH v1 bpf-next 1/4] bpf: Fix btf_get_field_type to fail for multiple bpf_refcount fields Dave Marchevsky
@ 2023-10-23 22:00 ` Dave Marchevsky
2023-10-28 14:52 ` Jiri Olsa
` (2 more replies)
2023-10-23 22:00 ` [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search Dave Marchevsky
2023-10-23 22:00 ` [PATCH v1 bpf-next 4/4] selftests/bpf: Add tests exercising aggregate type BTF " Dave Marchevsky
3 siblings, 3 replies; 18+ messages in thread
From: Dave Marchevsky @ 2023-10-23 22:00 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team, Dave Marchevsky
btf_find_field takes (btf_type, special_field_types) and returns info
about the specific special fields in btf_type, in the form of an array
of struct btf_field info. The meat of this 'search for special fields'
process happens in btf_find_datasec_var and btf_find_struct_field
helpers: each member is examined and, if it's special, a struct
btf_field_info describing it is added to the return array. Indeed, any
function that might add to the output probably also looks at struct
members or datasec vars.
Most of the parameters passed around between helpers doing the search
can be grouped into two categories: "info about the output array" and
"info about which fields to search for". This patch joins those together
in struct btf_field_info_search, simplifying the signatures of most
helpers involved in the search, including array flattening helper that
later patches in the series will add.
The aforementioned array flattening logic will greatly increase the
number of btf_field_info's needed to describe some structs, so this
patch also turns the statically-sized struct btf_field_info
info_arr[BTF_FIELDS_MAX] into a growable array with a larger max size.
Implementation notes:
* BTF_FIELDS_MAX is now max size of growable btf_field_info *infos
instead of initial (and max) size of static result array
* Static array before had 10 elems (+1 tmp btf_field_info)
* growable array starts with 16 and doubles every time it needs to
grow, up to BTF_FIELDS_MAX of 256
* __next_field_infos is used with next_cnt > 1 later in the series
* btf_find_{datasec_var, struct_field} have special logic for an edge
case where the result array is full but the field being examined
gets BTF_FIELD_IGNORE return from btf_find_{struct, kptr,graph_root}
* If result wasn't BTF_FIELD_IGNORE, a btf_field_info would have to
be added to the array. Since it is we can look at next field.
* Before this patch the logic handling this edge case was hard to
follow and used a tmp btf_struct_info. This patch moves the
add-if-not-ignore logic down into btf_find_{struct, kptr,
graph_root}, removing the need to opportunistically grab a
btf_field_info to populate before knowing if it's actually
necessary. Now a new one is grabbed only if the field shouldn't
be ignored.
* Within btf_find_{datasec_var, struct_field}, each member is
currently examined in two phases: first btf_get_field_type checks
the member type name, then btf_find_{struct,graph_root,kptr} do
additional sanity checking and populate struct btf_field_info. Kptr
fields don't have a specific type name, though, so
btf_get_field_type assumes that - if we're looking for kptrs - any
member that fails type name check could be a kptr field.
* As a result btf_find_kptr effectively does all the pointer
hopping, sanity checking, and info population. Instead of trying
to fit kptr handling into this two-phase model, where it's
unwieldy, handle it in a separate codepath when name matching
fails.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
include/linux/bpf.h | 4 +-
kernel/bpf/btf.c | 331 +++++++++++++++++++++++++++++---------------
2 files changed, 219 insertions(+), 116 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index b4825d3cdb29..e07cac5cc3cf 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -171,8 +171,8 @@ struct bpf_map_ops {
};
enum {
- /* Support at most 10 fields in a BTF type */
- BTF_FIELDS_MAX = 10,
+ /* Support at most 256 fields in a BTF type */
+ BTF_FIELDS_MAX = 256,
};
enum btf_field_type {
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 975ef8e73393..e999ba85c363 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3257,25 +3257,94 @@ struct btf_field_info {
};
};
+struct btf_field_info_search {
+ /* growable array. allocated in __next_field_infos
+ * free'd in btf_parse_fields
+ */
+ struct btf_field_info *infos;
+ /* size of infos */
+ int info_cnt;
+ /* index of next btf_field_info to populate */
+ int idx;
+
+ /* btf_field_types to search for */
+ u32 field_mask;
+ /* btf_field_types found earlier */
+ u32 seen_mask;
+};
+
+/* Reserve next_cnt contiguous btf_field_info's for caller to populate
+ * Returns ptr to first reserved btf_field_info
+ */
+static struct btf_field_info *__next_field_infos(struct btf_field_info_search *srch,
+ u32 next_cnt)
+{
+ struct btf_field_info *new_infos, *ret;
+
+ if (!next_cnt)
+ return ERR_PTR(-EINVAL);
+
+ if (srch->idx + next_cnt < srch->info_cnt)
+ goto nogrow_out;
+
+ /* Need to grow */
+ if (srch->idx + next_cnt > BTF_FIELDS_MAX)
+ return ERR_PTR(-E2BIG);
+
+ while (srch->idx + next_cnt >= srch->info_cnt)
+ srch->info_cnt = srch->infos ? srch->info_cnt * 2 : 16;
+
+ new_infos = krealloc(srch->infos,
+ srch->info_cnt * sizeof(struct btf_field_info),
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!new_infos)
+ return ERR_PTR(-ENOMEM);
+ srch->infos = new_infos;
+
+nogrow_out:
+ ret = &srch->infos[srch->idx];
+ srch->idx += next_cnt;
+ return ret;
+}
+
+/* Request srch's next free btf_field_info to populate, possibly growing
+ * srch->infos
+ */
+static struct btf_field_info *__next_field_info(struct btf_field_info_search *srch)
+{
+ return __next_field_infos(srch, 1);
+}
+
static int btf_find_struct(const struct btf *btf, const struct btf_type *t,
u32 off, int sz, enum btf_field_type field_type,
- struct btf_field_info *info)
+ struct btf_field_info_search *srch)
{
+ struct btf_field_info *info;
+
if (!__btf_type_is_struct(t))
return BTF_FIELD_IGNORE;
if (t->size != sz)
return BTF_FIELD_IGNORE;
+
+ info = __next_field_info(srch);
+ if (IS_ERR_OR_NULL(info))
+ return PTR_ERR(info);
+
info->type = field_type;
info->off = off;
return BTF_FIELD_FOUND;
}
-static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
- u32 off, int sz, struct btf_field_info *info)
+static int btf_maybe_find_kptr(const struct btf *btf, const struct btf_type *t,
+ u32 off, struct btf_field_info_search *srch)
{
+ struct btf_field_info *info;
enum btf_field_type type;
u32 res_id;
+ if (!(srch->field_mask & BPF_KPTR))
+ return BTF_FIELD_IGNORE;
+
/* Permit modifiers on the pointer itself */
if (btf_type_is_volatile(t))
t = btf_type_by_id(btf, t->type);
@@ -3304,6 +3373,10 @@ static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
if (!__btf_type_is_struct(t))
return -EINVAL;
+ info = __next_field_info(srch);
+ if (IS_ERR_OR_NULL(info))
+ return PTR_ERR(info);
+
info->type = type;
info->off = off;
info->kptr.type_id = res_id;
@@ -3340,9 +3413,10 @@ const char *btf_find_decl_tag_value(const struct btf *btf, const struct btf_type
static int
btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
const struct btf_type *t, int comp_idx, u32 off,
- int sz, struct btf_field_info *info,
+ int sz, struct btf_field_info_search *srch,
enum btf_field_type head_type)
{
+ struct btf_field_info *info;
const char *node_field_name;
const char *value_type;
s32 id;
@@ -3367,6 +3441,11 @@ btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
node_field_name++;
if (str_is_empty(node_field_name))
return -EINVAL;
+
+ info = __next_field_info(srch);
+ if (IS_ERR_OR_NULL(info))
+ return PTR_ERR(info);
+
info->type = head_type;
info->off = off;
info->graph_root.value_btf_id = id;
@@ -3374,25 +3453,24 @@ btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
return BTF_FIELD_FOUND;
}
-#define field_mask_test_name(field_type, field_type_str) \
- if (field_mask & field_type && !strcmp(name, field_type_str)) { \
- type = field_type; \
- goto end; \
+#define field_mask_test_name(field_type, field_type_str) \
+ if (srch->field_mask & field_type && !strcmp(name, field_type_str)) { \
+ return field_type; \
}
-#define field_mask_test_name_check_seen(field_type, field_type_str) \
- if (field_mask & field_type && !strcmp(name, field_type_str)) { \
- if (*seen_mask & field_type) \
- return -E2BIG; \
- *seen_mask |= field_type; \
- type = field_type; \
- goto end; \
+#define field_mask_test_name_check_seen(field_type, field_type_str) \
+ if (srch->field_mask & field_type && !strcmp(name, field_type_str)) { \
+ if (srch->seen_mask & field_type) \
+ return -E2BIG; \
+ srch->seen_mask |= field_type; \
+ return field_type; \
}
-static int btf_get_field_type(const char *name, u32 field_mask, u32 *seen_mask,
- int *align, int *sz)
+static int btf_get_field_type_by_name(const struct btf *btf,
+ const struct btf_type *t,
+ struct btf_field_info_search *srch)
{
- int type = 0;
+ const char *name = __btf_name_by_offset(btf, t->name_off);
field_mask_test_name_check_seen(BPF_SPIN_LOCK, "bpf_spin_lock");
field_mask_test_name_check_seen(BPF_TIMER, "bpf_timer");
@@ -3403,47 +3481,58 @@ static int btf_get_field_type(const char *name, u32 field_mask, u32 *seen_mask,
field_mask_test_name(BPF_RB_ROOT, "bpf_rb_root");
field_mask_test_name(BPF_RB_NODE, "bpf_rb_node");
- /* Only return BPF_KPTR when all other types with matchable names fail */
- if (field_mask & BPF_KPTR) {
- type = BPF_KPTR_REF;
- goto end;
- }
return 0;
-end:
- *sz = btf_field_type_size(type);
- *align = btf_field_type_align(type);
- return type;
}
#undef field_mask_test_name_check_seen
#undef field_mask_test_name
+static int __struct_member_check_align(u32 off, enum btf_field_type field_type)
+{
+ u32 align = btf_field_type_align(field_type);
+
+ if (off % align)
+ return -EINVAL;
+ return 0;
+}
+
static int btf_find_struct_field(const struct btf *btf,
- const struct btf_type *t, u32 field_mask,
- struct btf_field_info *info, int info_cnt)
+ const struct btf_type *t,
+ struct btf_field_info_search *srch)
{
- int ret, idx = 0, align, sz, field_type;
const struct btf_member *member;
- struct btf_field_info tmp;
- u32 i, off, seen_mask = 0;
+ int ret, field_type;
+ u32 i, off, sz;
for_each_member(i, t, member) {
const struct btf_type *member_type = btf_type_by_id(btf,
member->type);
-
- field_type = btf_get_field_type(__btf_name_by_offset(btf, member_type->name_off),
- field_mask, &seen_mask, &align, &sz);
- if (field_type == 0)
- continue;
- if (field_type < 0)
- return field_type;
-
off = __btf_member_bit_offset(t, member);
if (off % 8)
/* valid C code cannot generate such BTF */
return -EINVAL;
off /= 8;
- if (off % align)
+
+ field_type = btf_get_field_type_by_name(btf, member_type, srch);
+ if (field_type < 0)
+ return field_type;
+
+ if (field_type == 0) {
+ /* Maybe it's a kptr. Use BPF_KPTR_REF for align
+ * checks, all ptrs have same align.
+ * btf_maybe_find_kptr will find actual kptr type
+ */
+ if (__struct_member_check_align(off, BPF_KPTR_REF))
+ continue;
+
+ ret = btf_maybe_find_kptr(btf, member_type, off, srch);
+ if (ret < 0)
+ return ret;
+ continue;
+ }
+
+ sz = btf_field_type_size(field_type);
+ if (__struct_member_check_align(off, field_type))
continue;
switch (field_type) {
@@ -3453,64 +3542,81 @@ static int btf_find_struct_field(const struct btf *btf,
case BPF_RB_NODE:
case BPF_REFCOUNT:
ret = btf_find_struct(btf, member_type, off, sz, field_type,
- idx < info_cnt ? &info[idx] : &tmp);
- if (ret < 0)
- return ret;
- break;
- case BPF_KPTR_UNREF:
- case BPF_KPTR_REF:
- case BPF_KPTR_PERCPU:
- ret = btf_find_kptr(btf, member_type, off, sz,
- idx < info_cnt ? &info[idx] : &tmp);
+ srch);
if (ret < 0)
return ret;
break;
case BPF_LIST_HEAD:
case BPF_RB_ROOT:
ret = btf_find_graph_root(btf, t, member_type,
- i, off, sz,
- idx < info_cnt ? &info[idx] : &tmp,
- field_type);
+ i, off, sz, srch, field_type);
if (ret < 0)
return ret;
break;
+ /* kptr fields are not handled in this switch, see
+ * btf_maybe_find_kptr above
+ */
+ case BPF_KPTR_UNREF:
+ case BPF_KPTR_REF:
+ case BPF_KPTR_PERCPU:
default:
return -EFAULT;
}
-
- if (ret == BTF_FIELD_IGNORE)
- continue;
- if (idx >= info_cnt)
- return -E2BIG;
- ++idx;
}
- return idx;
+ return srch->idx;
+}
+
+static int __datasec_vsi_check_align_sz(const struct btf_var_secinfo *vsi,
+ enum btf_field_type field_type,
+ u32 expected_sz)
+{
+ u32 off, align;
+
+ off = vsi->offset;
+ align = btf_field_type_align(field_type);
+
+ if (vsi->size != expected_sz)
+ return -EINVAL;
+ if (off % align)
+ return -EINVAL;
+
+ return 0;
}
static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
- u32 field_mask, struct btf_field_info *info,
- int info_cnt)
+ struct btf_field_info_search *srch)
{
- int ret, idx = 0, align, sz, field_type;
const struct btf_var_secinfo *vsi;
- struct btf_field_info tmp;
- u32 i, off, seen_mask = 0;
+ int ret, field_type;
+ u32 i, off, sz;
for_each_vsi(i, t, vsi) {
const struct btf_type *var = btf_type_by_id(btf, vsi->type);
const struct btf_type *var_type = btf_type_by_id(btf, var->type);
- field_type = btf_get_field_type(__btf_name_by_offset(btf, var_type->name_off),
- field_mask, &seen_mask, &align, &sz);
- if (field_type == 0)
- continue;
+ off = vsi->offset;
+ field_type = btf_get_field_type_by_name(btf, var_type, srch);
if (field_type < 0)
return field_type;
- off = vsi->offset;
- if (vsi->size != sz)
+ if (field_type == 0) {
+ /* Maybe it's a kptr. Use BPF_KPTR_REF for sz / align
+ * checks, all ptrs have same sz / align.
+ * btf_maybe_find_kptr will find actual kptr type
+ */
+ sz = btf_field_type_size(BPF_KPTR_REF);
+ if (__datasec_vsi_check_align_sz(vsi, BPF_KPTR_REF, sz))
+ continue;
+
+ ret = btf_maybe_find_kptr(btf, var_type, off, srch);
+ if (ret < 0)
+ return ret;
continue;
- if (off % align)
+ }
+
+ sz = btf_field_type_size(field_type);
+
+ if (__datasec_vsi_check_align_sz(vsi, field_type, sz))
continue;
switch (field_type) {
@@ -3520,48 +3626,38 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
case BPF_RB_NODE:
case BPF_REFCOUNT:
ret = btf_find_struct(btf, var_type, off, sz, field_type,
- idx < info_cnt ? &info[idx] : &tmp);
- if (ret < 0)
- return ret;
- break;
- case BPF_KPTR_UNREF:
- case BPF_KPTR_REF:
- case BPF_KPTR_PERCPU:
- ret = btf_find_kptr(btf, var_type, off, sz,
- idx < info_cnt ? &info[idx] : &tmp);
+ srch);
if (ret < 0)
return ret;
break;
case BPF_LIST_HEAD:
case BPF_RB_ROOT:
ret = btf_find_graph_root(btf, var, var_type,
- -1, off, sz,
- idx < info_cnt ? &info[idx] : &tmp,
+ -1, off, sz, srch,
field_type);
if (ret < 0)
return ret;
break;
+ /* kptr fields are not handled in this switch, see
+ * btf_maybe_find_kptr above
+ */
+ case BPF_KPTR_UNREF:
+ case BPF_KPTR_REF:
+ case BPF_KPTR_PERCPU:
default:
return -EFAULT;
}
-
- if (ret == BTF_FIELD_IGNORE)
- continue;
- if (idx >= info_cnt)
- return -E2BIG;
- ++idx;
}
- return idx;
+ return srch->idx;
}
static int btf_find_field(const struct btf *btf, const struct btf_type *t,
- u32 field_mask, struct btf_field_info *info,
- int info_cnt)
+ struct btf_field_info_search *srch)
{
if (__btf_type_is_struct(t))
- return btf_find_struct_field(btf, t, field_mask, info, info_cnt);
+ return btf_find_struct_field(btf, t, srch);
else if (btf_type_is_datasec(t))
- return btf_find_datasec_var(btf, t, field_mask, info, info_cnt);
+ return btf_find_datasec_var(btf, t, srch);
return -EINVAL;
}
@@ -3729,47 +3825,51 @@ static int btf_field_cmp(const void *_a, const void *_b, const void *priv)
struct btf_record *btf_parse_fields(const struct btf *btf, const struct btf_type *t,
u32 field_mask, u32 value_size)
{
- struct btf_field_info info_arr[BTF_FIELDS_MAX];
+ struct btf_field_info_search srch;
u32 next_off = 0, field_type_size;
+ struct btf_field_info *info;
struct btf_record *rec;
int ret, i, cnt;
- ret = btf_find_field(btf, t, field_mask, info_arr, ARRAY_SIZE(info_arr));
- if (ret < 0)
- return ERR_PTR(ret);
- if (!ret)
- return NULL;
+ memset(&srch, 0, sizeof(srch));
+ srch.field_mask = field_mask;
+ ret = btf_find_field(btf, t, &srch);
+ if (ret <= 0)
+ goto end_srch;
cnt = ret;
/* This needs to be kzalloc to zero out padding and unused fields, see
* comment in btf_record_equal.
*/
rec = kzalloc(offsetof(struct btf_record, fields[cnt]), GFP_KERNEL | __GFP_NOWARN);
- if (!rec)
- return ERR_PTR(-ENOMEM);
+ if (!rec) {
+ ret = -ENOMEM;
+ goto end_srch;
+ }
rec->spin_lock_off = -EINVAL;
rec->timer_off = -EINVAL;
rec->refcount_off = -EINVAL;
for (i = 0; i < cnt; i++) {
- field_type_size = btf_field_type_size(info_arr[i].type);
- if (info_arr[i].off + field_type_size > value_size) {
- WARN_ONCE(1, "verifier bug off %d size %d", info_arr[i].off, value_size);
+ info = &srch.infos[i];
+ field_type_size = btf_field_type_size(info->type);
+ if (info->off + field_type_size > value_size) {
+ WARN_ONCE(1, "verifier bug off %d size %d", info->off, value_size);
ret = -EFAULT;
goto end;
}
- if (info_arr[i].off < next_off) {
+ if (info->off < next_off) {
ret = -EEXIST;
goto end;
}
- next_off = info_arr[i].off + field_type_size;
+ next_off = info->off + field_type_size;
- rec->field_mask |= info_arr[i].type;
- rec->fields[i].offset = info_arr[i].off;
- rec->fields[i].type = info_arr[i].type;
+ rec->field_mask |= info->type;
+ rec->fields[i].offset = info->off;
+ rec->fields[i].type = info->type;
rec->fields[i].size = field_type_size;
- switch (info_arr[i].type) {
+ switch (info->type) {
case BPF_SPIN_LOCK:
WARN_ON_ONCE(rec->spin_lock_off >= 0);
/* Cache offset for faster lookup at runtime */
@@ -3788,17 +3888,17 @@ struct btf_record *btf_parse_fields(const struct btf *btf, const struct btf_type
case BPF_KPTR_UNREF:
case BPF_KPTR_REF:
case BPF_KPTR_PERCPU:
- ret = btf_parse_kptr(btf, &rec->fields[i], &info_arr[i]);
+ ret = btf_parse_kptr(btf, &rec->fields[i], info);
if (ret < 0)
goto end;
break;
case BPF_LIST_HEAD:
- ret = btf_parse_list_head(btf, &rec->fields[i], &info_arr[i]);
+ ret = btf_parse_list_head(btf, &rec->fields[i], info);
if (ret < 0)
goto end;
break;
case BPF_RB_ROOT:
- ret = btf_parse_rb_root(btf, &rec->fields[i], &info_arr[i]);
+ ret = btf_parse_rb_root(btf, &rec->fields[i], info);
if (ret < 0)
goto end;
break;
@@ -3828,10 +3928,13 @@ struct btf_record *btf_parse_fields(const struct btf *btf, const struct btf_type
sort_r(rec->fields, rec->cnt, sizeof(struct btf_field), btf_field_cmp,
NULL, rec);
+ kfree(srch.infos);
return rec;
end:
btf_record_free(rec);
+end_srch:
+ kfree(srch.infos);
return ERR_PTR(ret);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search
2023-10-23 22:00 [PATCH v1 bpf-next 0/4] Descend into struct, array types when searching for fields Dave Marchevsky
2023-10-23 22:00 ` [PATCH v1 bpf-next 1/4] bpf: Fix btf_get_field_type to fail for multiple bpf_refcount fields Dave Marchevsky
2023-10-23 22:00 ` [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search Dave Marchevsky
@ 2023-10-23 22:00 ` Dave Marchevsky
2023-10-26 1:24 ` kernel test robot
` (3 more replies)
2023-10-23 22:00 ` [PATCH v1 bpf-next 4/4] selftests/bpf: Add tests exercising aggregate type BTF " Dave Marchevsky
3 siblings, 4 replies; 18+ messages in thread
From: Dave Marchevsky @ 2023-10-23 22:00 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team, Dave Marchevsky
Structs and arrays are aggregate types which contain some inner
type(s) - members and elements - at various offsets. Currently, when
examining a struct or datasec for special fields, the verifier does
not look into the inner type of the structs or arrays it contains.
This patch adds logic to descend into struct and array types when
searching for special fields.
If we have struct x containing an array:
struct x {
int a;
u64 b[10];
};
we can construct some struct y with no array or struct members that
has the same types at the same offsets:
struct y {
int a;
u64 b1;
u64 b2;
/* ... */
u64 b10;
};
Similarly for a struct containing a struct:
struct x {
char a;
struct {
int b;
u64 c;
} inner;
};
there's a struct y with no aggregate members and same types/offsets:
struct y {
char a;
int inner_b __attribute__ ((aligned (8))); /* See [0] */
u64 inner_c __attribute__ ((aligned (8)));
};
This patch takes advantage of this equivalence to 'flatten' the
field info found while descending into struct or array members into
the btf_field_info result array of the original type being examined.
The resultant btf_record of the original type being searched will
have the correct fields at the correct offsets, but without any
differentiation between "this field is one of my members" and "this
field is actually in my some struct / array member".
For now this descendant search logic looks for kptr fields only.
Implementation notes:
* Search starts at btf_find_field - we're either looking at a struct
that's the type of some mapval (btf_find_struct_field), or a
datasec representing a .bss or .data map (btf_find_datasec_var).
Newly-added btf_find_aggregate_field is a "disambiguation helper"
like btf_find_field, but is meant to be called from one of the
starting points of the search - btf_find_{struct_field,
datasec_var}.
* btf_find_aggregate_field may itself call btf_find_struct_field,
so there's some recursive digging possible here
* Newly-added btf_flatten_array_field handles array fields by
finding the type of their element and continuing the dig based on
elem type.
[0]: Structs have the alignment of their largest field, so the
explicit alignment is necessary here. Luckily this patch's
changes don't need to care about alignment / padding, since
the BTF created during compilation is being searched, and
it already has the correct information.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
kernel/bpf/btf.c | 151 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 142 insertions(+), 9 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index e999ba85c363..b982bf6fef9d 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3496,9 +3496,41 @@ static int __struct_member_check_align(u32 off, enum btf_field_type field_type)
return 0;
}
+/* Return number of elems and elem_type of a btf_array
+ *
+ * If the array is multi-dimensional, return elem count of
+ * equivalent single-dimensional array
+ * e.g. int x[10][10][10] has same layout as int x[1000]
+ */
+static u32 __multi_dim_elem_type_nelems(const struct btf *btf,
+ const struct btf_type *t,
+ const struct btf_type **elem_type)
+{
+ u32 nelems = btf_array(t)->nelems;
+
+ if (!nelems)
+ return 0;
+
+ *elem_type = btf_type_by_id(btf, btf_array(t)->type);
+
+ while (btf_type_is_array(*elem_type)) {
+ if (!btf_array(*elem_type)->nelems)
+ return 0;
+ nelems *= btf_array(*elem_type)->nelems;
+ *elem_type = btf_type_by_id(btf, btf_array(*elem_type)->type);
+ }
+ return nelems;
+}
+
+static int btf_find_aggregate_field(const struct btf *btf,
+ const struct btf_type *t,
+ struct btf_field_info_search *srch,
+ int field_off, int rec);
+
static int btf_find_struct_field(const struct btf *btf,
const struct btf_type *t,
- struct btf_field_info_search *srch)
+ struct btf_field_info_search *srch,
+ int struct_field_off, int rec)
{
const struct btf_member *member;
int ret, field_type;
@@ -3522,10 +3554,24 @@ static int btf_find_struct_field(const struct btf *btf,
* checks, all ptrs have same align.
* btf_maybe_find_kptr will find actual kptr type
*/
- if (__struct_member_check_align(off, BPF_KPTR_REF))
+ if (srch->field_mask & BPF_KPTR &&
+ !__struct_member_check_align(off, BPF_KPTR_REF)) {
+ ret = btf_maybe_find_kptr(btf, member_type,
+ struct_field_off + off,
+ srch);
+ if (ret < 0)
+ return ret;
+ if (ret == BTF_FIELD_FOUND)
+ continue;
+ }
+
+ if (!(btf_type_is_array(member_type) ||
+ __btf_type_is_struct(member_type)))
continue;
- ret = btf_maybe_find_kptr(btf, member_type, off, srch);
+ ret = btf_find_aggregate_field(btf, member_type, srch,
+ struct_field_off + off,
+ rec);
if (ret < 0)
return ret;
continue;
@@ -3541,15 +3587,17 @@ static int btf_find_struct_field(const struct btf *btf,
case BPF_LIST_NODE:
case BPF_RB_NODE:
case BPF_REFCOUNT:
- ret = btf_find_struct(btf, member_type, off, sz, field_type,
- srch);
+ ret = btf_find_struct(btf, member_type,
+ struct_field_off + off,
+ sz, field_type, srch);
if (ret < 0)
return ret;
break;
case BPF_LIST_HEAD:
case BPF_RB_ROOT:
ret = btf_find_graph_root(btf, t, member_type,
- i, off, sz, srch, field_type);
+ i, struct_field_off + off, sz,
+ srch, field_type);
if (ret < 0)
return ret;
break;
@@ -3566,6 +3614,82 @@ static int btf_find_struct_field(const struct btf *btf,
return srch->idx;
}
+static int btf_flatten_array_field(const struct btf *btf,
+ const struct btf_type *t,
+ struct btf_field_info_search *srch,
+ int array_field_off, int rec)
+{
+ int ret, start_idx, elem_field_cnt;
+ const struct btf_type *elem_type;
+ struct btf_field_info *info;
+ u32 i, j, off, nelems;
+
+ if (!btf_type_is_array(t))
+ return -EINVAL;
+ nelems = __multi_dim_elem_type_nelems(btf, t, &elem_type);
+ if (!nelems || !__btf_type_is_struct(elem_type))
+ return srch->idx;
+
+ start_idx = srch->idx;
+ ret = btf_find_struct_field(btf, elem_type, srch, array_field_off + off, rec);
+ if (ret < 0)
+ return ret;
+
+ /* No btf_field_info's added */
+ if (srch->idx == start_idx)
+ return srch->idx;
+
+ elem_field_cnt = srch->idx - start_idx;
+ info = __next_field_infos(srch, elem_field_cnt * (nelems - 1));
+ if (IS_ERR_OR_NULL(info))
+ return PTR_ERR(info);
+
+ /* Array elems after the first can copy first elem's btf_field_infos
+ * and adjust offset
+ */
+ for (i = 1; i < nelems; i++) {
+ memcpy(info, &srch->infos[start_idx],
+ elem_field_cnt * sizeof(struct btf_field_info));
+ for (j = 0; j < elem_field_cnt; j++) {
+ info->off += (i * elem_type->size);
+ info++;
+ }
+ }
+ return srch->idx;
+}
+
+static int btf_find_aggregate_field(const struct btf *btf,
+ const struct btf_type *t,
+ struct btf_field_info_search *srch,
+ int field_off, int rec)
+{
+ u32 orig_field_mask;
+ int ret;
+
+ /* Dig up to 4 levels deep */
+ if (rec >= 4)
+ return -E2BIG;
+
+ orig_field_mask = srch->field_mask;
+ srch->field_mask &= BPF_KPTR;
+
+ if (!srch->field_mask) {
+ ret = 0;
+ goto reset_field_mask;
+ }
+
+ if (__btf_type_is_struct(t))
+ ret = btf_find_struct_field(btf, t, srch, field_off, rec + 1);
+ else if (btf_type_is_array(t))
+ ret = btf_flatten_array_field(btf, t, srch, field_off, rec + 1);
+ else
+ ret = -EINVAL;
+
+reset_field_mask:
+ srch->field_mask = orig_field_mask;
+ return ret;
+}
+
static int __datasec_vsi_check_align_sz(const struct btf_var_secinfo *vsi,
enum btf_field_type field_type,
u32 expected_sz)
@@ -3605,10 +3729,19 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
* btf_maybe_find_kptr will find actual kptr type
*/
sz = btf_field_type_size(BPF_KPTR_REF);
- if (__datasec_vsi_check_align_sz(vsi, BPF_KPTR_REF, sz))
+ if (srch->field_mask & BPF_KPTR &&
+ !__datasec_vsi_check_align_sz(vsi, BPF_KPTR_REF, sz)) {
+ ret = btf_maybe_find_kptr(btf, var_type, off, srch);
+ if (ret < 0)
+ return ret;
+ if (ret == BTF_FIELD_FOUND)
+ continue;
+ }
+
+ if (!(btf_type_is_array(var_type) || __btf_type_is_struct(var_type)))
continue;
- ret = btf_maybe_find_kptr(btf, var_type, off, srch);
+ ret = btf_find_aggregate_field(btf, var_type, srch, off, 0);
if (ret < 0)
return ret;
continue;
@@ -3655,7 +3788,7 @@ static int btf_find_field(const struct btf *btf, const struct btf_type *t,
struct btf_field_info_search *srch)
{
if (__btf_type_is_struct(t))
- return btf_find_struct_field(btf, t, srch);
+ return btf_find_struct_field(btf, t, srch, 0, 0);
else if (btf_type_is_datasec(t))
return btf_find_datasec_var(btf, t, srch);
return -EINVAL;
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v1 bpf-next 4/4] selftests/bpf: Add tests exercising aggregate type BTF field search
2023-10-23 22:00 [PATCH v1 bpf-next 0/4] Descend into struct, array types when searching for fields Dave Marchevsky
` (2 preceding siblings ...)
2023-10-23 22:00 ` [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search Dave Marchevsky
@ 2023-10-23 22:00 ` Dave Marchevsky
2023-10-23 23:32 ` kernel test robot
` (2 more replies)
3 siblings, 3 replies; 18+ messages in thread
From: Dave Marchevsky @ 2023-10-23 22:00 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team, Dave Marchevsky
The newly-added test file attempts to kptr_xchg a prog_test_ref_kfunc
kptr into a kptr field in a variety of nested aggregate types. If the
verifier recognizes that there's a kptr field where we're trying to
kptr_xchg, then the aggregate type digging logic works as expected.
Some of the refactoring changes in this series are tested as well.
Specifically:
* BTF_FIELDS_MAX is now higher and represents the max size of the
growable array. Confirm that btf_parse_fields fails for a type which
contains too many fields.
* If we've already seen BTF_FIELDS_MAX fields, we should continue
looking for fields and fail if we find another one, otherwise the
search should succeed and return BTF_FIELDS_MAX btf_field_infos.
Confirm that this edge case works as expected.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
.../selftests/bpf/prog_tests/array_kptr.c | 12 ++
.../testing/selftests/bpf/progs/array_kptr.c | 179 ++++++++++++++++++
2 files changed, 191 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/array_kptr.c
create mode 100644 tools/testing/selftests/bpf/progs/array_kptr.c
diff --git a/tools/testing/selftests/bpf/prog_tests/array_kptr.c b/tools/testing/selftests/bpf/prog_tests/array_kptr.c
new file mode 100644
index 000000000000..9d088520bdfe
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/array_kptr.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+
+#include <test_progs.h>
+
+#include "array_kptr.skel.h"
+
+void test_array_kptr(void)
+{
+ if (env.has_testmod)
+ RUN_TESTS(array_kptr);
+}
diff --git a/tools/testing/selftests/bpf/progs/array_kptr.c b/tools/testing/selftests/bpf/progs/array_kptr.c
new file mode 100644
index 000000000000..f34872e74024
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/array_kptr.c
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_helpers.h>
+#include "../bpf_testmod/bpf_testmod_kfunc.h"
+#include "bpf_misc.h"
+
+struct val {
+ int d;
+ struct prog_test_ref_kfunc __kptr *ref_ptr;
+};
+
+struct val2 {
+ char c;
+ struct val v;
+};
+
+struct val_holder {
+ int e;
+ struct val2 first[2];
+ int f;
+ struct val second[2];
+};
+
+struct array_map {
+ __uint(type, BPF_MAP_TYPE_ARRAY);
+ __type(key, int);
+ __type(value, struct val);
+ __uint(max_entries, 10);
+} array_map SEC(".maps");
+
+struct array_map2 {
+ __uint(type, BPF_MAP_TYPE_ARRAY);
+ __type(key, int);
+ __type(value, struct val2);
+ __uint(max_entries, 10);
+} array_map2 SEC(".maps");
+
+__hidden struct val array[25];
+__hidden struct val double_array[5][5];
+__hidden struct val_holder double_holder_array[2][2];
+
+/* Some tests need their own section to force separate bss arraymap,
+ * otherwise above arrays wouldn't have btf_field_info either
+ */
+#define private(name) SEC(".bss." #name) __hidden __attribute__((aligned(8)))
+private(A) struct val array_too_big[300];
+
+private(B) struct val exactly_max_fields[256];
+private(B) int ints[50];
+
+SEC("tc")
+__success __retval(0)
+int test_arraymap(void *ctx)
+{
+ struct prog_test_ref_kfunc *p;
+ unsigned long dummy = 0;
+ struct val *v;
+ int idx = 0;
+
+ v = bpf_map_lookup_elem(&array_map, &idx);
+ if (!v)
+ return 1;
+
+ p = bpf_kfunc_call_test_acquire(&dummy);
+ if (!p)
+ return 2;
+
+ p = bpf_kptr_xchg(&v->ref_ptr, p);
+ if (p) {
+ bpf_kfunc_call_test_release(p);
+ return 3;
+ }
+
+ return 0;
+}
+
+SEC("tc")
+__success __retval(0)
+int test_arraymap2(void *ctx)
+{
+ struct prog_test_ref_kfunc *p;
+ unsigned long dummy = 0;
+ struct val2 *v;
+ int idx = 0;
+
+ v = bpf_map_lookup_elem(&array_map2, &idx);
+ if (!v)
+ return 1;
+
+ p = bpf_kfunc_call_test_acquire(&dummy);
+ if (!p)
+ return 2;
+
+ p = bpf_kptr_xchg(&v->v.ref_ptr, p);
+ if (p) {
+ bpf_kfunc_call_test_release(p);
+ return 3;
+ }
+
+ return 0;
+}
+
+/* elem must be contained within some mapval so it can be used as
+ * bpf_kptr_xchg's first param
+ */
+static __always_inline int test_array_xchg(struct val *elem)
+{
+ struct prog_test_ref_kfunc *p;
+ unsigned long dummy = 0;
+
+ p = bpf_kfunc_call_test_acquire(&dummy);
+ if (!p)
+ return 1;
+
+ p = bpf_kptr_xchg(&elem->ref_ptr, p);
+ if (p) {
+ bpf_kfunc_call_test_release(p);
+ return 2;
+ }
+
+ return 0;
+}
+
+SEC("tc")
+__success __retval(0)
+int test_array(void *ctx)
+{
+ return test_array_xchg(&array[10]);
+}
+
+SEC("tc")
+__success __retval(0)
+int test_double_array(void *ctx)
+{
+ /* array -> array -> struct -> kptr */
+ return test_array_xchg(&double_array[4][3]);
+}
+
+SEC("tc")
+__success __retval(0)
+int test_double_holder_array_first(void *ctx)
+{
+ /* array -> array -> struct -> array -> struct -> struct -> kptr */
+ return test_array_xchg(&double_holder_array[1][1].first[1].v);
+}
+
+SEC("tc")
+__success __retval(0)
+int test_double_holder_array_second(void *ctx)
+{
+ /* array -> array -> struct -> array -> struct -> kptr */
+ return test_array_xchg(&double_holder_array[1][1].second[1]);
+}
+
+SEC("tc")
+__success __retval(0)
+int test_exactly_max_fields(void *ctx)
+{
+ /* Edge case where verifier finds BTF_FIELDS_MAX fields. It should be
+ * safe to examine .bss.B's other array, and .bss.B will have a valid
+ * btf_record if no more fields are found
+ */
+ return test_array_xchg(&exactly_max_fields[255]);
+}
+
+SEC("tc")
+__failure __msg("map '.bss.A' has no valid kptr")
+int test_array_fail__too_big(void *ctx)
+{
+ /* array_too_big's btf_record parsing will fail due to the
+ * number of btf_field_infos being > BTF_FIELDS_MAX
+ */
+ return test_array_xchg(&array_too_big[50]);
+}
+
+char _license[] SEC("license") = "GPL";
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 4/4] selftests/bpf: Add tests exercising aggregate type BTF field search
2023-10-23 22:00 ` [PATCH v1 bpf-next 4/4] selftests/bpf: Add tests exercising aggregate type BTF " Dave Marchevsky
@ 2023-10-23 23:32 ` kernel test robot
2023-10-30 21:10 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
2 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2023-10-23 23:32 UTC (permalink / raw)
To: Dave Marchevsky, bpf; +Cc: oe-kbuild-all
Hi Dave,
kernel test robot noticed the following build warnings:
[auto build test WARNING on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Dave-Marchevsky/bpf-Fix-btf_get_field_type-to-fail-for-multiple-bpf_refcount-fields/20231024-060227
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20231023220030.2556229-5-davemarchevsky%40fb.com
patch subject: [PATCH v1 bpf-next 4/4] selftests/bpf: Add tests exercising aggregate type BTF field search
reproduce: (https://download.01.org/0day-ci/archive/20231024/202310240704.3BxYlwQh-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310240704.3BxYlwQh-lkp@intel.com/
# many are suggestions rather than must-fix
ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#65: FILE: tools/testing/selftests/bpf/progs/array_kptr.c:12:
+ struct prog_test_ref_kfunc __kptr *ref_ptr;
^
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search
2023-10-23 22:00 ` [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search Dave Marchevsky
@ 2023-10-26 1:24 ` kernel test robot
2023-10-30 12:56 ` Jiri Olsa
` (2 subsequent siblings)
3 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2023-10-26 1:24 UTC (permalink / raw)
To: Dave Marchevsky, bpf
Cc: llvm, oe-kbuild-all, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Kernel Team, Dave Marchevsky
Hi Dave,
kernel test robot noticed the following build warnings:
[auto build test WARNING on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Dave-Marchevsky/bpf-Fix-btf_get_field_type-to-fail-for-multiple-bpf_refcount-fields/20231024-060227
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20231023220030.2556229-4-davemarchevsky%40fb.com
patch subject: [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231026/202310260952.C9Gb9Avi-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231026/202310260952.C9Gb9Avi-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310260952.C9Gb9Avi-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> kernel/bpf/btf.c:3634:70: warning: variable 'off' is uninitialized when used here [-Wuninitialized]
ret = btf_find_struct_field(btf, elem_type, srch, array_field_off + off, rec);
^~~
kernel/bpf/btf.c:3625:15: note: initialize the variable 'off' to silence this warning
u32 i, j, off, nelems;
^
= 0
1 warning generated.
vim +/off +3634 kernel/bpf/btf.c
3616
3617 static int btf_flatten_array_field(const struct btf *btf,
3618 const struct btf_type *t,
3619 struct btf_field_info_search *srch,
3620 int array_field_off, int rec)
3621 {
3622 int ret, start_idx, elem_field_cnt;
3623 const struct btf_type *elem_type;
3624 struct btf_field_info *info;
3625 u32 i, j, off, nelems;
3626
3627 if (!btf_type_is_array(t))
3628 return -EINVAL;
3629 nelems = __multi_dim_elem_type_nelems(btf, t, &elem_type);
3630 if (!nelems || !__btf_type_is_struct(elem_type))
3631 return srch->idx;
3632
3633 start_idx = srch->idx;
> 3634 ret = btf_find_struct_field(btf, elem_type, srch, array_field_off + off, rec);
3635 if (ret < 0)
3636 return ret;
3637
3638 /* No btf_field_info's added */
3639 if (srch->idx == start_idx)
3640 return srch->idx;
3641
3642 elem_field_cnt = srch->idx - start_idx;
3643 info = __next_field_infos(srch, elem_field_cnt * (nelems - 1));
3644 if (IS_ERR_OR_NULL(info))
3645 return PTR_ERR(info);
3646
3647 /* Array elems after the first can copy first elem's btf_field_infos
3648 * and adjust offset
3649 */
3650 for (i = 1; i < nelems; i++) {
3651 memcpy(info, &srch->infos[start_idx],
3652 elem_field_cnt * sizeof(struct btf_field_info));
3653 for (j = 0; j < elem_field_cnt; j++) {
3654 info->off += (i * elem_type->size);
3655 info++;
3656 }
3657 }
3658 return srch->idx;
3659 }
3660
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search
2023-10-23 22:00 ` [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search Dave Marchevsky
@ 2023-10-28 14:52 ` Jiri Olsa
2023-10-30 19:31 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
2 siblings, 0 replies; 18+ messages in thread
From: Jiri Olsa @ 2023-10-28 14:52 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team
On Mon, Oct 23, 2023 at 03:00:28PM -0700, Dave Marchevsky wrote:
SNIP
>
> #undef field_mask_test_name_check_seen
> #undef field_mask_test_name
>
> +static int __struct_member_check_align(u32 off, enum btf_field_type field_type)
> +{
> + u32 align = btf_field_type_align(field_type);
> +
> + if (off % align)
> + return -EINVAL;
> + return 0;
> +}
> +
> static int btf_find_struct_field(const struct btf *btf,
> - const struct btf_type *t, u32 field_mask,
> - struct btf_field_info *info, int info_cnt)
> + const struct btf_type *t,
> + struct btf_field_info_search *srch)
> {
> - int ret, idx = 0, align, sz, field_type;
> const struct btf_member *member;
> - struct btf_field_info tmp;
> - u32 i, off, seen_mask = 0;
> + int ret, field_type;
> + u32 i, off, sz;
>
> for_each_member(i, t, member) {
> const struct btf_type *member_type = btf_type_by_id(btf,
> member->type);
> -
> - field_type = btf_get_field_type(__btf_name_by_offset(btf, member_type->name_off),
> - field_mask, &seen_mask, &align, &sz);
> - if (field_type == 0)
> - continue;
> - if (field_type < 0)
> - return field_type;
> -
> off = __btf_member_bit_offset(t, member);
> if (off % 8)
> /* valid C code cannot generate such BTF */
> return -EINVAL;
> off /= 8;
> - if (off % align)
> +
> + field_type = btf_get_field_type_by_name(btf, member_type, srch);
> + if (field_type < 0)
> + return field_type;
> +
> + if (field_type == 0) {
> + /* Maybe it's a kptr. Use BPF_KPTR_REF for align
> + * checks, all ptrs have same align.
> + * btf_maybe_find_kptr will find actual kptr type
> + */
> + if (__struct_member_check_align(off, BPF_KPTR_REF))
> + continue;
> +
> + ret = btf_maybe_find_kptr(btf, member_type, off, srch);
> + if (ret < 0)
> + return ret;
> + continue;
> + }
would it be possible to split the change to:
- factor the arguments to 'struct btf_field_info_search *srch' and
passing it to btf_find_field and all related descedant calls
- factor the allignment handling
I can't see these two changes being dependent on each other,
if that's the case I think the change would be simpler
jirka
> +
> + sz = btf_field_type_size(field_type);
> + if (__struct_member_check_align(off, field_type))
> continue;
>
> switch (field_type) {
> @@ -3453,64 +3542,81 @@ static int btf_find_struct_field(const struct btf *btf,
> case BPF_RB_NODE:
> case BPF_REFCOUNT:
> ret = btf_find_struct(btf, member_type, off, sz, field_type,
> - idx < info_cnt ? &info[idx] : &tmp);
> - if (ret < 0)
> - return ret;
> - break;
> - case BPF_KPTR_UNREF:
> - case BPF_KPTR_REF:
> - case BPF_KPTR_PERCPU:
> - ret = btf_find_kptr(btf, member_type, off, sz,
> - idx < info_cnt ? &info[idx] : &tmp);
> + srch);
> if (ret < 0)
> return ret;
> break;
> case BPF_LIST_HEAD:
> case BPF_RB_ROOT:
> ret = btf_find_graph_root(btf, t, member_type,
> - i, off, sz,
> - idx < info_cnt ? &info[idx] : &tmp,
> - field_type);
> + i, off, sz, srch, field_type);
> if (ret < 0)
> return ret;
> break;
> + /* kptr fields are not handled in this switch, see
> + * btf_maybe_find_kptr above
> + */
> + case BPF_KPTR_UNREF:
> + case BPF_KPTR_REF:
> + case BPF_KPTR_PERCPU:
> default:
> return -EFAULT;
> }
> -
> - if (ret == BTF_FIELD_IGNORE)
> - continue;
> - if (idx >= info_cnt)
> - return -E2BIG;
> - ++idx;
> }
> - return idx;
> + return srch->idx;
> +}
> +
> +static int __datasec_vsi_check_align_sz(const struct btf_var_secinfo *vsi,
> + enum btf_field_type field_type,
> + u32 expected_sz)
> +{
> + u32 off, align;
> +
> + off = vsi->offset;
> + align = btf_field_type_align(field_type);
> +
> + if (vsi->size != expected_sz)
> + return -EINVAL;
> + if (off % align)
> + return -EINVAL;
> +
> + return 0;
> }
>
> static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
> - u32 field_mask, struct btf_field_info *info,
> - int info_cnt)
> + struct btf_field_info_search *srch)
> {
> - int ret, idx = 0, align, sz, field_type;
> const struct btf_var_secinfo *vsi;
> - struct btf_field_info tmp;
> - u32 i, off, seen_mask = 0;
> + int ret, field_type;
> + u32 i, off, sz;
>
> for_each_vsi(i, t, vsi) {
> const struct btf_type *var = btf_type_by_id(btf, vsi->type);
> const struct btf_type *var_type = btf_type_by_id(btf, var->type);
>
> - field_type = btf_get_field_type(__btf_name_by_offset(btf, var_type->name_off),
> - field_mask, &seen_mask, &align, &sz);
> - if (field_type == 0)
> - continue;
> + off = vsi->offset;
> + field_type = btf_get_field_type_by_name(btf, var_type, srch);
> if (field_type < 0)
> return field_type;
>
> - off = vsi->offset;
> - if (vsi->size != sz)
> + if (field_type == 0) {
> + /* Maybe it's a kptr. Use BPF_KPTR_REF for sz / align
> + * checks, all ptrs have same sz / align.
> + * btf_maybe_find_kptr will find actual kptr type
> + */
> + sz = btf_field_type_size(BPF_KPTR_REF);
> + if (__datasec_vsi_check_align_sz(vsi, BPF_KPTR_REF, sz))
> + continue;
> +
> + ret = btf_maybe_find_kptr(btf, var_type, off, srch);
> + if (ret < 0)
> + return ret;
> continue;
> - if (off % align)
> + }
> +
> + sz = btf_field_type_size(field_type);
> +
> + if (__datasec_vsi_check_align_sz(vsi, field_type, sz))
> continue;
>
> switch (field_type) {
> @@ -3520,48 +3626,38 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
> case BPF_RB_NODE:
> case BPF_REFCOUNT:
> ret = btf_find_struct(btf, var_type, off, sz, field_type,
> - idx < info_cnt ? &info[idx] : &tmp);
> - if (ret < 0)
> - return ret;
> - break;
> - case BPF_KPTR_UNREF:
> - case BPF_KPTR_REF:
> - case BPF_KPTR_PERCPU:
> - ret = btf_find_kptr(btf, var_type, off, sz,
> - idx < info_cnt ? &info[idx] : &tmp);
> + srch);
> if (ret < 0)
> return ret;
> break;
> case BPF_LIST_HEAD:
> case BPF_RB_ROOT:
> ret = btf_find_graph_root(btf, var, var_type,
> - -1, off, sz,
> - idx < info_cnt ? &info[idx] : &tmp,
> + -1, off, sz, srch,
> field_type);
> if (ret < 0)
> return ret;
> break;
> + /* kptr fields are not handled in this switch, see
> + * btf_maybe_find_kptr above
> + */
> + case BPF_KPTR_UNREF:
> + case BPF_KPTR_REF:
> + case BPF_KPTR_PERCPU:
> default:
> return -EFAULT;
> }
> -
> - if (ret == BTF_FIELD_IGNORE)
> - continue;
> - if (idx >= info_cnt)
> - return -E2BIG;
> - ++idx;
> }
> - return idx;
> + return srch->idx;
> }
>
> static int btf_find_field(const struct btf *btf, const struct btf_type *t,
> - u32 field_mask, struct btf_field_info *info,
> - int info_cnt)
> + struct btf_field_info_search *srch)
> {
> if (__btf_type_is_struct(t))
> - return btf_find_struct_field(btf, t, field_mask, info, info_cnt);
> + return btf_find_struct_field(btf, t, srch);
> else if (btf_type_is_datasec(t))
> - return btf_find_datasec_var(btf, t, field_mask, info, info_cnt);
> + return btf_find_datasec_var(btf, t, srch);
> return -EINVAL;
> }
>
> @@ -3729,47 +3825,51 @@ static int btf_field_cmp(const void *_a, const void *_b, const void *priv)
> struct btf_record *btf_parse_fields(const struct btf *btf, const struct btf_type *t,
> u32 field_mask, u32 value_size)
> {
> - struct btf_field_info info_arr[BTF_FIELDS_MAX];
> + struct btf_field_info_search srch;
> u32 next_off = 0, field_type_size;
> + struct btf_field_info *info;
> struct btf_record *rec;
> int ret, i, cnt;
>
> - ret = btf_find_field(btf, t, field_mask, info_arr, ARRAY_SIZE(info_arr));
> - if (ret < 0)
> - return ERR_PTR(ret);
> - if (!ret)
> - return NULL;
> + memset(&srch, 0, sizeof(srch));
> + srch.field_mask = field_mask;
> + ret = btf_find_field(btf, t, &srch);
> + if (ret <= 0)
> + goto end_srch;
>
> cnt = ret;
> /* This needs to be kzalloc to zero out padding and unused fields, see
> * comment in btf_record_equal.
> */
> rec = kzalloc(offsetof(struct btf_record, fields[cnt]), GFP_KERNEL | __GFP_NOWARN);
> - if (!rec)
> - return ERR_PTR(-ENOMEM);
> + if (!rec) {
> + ret = -ENOMEM;
> + goto end_srch;
> + }
>
> rec->spin_lock_off = -EINVAL;
> rec->timer_off = -EINVAL;
> rec->refcount_off = -EINVAL;
> for (i = 0; i < cnt; i++) {
> - field_type_size = btf_field_type_size(info_arr[i].type);
> - if (info_arr[i].off + field_type_size > value_size) {
> - WARN_ONCE(1, "verifier bug off %d size %d", info_arr[i].off, value_size);
> + info = &srch.infos[i];
> + field_type_size = btf_field_type_size(info->type);
> + if (info->off + field_type_size > value_size) {
> + WARN_ONCE(1, "verifier bug off %d size %d", info->off, value_size);
> ret = -EFAULT;
> goto end;
> }
> - if (info_arr[i].off < next_off) {
> + if (info->off < next_off) {
> ret = -EEXIST;
> goto end;
> }
> - next_off = info_arr[i].off + field_type_size;
> + next_off = info->off + field_type_size;
>
> - rec->field_mask |= info_arr[i].type;
> - rec->fields[i].offset = info_arr[i].off;
> - rec->fields[i].type = info_arr[i].type;
> + rec->field_mask |= info->type;
> + rec->fields[i].offset = info->off;
> + rec->fields[i].type = info->type;
> rec->fields[i].size = field_type_size;
>
> - switch (info_arr[i].type) {
> + switch (info->type) {
> case BPF_SPIN_LOCK:
> WARN_ON_ONCE(rec->spin_lock_off >= 0);
> /* Cache offset for faster lookup at runtime */
> @@ -3788,17 +3888,17 @@ struct btf_record *btf_parse_fields(const struct btf *btf, const struct btf_type
> case BPF_KPTR_UNREF:
> case BPF_KPTR_REF:
> case BPF_KPTR_PERCPU:
> - ret = btf_parse_kptr(btf, &rec->fields[i], &info_arr[i]);
> + ret = btf_parse_kptr(btf, &rec->fields[i], info);
> if (ret < 0)
> goto end;
> break;
> case BPF_LIST_HEAD:
> - ret = btf_parse_list_head(btf, &rec->fields[i], &info_arr[i]);
> + ret = btf_parse_list_head(btf, &rec->fields[i], info);
> if (ret < 0)
> goto end;
> break;
> case BPF_RB_ROOT:
> - ret = btf_parse_rb_root(btf, &rec->fields[i], &info_arr[i]);
> + ret = btf_parse_rb_root(btf, &rec->fields[i], info);
> if (ret < 0)
> goto end;
> break;
> @@ -3828,10 +3928,13 @@ struct btf_record *btf_parse_fields(const struct btf *btf, const struct btf_type
>
> sort_r(rec->fields, rec->cnt, sizeof(struct btf_field), btf_field_cmp,
> NULL, rec);
> + kfree(srch.infos);
>
> return rec;
> end:
> btf_record_free(rec);
> +end_srch:
> + kfree(srch.infos);
> return ERR_PTR(ret);
> }
>
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search
2023-10-23 22:00 ` [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search Dave Marchevsky
2023-10-26 1:24 ` kernel test robot
@ 2023-10-30 12:56 ` Jiri Olsa
2023-10-30 20:56 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
3 siblings, 0 replies; 18+ messages in thread
From: Jiri Olsa @ 2023-10-30 12:56 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team
On Mon, Oct 23, 2023 at 03:00:29PM -0700, Dave Marchevsky wrote:
SNIP
> - ret = btf_maybe_find_kptr(btf, member_type, off, srch);
> + ret = btf_find_aggregate_field(btf, member_type, srch,
> + struct_field_off + off,
> + rec);
> if (ret < 0)
> return ret;
> continue;
> @@ -3541,15 +3587,17 @@ static int btf_find_struct_field(const struct btf *btf,
> case BPF_LIST_NODE:
> case BPF_RB_NODE:
> case BPF_REFCOUNT:
> - ret = btf_find_struct(btf, member_type, off, sz, field_type,
> - srch);
> + ret = btf_find_struct(btf, member_type,
> + struct_field_off + off,
> + sz, field_type, srch);
> if (ret < 0)
> return ret;
> break;
> case BPF_LIST_HEAD:
> case BPF_RB_ROOT:
> ret = btf_find_graph_root(btf, t, member_type,
> - i, off, sz, srch, field_type);
> + i, struct_field_off + off, sz,
> + srch, field_type);
> if (ret < 0)
> return ret;
> break;
> @@ -3566,6 +3614,82 @@ static int btf_find_struct_field(const struct btf *btf,
> return srch->idx;
> }
>
> +static int btf_flatten_array_field(const struct btf *btf,
> + const struct btf_type *t,
> + struct btf_field_info_search *srch,
> + int array_field_off, int rec)
> +{
> + int ret, start_idx, elem_field_cnt;
> + const struct btf_type *elem_type;
> + struct btf_field_info *info;
> + u32 i, j, off, nelems;
> +
> + if (!btf_type_is_array(t))
> + return -EINVAL;
seems this check is not needed, it's called only for
btf_type_is_array(t)
> + nelems = __multi_dim_elem_type_nelems(btf, t, &elem_type);
> + if (!nelems || !__btf_type_is_struct(elem_type))
> + return srch->idx;
> +
> + start_idx = srch->idx;
> + ret = btf_find_struct_field(btf, elem_type, srch, array_field_off + off, rec);
> + if (ret < 0)
> + return ret;
> +
> + /* No btf_field_info's added */
> + if (srch->idx == start_idx)
> + return srch->idx;
> +
> + elem_field_cnt = srch->idx - start_idx;
> + info = __next_field_infos(srch, elem_field_cnt * (nelems - 1));
> + if (IS_ERR_OR_NULL(info))
> + return PTR_ERR(info);
> +
> + /* Array elems after the first can copy first elem's btf_field_infos
> + * and adjust offset
> + */
> + for (i = 1; i < nelems; i++) {
> + memcpy(info, &srch->infos[start_idx],
> + elem_field_cnt * sizeof(struct btf_field_info));
> + for (j = 0; j < elem_field_cnt; j++) {
> + info->off += (i * elem_type->size);
> + info++;
> + }
> + }
> + return srch->idx;
> +}
> +
> +static int btf_find_aggregate_field(const struct btf *btf,
> + const struct btf_type *t,
> + struct btf_field_info_search *srch,
> + int field_off, int rec)
> +{
> + u32 orig_field_mask;
> + int ret;
> +
> + /* Dig up to 4 levels deep */
> + if (rec >= 4)
> + return -E2BIG;
do we need to fails in here? should we just stop descend?
and continue the search in upper layers
> +
> + orig_field_mask = srch->field_mask;
> + srch->field_mask &= BPF_KPTR;
> +
> + if (!srch->field_mask) {
> + ret = 0;
> + goto reset_field_mask;
> + }
could this be just
if (!(srch->field_mask & BPF_KPTR))
return 0;
but I don't understand why there's the BPF_KPTR restriction in here
jirka
> +
> + if (__btf_type_is_struct(t))
> + ret = btf_find_struct_field(btf, t, srch, field_off, rec + 1);
> + else if (btf_type_is_array(t))
> + ret = btf_flatten_array_field(btf, t, srch, field_off, rec + 1);
> + else
> + ret = -EINVAL;
> +
> +reset_field_mask:
> + srch->field_mask = orig_field_mask;
> + return ret;
> +}
> +
> static int __datasec_vsi_check_align_sz(const struct btf_var_secinfo *vsi,
> enum btf_field_type field_type,
> u32 expected_sz)
> @@ -3605,10 +3729,19 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
> * btf_maybe_find_kptr will find actual kptr type
> */
> sz = btf_field_type_size(BPF_KPTR_REF);
> - if (__datasec_vsi_check_align_sz(vsi, BPF_KPTR_REF, sz))
> + if (srch->field_mask & BPF_KPTR &&
> + !__datasec_vsi_check_align_sz(vsi, BPF_KPTR_REF, sz)) {
> + ret = btf_maybe_find_kptr(btf, var_type, off, srch);
> + if (ret < 0)
> + return ret;
> + if (ret == BTF_FIELD_FOUND)
> + continue;
> + }
> +
> + if (!(btf_type_is_array(var_type) || __btf_type_is_struct(var_type)))
> continue;
>
> - ret = btf_maybe_find_kptr(btf, var_type, off, srch);
> + ret = btf_find_aggregate_field(btf, var_type, srch, off, 0);
> if (ret < 0)
> return ret;
> continue;
> @@ -3655,7 +3788,7 @@ static int btf_find_field(const struct btf *btf, const struct btf_type *t,
> struct btf_field_info_search *srch)
> {
> if (__btf_type_is_struct(t))
> - return btf_find_struct_field(btf, t, srch);
> + return btf_find_struct_field(btf, t, srch, 0, 0);
> else if (btf_type_is_datasec(t))
> return btf_find_datasec_var(btf, t, srch);
> return -EINVAL;
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 1/4] bpf: Fix btf_get_field_type to fail for multiple bpf_refcount fields
2023-10-23 22:00 ` [PATCH v1 bpf-next 1/4] bpf: Fix btf_get_field_type to fail for multiple bpf_refcount fields Dave Marchevsky
@ 2023-10-30 17:56 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
1 sibling, 0 replies; 18+ messages in thread
From: Yonghong Song @ 2023-10-30 17:56 UTC (permalink / raw)
To: Dave Marchevsky, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team
On 10/23/23 3:00 PM, Dave Marchevsky wrote:
> If a struct has a bpf_refcount field, the refcount controls lifetime of
> the entire struct. Currently there's no usecase or support for multiple
> bpf_refcount fields in a struct.
>
> bpf_spin_lock and bpf_timer fields don't support multiples either, but
> with better error behavior. Parsing BTF w/ a struct containing multiple
> {bpf_spin_lock, bpf_timer} fields fails in btf_get_field_type, while
> multiple bpf_refcount fields doesn't fail BTF parsing at all, instead
> triggering a WARN_ON_ONCE in btf_parse_fields, with the verifier using
> the last bpf_refcount field to actually do refcounting.
>
> This patch changes bpf_refcount handling in btf_get_field_type to use
> same error logic as bpf_spin_lock and bpf_timer. Since it's being used
> 3x and is boilerplatey, the logic is shoved into
> field_mask_test_name_check_seen helper macro.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> Fixes: d54730b50bae ("bpf: Introduce opaque bpf_refcount struct and add btf_record plumbing")
Acked-by: Yonghong Song <yonghong.song@linux.dev>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search
2023-10-23 22:00 ` [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search Dave Marchevsky
2023-10-28 14:52 ` Jiri Olsa
@ 2023-10-30 19:31 ` Yonghong Song
2023-10-30 19:56 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
2 siblings, 1 reply; 18+ messages in thread
From: Yonghong Song @ 2023-10-30 19:31 UTC (permalink / raw)
To: Dave Marchevsky, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team
On 10/23/23 3:00 PM, Dave Marchevsky wrote:
> btf_find_field takes (btf_type, special_field_types) and returns info
> about the specific special fields in btf_type, in the form of an array
> of struct btf_field info. The meat of this 'search for special fields'
struct btf_field_info.
> process happens in btf_find_datasec_var and btf_find_struct_field
> helpers: each member is examined and, if it's special, a struct
> btf_field_info describing it is added to the return array. Indeed, any
> function that might add to the output probably also looks at struct
> members or datasec vars.
>
> Most of the parameters passed around between helpers doing the search
> can be grouped into two categories: "info about the output array" and
> "info about which fields to search for". This patch joins those together
> in struct btf_field_info_search, simplifying the signatures of most
> helpers involved in the search, including array flattening helper that
> later patches in the series will add.
>
> The aforementioned array flattening logic will greatly increase the
> number of btf_field_info's needed to describe some structs, so this
> patch also turns the statically-sized struct btf_field_info
> info_arr[BTF_FIELDS_MAX] into a growable array with a larger max size.
Since this patch is a 'refactoring' patch, let us delay this patch
until the next one. This patch should be strictly a refactoring
patch so it becomes easier to verify changes.
>
> Implementation notes:
> * BTF_FIELDS_MAX is now max size of growable btf_field_info *infos
> instead of initial (and max) size of static result array
> * Static array before had 10 elems (+1 tmp btf_field_info)
> * growable array starts with 16 and doubles every time it needs to
> grow, up to BTF_FIELDS_MAX of 256
>
> * __next_field_infos is used with next_cnt > 1 later in the series
>
> * btf_find_{datasec_var, struct_field} have special logic for an edge
> case where the result array is full but the field being examined
> gets BTF_FIELD_IGNORE return from btf_find_{struct, kptr,graph_root}
> * If result wasn't BTF_FIELD_IGNORE, a btf_field_info would have to
> be added to the array. Since it is we can look at next field.
> * Before this patch the logic handling this edge case was hard to
> follow and used a tmp btf_struct_info. This patch moves the
> add-if-not-ignore logic down into btf_find_{struct, kptr,
> graph_root}, removing the need to opportunistically grab a
> btf_field_info to populate before knowing if it's actually
> necessary. Now a new one is grabbed only if the field shouldn't
> be ignored.
This extra 'tmp' btf_field_info approach sounds okay to me
in the original code. The previous code has a static size
and there is no realloc. Now you introduced realloc, so
removing extra 'tmp' seems do make sense.
>
> * Within btf_find_{datasec_var, struct_field}, each member is
> currently examined in two phases: first btf_get_field_type checks
> the member type name, then btf_find_{struct,graph_root,kptr} do
> additional sanity checking and populate struct btf_field_info. Kptr
> fields don't have a specific type name, though, so
> btf_get_field_type assumes that - if we're looking for kptrs - any
> member that fails type name check could be a kptr field.
> * As a result btf_find_kptr effectively does all the pointer
> hopping, sanity checking, and info population. Instead of trying
> to fit kptr handling into this two-phase model, where it's
> unwieldy, handle it in a separate codepath when name matching
> fails.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
> include/linux/bpf.h | 4 +-
> kernel/bpf/btf.c | 331 +++++++++++++++++++++++++++++---------------
> 2 files changed, 219 insertions(+), 116 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b4825d3cdb29..e07cac5cc3cf 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -171,8 +171,8 @@ struct bpf_map_ops {
> };
>
> enum {
> - /* Support at most 10 fields in a BTF type */
> - BTF_FIELDS_MAX = 10,
> + /* Support at most 256 fields in a BTF type */
> + BTF_FIELDS_MAX = 256,
> };
>
> enum btf_field_type {
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 975ef8e73393..e999ba85c363 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3257,25 +3257,94 @@ struct btf_field_info {
> };
> };
>
> +struct btf_field_info_search {
> + /* growable array. allocated in __next_field_infos
> + * free'd in btf_parse_fields
> + */
> + struct btf_field_info *infos;
> + /* size of infos */
> + int info_cnt;
> + /* index of next btf_field_info to populate */
> + int idx;
> +
> + /* btf_field_types to search for */
> + u32 field_mask;
> + /* btf_field_types found earlier */
> + u32 seen_mask;
> +};
> +
> +/* Reserve next_cnt contiguous btf_field_info's for caller to populate
> + * Returns ptr to first reserved btf_field_info
> + */
> +static struct btf_field_info *__next_field_infos(struct btf_field_info_search *srch,
> + u32 next_cnt)
> +{
> + struct btf_field_info *new_infos, *ret;
> +
> + if (!next_cnt)
> + return ERR_PTR(-EINVAL);
Looks next_cnt is never 0.
> +
> + if (srch->idx + next_cnt < srch->info_cnt)
> + goto nogrow_out;
> +
> + /* Need to grow */
> + if (srch->idx + next_cnt > BTF_FIELDS_MAX)
> + return ERR_PTR(-E2BIG);
> +
> + while (srch->idx + next_cnt >= srch->info_cnt)
> + srch->info_cnt = srch->infos ? srch->info_cnt * 2 : 16;
> +
> + new_infos = krealloc(srch->infos,
> + srch->info_cnt * sizeof(struct btf_field_info),
> + GFP_KERNEL | __GFP_NOWARN);
> + if (!new_infos)
> + return ERR_PTR(-ENOMEM);
> + srch->infos = new_infos;
> +
> +nogrow_out:
> + ret = &srch->infos[srch->idx];
> + srch->idx += next_cnt;
> + return ret;
> +}
> +
> +/* Request srch's next free btf_field_info to populate, possibly growing
> + * srch->infos
> + */
> +static struct btf_field_info *__next_field_info(struct btf_field_info_search *srch)
> +{
> + return __next_field_infos(srch, 1);
> +}
> +
> static int btf_find_struct(const struct btf *btf, const struct btf_type *t,
> u32 off, int sz, enum btf_field_type field_type,
> - struct btf_field_info *info)
> + struct btf_field_info_search *srch)
> {
> + struct btf_field_info *info;
> +
> if (!__btf_type_is_struct(t))
> return BTF_FIELD_IGNORE;
> if (t->size != sz)
> return BTF_FIELD_IGNORE;
> +
> + info = __next_field_info(srch);
> + if (IS_ERR_OR_NULL(info))
> + return PTR_ERR(info);
info cannot be NULL.
> +
> info->type = field_type;
> info->off = off;
> return BTF_FIELD_FOUND;
> }
>
> -static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
> - u32 off, int sz, struct btf_field_info *info)
> +static int btf_maybe_find_kptr(const struct btf *btf, const struct btf_type *t,
> + u32 off, struct btf_field_info_search *srch)
> {
> + struct btf_field_info *info;
> enum btf_field_type type;
> u32 res_id;
>
> + if (!(srch->field_mask & BPF_KPTR))
> + return BTF_FIELD_IGNORE;
> +
> /* Permit modifiers on the pointer itself */
> if (btf_type_is_volatile(t))
> t = btf_type_by_id(btf, t->type);
> @@ -3304,6 +3373,10 @@ static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
> if (!__btf_type_is_struct(t))
> return -EINVAL;
>
> + info = __next_field_info(srch);
> + if (IS_ERR_OR_NULL(info))
> + return PTR_ERR(info);
info cannot be NULL.
> +
> info->type = type;
> info->off = off;
> info->kptr.type_id = res_id;
> @@ -3340,9 +3413,10 @@ const char *btf_find_decl_tag_value(const struct btf *btf, const struct btf_type
> static int
> btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
> const struct btf_type *t, int comp_idx, u32 off,
> - int sz, struct btf_field_info *info,
> + int sz, struct btf_field_info_search *srch,
> enum btf_field_type head_type)
> {
> + struct btf_field_info *info;
> const char *node_field_name;
> const char *value_type;
> s32 id;
> @@ -3367,6 +3441,11 @@ btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
> node_field_name++;
> if (str_is_empty(node_field_name))
> return -EINVAL;
> +
> + info = __next_field_info(srch);
> + if (IS_ERR_OR_NULL(info))
> + return PTR_ERR(info);
> +
ditto.
> info->type = head_type;
> info->off = off;
> info->graph_root.value_btf_id = id;
> @@ -3374,25 +3453,24 @@ btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
> return BTF_FIELD_FOUND;
> }
>
> -#define field_mask_test_name(field_type, field_type_str) \
> - if (field_mask & field_type && !strcmp(name, field_type_str)) { \
> - type = field_type; \
> - goto end; \
> +#define field_mask_test_name(field_type, field_type_str) \
> + if (srch->field_mask & field_type && !strcmp(name, field_type_str)) { \
> + return field_type; \
> }
>
> -#define field_mask_test_name_check_seen(field_type, field_type_str) \
> - if (field_mask & field_type && !strcmp(name, field_type_str)) { \
> - if (*seen_mask & field_type) \
> - return -E2BIG; \
> - *seen_mask |= field_type; \
> - type = field_type; \
> - goto end; \
> +#define field_mask_test_name_check_seen(field_type, field_type_str) \
> + if (srch->field_mask & field_type && !strcmp(name, field_type_str)) { \
> + if (srch->seen_mask & field_type) \
> + return -E2BIG; \
> + srch->seen_mask |= field_type; \
> + return field_type; \
> }
>
> -static int btf_get_field_type(const char *name, u32 field_mask, u32 *seen_mask,
> - int *align, int *sz)
[...]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search
2023-10-30 19:31 ` Yonghong Song
@ 2023-10-30 19:56 ` Yonghong Song
0 siblings, 0 replies; 18+ messages in thread
From: Yonghong Song @ 2023-10-30 19:56 UTC (permalink / raw)
To: Dave Marchevsky, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team
On 10/30/23 12:31 PM, Yonghong Song wrote:
>
> On 10/23/23 3:00 PM, Dave Marchevsky wrote:
>> btf_find_field takes (btf_type, special_field_types) and returns info
>> about the specific special fields in btf_type, in the form of an array
>> of struct btf_field info. The meat of this 'search for special fields'
>
> struct btf_field_info.
>
>> process happens in btf_find_datasec_var and btf_find_struct_field
>> helpers: each member is examined and, if it's special, a struct
>> btf_field_info describing it is added to the return array. Indeed, any
>> function that might add to the output probably also looks at struct
>> members or datasec vars.
>>
>> Most of the parameters passed around between helpers doing the search
>> can be grouped into two categories: "info about the output array" and
>> "info about which fields to search for". This patch joins those together
>> in struct btf_field_info_search, simplifying the signatures of most
>> helpers involved in the search, including array flattening helper that
>> later patches in the series will add.
>>
>> The aforementioned array flattening logic will greatly increase the
>> number of btf_field_info's needed to describe some structs, so this
>> patch also turns the statically-sized struct btf_field_info
>> info_arr[BTF_FIELDS_MAX] into a growable array with a larger max size.
>
> Since this patch is a 'refactoring' patch, let us delay this patch
sorry. typo. "this patch" -> "this change".
> until the next one. This patch should be strictly a refactoring
> patch so it becomes easier to verify changes.
>
>>
>> Implementation notes:
>> * BTF_FIELDS_MAX is now max size of growable btf_field_info *infos
>> instead of initial (and max) size of static result array
>> * Static array before had 10 elems (+1 tmp btf_field_info)
>> * growable array starts with 16 and doubles every time it needs to
>> grow, up to BTF_FIELDS_MAX of 256
>>
>> * __next_field_infos is used with next_cnt > 1 later in the series
>>
>> * btf_find_{datasec_var, struct_field} have special logic for an edge
>> case where the result array is full but the field being examined
>> gets BTF_FIELD_IGNORE return from btf_find_{struct,
>> kptr,graph_root}
>> * If result wasn't BTF_FIELD_IGNORE, a btf_field_info would have to
>> be added to the array. Since it is we can look at next field.
>> * Before this patch the logic handling this edge case was hard to
>> follow and used a tmp btf_struct_info. This patch moves the
>> add-if-not-ignore logic down into btf_find_{struct, kptr,
>> graph_root}, removing the need to opportunistically grab a
>> btf_field_info to populate before knowing if it's actually
>> necessary. Now a new one is grabbed only if the field shouldn't
>> be ignored.
>
> This extra 'tmp' btf_field_info approach sounds okay to me
> in the original code. The previous code has a static size
> and there is no realloc. Now you introduced realloc, so
> removing extra 'tmp' seems do make sense.
>
>
>>
>> * Within btf_find_{datasec_var, struct_field}, each member is
>> currently examined in two phases: first btf_get_field_type checks
>> the member type name, then btf_find_{struct,graph_root,kptr} do
>> additional sanity checking and populate struct btf_field_info. Kptr
>> fields don't have a specific type name, though, so
>> btf_get_field_type assumes that - if we're looking for kptrs - any
>> member that fails type name check could be a kptr field.
>> * As a result btf_find_kptr effectively does all the pointer
>> hopping, sanity checking, and info population. Instead of trying
>> to fit kptr handling into this two-phase model, where it's
>> unwieldy, handle it in a separate codepath when name matching
>> fails.
>>
>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
>> ---
>> include/linux/bpf.h | 4 +-
>> kernel/bpf/btf.c | 331 +++++++++++++++++++++++++++++---------------
>> 2 files changed, 219 insertions(+), 116 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index b4825d3cdb29..e07cac5cc3cf 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -171,8 +171,8 @@ struct bpf_map_ops {
>> };
>> enum {
>> - /* Support at most 10 fields in a BTF type */
>> - BTF_FIELDS_MAX = 10,
>> + /* Support at most 256 fields in a BTF type */
>> + BTF_FIELDS_MAX = 256,
>> };
>> enum btf_field_type {
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 975ef8e73393..e999ba85c363 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -3257,25 +3257,94 @@ struct btf_field_info {
>> };
>> };
>> +struct btf_field_info_search {
>> + /* growable array. allocated in __next_field_infos
>> + * free'd in btf_parse_fields
>> + */
>> + struct btf_field_info *infos;
>> + /* size of infos */
>> + int info_cnt;
>> + /* index of next btf_field_info to populate */
>> + int idx;
>> +
>> + /* btf_field_types to search for */
>> + u32 field_mask;
>> + /* btf_field_types found earlier */
>> + u32 seen_mask;
>> +};
>> +
>> +/* Reserve next_cnt contiguous btf_field_info's for caller to populate
>> + * Returns ptr to first reserved btf_field_info
>> + */
>> +static struct btf_field_info *__next_field_infos(struct
>> btf_field_info_search *srch,
>> + u32 next_cnt)
>> +{
>> + struct btf_field_info *new_infos, *ret;
>> +
>> + if (!next_cnt)
>> + return ERR_PTR(-EINVAL);
>
> Looks next_cnt is never 0.
>
>> +
>> + if (srch->idx + next_cnt < srch->info_cnt)
>> + goto nogrow_out;
>> +
>> + /* Need to grow */
>> + if (srch->idx + next_cnt > BTF_FIELDS_MAX)
>> + return ERR_PTR(-E2BIG);
>> +
>> + while (srch->idx + next_cnt >= srch->info_cnt)
>> + srch->info_cnt = srch->infos ? srch->info_cnt * 2 : 16;
>> +
>> + new_infos = krealloc(srch->infos,
>> + srch->info_cnt * sizeof(struct btf_field_info),
>> + GFP_KERNEL | __GFP_NOWARN);
>> + if (!new_infos)
>> + return ERR_PTR(-ENOMEM);
>> + srch->infos = new_infos;
>> +
>> +nogrow_out:
>> + ret = &srch->infos[srch->idx];
>> + srch->idx += next_cnt;
>> + return ret;
>> +}
>> +
>> +/* Request srch's next free btf_field_info to populate, possibly
>> growing
>> + * srch->infos
>> + */
>> +static struct btf_field_info *__next_field_info(struct
>> btf_field_info_search *srch)
>> +{
>> + return __next_field_infos(srch, 1);
>> +}
>> +
>> static int btf_find_struct(const struct btf *btf, const struct
>> btf_type *t,
>> u32 off, int sz, enum btf_field_type field_type,
>> - struct btf_field_info *info)
>> + struct btf_field_info_search *srch)
>> {
>> + struct btf_field_info *info;
>> +
>> if (!__btf_type_is_struct(t))
>> return BTF_FIELD_IGNORE;
>> if (t->size != sz)
>> return BTF_FIELD_IGNORE;
>> +
>> + info = __next_field_info(srch);
>> + if (IS_ERR_OR_NULL(info))
>> + return PTR_ERR(info);
>
> info cannot be NULL.
>
>> +
>> info->type = field_type;
>> info->off = off;
>> return BTF_FIELD_FOUND;
>> }
>> -static int btf_find_kptr(const struct btf *btf, const struct
>> btf_type *t,
>> - u32 off, int sz, struct btf_field_info *info)
>> +static int btf_maybe_find_kptr(const struct btf *btf, const struct
>> btf_type *t,
>> + u32 off, struct btf_field_info_search *srch)
>> {
>> + struct btf_field_info *info;
>> enum btf_field_type type;
>> u32 res_id;
>> + if (!(srch->field_mask & BPF_KPTR))
>> + return BTF_FIELD_IGNORE;
>> +
>> /* Permit modifiers on the pointer itself */
>> if (btf_type_is_volatile(t))
>> t = btf_type_by_id(btf, t->type);
>> @@ -3304,6 +3373,10 @@ static int btf_find_kptr(const struct btf
>> *btf, const struct btf_type *t,
>> if (!__btf_type_is_struct(t))
>> return -EINVAL;
>> + info = __next_field_info(srch);
>> + if (IS_ERR_OR_NULL(info))
>> + return PTR_ERR(info);
>
> info cannot be NULL.
>
>> +
>> info->type = type;
>> info->off = off;
>> info->kptr.type_id = res_id;
>> @@ -3340,9 +3413,10 @@ const char *btf_find_decl_tag_value(const
>> struct btf *btf, const struct btf_type
>> static int
>> btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
>> const struct btf_type *t, int comp_idx, u32 off,
>> - int sz, struct btf_field_info *info,
>> + int sz, struct btf_field_info_search *srch,
>> enum btf_field_type head_type)
>> {
>> + struct btf_field_info *info;
>> const char *node_field_name;
>> const char *value_type;
>> s32 id;
>> @@ -3367,6 +3441,11 @@ btf_find_graph_root(const struct btf *btf,
>> const struct btf_type *pt,
>> node_field_name++;
>> if (str_is_empty(node_field_name))
>> return -EINVAL;
>> +
>> + info = __next_field_info(srch);
>> + if (IS_ERR_OR_NULL(info))
>> + return PTR_ERR(info);
>> +
>
> ditto.
>
>> info->type = head_type;
>> info->off = off;
>> info->graph_root.value_btf_id = id;
>> @@ -3374,25 +3453,24 @@ btf_find_graph_root(const struct btf *btf,
>> const struct btf_type *pt,
>> return BTF_FIELD_FOUND;
>> }
>> -#define field_mask_test_name(field_type, field_type_str) \
>> - if (field_mask & field_type && !strcmp(name, field_type_str))
>> { \
>> - type = field_type; \
>> - goto end; \
>> +#define field_mask_test_name(field_type, field_type_str) \
>> + if (srch->field_mask & field_type && !strcmp(name,
>> field_type_str)) { \
>> + return field_type; \
>> }
>> -#define field_mask_test_name_check_seen(field_type,
>> field_type_str) \
>> - if (field_mask & field_type && !strcmp(name, field_type_str))
>> { \
>> - if (*seen_mask & field_type) \
>> - return -E2BIG; \
>> - *seen_mask |= field_type; \
>> - type = field_type; \
>> - goto end; \
>> +#define field_mask_test_name_check_seen(field_type,
>> field_type_str) \
>> + if (srch->field_mask & field_type && !strcmp(name,
>> field_type_str)) { \
>> + if (srch->seen_mask & field_type) \
>> + return -E2BIG; \
>> + srch->seen_mask |= field_type; \
>> + return field_type; \
>> }
>> -static int btf_get_field_type(const char *name, u32 field_mask,
>> u32 *seen_mask,
>> - int *align, int *sz)
>
> [...]
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search
2023-10-23 22:00 ` [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search Dave Marchevsky
2023-10-26 1:24 ` kernel test robot
2023-10-30 12:56 ` Jiri Olsa
@ 2023-10-30 20:56 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
3 siblings, 0 replies; 18+ messages in thread
From: Yonghong Song @ 2023-10-30 20:56 UTC (permalink / raw)
To: Dave Marchevsky, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team
On 10/23/23 3:00 PM, Dave Marchevsky wrote:
> Structs and arrays are aggregate types which contain some inner
> type(s) - members and elements - at various offsets. Currently, when
> examining a struct or datasec for special fields, the verifier does
> not look into the inner type of the structs or arrays it contains.
> This patch adds logic to descend into struct and array types when
> searching for special fields.
This indeed a great improvement. Thanks!
>
> If we have struct x containing an array:
>
> struct x {
> int a;
> u64 b[10];
> };
>
> we can construct some struct y with no array or struct members that
> has the same types at the same offsets:
>
> struct y {
> int a;
> u64 b1;
> u64 b2;
> /* ... */
> u64 b10;
> };
>
> Similarly for a struct containing a struct:
>
> struct x {
> char a;
> struct {
> int b;
> u64 c;
> } inner;
> };
>
> there's a struct y with no aggregate members and same types/offsets:
>
> struct y {
> char a;
> int inner_b __attribute__ ((aligned (8))); /* See [0] */
> u64 inner_c __attribute__ ((aligned (8)));
> };
>
> This patch takes advantage of this equivalence to 'flatten' the
> field info found while descending into struct or array members into
> the btf_field_info result array of the original type being examined.
> The resultant btf_record of the original type being searched will
> have the correct fields at the correct offsets, but without any
> differentiation between "this field is one of my members" and "this
> field is actually in my some struct / array member".
>
> For now this descendant search logic looks for kptr fields only.
>
> Implementation notes:
> * Search starts at btf_find_field - we're either looking at a struct
> that's the type of some mapval (btf_find_struct_field), or a
> datasec representing a .bss or .data map (btf_find_datasec_var).
> Newly-added btf_find_aggregate_field is a "disambiguation helper"
> like btf_find_field, but is meant to be called from one of the
> starting points of the search - btf_find_{struct_field,
> datasec_var}.
> * btf_find_aggregate_field may itself call btf_find_struct_field,
> so there's some recursive digging possible here
>
> * Newly-added btf_flatten_array_field handles array fields by
> finding the type of their element and continuing the dig based on
> elem type.
>
> [0]: Structs have the alignment of their largest field, so the
> explicit alignment is necessary here. Luckily this patch's
> changes don't need to care about alignment / padding, since
> the BTF created during compilation is being searched, and
> it already has the correct information.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
> kernel/bpf/btf.c | 151 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 142 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index e999ba85c363..b982bf6fef9d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3496,9 +3496,41 @@ static int __struct_member_check_align(u32 off, enum btf_field_type field_type)
> return 0;
> }
>
> +/* Return number of elems and elem_type of a btf_array
> + *
> + * If the array is multi-dimensional, return elem count of
> + * equivalent single-dimensional array
> + * e.g. int x[10][10][10] has same layout as int x[1000]
> + */
> +static u32 __multi_dim_elem_type_nelems(const struct btf *btf,
> + const struct btf_type *t,
> + const struct btf_type **elem_type)
> +{
> + u32 nelems = btf_array(t)->nelems;
> +
> + if (!nelems)
> + return 0;
> +
> + *elem_type = btf_type_by_id(btf, btf_array(t)->type);
> +
> + while (btf_type_is_array(*elem_type)) {
> + if (!btf_array(*elem_type)->nelems)
> + return 0;
btf_array(*elem_type)->nelems == 0 is a very rare case.
I guess we do not need it here. If it is indeed 0,
the final nelems will be 0 any way.
> + nelems *= btf_array(*elem_type)->nelems;
> + *elem_type = btf_type_by_id(btf, btf_array(*elem_type)->type);
> + }
> + return nelems;
> +}
> +
> +static int btf_find_aggregate_field(const struct btf *btf,
> + const struct btf_type *t,
> + struct btf_field_info_search *srch,
> + int field_off, int rec);
> +
> static int btf_find_struct_field(const struct btf *btf,
> const struct btf_type *t,
> - struct btf_field_info_search *srch)
> + struct btf_field_info_search *srch,
> + int struct_field_off, int rec)
> {
> const struct btf_member *member;
> int ret, field_type;
> @@ -3522,10 +3554,24 @@ static int btf_find_struct_field(const struct btf *btf,
> * checks, all ptrs have same align.
> * btf_maybe_find_kptr will find actual kptr type
> */
> - if (__struct_member_check_align(off, BPF_KPTR_REF))
> + if (srch->field_mask & BPF_KPTR &&
> + !__struct_member_check_align(off, BPF_KPTR_REF)) {
> + ret = btf_maybe_find_kptr(btf, member_type,
> + struct_field_off + off,
> + srch);
> + if (ret < 0)
> + return ret;
> + if (ret == BTF_FIELD_FOUND)
> + continue;
> + }
> +
> + if (!(btf_type_is_array(member_type) ||
> + __btf_type_is_struct(member_type)))
> continue;
>
> - ret = btf_maybe_find_kptr(btf, member_type, off, srch);
> + ret = btf_find_aggregate_field(btf, member_type, srch,
> + struct_field_off + off,
> + rec);
> if (ret < 0)
> return ret;
> continue;
> @@ -3541,15 +3587,17 @@ static int btf_find_struct_field(const struct btf *btf,
> case BPF_LIST_NODE:
> case BPF_RB_NODE:
> case BPF_REFCOUNT:
> - ret = btf_find_struct(btf, member_type, off, sz, field_type,
> - srch);
> + ret = btf_find_struct(btf, member_type,
> + struct_field_off + off,
> + sz, field_type, srch);
> if (ret < 0)
> return ret;
> break;
> case BPF_LIST_HEAD:
> case BPF_RB_ROOT:
> ret = btf_find_graph_root(btf, t, member_type,
> - i, off, sz, srch, field_type);
> + i, struct_field_off + off, sz,
> + srch, field_type);
> if (ret < 0)
> return ret;
> break;
> @@ -3566,6 +3614,82 @@ static int btf_find_struct_field(const struct btf *btf,
> return srch->idx;
> }
>
> +static int btf_flatten_array_field(const struct btf *btf,
> + const struct btf_type *t,
> + struct btf_field_info_search *srch,
> + int array_field_off, int rec)
> +{
> + int ret, start_idx, elem_field_cnt;
> + const struct btf_type *elem_type;
> + struct btf_field_info *info;
> + u32 i, j, off, nelems;
> +
> + if (!btf_type_is_array(t))
> + return -EINVAL;
> + nelems = __multi_dim_elem_type_nelems(btf, t, &elem_type);
> + if (!nelems || !__btf_type_is_struct(elem_type))
> + return srch->idx;
> +
> + start_idx = srch->idx;
> + ret = btf_find_struct_field(btf, elem_type, srch, array_field_off + off, rec);
As kerneltest bot mentioned, 'off' is undefined. The array_field_off
should equal the first array element struct_elem_off, right?
So I think here 'off' can be replaced with 0?
> + if (ret < 0)
> + return ret;
> +
> + /* No btf_field_info's added */
> + if (srch->idx == start_idx)
> + return srch->idx;
> +
> + elem_field_cnt = srch->idx - start_idx;
> + info = __next_field_infos(srch, elem_field_cnt * (nelems - 1));
> + if (IS_ERR_OR_NULL(info))
> + return PTR_ERR(info);
What happens if nelems = 1?
> +
> + /* Array elems after the first can copy first elem's btf_field_infos
> + * and adjust offset
> + */
> + for (i = 1; i < nelems; i++) {
> + memcpy(info, &srch->infos[start_idx],
> + elem_field_cnt * sizeof(struct btf_field_info));
> + for (j = 0; j < elem_field_cnt; j++) {
> + info->off += (i * elem_type->size);
> + info++;
> + }
> + }
> + return srch->idx;
> +}
> +
> [...]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 4/4] selftests/bpf: Add tests exercising aggregate type BTF field search
2023-10-23 22:00 ` [PATCH v1 bpf-next 4/4] selftests/bpf: Add tests exercising aggregate type BTF " Dave Marchevsky
2023-10-23 23:32 ` kernel test robot
@ 2023-10-30 21:10 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
2 siblings, 0 replies; 18+ messages in thread
From: Yonghong Song @ 2023-10-30 21:10 UTC (permalink / raw)
To: Dave Marchevsky, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team
On 10/23/23 3:00 PM, Dave Marchevsky wrote:
> The newly-added test file attempts to kptr_xchg a prog_test_ref_kfunc
> kptr into a kptr field in a variety of nested aggregate types. If the
> verifier recognizes that there's a kptr field where we're trying to
> kptr_xchg, then the aggregate type digging logic works as expected.
>
> Some of the refactoring changes in this series are tested as well.
> Specifically:
> * BTF_FIELDS_MAX is now higher and represents the max size of the
> growable array. Confirm that btf_parse_fields fails for a type which
> contains too many fields.
> * If we've already seen BTF_FIELDS_MAX fields, we should continue
> looking for fields and fail if we find another one, otherwise the
> search should succeed and return BTF_FIELDS_MAX btf_field_infos.
> Confirm that this edge case works as expected.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
> .../selftests/bpf/prog_tests/array_kptr.c | 12 ++
> .../testing/selftests/bpf/progs/array_kptr.c | 179 ++++++++++++++++++
> 2 files changed, 191 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/array_kptr.c
> create mode 100644 tools/testing/selftests/bpf/progs/array_kptr.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/array_kptr.c b/tools/testing/selftests/bpf/prog_tests/array_kptr.c
> new file mode 100644
> index 000000000000..9d088520bdfe
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/array_kptr.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
> +
> +#include <test_progs.h>
> +
> +#include "array_kptr.skel.h"
> +
> +void test_array_kptr(void)
> +{
> + if (env.has_testmod)
> + RUN_TESTS(array_kptr);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/array_kptr.c b/tools/testing/selftests/bpf/progs/array_kptr.c
> new file mode 100644
> index 000000000000..f34872e74024
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/array_kptr.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_tracing.h>
> +#include <bpf/bpf_helpers.h>
> +#include "../bpf_testmod/bpf_testmod_kfunc.h"
> +#include "bpf_misc.h"
> +
> +struct val {
> + int d;
> + struct prog_test_ref_kfunc __kptr *ref_ptr;
> +};
> +
> +struct val2 {
> + char c;
> + struct val v;
> +};
> +
> +struct val_holder {
> + int e;
> + struct val2 first[2];
> + int f;
> + struct val second[2];
> +};
> +
> +struct array_map {
> + __uint(type, BPF_MAP_TYPE_ARRAY);
> + __type(key, int);
> + __type(value, struct val);
> + __uint(max_entries, 10);
> +} array_map SEC(".maps");
> +
> +struct array_map2 {
> + __uint(type, BPF_MAP_TYPE_ARRAY);
> + __type(key, int);
> + __type(value, struct val2);
> + __uint(max_entries, 10);
> +} array_map2 SEC(".maps");
> +
> +__hidden struct val array[25];
> +__hidden struct val double_array[5][5];
> +__hidden struct val_holder double_holder_array[2][2];
> +
> +/* Some tests need their own section to force separate bss arraymap,
> + * otherwise above arrays wouldn't have btf_field_info either
> + */
> +#define private(name) SEC(".bss." #name) __hidden __attribute__((aligned(8)))
> +private(A) struct val array_too_big[300];
> +
> +private(B) struct val exactly_max_fields[256];
> +private(B) int ints[50];
> +
> +SEC("tc")
> +__success __retval(0)
> +int test_arraymap(void *ctx)
> +{
> + struct prog_test_ref_kfunc *p;
> + unsigned long dummy = 0;
> + struct val *v;
> + int idx = 0;
> +
> + v = bpf_map_lookup_elem(&array_map, &idx);
> + if (!v)
> + return 1;
> +
> + p = bpf_kfunc_call_test_acquire(&dummy);
> + if (!p)
> + return 2;
> +
> + p = bpf_kptr_xchg(&v->ref_ptr, p);
> + if (p) {
> + bpf_kfunc_call_test_release(p);
> + return 3;
> + }
> +
> + return 0;
> +}
> +
> ...
> +
> +SEC("tc")
> +__failure __msg("map '.bss.A' has no valid kptr")
The .bss.A might have valid kptr.
To reflect realiaty, maybe error message can be
'has too many special fields'?
> +int test_array_fail__too_big(void *ctx)
> +{
> + /* array_too_big's btf_record parsing will fail due to the
> + * number of btf_field_infos being > BTF_FIELDS_MAX
> + */
> + return test_array_xchg(&array_too_big[50]);
> +}
> +
> +char _license[] SEC("license") = "GPL";
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 1/4] bpf: Fix btf_get_field_type to fail for multiple bpf_refcount fields
2023-10-23 22:00 ` [PATCH v1 bpf-next 1/4] bpf: Fix btf_get_field_type to fail for multiple bpf_refcount fields Dave Marchevsky
2023-10-30 17:56 ` Yonghong Song
@ 2023-11-01 21:56 ` Andrii Nakryiko
1 sibling, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2023-11-01 21:56 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team
On Mon, Oct 23, 2023 at 3:00 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> If a struct has a bpf_refcount field, the refcount controls lifetime of
> the entire struct. Currently there's no usecase or support for multiple
> bpf_refcount fields in a struct.
>
> bpf_spin_lock and bpf_timer fields don't support multiples either, but
> with better error behavior. Parsing BTF w/ a struct containing multiple
> {bpf_spin_lock, bpf_timer} fields fails in btf_get_field_type, while
> multiple bpf_refcount fields doesn't fail BTF parsing at all, instead
> triggering a WARN_ON_ONCE in btf_parse_fields, with the verifier using
> the last bpf_refcount field to actually do refcounting.
>
> This patch changes bpf_refcount handling in btf_get_field_type to use
> same error logic as bpf_spin_lock and bpf_timer. Since it's being used
> 3x and is boilerplatey, the logic is shoved into
> field_mask_test_name_check_seen helper macro.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> Fixes: d54730b50bae ("bpf: Introduce opaque bpf_refcount struct and add btf_record plumbing")
> ---
> kernel/bpf/btf.c | 37 ++++++++++++++++---------------------
> 1 file changed, 16 insertions(+), 21 deletions(-)
>
LGTM
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 15d71d2986d3..975ef8e73393 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3374,8 +3374,17 @@ btf_find_graph_root(const struct btf *btf, const struct btf_type *pt,
> return BTF_FIELD_FOUND;
> }
>
[...]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search
2023-10-23 22:00 ` [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search Dave Marchevsky
2023-10-28 14:52 ` Jiri Olsa
2023-10-30 19:31 ` Yonghong Song
@ 2023-11-01 21:56 ` Andrii Nakryiko
2 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2023-11-01 21:56 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team
On Mon, Oct 23, 2023 at 3:00 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> btf_find_field takes (btf_type, special_field_types) and returns info
> about the specific special fields in btf_type, in the form of an array
> of struct btf_field info. The meat of this 'search for special fields'
> process happens in btf_find_datasec_var and btf_find_struct_field
> helpers: each member is examined and, if it's special, a struct
> btf_field_info describing it is added to the return array. Indeed, any
> function that might add to the output probably also looks at struct
> members or datasec vars.
>
> Most of the parameters passed around between helpers doing the search
> can be grouped into two categories: "info about the output array" and
> "info about which fields to search for". This patch joins those together
> in struct btf_field_info_search, simplifying the signatures of most
> helpers involved in the search, including array flattening helper that
> later patches in the series will add.
>
> The aforementioned array flattening logic will greatly increase the
> number of btf_field_info's needed to describe some structs, so this
> patch also turns the statically-sized struct btf_field_info
> info_arr[BTF_FIELDS_MAX] into a growable array with a larger max size.
>
> Implementation notes:
> * BTF_FIELDS_MAX is now max size of growable btf_field_info *infos
> instead of initial (and max) size of static result array
> * Static array before had 10 elems (+1 tmp btf_field_info)
> * growable array starts with 16 and doubles every time it needs to
> grow, up to BTF_FIELDS_MAX of 256
>
> * __next_field_infos is used with next_cnt > 1 later in the series
>
> * btf_find_{datasec_var, struct_field} have special logic for an edge
> case where the result array is full but the field being examined
> gets BTF_FIELD_IGNORE return from btf_find_{struct, kptr,graph_root}
> * If result wasn't BTF_FIELD_IGNORE, a btf_field_info would have to
> be added to the array. Since it is we can look at next field.
> * Before this patch the logic handling this edge case was hard to
> follow and used a tmp btf_struct_info. This patch moves the
> add-if-not-ignore logic down into btf_find_{struct, kptr,
> graph_root}, removing the need to opportunistically grab a
> btf_field_info to populate before knowing if it's actually
> necessary. Now a new one is grabbed only if the field shouldn't
> be ignored.
>
> * Within btf_find_{datasec_var, struct_field}, each member is
> currently examined in two phases: first btf_get_field_type checks
> the member type name, then btf_find_{struct,graph_root,kptr} do
> additional sanity checking and populate struct btf_field_info. Kptr
> fields don't have a specific type name, though, so
> btf_get_field_type assumes that - if we're looking for kptrs - any
> member that fails type name check could be a kptr field.
> * As a result btf_find_kptr effectively does all the pointer
> hopping, sanity checking, and info population. Instead of trying
> to fit kptr handling into this two-phase model, where it's
> unwieldy, handle it in a separate codepath when name matching
> fails.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
> include/linux/bpf.h | 4 +-
> kernel/bpf/btf.c | 331 +++++++++++++++++++++++++++++---------------
> 2 files changed, 219 insertions(+), 116 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b4825d3cdb29..e07cac5cc3cf 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -171,8 +171,8 @@ struct bpf_map_ops {
> };
>
> enum {
> - /* Support at most 10 fields in a BTF type */
> - BTF_FIELDS_MAX = 10,
> + /* Support at most 256 fields in a BTF type */
> + BTF_FIELDS_MAX = 256,
> };
>
> enum btf_field_type {
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 975ef8e73393..e999ba85c363 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3257,25 +3257,94 @@ struct btf_field_info {
> };
> };
>
> +struct btf_field_info_search {
> + /* growable array. allocated in __next_field_infos
> + * free'd in btf_parse_fields
> + */
> + struct btf_field_info *infos;
> + /* size of infos */
> + int info_cnt;
> + /* index of next btf_field_info to populate */
> + int idx;
this seems pretty unconventional naming, typically info_cnt would be
called "cap" (for capacity) and idx would be "cnt" (number of actually
filled elements)
> +
> + /* btf_field_types to search for */
> + u32 field_mask;
> + /* btf_field_types found earlier */
> + u32 seen_mask;
> +};
> +
> +/* Reserve next_cnt contiguous btf_field_info's for caller to populate
> + * Returns ptr to first reserved btf_field_info
> + */
> +static struct btf_field_info *__next_field_infos(struct btf_field_info_search *srch,
> + u32 next_cnt)
both next_cnt and __next_field_infos that do allocation are quite
surprising, this is a function to add/append more elements, so why not
add_field_infos() and new_cnt or add_cnt? The terminology around
"next" is confusing, IMO, because you'd expect this to be about
iteration, not memory allocation.
> +{
> + struct btf_field_info *new_infos, *ret;
> +
> + if (!next_cnt)
> + return ERR_PTR(-EINVAL);
> +
> + if (srch->idx + next_cnt < srch->info_cnt)
> + goto nogrow_out;
why goto? just update count and return a pointer? Goto makes sense if
there is at least two code paths with the same exit logic.
> +
> + /* Need to grow */
> + if (srch->idx + next_cnt > BTF_FIELDS_MAX)
> + return ERR_PTR(-E2BIG);
> +
> + while (srch->idx + next_cnt >= srch->info_cnt)
> + srch->info_cnt = srch->infos ? srch->info_cnt * 2 : 16;
I think krealloc() is smart enough to not allocate exact number of
bytes, and instead is rounding it up to closes bucket size. So I think
you can just keep it simple and ask for `srch->idx + next_cnt`
elements, and leave smartness to krealloc(). Not sure why 16 is the
starting size, though? How many elements do we typically have? 1, 2,
3, 5?
> +
> + new_infos = krealloc(srch->infos,
> + srch->info_cnt * sizeof(struct btf_field_info),
> + GFP_KERNEL | __GFP_NOWARN);
> + if (!new_infos)
> + return ERR_PTR(-ENOMEM);
> + srch->infos = new_infos;
> +
> +nogrow_out:
> + ret = &srch->infos[srch->idx];
> + srch->idx += next_cnt;
> + return ret;
> +}
> +
> +/* Request srch's next free btf_field_info to populate, possibly growing
> + * srch->infos
> + */
> +static struct btf_field_info *__next_field_info(struct btf_field_info_search *srch)
> +{
> + return __next_field_infos(srch, 1);
> +}
> +
> static int btf_find_struct(const struct btf *btf, const struct btf_type *t,
> u32 off, int sz, enum btf_field_type field_type,
> - struct btf_field_info *info)
> + struct btf_field_info_search *srch)
> {
> + struct btf_field_info *info;
> +
> if (!__btf_type_is_struct(t))
> return BTF_FIELD_IGNORE;
> if (t->size != sz)
> return BTF_FIELD_IGNORE;
> +
> + info = __next_field_info(srch);
> + if (IS_ERR_OR_NULL(info))
Can it return NULL? If not, let's not check for NULL at all, it's misleading.
> + return PTR_ERR(info);
> +
> info->type = field_type;
> info->off = off;
> return BTF_FIELD_FOUND;
> }
>
> -static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
> - u32 off, int sz, struct btf_field_info *info)
> +static int btf_maybe_find_kptr(const struct btf *btf, const struct btf_type *t,
> + u32 off, struct btf_field_info_search *srch)
> {
> + struct btf_field_info *info;
> enum btf_field_type type;
> u32 res_id;
>
> + if (!(srch->field_mask & BPF_KPTR))
> + return BTF_FIELD_IGNORE;
> +
> /* Permit modifiers on the pointer itself */
> if (btf_type_is_volatile(t))
> t = btf_type_by_id(btf, t->type);
> @@ -3304,6 +3373,10 @@ static int btf_find_kptr(const struct btf *btf, const struct btf_type *t,
> if (!__btf_type_is_struct(t))
> return -EINVAL;
>
> + info = __next_field_info(srch);
> + if (IS_ERR_OR_NULL(info))
> + return PTR_ERR(info);
> +
ditto
> info->type = type;
> info->off = off;
> info->kptr.type_id = res_id;
[...]
> #undef field_mask_test_name_check_seen
> #undef field_mask_test_name
>
> +static int __struct_member_check_align(u32 off, enum btf_field_type field_type)
> +{
> + u32 align = btf_field_type_align(field_type);
> +
> + if (off % align)
maybe guard against zero division? WARN_ON_ONCE() in
btf_field_type_align() shouldn't cause crash here
> + return -EINVAL;
> + return 0;
> +}
> +
> static int btf_find_struct_field(const struct btf *btf,
> - const struct btf_type *t, u32 field_mask,
> - struct btf_field_info *info, int info_cnt)
> + const struct btf_type *t,
> + struct btf_field_info_search *srch)
> {
> - int ret, idx = 0, align, sz, field_type;
> const struct btf_member *member;
> - struct btf_field_info tmp;
> - u32 i, off, seen_mask = 0;
> + int ret, field_type;
> + u32 i, off, sz;
>
[...]
> +
> +static int __datasec_vsi_check_align_sz(const struct btf_var_secinfo *vsi,
> + enum btf_field_type field_type,
> + u32 expected_sz)
> +{
> + u32 off, align;
> +
> + off = vsi->offset;
> + align = btf_field_type_align(field_type);
> +
> + if (vsi->size != expected_sz)
> + return -EINVAL;
> + if (off % align)
> + return -EINVAL;
same about possible align == 0
> +
> + return 0;
> }
>
[...]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search
2023-10-23 22:00 ` [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search Dave Marchevsky
` (2 preceding siblings ...)
2023-10-30 20:56 ` Yonghong Song
@ 2023-11-01 21:56 ` Andrii Nakryiko
3 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2023-11-01 21:56 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team
On Mon, Oct 23, 2023 at 3:00 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> Structs and arrays are aggregate types which contain some inner
> type(s) - members and elements - at various offsets. Currently, when
> examining a struct or datasec for special fields, the verifier does
> not look into the inner type of the structs or arrays it contains.
> This patch adds logic to descend into struct and array types when
> searching for special fields.
>
> If we have struct x containing an array:
>
> struct x {
> int a;
> u64 b[10];
> };
>
> we can construct some struct y with no array or struct members that
> has the same types at the same offsets:
>
> struct y {
> int a;
> u64 b1;
> u64 b2;
> /* ... */
> u64 b10;
> };
>
> Similarly for a struct containing a struct:
>
> struct x {
> char a;
> struct {
> int b;
> u64 c;
> } inner;
> };
>
> there's a struct y with no aggregate members and same types/offsets:
>
> struct y {
> char a;
> int inner_b __attribute__ ((aligned (8))); /* See [0] */
> u64 inner_c __attribute__ ((aligned (8)));
> };
>
> This patch takes advantage of this equivalence to 'flatten' the
> field info found while descending into struct or array members into
> the btf_field_info result array of the original type being examined.
> The resultant btf_record of the original type being searched will
> have the correct fields at the correct offsets, but without any
> differentiation between "this field is one of my members" and "this
> field is actually in my some struct / array member".
>
> For now this descendant search logic looks for kptr fields only.
>
> Implementation notes:
> * Search starts at btf_find_field - we're either looking at a struct
> that's the type of some mapval (btf_find_struct_field), or a
> datasec representing a .bss or .data map (btf_find_datasec_var).
> Newly-added btf_find_aggregate_field is a "disambiguation helper"
> like btf_find_field, but is meant to be called from one of the
> starting points of the search - btf_find_{struct_field,
> datasec_var}.
> * btf_find_aggregate_field may itself call btf_find_struct_field,
> so there's some recursive digging possible here
>
> * Newly-added btf_flatten_array_field handles array fields by
> finding the type of their element and continuing the dig based on
> elem type.
>
> [0]: Structs have the alignment of their largest field, so the
> explicit alignment is necessary here. Luckily this patch's
> changes don't need to care about alignment / padding, since
> the BTF created during compilation is being searched, and
> it already has the correct information.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
> kernel/bpf/btf.c | 151 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 142 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index e999ba85c363..b982bf6fef9d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3496,9 +3496,41 @@ static int __struct_member_check_align(u32 off, enum btf_field_type field_type)
> return 0;
> }
>
> +/* Return number of elems and elem_type of a btf_array
> + *
> + * If the array is multi-dimensional, return elem count of
> + * equivalent single-dimensional array
> + * e.g. int x[10][10][10] has same layout as int x[1000]
> + */
> +static u32 __multi_dim_elem_type_nelems(const struct btf *btf,
What's the purpose of these double underscored names? Are we avoiding
a naming conflict here?
> + const struct btf_type *t,
> + const struct btf_type **elem_type)
> +{
> + u32 nelems = btf_array(t)->nelems;
> +
> + if (!nelems)
> + return 0;
> +
> + *elem_type = btf_type_by_id(btf, btf_array(t)->type);
> +
> + while (btf_type_is_array(*elem_type)) {
you need to strip modifiers and typedefs, presumably?
> + if (!btf_array(*elem_type)->nelems)
> + return 0;
I agree with Yonghong, this duplicated nelems == 0 check does look a
bit sloppy and unsure :) I'd rather us handle zero naturally
> + nelems *= btf_array(*elem_type)->nelems;
check for overflow?
> + *elem_type = btf_type_by_id(btf, btf_array(*elem_type)->type);
think about skipping modifiers and maybe typedefs?
> + }
> + return nelems;
> +}
> +
> +static int btf_find_aggregate_field(const struct btf *btf,
> + const struct btf_type *t,
> + struct btf_field_info_search *srch,
> + int field_off, int rec);
> +
> static int btf_find_struct_field(const struct btf *btf,
> const struct btf_type *t,
> - struct btf_field_info_search *srch)
> + struct btf_field_info_search *srch,
> + int struct_field_off, int rec)
> {
> const struct btf_member *member;
> int ret, field_type;
> @@ -3522,10 +3554,24 @@ static int btf_find_struct_field(const struct btf *btf,
> * checks, all ptrs have same align.
> * btf_maybe_find_kptr will find actual kptr type
> */
> - if (__struct_member_check_align(off, BPF_KPTR_REF))
> + if (srch->field_mask & BPF_KPTR &&
nit: () around & operation?
> + !__struct_member_check_align(off, BPF_KPTR_REF)) {
> + ret = btf_maybe_find_kptr(btf, member_type,
> + struct_field_off + off,
> + srch);
nit: does it fit in under 100 characters? If yes, go for it.
> + if (ret < 0)
> + return ret;
> + if (ret == BTF_FIELD_FOUND)
> + continue;
> + }
> +
> + if (!(btf_type_is_array(member_type) ||
> + __btf_type_is_struct(member_type)))
> continue;
>
> - ret = btf_maybe_find_kptr(btf, member_type, off, srch);
> + ret = btf_find_aggregate_field(btf, member_type, srch,
> + struct_field_off + off,
> + rec);
> if (ret < 0)
> return ret;
> continue;
> @@ -3541,15 +3587,17 @@ static int btf_find_struct_field(const struct btf *btf,
> case BPF_LIST_NODE:
> case BPF_RB_NODE:
> case BPF_REFCOUNT:
> - ret = btf_find_struct(btf, member_type, off, sz, field_type,
> - srch);
> + ret = btf_find_struct(btf, member_type,
> + struct_field_off + off,
> + sz, field_type, srch);
> if (ret < 0)
> return ret;
> break;
> case BPF_LIST_HEAD:
> case BPF_RB_ROOT:
> ret = btf_find_graph_root(btf, t, member_type,
> - i, off, sz, srch, field_type);
> + i, struct_field_off + off, sz,
> + srch, field_type);
> if (ret < 0)
> return ret;
> break;
> @@ -3566,6 +3614,82 @@ static int btf_find_struct_field(const struct btf *btf,
> return srch->idx;
> }
>
> +static int btf_flatten_array_field(const struct btf *btf,
> + const struct btf_type *t,
> + struct btf_field_info_search *srch,
> + int array_field_off, int rec)
> +{
> + int ret, start_idx, elem_field_cnt;
> + const struct btf_type *elem_type;
> + struct btf_field_info *info;
> + u32 i, j, off, nelems;
> +
> + if (!btf_type_is_array(t))
> + return -EINVAL;
> + nelems = __multi_dim_elem_type_nelems(btf, t, &elem_type);
> + if (!nelems || !__btf_type_is_struct(elem_type))
and typedef fails this check, so yeah, you do need to strip typedefs
> + return srch->idx;
> +
> + start_idx = srch->idx;
> + ret = btf_find_struct_field(btf, elem_type, srch, array_field_off + off, rec);
> + if (ret < 0)
> + return ret;
> +
> + /* No btf_field_info's added */
> + if (srch->idx == start_idx)
> + return srch->idx;
> +
> + elem_field_cnt = srch->idx - start_idx;
> + info = __next_field_infos(srch, elem_field_cnt * (nelems - 1));
> + if (IS_ERR_OR_NULL(info))
> + return PTR_ERR(info);
> +
> + /* Array elems after the first can copy first elem's btf_field_infos
> + * and adjust offset
> + */
> + for (i = 1; i < nelems; i++) {
> + memcpy(info, &srch->infos[start_idx],
> + elem_field_cnt * sizeof(struct btf_field_info));
> + for (j = 0; j < elem_field_cnt; j++) {
nit: instead of memcpy above, why not just
*info = srch->infos[start_idx + j];
inside the loop?
It seems a bit more natural in this case, as you are adjusting each
copied element either way.
Or, you know, if we go with memcpy, then why not single memcpy() with
elem_field_cnt * (nelems - 1) elements?
> + info->off += (i * elem_type->size);
> + info++;
can you please check if zero-sized structs are handled (and probably
rejected) correctly?
E.g.:
struct my_struct {
struct fancy_kptr __kptr kptrs[0];
}
> + }
> + }
> + return srch->idx;
> +}
> +
[...]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v1 bpf-next 4/4] selftests/bpf: Add tests exercising aggregate type BTF field search
2023-10-23 22:00 ` [PATCH v1 bpf-next 4/4] selftests/bpf: Add tests exercising aggregate type BTF " Dave Marchevsky
2023-10-23 23:32 ` kernel test robot
2023-10-30 21:10 ` Yonghong Song
@ 2023-11-01 21:56 ` Andrii Nakryiko
2 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2023-11-01 21:56 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Kernel Team
On Mon, Oct 23, 2023 at 3:01 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> The newly-added test file attempts to kptr_xchg a prog_test_ref_kfunc
> kptr into a kptr field in a variety of nested aggregate types. If the
> verifier recognizes that there's a kptr field where we're trying to
> kptr_xchg, then the aggregate type digging logic works as expected.
>
> Some of the refactoring changes in this series are tested as well.
> Specifically:
> * BTF_FIELDS_MAX is now higher and represents the max size of the
> growable array. Confirm that btf_parse_fields fails for a type which
> contains too many fields.
> * If we've already seen BTF_FIELDS_MAX fields, we should continue
> looking for fields and fail if we find another one, otherwise the
> search should succeed and return BTF_FIELDS_MAX btf_field_infos.
> Confirm that this edge case works as expected.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
> .../selftests/bpf/prog_tests/array_kptr.c | 12 ++
> .../testing/selftests/bpf/progs/array_kptr.c | 179 ++++++++++++++++++
> 2 files changed, 191 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/array_kptr.c
> create mode 100644 tools/testing/selftests/bpf/progs/array_kptr.c
>
[...]
> +SEC("tc")
> +__failure __msg("map '.bss.A' has no valid kptr")
> +int test_array_fail__too_big(void *ctx)
> +{
> + /* array_too_big's btf_record parsing will fail due to the
> + * number of btf_field_infos being > BTF_FIELDS_MAX
> + */
> + return test_array_xchg(&array_too_big[50]);
how often in practice people are going to index such arrays with a
fixed index? I'd imagine it's normally going to be a calculated index
based on something (CPU ID or whatnot). Is that supported? Can we add
a test for that?
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-11-01 21:57 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-23 22:00 [PATCH v1 bpf-next 0/4] Descend into struct, array types when searching for fields Dave Marchevsky
2023-10-23 22:00 ` [PATCH v1 bpf-next 1/4] bpf: Fix btf_get_field_type to fail for multiple bpf_refcount fields Dave Marchevsky
2023-10-30 17:56 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
2023-10-23 22:00 ` [PATCH v1 bpf-next 2/4] bpf: Refactor btf_find_field with btf_field_info_search Dave Marchevsky
2023-10-28 14:52 ` Jiri Olsa
2023-10-30 19:31 ` Yonghong Song
2023-10-30 19:56 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
2023-10-23 22:00 ` [PATCH v1 bpf-next 3/4] btf: Descend into structs and arrays during special field search Dave Marchevsky
2023-10-26 1:24 ` kernel test robot
2023-10-30 12:56 ` Jiri Olsa
2023-10-30 20:56 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
2023-10-23 22:00 ` [PATCH v1 bpf-next 4/4] selftests/bpf: Add tests exercising aggregate type BTF " Dave Marchevsky
2023-10-23 23:32 ` kernel test robot
2023-10-30 21:10 ` Yonghong Song
2023-11-01 21:56 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox