[PATCH 0/3] perf tool: Fix multiple memory leakages

[PATCH 0/3] perf tool: Fix multiple memory leakages

[Zhongqiu Han]

Fix memory leakages when btf_node or bpf_prog_info_node is duplicated
during insertion into perf_env.

Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
---
Zhongqiu Han (3):
  perf header: Fix one memory leakage in process_bpf_btf()
  perf header: Fix one memory leakage in process_bpf_prog_info()
  perf bpf: Fix two memory leakages when calling
    perf_env__insert_bpf_prog_info()

 tools/perf/util/bpf-event.c | 10 ++++++++--
 tools/perf/util/env.c       | 12 ++++++++----
 tools/perf/util/env.h       |  4 ++--
 tools/perf/util/header.c    |  8 ++++++--
 4 files changed, 24 insertions(+), 10 deletions(-)


base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
-- 
2.25.1

[PATCH 1/3] perf header: Fix one memory leakage in process_bpf_btf()

[Zhongqiu Han]

If __perf_env__insert_btf() returns false due to a duplicate btf node
insertion, the temporary node will leak. Add a check to ensure the memory
is freed if the function returns false.

Fixes: 9c51f8788b5d ("perf env: Avoid recursively taking env->bpf_progs.lock")
Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
---
 tools/perf/util/header.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 3451e542b69a..fbba6ffafec4 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -3205,7 +3205,8 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
 		if (__do_read(ff, node->data, data_size))
 			goto out;
 
-		__perf_env__insert_btf(env, node);
+		if (!__perf_env__insert_btf(env, node))
+			free(node);
 		node = NULL;
 	}
 
-- 
2.25.1

[PATCH 2/3] perf header: Fix one memory leakage in process_bpf_prog_info()

[Zhongqiu Han]

Function __perf_env__insert_bpf_prog_info() will return without inserting
bpf prog info node into perf env again due to a duplicate bpf prog info
node insertion, causing the temporary info_linear and info_node memory to
leak. Modify the return type of this function to bool and add a check to
ensure the memory is freed if the function returns false.

Fixes: 9c51f8788b5d ("perf env: Avoid recursively taking env->bpf_progs.lock")
Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
---
 tools/perf/util/env.c    | 5 +++--
 tools/perf/util/env.h    | 2 +-
 tools/perf/util/header.c | 5 ++++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index e2843ca2edd9..d7865ae5f8f5 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -32,7 +32,7 @@ void perf_env__insert_bpf_prog_info(struct perf_env *env,
 	up_write(&env->bpf_progs.lock);
 }
 
-void __perf_env__insert_bpf_prog_info(struct perf_env *env, struct bpf_prog_info_node *info_node)
+bool __perf_env__insert_bpf_prog_info(struct perf_env *env, struct bpf_prog_info_node *info_node)
 {
 	__u32 prog_id = info_node->info_linear->info.id;
 	struct bpf_prog_info_node *node;
@@ -50,13 +50,14 @@ void __perf_env__insert_bpf_prog_info(struct perf_env *env, struct bpf_prog_info
 			p = &(*p)->rb_right;
 		} else {
 			pr_debug("duplicated bpf prog info %u\n", prog_id);
-			return;
+			return false;
 		}
 	}
 
 	rb_link_node(&info_node->rb_node, parent, p);
 	rb_insert_color(&info_node->rb_node, &env->bpf_progs.infos);
 	env->bpf_progs.infos_cnt++;
