On Wed, 2024-05-01 at 13:47 -0700, Kui-Feng Lee wrote:
> The verifier has field information for specific special types, such as
> kptr, rbtree root, and list head. These types are handled
> differently. However, we did not previously examine the types of fields of
> a struct type variable. Field information records were not generated for
> the kptrs, rbtree roots, and linked_list heads that are not located at the
> outermost struct type of a variable.
>
> For example,
>
> struct A {
> struct task_struct __kptr * task;
> };
>
> struct B {
> struct A mem_a;
> }
>
> struct B var_b;
>
> It did not examine "struct A" so as not to generate field information for
> the kptr in "struct A" for "var_b".
>
> This patch enables BPF programs to define fields of these special types in
> a struct type other than the direct type of a variable or in a struct type
> that is the type of a field in the value type of a map.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> ---
I think that the main logic of this commit is fine.
A few nitpicks about code organization below.
> kernel/bpf/btf.c | 118 +++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 98 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 4a78cd28fab0..2ceff77b7e71 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3493,41 +3493,83 @@ static int btf_get_field_type(const char *name, u32 field_mask, u32 *seen_mask,
[...]
> +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);
> +
> +/* Find special fields in the struct type of a field.
> + *
> + * This function is used to find fields of special types that is not a
> + * global variable or a direct field of a struct type. It also handles the
> + * repetition if it is the element type of an array.
> + */
> +static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *t,
> + u32 off, u32 nelems,
> + u32 field_mask, struct btf_field_info *info,
> + int info_cnt)
> +{
> + int ret, err, i;
> +
> + ret = btf_find_struct_field(btf, t, field_mask, info, info_cnt);
btf_find_nested_struct() and btf_find_struct_field() are mutually recursive,
as far as I can see this is usually avoided in kernel source.
Would it be possible to make stack explicit or limit traversal depth here?
The 'info_cnt' field won't work as it could be unchanged in
btf_find_struct_field() if 'idx == 0'.
> +
> + if (ret <= 0)
> + return ret;
> +
> + /* Shift the offsets of the nested struct fields to the offsets
> + * related to the container.
> + */
> + for (i = 0; i < ret; i++)
> + info[i].off += off;
> +
> + if (nelems > 1) {
> + err = btf_repeat_fields(info, 0, ret, nelems - 1, t->size);
> + if (err == 0)
> + ret *= nelems;
> + else
> + ret = err;
> + }
> +
> + return ret;
> +}
> +
> 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)
[...]
> @@ -3644,6 +3707,21 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
>
> field_type = btf_get_field_type(__btf_name_by_offset(btf, var_type->name_off),
> field_mask, &seen_mask, &align, &sz);
Actions taken for members in btf_find_datasec_var() and
btf_find_struct_field() are almost identical, would it be possible to
add a refactoring commit this patch-set so that common logic is moved
to a separate function? It looks like this function would have to be
parameterized only by member size and offset.
> + /* Look into variables of struct types */
> + if ((field_type == BPF_KPTR_REF || !field_type) &&
> + __btf_type_is_struct(var_type)) {
> + sz = var_type->size;
> + if (vsi->size != sz * nelems)
> + continue;
> + off = vsi->offset;
> + ret = btf_find_nested_struct(btf, var_type, off, nelems, field_mask,
> + &info[idx], info_cnt - idx);
> + if (ret < 0)
> + return ret;
> + idx += ret;
> + continue;
> + }
> +
> if (field_type == 0)
> continue;
> if (field_type < 0)
[...]
