| Message ID | 20231023220030.2556229-4-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Descend into struct, array types when searching for fields | expand |
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
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 > >
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; > +} > + > [...]
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; > +} > + [...]
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;
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(-)