+	return true;
 }
 
 struct bpf_prog_info_node *perf_env__find_bpf_prog_info(struct perf_env *env,
diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
index ae604c4edbb7..9db2e5a625ed 100644
--- a/tools/perf/util/env.h
+++ b/tools/perf/util/env.h
@@ -176,7 +176,7 @@ const char *perf_env__raw_arch(struct perf_env *env);
 int perf_env__nr_cpus_avail(struct perf_env *env);
 
 void perf_env__init(struct perf_env *env);
-void __perf_env__insert_bpf_prog_info(struct perf_env *env,
+bool __perf_env__insert_bpf_prog_info(struct perf_env *env,
 				      struct bpf_prog_info_node *info_node);
 void perf_env__insert_bpf_prog_info(struct perf_env *env,
 				    struct bpf_prog_info_node *info_node);
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index fbba6ffafec4..d06aa86352d3 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -3158,7 +3158,10 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
 		/* after reading from file, translate offset to address */
 		bpil_offs_to_addr(info_linear);
 		info_node->info_linear = info_linear;
-		__perf_env__insert_bpf_prog_info(env, info_node);
+		if (!__perf_env__insert_bpf_prog_info(env, info_node)) {
+			free(info_linear);
+			free(info_node);
+		}
 	}
 
 	up_write(&env->bpf_progs.lock);
-- 
2.25.1

[PATCH 3/3] perf bpf: Fix two memory leakages when calling perf_env__insert_bpf_prog_info()

[Zhongqiu Han]

If perf_env__insert_bpf_prog_info() returns false due to a duplicate bpf
prog info node insertion, the temporary info_node and info_linear memory
will leak. Add a check to ensure the memory is freed if the function
returns false.

Fixes: 9c51f8788b5d ("perf env: Avoid recursively taking env->bpf_progs.lock")
Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
---
 tools/perf/util/bpf-event.c | 10 ++++++++--
 tools/perf/util/env.c       |  7 +++++--
 tools/perf/util/env.h       |  2 +-
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
index 13608237c50e..c81444059ad0 100644
--- a/tools/perf/util/bpf-event.c
+++ b/tools/perf/util/bpf-event.c
@@ -289,7 +289,10 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
 		}
 
 		info_node->info_linear = info_linear;
-		perf_env__insert_bpf_prog_info(env, info_node);
+		if (!perf_env__insert_bpf_prog_info(env, info_node)) {
+			free(info_linear);
+			free(info_node);
+		}
 		info_linear = NULL;
 
 		/*
@@ -480,7 +483,10 @@ static void perf_env__add_bpf_info(struct perf_env *env, u32 id)
 	info_node = malloc(sizeof(struct bpf_prog_info_node));
 	if (info_node) {
 		info_node->info_linear = info_linear;
-		perf_env__insert_bpf_prog_info(env, info_node);
+		if (!perf_env__insert_bpf_prog_info(env, info_node)) {
+			free(info_linear);
+			free(info_node);
+		}
 	} else
 		free(info_linear);
 
diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index d7865ae5f8f5..38401a289c24 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -24,12 +24,15 @@ struct perf_env perf_env;
 #include "bpf-utils.h"
 #include <bpf/libbpf.h>
 
-void perf_env__insert_bpf_prog_info(struct perf_env *env,
+bool perf_env__insert_bpf_prog_info(struct perf_env *env,
 				    struct bpf_prog_info_node *info_node)
 {
+	bool ret = true;
 	down_write(&env->bpf_progs.lock);
-	__perf_env__insert_bpf_prog_info(env, info_node);
+	if (!__perf_env__insert_bpf_prog_info(env, info_node))
+		ret = false;
 	up_write(&env->bpf_progs.lock);
+	return ret;
 }
 
 bool __perf_env__insert_bpf_prog_info(struct perf_env *env, struct bpf_prog_info_node *info_node)
diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
index 9db2e5a625ed..da11add761d0 100644
--- a/tools/perf/util/env.h
+++ b/tools/perf/util/env.h
@@ -178,7 +178,7 @@ int perf_env__nr_cpus_avail(struct perf_env *env);
 void perf_env__init(struct perf_env *env);
 bool __perf_env__insert_bpf_prog_info(struct perf_env *env,
 				      struct bpf_prog_info_node *info_node);
-void perf_env__insert_bpf_prog_info(struct perf_env *env,
+bool perf_env__insert_bpf_prog_info(struct perf_env *env,
 				    struct bpf_prog_info_node *info_node);
 struct bpf_prog_info_node *perf_env__find_bpf_prog_info(struct perf_env *env,
 							__u32 prog_id);
-- 
2.25.1

Re: [PATCH 3/3] perf bpf: Fix two memory leakages when calling perf_env__insert_bpf_prog_info()

[Namhyung Kim]

Hello,

On Thu, Nov 28, 2024 at 08:54:32PM +0800, Zhongqiu Han wrote:
> If perf_env__insert_bpf_prog_info() returns false due to a duplicate bpf
> prog info node insertion, the temporary info_node and info_linear memory
> will leak. Add a check to ensure the memory is freed if the function
> returns false.
> 
> Fixes: 9c51f8788b5d ("perf env: Avoid recursively taking env->bpf_progs.lock")
> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
> ---
>  tools/perf/util/bpf-event.c | 10 ++++++++--
>  tools/perf/util/env.c       |  7 +++++--
>  tools/perf/util/env.h       |  2 +-
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
> index 13608237c50e..c81444059ad0 100644
> --- a/tools/perf/util/bpf-event.c
> +++ b/tools/perf/util/bpf-event.c
> @@ -289,7 +289,10 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
>  		}
>  
>  		info_node->info_linear = info_linear;
> -		perf_env__insert_bpf_prog_info(env, info_node);
> +		if (!perf_env__insert_bpf_prog_info(env, info_node)) {
> +			free(info_linear);
> +			free(info_node);
> +		}
>  		info_linear = NULL;
>  
>  		/*
> @@ -480,7 +483,10 @@ static void perf_env__add_bpf_info(struct perf_env *env, u32 id)
>  	info_node = malloc(sizeof(struct bpf_prog_info_node));
>  	if (info_node) {
>  		info_node->info_linear = info_linear;
> -		perf_env__insert_bpf_prog_info(env, info_node);
> +		if (!perf_env__insert_bpf_prog_info(env, info_node)) {
> +			free(info_linear);
> +			free(info_node);
> +		}
>  	} else
>  		free(info_linear);
>  
> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index d7865ae5f8f5..38401a289c24 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
> @@ -24,12 +24,15 @@ struct perf_env perf_env;
>  #include "bpf-utils.h"
>  #include <bpf/libbpf.h>
>  
> -void perf_env__insert_bpf_prog_info(struct perf_env *env,
> +bool perf_env__insert_bpf_prog_info(struct perf_env *env,
>  				    struct bpf_prog_info_node *info_node)
>  {
> +	bool ret = true;

Please add a blank line between declaration and the other statements.
Also I think you can just use the return value of the internal function
instead of initializaing it to true.

Thanks,
Namhyung


>  	down_write(&env->bpf_progs.lock);
> -	__perf_env__insert_bpf_prog_info(env, info_node);
> +	if (!__perf_env__insert_bpf_prog_info(env, info_node))
> +		ret = false;
>  	up_write(&env->bpf_progs.lock);
> +	return ret;
>  }
>  
>  bool __perf_env__insert_bpf_prog_info(struct perf_env *env, struct bpf_prog_info_node *info_node)
> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
> index 9db2e5a625ed..da11add761d0 100644
> --- a/tools/perf/util/env.h
> +++ b/tools/perf/util/env.h
> @@ -178,7 +178,7 @@ int perf_env__nr_cpus_avail(struct perf_env *env);
>  void perf_env__init(struct perf_env *env);
>  bool __perf_env__insert_bpf_prog_info(struct perf_env *env,
>  				      struct bpf_prog_info_node *info_node);
> -void perf_env__insert_bpf_prog_info(struct perf_env *env,
> +bool perf_env__insert_bpf_prog_info(struct perf_env *env,
>  				    struct bpf_prog_info_node *info_node);
>  struct bpf_prog_info_node *perf_env__find_bpf_prog_info(struct perf_env *env,
>  							__u32 prog_id);
> -- 
> 2.25.1
> 

Re: [PATCH 3/3] perf bpf: Fix two memory leakages when calling perf_env__insert_bpf_prog_info()

[Zhongqiu Han]

On 12/3/2024 6:02 AM, Namhyung Kim wrote:
> Hello,
> 
> On Thu, Nov 28, 2024 at 08:54:32PM +0800, Zhongqiu Han wrote:
>> If perf_env__insert_bpf_prog_info() returns false due to a duplicate bpf
>> prog info node insertion, the temporary info_node and info_linear memory
>> will leak. Add a check to ensure the memory is freed if the function
>> returns false.
>>
>> Fixes: 9c51f8788b5d ("perf env: Avoid recursively taking env->bpf_progs.lock")
>> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
>> ---
>>   tools/perf/util/bpf-event.c | 10 ++++++++--
>>   tools/perf/util/env.c       |  7 +++++--
>>   tools/perf/util/env.h       |  2 +-
>>   3 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
>> index 13608237c50e..c81444059ad0 100644
>> --- a/tools/perf/util/bpf-event.c
>> +++ b/tools/perf/util/bpf-event.c
>> @@ -289,7 +289,10 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
>>   		}
>>   
>>   		info_node->info_linear = info_linear;
>> -		perf_env__insert_bpf_prog_info(env, info_node);
>> +		if (!perf_env__insert_bpf_prog_info(env, info_node)) {
>> +			free(info_linear);
>> +			free(info_node);
>> +		}
>>   		info_linear = NULL;
>>   
>>   		/*
>> @@ -480,7 +483,10 @@ static void perf_env__add_bpf_info(struct perf_env *env, u32 id)
>>   	info_node = malloc(sizeof(struct bpf_prog_info_node));
>>   	if (info_node) {
>>   		info_node->info_linear = info_linear;
>> -		perf_env__insert_bpf_prog_info(env, info_node);
>> +		if (!perf_env__insert_bpf_prog_info(env, info_node)) {
>> +			free(info_linear);
>> +			free(info_node);
>> +		}
>>   	} else
>>   		free(info_linear);
>>   
>> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
>> index d7865ae5f8f5..38401a289c24 100644
>> --- a/tools/perf/util/env.c
>> +++ b/tools/perf/util/env.c
>> @@ -24,12 +24,15 @@ struct perf_env perf_env;
>>   #include "bpf-utils.h"
>>   #include <bpf/libbpf.h>
>>   
>> -void perf_env__insert_bpf_prog_info(struct perf_env *env,
>> +bool perf_env__insert_bpf_prog_info(struct perf_env *env,
>>   				    struct bpf_prog_info_node *info_node)
>>   {
>> +	bool ret = true;
> 
> Please add a blank line between declaration and the other statements.
> Also I think you can just use the return value of the internal function
> instead of initializaing it to true.
> 
> Thanks,
> Namhyung
> 
> 

Hi Namhyung,
Thanks for your review~

I will add a blank line between the declaration and the other
statements, and optimize it as below:


+bool perf_env__insert_bpf_prog_info(struct perf_env *env,
  				    struct bpf_prog_info_node
*info_node)
  {
+	bool ret;
+
  	down_write(&env->bpf_progs.lock);
-	__perf_env__insert_bpf_prog_info(env, info_node);
+	ret = __perf_env__insert_bpf_prog_info(env, info_node);
  	up_write(&env->bpf_progs.lock);
+	return ret;
  }


>>   	down_write(&env->bpf_progs.lock);
>> -	__perf_env__insert_bpf_prog_info(env, info_node);
>> +	if (!__perf_env__insert_bpf_prog_info(env, info_node))
>> +		ret = false;
>>   	up_write(&env->bpf_progs.lock);
>> +	return ret;
>>   }
>>   
>>   bool __perf_env__insert_bpf_prog_info(struct perf_env *env, struct bpf_prog_info_node *info_node)
>> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
>> index 9db2e5a625ed..da11add761d0 100644
>> --- a/tools/perf/util/env.h
>> +++ b/tools/perf/util/env.h
>> @@ -178,7 +178,7 @@ int perf_env__nr_cpus_avail(struct perf_env *env);
>>   void perf_env__init(struct perf_env *env);
>>   bool __perf_env__insert_bpf_prog_info(struct perf_env *env,
>>   				      struct bpf_prog_info_node *info_node);
>> -void perf_env__insert_bpf_prog_info(struct perf_env *env,
>> +bool perf_env__insert_bpf_prog_info(struct perf_env *env,
>>   				    struct bpf_prog_info_node *info_node);
>>   struct bpf_prog_info_node *perf_env__find_bpf_prog_info(struct perf_env *env,
>>   							__u32 prog_id);
>> -- 
>> 2.25.1
>>


-- 
Thx and BRs,
Zhongqiu Han

Re: [PATCH 0/3] perf tool: Fix multiple memory leakages

[Namhyung Kim]

On Thu, Nov 28, 2024 at 08:54:29PM +0800, Zhongqiu Han wrote:
> Fix memory leakages when btf_node or bpf_prog_info_node is duplicated
> during insertion into perf_env.
> 
> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
> ---
> Zhongqiu Han (3):
>   perf header: Fix one memory leakage in process_bpf_btf()
>   perf header: Fix one memory leakage in process_bpf_prog_info()
>   perf bpf: Fix two memory leakages when calling
>     perf_env__insert_bpf_prog_info()

Although I have a nitpick in the patch 3, it looks good otherwise.

Reviewed-by: Namhyung Kim <namhyung@kernel.org>

And I don't think the Fixes tags are correct, but it won't apply before
the change it points to.  So for practical reason, I'm ok with that.

Thanks,
Namhyung

> 
>  tools/perf/util/bpf-event.c | 10 ++++++++--
>  tools/perf/util/env.c       | 12 ++++++++----
>  tools/perf/util/env.h       |  4 ++--
>  tools/perf/util/header.c    |  8 ++++++--
>  4 files changed, 24 insertions(+), 10 deletions(-)
> 
> 
> base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
> -- 
> 2.25.1
> 

Re: [PATCH 0/3] perf tool: Fix multiple memory leakages

[Zhongqiu Han]

On 12/3/2024 6:04 AM, Namhyung Kim wrote:
> On Thu, Nov 28, 2024 at 08:54:29PM +0800, Zhongqiu Han wrote:
>> Fix memory leakages when btf_node or bpf_prog_info_node is duplicated
>> during insertion into perf_env.
>>
>> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
>> ---
>> Zhongqiu Han (3):
>>    perf header: Fix one memory leakage in process_bpf_btf()
>>    perf header: Fix one memory leakage in process_bpf_prog_info()
>>    perf bpf: Fix two memory leakages when calling
>>      perf_env__insert_bpf_prog_info()
> 
> Although I have a nitpick in the patch 3, it looks good otherwise.
> 
> Reviewed-by: Namhyung Kim <namhyung@kernel.org>
> 
Hi Namhyung,
Thanks for your review~
I will arise the V2 to optimize patch 3.

> And I don't think the Fixes tags are correct, but it won't apply before
> the change it points to.  So for practical reason, I'm ok with that.
> 
> Thanks,
> Namhyung
> 

I will fix the Fixes tag as follows on V2:

[PATCH 1/3] perf header: Fix one memory leakage in process_bpf_btf()
Fixes: a70a1123174a ("perf bpf: Save BTF information as headers to
perf.data")

[PATCH 2/3] perf header: Fix one memory leakage in
process_bpf_prog_info()
Fixes: 606f972b1361 ("perf bpf: Save bpf_prog_info information as
headers to perf.data")


[PATCH 3/3] perf bpf: Fix two memory leakages when calling
perf_env__insert_bpf_prog_info()
Fixes: e4378f0cb90b ("perf bpf: Save bpf_prog_info in a rbtree in
perf_env")
Fixes: d56354dc4909 ("perf tools: Save bpf_prog_info and BTF of new BPF
programs")


>>
>>   tools/perf/util/bpf-event.c | 10 ++++++++--
>>   tools/perf/util/env.c       | 12 ++++++++----
>>   tools/perf/util/env.h       |  4 ++--
>>   tools/perf/util/header.c    |  8 ++++++--
>>   4 files changed, 24 insertions(+), 10 deletions(-)
>>
>>
>> base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
>> -- 
>> 2.25.1
>>


-- 
Thx and BRs,
Zhongqiu Han