[bpf-next,v3,3/7] bpf: create repeated fields for arrays.

Commit Message

Kui-Feng Lee May 1, 2024, 8:47 p.m. UTC

The verifier uses field information for certain special types, such as
kptr, rbtree root, and list head. These types are treated
differently. However, we did not previously support these types in
arrays. This update examines arrays and duplicates field information the
same number of times as the length of the array if the element type is one
of the special types.

Signed-off-by: Kui-Feng Lee <[e-mail]>
---
 kernel/bpf/btf.c | 81 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 5 deletions(-)

Comments

Eduard Zingerman May 2, 2024, 5:20 p.m. UTC | #1

On Wed, 2024-05-01 at 13:47 -0700, Kui-Feng Lee wrote:

I think this looks good for repeating fields of nested arrays
(w/o visiting nested structures), two nits below.

> @@ -3575,6 +3628,19 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
>  	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);
> +		const struct btf_array *array;
> +		u32 j, nelems = 1;
> +
> +		/* Walk into array types to find the element type and the
> +		 * number of elements in the (flattened) array.
> +		 */
> +		for (j = 0; j < MAX_RESOLVE_DEPTH && btf_type_is_array(var_type); j++) {
> +			array = btf_array(var_type);
> +			nelems *= array->nelems;
> +			var_type = btf_type_by_id(btf, array->type);
> +		}
> +		if (nelems == 0)
> +			continue;

Nit: Should this return an error if j == MAX_RESOLVE_DEPTH after the loop?

>  
>  		field_type = btf_get_field_type(__btf_name_by_offset(btf, var_type->name_off),
>  						field_mask, &seen_mask, &align, &sz);
> @@ -3584,7 +3650,7 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
>  			return field_type;
>  
>  		off = vsi->offset;
> -		if (vsi->size != sz)
> +		if (vsi->size != sz * nelems)
>  			continue;
>  		if (off % align)
>  			continue;
> @@ -3624,9 +3690,14 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
>  
>  		if (ret == BTF_FIELD_IGNORE)
>  			continue;
> -		if (idx >= info_cnt)
> +		if (idx + nelems > info_cnt)
>  			return -E2BIG;

Nit: This is bounded by BTF_FIELDS_MAX which has value of 11,
     would that be enough?

> -		++idx;
> +		if (nelems > 1) {
> +			ret = btf_repeat_field(info, idx, nelems - 1, sz);
> +			if (ret < 0)
> +				return ret;
> +		}
> +		idx += nelems;
>  	}
>  	return idx;
>  }

Kui-Feng Lee May 3, 2024, 6:02 p.m. UTC | #2

On 5/2/24 10:20, Eduard Zingerman wrote:
> On Wed, 2024-05-01 at 13:47 -0700, Kui-Feng Lee wrote:
> 
> I think this looks good for repeating fields of nested arrays
> (w/o visiting nested structures), two nits below.
> 
>> @@ -3575,6 +3628,19 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
>>   	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);
>> +		const struct btf_array *array;
>> +		u32 j, nelems = 1;
>> +
>> +		/* Walk into array types to find the element type and the
>> +		 * number of elements in the (flattened) array.
>> +		 */
>> +		for (j = 0; j < MAX_RESOLVE_DEPTH && btf_type_is_array(var_type); j++) {
>> +			array = btf_array(var_type);
>> +			nelems *= array->nelems;
>> +			var_type = btf_type_by_id(btf, array->type);
>> +		}
>> +		if (nelems == 0)
>> +			continue;
> 
> Nit: Should this return an error if j == MAX_RESOLVE_DEPTH after the loop?

agree


> 
>>   
>>   		field_type = btf_get_field_type(__btf_name_by_offset(btf, var_type->name_off),
>>   						field_mask, &seen_mask, &align, &sz);
>> @@ -3584,7 +3650,7 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
>>   			return field_type;
>>   
>>   		off = vsi->offset;
>> -		if (vsi->size != sz)
>> +		if (vsi->size != sz * nelems)
>>   			continue;
>>   		if (off % align)
>>   			continue;
>> @@ -3624,9 +3690,14 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
>>   
>>   		if (ret == BTF_FIELD_IGNORE)
>>   			continue;
>> -		if (idx >= info_cnt)
>> +		if (idx + nelems > info_cnt)
>>   			return -E2BIG;
> 
> Nit: This is bounded by BTF_FIELDS_MAX which has value of 11,
>       would that be enough?

So far, no one has complained it yet!
But, some one will reach the limit in future.
If people want a flexible length, I will solve it in a follow-up.
WDYT?

> 
>> -		++idx;
>> +		if (nelems > 1) {
>> +			ret = btf_repeat_field(info, idx, nelems - 1, sz);
>> +			if (ret < 0)
>> +				return ret;
>> +		}
>> +		idx += nelems;
>>   	}
>>   	return idx;
>>   }
> 
>

Eduard Zingerman May 3, 2024, 6:10 p.m. UTC | #3

On Fri, 2024-05-03 at 11:02 -0700, Kui-Feng Lee wrote:

[...]

> > > @@ -3624,9 +3690,14 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
> > >   
> > >   		if (ret == BTF_FIELD_IGNORE)
> > >   			continue;
> > > -		if (idx >= info_cnt)
> > > +		if (idx + nelems > info_cnt)
> > >   			return -E2BIG;
> > 
> > Nit: This is bounded by BTF_FIELDS_MAX which has value of 11,
> >       would that be enough?
> 
> So far, no one has complained it yet!
> But, some one will reach the limit in future.
> If people want a flexible length, I will solve it in a follow-up.
> WDYT?

Sure, follow-up works.
Just that 11 is not much for an array.
I think sched_ext is the only user for this feature at the moment,
so you are in the best position to judge which size is appropriate.

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index d4c3342e2027..4a78cd28fab0 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3493,6 +3493,41 @@  static int btf_get_field_type(const char *name, u32 field_mask, u32 *seen_mask,
 
 #undef field_mask_test_name
 
+/* Repeat a field for a specified number of times.
+ *
+ * Copy and repeat a field for repeat_cnt
+ * times. The field is repeated by adding the offset of each field
+ * with
+ *   (i + 1) * elem_size
+ * where i is the repeat index and elem_size is the size of the element.
+ */
+static int btf_repeat_field(struct btf_field_info *info, u32 field,
+			    u32 repeat_cnt, u32 elem_size)
+{
+	u32 i;
+	u32 cur;
+
+	/* Ensure not repeating fields that should not be repeated. */
+	switch (info[field].type) {
+	case BPF_KPTR_UNREF:
+	case BPF_KPTR_REF:
+	case BPF_KPTR_PERCPU:
+	case BPF_LIST_HEAD:
+	case BPF_RB_ROOT:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	cur = field + 1;
+	for (i = 0; i < repeat_cnt; i++) {
+		memcpy(&info[cur], &info[field], sizeof(info[0]));
+		info[cur++].off += (i + 1) * elem_size;
+	}
+
+	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)
@@ -3505,6 +3540,19 @@  static int btf_find_struct_field(const struct btf *btf,
 	for_each_member(i, t, member) {
 		const struct btf_type *member_type = btf_type_by_id(btf,
 								    member->type);
+		const struct btf_array *array;
+		u32 j, nelems = 1;
+
+		/* Walk into array types to find the element type and the
+		 * number of elements in the (flattened) array.
+		 */
+		for (j = 0; j < MAX_RESOLVE_DEPTH && btf_type_is_array(member_type); j++) {
+			array = btf_array(member_type);
+			nelems *= array->nelems;
+			member_type = btf_type_by_id(btf, array->type);
+		}
+		if (nelems == 0)
+			continue;
 
 		field_type = btf_get_field_type(__btf_name_by_offset(btf, member_type->name_off),
 						field_mask, &seen_mask, &align, &sz);
@@ -3556,9 +3604,14 @@  static int btf_find_struct_field(const struct btf *btf,
 
 		if (ret == BTF_FIELD_IGNORE)
 			continue;
-		if (idx >= info_cnt)
+		if (idx + nelems > info_cnt)
 			return -E2BIG;
-		++idx;
+		if (nelems > 1) {
+			ret = btf_repeat_field(info, idx, nelems - 1, sz);
+			if (ret < 0)
+				return ret;
+		}
+		idx += nelems;
 	}
 	return idx;
 }
@@ -3575,6 +3628,19 @@  static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
 	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);
+		const struct btf_array *array;
+		u32 j, nelems = 1;
+
+		/* Walk into array types to find the element type and the
+		 * number of elements in the (flattened) array.
+		 */
+		for (j = 0; j < MAX_RESOLVE_DEPTH && btf_type_is_array(var_type); j++) {
+			array = btf_array(var_type);
+			nelems *= array->nelems;
+			var_type = btf_type_by_id(btf, array->type);
+		}
+		if (nelems == 0)
+			continue;
 
 		field_type = btf_get_field_type(__btf_name_by_offset(btf, var_type->name_off),
 						field_mask, &seen_mask, &align, &sz);
@@ -3584,7 +3650,7 @@  static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
 			return field_type;
 
 		off = vsi->offset;
-		if (vsi->size != sz)
+		if (vsi->size != sz * nelems)
 			continue;
 		if (off % align)
 			continue;
@@ -3624,9 +3690,14 @@  static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
 
 		if (ret == BTF_FIELD_IGNORE)
 			continue;
-		if (idx >= info_cnt)
+		if (idx + nelems > info_cnt)
 			return -E2BIG;
-		++idx;
+		if (nelems > 1) {
+			ret = btf_repeat_field(info, idx, nelems - 1, sz);
+			if (ret < 0)
+				return ret;
+		}
+		idx += nelems;
 	}
 	return idx;
 }