Re: [PATCH 3/4] perf kvm: Kill STRDUP_FAIL_EXIT()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Memory leak of dynamically allocated strings in PowerPC default arch events due to removal of array cleanup loops.

Pre-existing issues:
- [High] perf kvm record unconditionally crashes via BUG_ON() on non-Intel x86 hosts and architectures without default events.
--

commit d3c8251de71559c69211fb2d23ba99dd14669107
Author: Namhyung Kim <namhyung@kernel.org>

perf kvm: Kill STRDUP_FAIL_EXIT()

This commit removes the STRDUP_FAIL_EXIT macro and replaces dynamically
allocated copied strings with stack variables for passing command line
options. It also removes the cleanup loops that freed these string
copies, addressing a subtle double free issue.

> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index acc6f2a706b35..477c28915c037 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
[ ... ]
> @@ -2008,9 +2004,9 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
[ ... ]
>  	for (j = 1; j < argc; j++, i++)
> -		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
> +		rec_argv[i] = argv[j];
>  
>  	BUG_ON(i != rec_argc);

[Severity: High]
This is a pre-existing issue, but can this BUG_ON() crash the tool on
non-Intel x86 hosts or architectures without default events?

If need_arch_event is true but the architecture hook returns early without
adding events, rec_argc will be greater than the final value of i.

For example, in tools/perf/util/kvm-stat-arch/kvm-stat-x86.c:

__kvm_add_default_arch_event_x86() {
        if (!x86__is_intel_cpu())
                return 0;
        ...
}

On AMD hosts, this returns 0 without incrementing the argument count, leaving
i less than rec_argc and triggering the BUG_ON() assertion.

>  
>  	ret = cmd_record(i, rec_argv);
>  
>  EXIT:
> -	for (j = 0; j < i; j++)
> -		free((void *)rec_argv[j]);

[Severity: Medium]
Does removing this cleanup loop leak memory on PowerPC architectures?

While the central code now uses stack variables, the PowerPC architecture
hook still dynamically allocates its default events. 

In tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c:

__kvm_add_default_arch_event_powerpc() {
        ...
        argv[j++] = strdup("-e");
        argv[j++] = strdup("trace_imc/trace_cycles/");
        ...
}

Since these strings are allocated with strdup() and the free() loops in
__cmd_record(), __cmd_top(), and kvm_events_record() have been removed,
will these strings leak during command execution?

>  	free(rec_argv);
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623070313.55225-1-namhyung@kernel.org?part=3