Re: [PATCH bpf-next v3 4/7] bpf: look into the types of the fields of a struct type recursively.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 5/2/24 12:34, Eduard Zingerman wrote:
> 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@gmail.com>
>> ---
> 
> 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?

Sure!

> 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.

Of course, it could be.

> 
>> +		/* 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)
> 
> [...]