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