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@xxxxxxxxxxx>
> > ---
> >   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