[v3,bpf-next,1/5] bpf: Let callers of btf_parse_kptr() track life cycle of prog btf

Commit Message

Amery Hung Aug. 9, 2024, 12:51 a.m. UTC

btf_parse_kptr() and btf_record_free() do btf_get() and btf_put()
respectively when working on btf_record in program and map if there are
kptr fields. If the kptr is from program BTF, since both callers has
already tracked the life cycle of program BTF, it is safe to remove the
btf_get() and btf_put().

This change prevents memory leak of program BTF later when we start
searching for kptr fields when building btf_record for program. It can
happen when the btf fd is closed. The btf_put() corresponding to the
btf_get() in btf_parse_kptr() was supposed to be called by
btf_record_free() in btf_free_struct_meta_tab() in btf_free(). However,
it will never happen since the invocation of btf_free() depends on the
refcount of the btf to become 0 in the first place.

Signed-off-by: Amery Hung <[e-mail]>
---
 kernel/bpf/btf.c     | 2 +-
 kernel/bpf/syscall.c | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

Comments

Martin KaFai Lau Aug. 9, 2024, 11:06 p.m. UTC | #1

On 8/8/24 5:51 PM, Amery Hung wrote:
> btf_parse_kptr() and btf_record_free() do btf_get() and btf_put()
> respectively when working on btf_record in program and map if there are
> kptr fields. If the kptr is from program BTF, since both callers has
> already tracked the life cycle of program BTF, it is safe to remove the
> btf_get() and btf_put().
> 
> This change prevents memory leak of program BTF later when we start
> searching for kptr fields when building btf_record for program. It can
> happen when the btf fd is closed. The btf_put() corresponding to the
> btf_get() in btf_parse_kptr() was supposed to be called by
> btf_record_free() in btf_free_struct_meta_tab() in btf_free(). However,
> it will never happen since the invocation of btf_free() depends on the
> refcount of the btf to become 0 in the first place.
> 
> Signed-off-by: Amery Hung <[e-mail]>

Acked-by: Martin KaFai Lau <[e-mail]>

Need to fix the checkpatch warning though:

WARNING: From:/Signed-off-by: email address mismatch: 'From: Amery Hung 
<[e-mail]>' != 'Signed-off-by: Amery Hung <[e-mail]>'

Martin KaFai Lau Aug. 10, 2024, 1:29 a.m. UTC | #2

On 8/9/24 4:06 PM, Martin KaFai Lau wrote:
> On 8/8/24 5:51 PM, Amery Hung wrote:
>> btf_parse_kptr() and btf_record_free() do btf_get() and btf_put()
>> respectively when working on btf_record in program and map if there are
>> kptr fields. If the kptr is from program BTF, since both callers has
>> already tracked the life cycle of program BTF, it is safe to remove the
>> btf_get() and btf_put().
>>
>> This change prevents memory leak of program BTF later when we start
>> searching for kptr fields when building btf_record for program. It can
>> happen when the btf fd is closed. The btf_put() corresponding to the
>> btf_get() in btf_parse_kptr() was supposed to be called by
>> btf_record_free() in btf_free_struct_meta_tab() in btf_free(). However,
>> it will never happen since the invocation of btf_free() depends on the
>> refcount of the btf to become 0 in the first place.
>>
>> Signed-off-by: Amery Hung <[e-mail]>
> 
> Acked-by: Martin KaFai Lau <[e-mail]>
> 
> Need to fix the checkpatch warning though:
> 
> WARNING: From:/Signed-off-by: email address mismatch: 'From: Amery Hung 
> <[e-mail]>' != 'Signed-off-by: Amery Hung <[e-mail]>'
> 

There is a veristat failure also:

https://github.com/kernel-patches/bpf/actions/runs/10311824065/job/28546213338

|File                  |Program                         |Verdict                |States Diff (%)|
|----------------------|--------------------------------|-----------------------|---------------|
|local_kptr_stash.bpf.o|refcount_acquire_without_unstash|success -> failure (!!)|-100.00 %      |
|local_kptr_stash.bpf.o|stash_local_with_root           |success -> failure (!!)|-100.00 %      |
|local_kptr_stash.bpf.o|stash_plain                     |success -> failure (!!)|-100.00 %      |
|local_kptr_stash.bpf.o|stash_rb_nodes                  |success -> failure (!!)|-100.00 %      |
|local_kptr_stash.bpf.o|stash_refcounted_node           |success -> failure (!!)|-100.00 %      |
|local_kptr_stash.bpf.o|stash_test_ref_kfunc            |success -> failure (!!)|-100.00 %      |
|local_kptr_stash.bpf.o|unstash_rb_node                 |success -> failure (!!)|-100.00 %      |

Hou Tao Aug. 10, 2024, 10:14 a.m. UTC | #3

On 8/9/2024 8:51 AM, Amery Hung wrote:
> btf_parse_kptr() and btf_record_free() do btf_get() and btf_put()
> respectively when working on btf_record in program and map if there are
> kptr fields. If the kptr is from program BTF, since both callers has
> already tracked the life cycle of program BTF, it is safe to remove the
> btf_get() and btf_put().
>
> This change prevents memory leak of program BTF later when we start
> searching for kptr fields when building btf_record for program. It can
> happen when the btf fd is closed. The btf_put() corresponding to the
> btf_get() in btf_parse_kptr() was supposed to be called by
> btf_record_free() in btf_free_struct_meta_tab() in btf_free(). However,
> it will never happen since the invocation of btf_free() depends on the
> refcount of the btf to become 0 in the first place.
>
> Signed-off-by: Amery Hung <[e-mail]>

Acked-by: Hou Tao <[e-mail]>

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 95426d5b634e..deacf9d7b276 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3759,6 +3759,7 @@  static int btf_find_field(const struct btf *btf, const struct btf_type *t,
 	return -EINVAL;
 }
 
+/* Callers have to ensure the life cycle of btf if it is program BTF */
 static int btf_parse_kptr(const struct btf *btf, struct btf_field *field,
 			  struct btf_field_info *info)
 {
@@ -3787,7 +3788,6 @@  static int btf_parse_kptr(const struct btf *btf, struct btf_field *field,
 		field->kptr.dtor = NULL;
 		id = info->kptr.type_id;
 		kptr_btf = (struct btf *)btf;
-		btf_get(kptr_btf);
 		goto found_dtor;
 	}
 	if (id < 0)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 869265852d51..4003e1025264 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -550,7 +550,8 @@  void btf_record_free(struct btf_record *rec)
 		case BPF_KPTR_PERCPU:
 			if (rec->fields[i].kptr.module)
 				module_put(rec->fields[i].kptr.module);
-			btf_put(rec->fields[i].kptr.btf);
+			if (btf_is_kernel(rec->fields[i].kptr.btf))
+				btf_put(rec->fields[i].kptr.btf);
 			break;
 		case BPF_LIST_HEAD:
 		case BPF_LIST_NODE:
@@ -596,7 +597,8 @@  struct btf_record *btf_record_dup(const struct btf_record *rec)
 		case BPF_KPTR_UNREF:
 		case BPF_KPTR_REF:
 		case BPF_KPTR_PERCPU:
-			btf_get(fields[i].kptr.btf);
+			if (btf_is_kernel(fields[i].kptr.btf))
+				btf_get(fields[i].kptr.btf);
 			if (fields[i].kptr.module && !try_module_get(fields[i].kptr.module)) {
 				ret = -ENXIO;
 				goto free;