[bpf-next,05/11] bpf: initialize/free array of btf_field(s).

Commit Message

Kui-Feng Lee April 10, 2024, 12:41 a.m. UTC

Initialize and free each element in a btf_field array based on the values
of nelems and size in btf_field. The value of nelems is the length of the
flatten array for nested arrays.

Signed-off-by: Kui-Feng Lee <[e-mail]>
---
 include/linux/bpf.h  |  7 +++++++
 kernel/bpf/syscall.c | 39 ++++++++++++++++++++++++---------------
 2 files changed, 31 insertions(+), 15 deletions(-)

Comments

Eduard Zingerman April 11, 2024, 10:13 p.m. UTC | #1

On Tue, 2024-04-09 at 17:41 -0700, Kui-Feng Lee wrote:
[...]

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index f397ccdc6d4b..ee53dcd14bd4 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -390,6 +390,9 @@ static inline u32 btf_field_type_align(enum btf_field_type type)
>  
>  static inline void bpf_obj_init_field(const struct btf_field *field, void *addr)
>  {
> +	u32 elem_size;
> +	int i;
> +
>  	memset(addr, 0, field->size);
>  
>  	switch (field->type) {
> @@ -400,6 +403,10 @@ static inline void bpf_obj_init_field(const struct btf_field *field, void *addr)
>  		RB_CLEAR_NODE((struct rb_node *)addr);
>  		break;
>  	case BPF_LIST_HEAD:
> +		elem_size = field->size / field->nelems;
> +		for (i = 0; i < field->nelems; i++, addr += elem_size)
> +			INIT_LIST_HEAD((struct list_head *)addr);
> +		break;

In btf_find_datasec_var() nelem > 1 is allowed for the following types:
- BPF_LIST_{NODE,HEAD}
- BPF_KPTR_{REF,UNREF,PERCPU}:
- BPF_RB_{NODE,ROOT}

Of these types bpf_obj_init_field() handles in a special way
BPF_RB_NODE, BPF_LIST_HEAD and BPF_LIST_NODE.
However, only BPF_LIST_HEAD handling is adjusted in this patch.
Is there a reason to omit BPF_RB_NODE and BPF_LIST_NODE?

>  	case BPF_LIST_NODE:
>  		INIT_LIST_HEAD((struct list_head *)addr);
>  		break;

[...]

Kui-Feng Lee April 12, 2024, 3:56 a.m. UTC | #2

On 4/11/24 15:13, Eduard Zingerman wrote:
> On Tue, 2024-04-09 at 17:41 -0700, Kui-Feng Lee wrote:
> [...]
> 
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index f397ccdc6d4b..ee53dcd14bd4 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -390,6 +390,9 @@ static inline u32 btf_field_type_align(enum btf_field_type type)
>>   
>>   static inline void bpf_obj_init_field(const struct btf_field *field, void *addr)
>>   {
>> +	u32 elem_size;
>> +	int i;
>> +
>>   	memset(addr, 0, field->size);
>>   
>>   	switch (field->type) {
>> @@ -400,6 +403,10 @@ static inline void bpf_obj_init_field(const struct btf_field *field, void *addr)
>>   		RB_CLEAR_NODE((struct rb_node *)addr);
>>   		break;
>>   	case BPF_LIST_HEAD:
>> +		elem_size = field->size / field->nelems;
>> +		for (i = 0; i < field->nelems; i++, addr += elem_size)
>> +			INIT_LIST_HEAD((struct list_head *)addr);
>> +		break;
> 
> In btf_find_datasec_var() nelem > 1 is allowed for the following types:
> - BPF_LIST_{NODE,HEAD}
> - BPF_KPTR_{REF,UNREF,PERCPU}:
> - BPF_RB_{NODE,ROOT}
> 
> Of these types bpf_obj_init_field() handles in a special way
> BPF_RB_NODE, BPF_LIST_HEAD and BPF_LIST_NODE.
> However, only BPF_LIST_HEAD handling is adjusted in this patch.
> Is there a reason to omit BPF_RB_NODE and BPF_LIST_NODE?

Declaring arrays of list nodes or rbtree nodes seems to be not useful
since they don't contain any useful information. Let me explain below.

We usually declare node fields in other struct types. I guess declaring
arrays of struct types containing node fields is what we actually want.
For example,

   struct foo {
      struct bpf_list_node node;
      ...
   };

   struct foo all_nodes[64];

However, the verifier doesn't look into fields of a outer struct type
except array fields handling by this patchset. In this case, a data
section, it doesn't look into foo even we declare a global variable of
struct foo. For example,

   struct foo gFoo;

gFoo would not work since the verifier doesn't follow gFoo and look into
struct foo.

So, I decided not to support rbtree nodes and list nodes.

However, there are a discussion about looking into fields of struct
types in an outer struct type. So, we will have another patchset to
implement it. Once we have it, we can support the case of gFoo and
all_nodes described earlier.


> 
>>   	case BPF_LIST_NODE:
>>   		INIT_LIST_HEAD((struct list_head *)addr);
>>   		break;
> 
> [...]
>

Eduard Zingerman April 12, 2024, 3:32 p.m. UTC | #3

On Thu, 2024-04-11 at 20:56 -0700, Kui-Feng Lee wrote:
[...]

> So, I decided not to support rbtree nodes and list nodes.
> 
> However, there are a discussion about looking into fields of struct
> types in an outer struct type. So, we will have another patchset to
> implement it. Once we have it, we can support the case of gFoo and
> all_nodes described earlier.

All this makes sense, but in such a case would it make sense to adjust
btf_find_datasec_var() to avoid adding BPF_RB_NODE and BPF_LIST_NODE
variables with nelem > 1? (Like it does for BPF_TIMER etc).

Kui-Feng Lee April 12, 2024, 5 p.m. UTC | #4

On 4/12/24 08:32, Eduard Zingerman wrote:
> On Thu, 2024-04-11 at 20:56 -0700, Kui-Feng Lee wrote:
> [...]
> 
>> So, I decided not to support rbtree nodes and list nodes.
>>
>> However, there are a discussion about looking into fields of struct
>> types in an outer struct type. So, we will have another patchset to
>> implement it. Once we have it, we can support the case of gFoo and
>> all_nodes described earlier.
> 
> All this makes sense, but in such a case would it make sense to adjust
> btf_find_datasec_var() to avoid adding BPF_RB_NODE and BPF_LIST_NODE
> variables with nelem > 1? (Like it does for BPF_TIMER etc).

That make sense. I will change it.

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f397ccdc6d4b..ee53dcd14bd4 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -390,6 +390,9 @@  static inline u32 btf_field_type_align(enum btf_field_type type)
 
 static inline void bpf_obj_init_field(const struct btf_field *field, void *addr)
 {
+	u32 elem_size;
+	int i;
+
 	memset(addr, 0, field->size);
 
 	switch (field->type) {
@@ -400,6 +403,10 @@  static inline void bpf_obj_init_field(const struct btf_field *field, void *addr)
 		RB_CLEAR_NODE((struct rb_node *)addr);
 		break;
 	case BPF_LIST_HEAD:
+		elem_size = field->size / field->nelems;
+		for (i = 0; i < field->nelems; i++, addr += elem_size)
+			INIT_LIST_HEAD((struct list_head *)addr);
+		break;
 	case BPF_LIST_NODE:
 		INIT_LIST_HEAD((struct list_head *)addr);
 		break;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index e44c276e8617..543ff0d944e8 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -672,6 +672,8 @@  void bpf_obj_free_fields(const struct btf_record *rec, void *obj)
 		const struct btf_field *field = &fields[i];
 		void *field_ptr = obj + field->offset;
 		void *xchgd_field;
+		u32 elem_size = field->size / field->nelems;
+		int j;
 
 		switch (fields[i].type) {
 		case BPF_SPIN_LOCK:
@@ -680,35 +682,42 @@  void bpf_obj_free_fields(const struct btf_record *rec, void *obj)
 			bpf_timer_cancel_and_free(field_ptr);
 			break;
 		case BPF_KPTR_UNREF:
-			WRITE_ONCE(*(u64 *)field_ptr, 0);
+			for (j = 0; j < field->nelems; j++, field_ptr += elem_size)
+				WRITE_ONCE(*(u64 *)field_ptr, 0);
 			break;
 		case BPF_KPTR_REF:
 		case BPF_KPTR_PERCPU:
-			xchgd_field = (void *)xchg((unsigned long *)field_ptr, 0);
-			if (!xchgd_field)
-				break;
-
-			if (!btf_is_kernel(field->kptr.btf)) {
+			if (!btf_is_kernel(field->kptr.btf))
 				pointee_struct_meta = btf_find_struct_meta(field->kptr.btf,
 									   field->kptr.btf_id);
-				migrate_disable();
-				__bpf_obj_drop_impl(xchgd_field, pointee_struct_meta ?
-								 pointee_struct_meta->record : NULL,
-								 fields[i].type == BPF_KPTR_PERCPU);
-				migrate_enable();
-			} else {
-				field->kptr.dtor(xchgd_field);
+
+			for (j = 0; j < field->nelems; j++, field_ptr += elem_size) {
+				xchgd_field = (void *)xchg((unsigned long *)field_ptr, 0);
+				if (!xchgd_field)
+					continue;
+
+				if (!btf_is_kernel(field->kptr.btf)) {
+					migrate_disable();
+					__bpf_obj_drop_impl(xchgd_field, pointee_struct_meta ?
+							    pointee_struct_meta->record : NULL,
+							    fields[i].type == BPF_KPTR_PERCPU);
+					migrate_enable();
+				} else {
+					field->kptr.dtor(xchgd_field);
+				}
 			}
 			break;
 		case BPF_LIST_HEAD:
 			if (WARN_ON_ONCE(rec->spin_lock_off < 0))
 				continue;
-			bpf_list_head_free(field, field_ptr, obj + rec->spin_lock_off);
+			for (j = 0; j < field->nelems; j++, field_ptr += elem_size)
+				bpf_list_head_free(field, field_ptr, obj + rec->spin_lock_off);
 			break;
 		case BPF_RB_ROOT:
 			if (WARN_ON_ONCE(rec->spin_lock_off < 0))
 				continue;
-			bpf_rb_root_free(field, field_ptr, obj + rec->spin_lock_off);
+			for (j = 0; j < field->nelems; j++, field_ptr += elem_size)
+				bpf_rb_root_free(field, field_ptr, obj + rec->spin_lock_off);
 			break;
 		case BPF_LIST_NODE:
 		case BPF_RB_NODE: