[PATCH v1 0/4] perf kvm: A small update in default arch event

[PATCH v1 0/4] perf kvm: A small update in default arch event

[Namhyung Kim]

Hello,

Ian said there's an issue in the perf kvm record command line option parsing.
This is my attempt to address the issue and simplify the code.

Thanks,
Namhyung


Namhyung Kim (4):
  perf kvm: Factor out kvm_need_default_arch_event()
  perf kvm: Check kvm_need_default_arch_event() early
  perf kvm: Kill STRDUP_FAIL_EXIT()
  perf test: Simplify perf kvm record/report tests

 tools/perf/builtin-kvm.c                      | 71 ++++++++-----------
 tools/perf/tests/shell/kvm.sh                 |  8 +--
 .../util/kvm-stat-arch/kvm-stat-powerpc.c     | 31 ++------
 tools/perf/util/kvm-stat-arch/kvm-stat-x86.c  | 32 ++-------
 tools/perf/util/kvm-stat.c                    | 27 +++++++
 tools/perf/util/kvm-stat.h                    | 17 ++---
 6 files changed, 73 insertions(+), 113 deletions(-)

-- 
2.54.0

[PATCH 1/4] perf kvm: Factor out kvm_need_default_arch_event()

[Namhyung Kim]

The kvm_add_default_arch_event() has a similar logic in each arch to
check if there's an existing command line option for events.  Let's
check it in the generic code and remove the duplication.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c                      |  8 +++--
 .../util/kvm-stat-arch/kvm-stat-powerpc.c     | 31 +++----------------
 tools/perf/util/kvm-stat-arch/kvm-stat-x86.c  | 29 +++--------------
 tools/perf/util/kvm-stat.c                    | 27 ++++++++++++++++
 tools/perf/util/kvm-stat.h                    |  7 +++++
 5 files changed, 48 insertions(+), 54 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 394302ebdb161077..acb1d2f8148c7c9b 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -2014,9 +2014,11 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 
 	BUG_ON(i + 2 != rec_argc);
 
-	ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
-	if (ret)
-		goto EXIT;
+	if (kvm_need_default_arch_event(argc, argv)) {
+		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
+		if (ret)
+			goto EXIT;
+	}
 
 	ret = cmd_record(i, rec_argv);
 
diff --git a/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c b/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
index 96d9c4ae020940f0..8d4133c35c12f14b 100644
--- a/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
+++ b/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
@@ -9,7 +9,6 @@
 
 #include "book3s_hv_exits.h"
 #include "book3s_hcalls.h"
-#include <subcmd/parse-options.h>
 
 #define NR_TPS 4
 
@@ -177,35 +176,15 @@ int __cpu_isa_init_powerpc(struct perf_kvm_stat *kvm)
  */
 int __kvm_add_default_arch_event_powerpc(int *argc, const char **argv)
 {
-	const char **tmp;
-	bool event = false;
-	int i, j = *argc;
+	int j = *argc;
 
-	const struct option event_options[] = {
-		OPT_BOOLEAN('e', "event", &event, NULL),
-		OPT_END()
-	};
-
-	tmp = calloc(j + 1, sizeof(char *));
-	if (!tmp)
+	if (!perf_pmus__have_event("trace_imc", "trace_cycles"))
 		return -EINVAL;
 
-	for (i = 0; i < j; i++)
-		tmp[i] = argv[i];
-
-	parse_options(j, tmp, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
-	if (!event) {
-		if (perf_pmus__have_event("trace_imc", "trace_cycles")) {
-			argv[j++] = strdup("-e");
-			argv[j++] = strdup("trace_imc/trace_cycles/");
-			*argc += 2;
-		} else {
-			free(tmp);
-			return -EINVAL;
-		}
-	}
+	argv[j++] = strdup("-e");
+	argv[j++] = strdup("trace_imc/trace_cycles/");
+	*argc += 2;
 
-	free(tmp);
 	return 0;
 }
 
diff --git a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
index 788d216f0852147d..7bef7657a68a3e55 100644
--- a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
+++ b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
@@ -7,7 +7,6 @@
 #include "../../../arch/x86/include/uapi/asm/svm.h"
 #include "../../../arch/x86/include/uapi/asm/vmx.h"
 #include "../../../arch/x86/include/uapi/asm/kvm.h"
-#include <subcmd/parse-options.h>
 
 define_exit_reasons_table(vmx_exit_reasons, VMX_EXIT_REASONS);
 define_exit_reasons_table(svm_exit_reasons, SVM_EXIT_REASONS);
@@ -211,38 +210,18 @@ int __cpu_isa_init_x86(struct perf_kvm_stat *kvm, const char *cpuid)
  */
 int __kvm_add_default_arch_event_x86(int *argc, const char **argv)
 {
-	const char **tmp;
-	bool event = false;
-	int ret = 0, i, j = *argc;
-
-	const struct option event_options[] = {
-		OPT_BOOLEAN('e', "event", &event, NULL),
-		OPT_BOOLEAN(0, "pfm-events", &event, NULL),
-		OPT_END()
-	};
+	int ret = 0, j = *argc;
 
 	if (!x86__is_intel_cpu())
 		return 0;
 
-	tmp = calloc(j + 1, sizeof(char *));
-	if (!tmp)
-		return -ENOMEM;
-
-	for (i = 0; i < j; i++)
-		tmp[i] = argv[i];
-
-	parse_options(j, tmp, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
-	if (!event) {
-		argv[j++] = STRDUP_FAIL_EXIT("-e");
-		argv[j++] = STRDUP_FAIL_EXIT("cycles");
-		*argc += 2;
-	}
+	argv[j++] = STRDUP_FAIL_EXIT("-e");
+	argv[j++] = STRDUP_FAIL_EXIT("cycles");
+	*argc += 2;
 
-	free(tmp);
 	return 0;
 
 EXIT:
-	free(tmp);
 	return ret;
 }
 
diff --git a/tools/perf/util/kvm-stat.c b/tools/perf/util/kvm-stat.c
index 755ab659a05c30f0..5c2d1e57672ae5dd 100644
--- a/tools/perf/util/kvm-stat.c
+++ b/tools/perf/util/kvm-stat.c
@@ -3,6 +3,7 @@
 #include "evsel.h"
 #include "kvm-stat.h"
 #include <dwarf-regs.h>
+#include <subcmd/parse-options.h>
 
 bool kvm_exit_event(struct evsel *evsel)
 {
@@ -268,3 +269,29 @@ int kvm_add_default_arch_event(uint16_t e_machine, int *argc, const char **argv)
 		return 0;
 	}
 }
+
+bool kvm_need_default_arch_event(int argc, const char **argv)
+{
+	const char **tmp_argv;
+	bool event = false;
+	int i;
+
+	const struct option event_options[] = {
+		OPT_BOOLEAN('e', "event", &event, NULL),
+		OPT_BOOLEAN(0, "pfm-events", &event, NULL),
+		OPT_END()
+	};
+
+	/* parse_options() may change the argv, let's make a copy */
+	tmp_argv = calloc(argc + 1, sizeof(char *));
+	if (!tmp_argv)
+		return false;
+
+	for (i = 0; i < argc; i++)
+		tmp_argv[i] = argv[i];
+
+	parse_options(argc, tmp_argv, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
+	free(tmp_argv);
+
+	return !event;
+}
diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
index cdbd921a555f4e51..111b02fd0878efc2 100644
--- a/tools/perf/util/kvm-stat.h
+++ b/tools/perf/util/kvm-stat.h
@@ -174,12 +174,19 @@ const char * const *__kvm_skip_events_riscv(void);
 const char * const *__kvm_skip_events_s390(void);
 const char * const *__kvm_skip_events_x86(void);
 
+bool kvm_need_default_arch_event(int argc, const char **argv);
 int kvm_add_default_arch_event(uint16_t e_machine, int *argc, const char **argv);
 int __kvm_add_default_arch_event_powerpc(int *argc, const char **argv);
 int __kvm_add_default_arch_event_x86(int *argc, const char **argv);
 
 #else /* !HAVE_LIBTRACEEVENT */
 
+static inline bool kvm_need_default_arch_event(int argc __maybe_unused,
+					       const char **argv __maybe_unused)
+{
+	return false;
+}
+
 static inline int kvm_add_default_arch_event(uint16_t e_machine __maybe_unused,
 					     int *argc __maybe_unused,
 					     const char **argv __maybe_unused)
-- 
2.54.0

[PATCH 2/4] perf kvm: Check kvm_need_default_arch_event() early

[Namhyung Kim]

There's a subtle issue with option parsing in perf record.  It calls the
function with PARSE_OPT_STOP_AT_NON_OPTION so that it can pass later
options to the external command it runs.

But perf kvm record passes the default arch events after the argv.  So
if user calls it with command, then it passes the event to the external
command and fails it like below:

  $ sudo perf kvm --host record sleep 1
  sleep: invalid option -- 'e'
  Try 'sleep --help' for more information.
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.046 MB perf.data.kvm (5 samples) ]

We can check if the default options are needed before passing the extra
command line to make sure it's passed to perf record.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index acb1d2f8148c7c9b..cec26f898bf6eaeb 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1995,13 +1995,14 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 {
 	int rec_argc, i = 0, j, ret;
 	const char **rec_argv;
+	int need_arch_event = !!kvm_need_default_arch_event(argc, argv);
 
 	/*
 	 * Besides the 2 more options "-o" and "filename",
 	 * kvm_add_default_arch_event() may add 2 extra options,
-	 * so allocate 4 more items.
+	 * so allocate more items conditionally.
 	 */
-	rec_argc = argc + 2 + 2;
+	rec_argc = argc + 2 + (2 * need_arch_event);
 	rec_argv = calloc(rec_argc + 1, sizeof(char *));
 	if (!rec_argv)
 		return -ENOMEM;
@@ -2009,22 +2010,22 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 	rec_argv[i++] = STRDUP_FAIL_EXIT("record");
 	rec_argv[i++] = STRDUP_FAIL_EXIT("-o");
 	rec_argv[i++] = STRDUP_FAIL_EXIT(file_name);
-	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
-
-	BUG_ON(i + 2 != rec_argc);
-
-	if (kvm_need_default_arch_event(argc, argv)) {
+	if (need_arch_event) {
 		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
 		if (ret)
 			goto EXIT;
 	}
 
+	for (j = 1; j < argc; j++, i++)
+		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+
+	BUG_ON(i != rec_argc);
+
 	ret = cmd_record(i, rec_argv);
 
 EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
+	for (j = 0; j < i; j++)
+		free((void *)rec_argv[j]);
 	free(rec_argv);
 	return ret;
 }
-- 
2.54.0

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

[Namhyung Kim]

It's used to pass command line options to a copied argv.  But there's no
reason to make the copies as it's all used in the same function.  It can
simply use stack variables.

In fact, it fixes a subtle double free issue.  As parse_options() can
move contents in argv[], some entries may point to the same item.  So
freeing all items in the argv could trigger a double free.  With stack
variables, we don't need to allocate and free them.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c                     | 54 +++++++-------------
 tools/perf/util/kvm-stat-arch/kvm-stat-x86.c |  9 ++--
 tools/perf/util/kvm-stat.h                   | 10 ----
 3 files changed, 22 insertions(+), 51 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index cec26f898bf6eaeb..84f9ce4481f73842 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1681,18 +1681,18 @@ kvm_events_record(struct perf_kvm_stat *kvm, int argc, const char **argv)
 		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(record_args); i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(record_args[i]);
+		rec_argv[i] = record_args[i];
 
 	for (j = 0; j < events_tp_size; j++) {
-		rec_argv[i++] = STRDUP_FAIL_EXIT("-e");
-		rec_argv[i++] = STRDUP_FAIL_EXIT(kvm_events_tp(e_machine)[j]);
+		rec_argv[i++] = "-e";
+		rec_argv[i++] = kvm_events_tp(e_machine)[j];
 	}
 
-	rec_argv[i++] = STRDUP_FAIL_EXIT("-o");
-	rec_argv[i++] = STRDUP_FAIL_EXIT(kvm->file_name);
+	rec_argv[i++] = "-o";
+	rec_argv[i++] = kvm->file_name;
 
 	for (j = 1; j < (unsigned int)argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+		rec_argv[i] = argv[j];
 
 	set_option_flag(record_options, 'e', "event", PARSE_OPT_HIDDEN);
 	set_option_flag(record_options, 0, "filter", PARSE_OPT_HIDDEN);
@@ -1716,10 +1716,6 @@ kvm_events_record(struct perf_kvm_stat *kvm, int argc, const char **argv)
 
 	record_usage = kvm_stat_record_usage;
 	ret = cmd_record(i, rec_argv);
-
-EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
 	free(rec_argv);
 	return ret;
 }
@@ -2007,9 +2003,9 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 	if (!rec_argv)
 		return -ENOMEM;
 
-	rec_argv[i++] = STRDUP_FAIL_EXIT("record");
-	rec_argv[i++] = STRDUP_FAIL_EXIT("-o");
-	rec_argv[i++] = STRDUP_FAIL_EXIT(file_name);
+	rec_argv[i++] = "record";
+	rec_argv[i++] = "-o";
+	rec_argv[i++] = file_name;
 	if (need_arch_event) {
 		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
 		if (ret)
@@ -2017,15 +2013,13 @@ 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);
 
 	ret = cmd_record(i, rec_argv);
 
 EXIT:
-	for (j = 0; j < i; j++)
-		free((void *)rec_argv[j]);
 	free(rec_argv);
 	return ret;
 }
@@ -2040,19 +2034,15 @@ static int __cmd_report(const char *file_name, int argc, const char **argv)
 	if (!rec_argv)
 		return -ENOMEM;
 
-	rec_argv[i++] = STRDUP_FAIL_EXIT("report");
-	rec_argv[i++] = STRDUP_FAIL_EXIT("-i");
-	rec_argv[i++] = STRDUP_FAIL_EXIT(file_name);
+	rec_argv[i++] = "report";
+	rec_argv[i++] = "-i";
+	rec_argv[i++] = file_name;
 	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);
 
 	ret = cmd_report(i, rec_argv);
-
-EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
 	free(rec_argv);
 	return ret;
 }
@@ -2068,19 +2058,15 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
 	if (!rec_argv)
 		return -ENOMEM;
 
-	rec_argv[i++] = STRDUP_FAIL_EXIT("buildid-list");
-	rec_argv[i++] = STRDUP_FAIL_EXIT("-i");
-	rec_argv[i++] = STRDUP_FAIL_EXIT(file_name);
+	rec_argv[i++] = "buildid-list";
+	rec_argv[i++] = "-i";
+	rec_argv[i++] = file_name;
 	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);
 
 	ret = cmd_buildid_list(i, rec_argv);
-
-EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
 	free(rec_argv);
 	return ret;
 }
@@ -2100,7 +2086,7 @@ static int __cmd_top(int argc, const char **argv)
 		return -ENOMEM;
 
 	for (i = 0; i < argc; i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[i]);
+		rec_argv[i] = argv[i];
 
 	BUG_ON(i != argc);
 
@@ -2111,8 +2097,6 @@ static int __cmd_top(int argc, const char **argv)
 	ret = cmd_top(i, rec_argv);
 
 EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
 	free(rec_argv);
 	return ret;
 }
diff --git a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
index 7bef7657a68a3e55..923b157bec2979b0 100644
--- a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
+++ b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
@@ -210,19 +210,16 @@ int __cpu_isa_init_x86(struct perf_kvm_stat *kvm, const char *cpuid)
  */
 int __kvm_add_default_arch_event_x86(int *argc, const char **argv)
 {
-	int ret = 0, j = *argc;
+	int j = *argc;
 
 	if (!x86__is_intel_cpu())
 		return 0;
 
-	argv[j++] = STRDUP_FAIL_EXIT("-e");
-	argv[j++] = STRDUP_FAIL_EXIT("cycles");
+	argv[j++] = "-e";
+	argv[j++] = "cycles";
 	*argc += 2;
 
 	return 0;
-
-EXIT:
-	return ret;
 }
 
 const char * const *__kvm_events_tp_x86(void)
diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
index 111b02fd0878efc2..47bb0bfcb9bf6b53 100644
--- a/tools/perf/util/kvm-stat.h
+++ b/tools/perf/util/kvm-stat.h
@@ -228,14 +228,4 @@ static inline struct kvm_info *kvm_info__new(void)
 	return ki;
 }
 
-#define STRDUP_FAIL_EXIT(s)		\
-	({	char *_p;		\
-		_p = strdup(s);		\
-		if (!_p) {		\
-			ret = -ENOMEM;	\
-			goto EXIT;	\
-		}			\
-		_p;			\
-	})
-
 #endif /* __PERF_KVM_STAT_H */
-- 
2.54.0

[PATCH 4/4] perf test: Simplify perf kvm record/report tests

[Namhyung Kim]

It can now pass command line arguments to perf kvm record --host.
Let's pass 'sleep 1' directly rather than running it in background.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/kvm.sh | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/tools/perf/tests/shell/kvm.sh b/tools/perf/tests/shell/kvm.sh
index f88e859025c42c32..4ca10388748dee93 100755
--- a/tools/perf/tests/shell/kvm.sh
+++ b/tools/perf/tests/shell/kvm.sh
@@ -63,13 +63,7 @@ test_kvm_record_report() {
 	echo "Recording kvm profile for pid ${qemu_pid}..."
 	# Use --host to avoid needing guest symbols/mounts for this simple test
 	# We just want to verify the command runs and produces data
-	# We run in background and kill it because 'perf kvm record' appends options
-	# after the command, which breaks 'sleep' (e.g. it gets '-e cycles').
-	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" &
-	rec_pid=$!
-	sleep 1
-	kill -INT "${rec_pid}"
-	wait "${rec_pid}" || true
+	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" sleep 1
 
 	echo "Reporting kvm profile..."
 	# Check for some standard output from report
-- 
2.54.0

Re: [PATCH 4/4] perf test: Simplify perf kvm record/report tests

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The synchronous `perf kvm record` command is not wrapped in an error-checking conditional, causing the script to unexpectedly abort via `set -e` upon failure.
--

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

perf test: Simplify perf kvm record/report tests

This commit simplifies the perf kvm tests by passing sleep directly as a
command line argument to perf kvm record, rather than running it in the
background and manually killing it.

> diff --git a/tools/perf/tests/shell/kvm.sh b/tools/perf/tests/shell/kvm.sh
> index f88e859025c42..4ca10388748de 100755
> --- a/tools/perf/tests/shell/kvm.sh
> +++ b/tools/perf/tests/shell/kvm.sh
[ ... ]
> @@ -63,13 +63,7 @@ test_kvm_record_report() {
>  	echo "Recording kvm profile for pid ${qemu_pid}..."
>  	# Use --host to avoid needing guest symbols/mounts for this simple test
>  	# We just want to verify the command runs and produces data
> -	# We run in background and kill it because 'perf kvm record' appends options
> -	# after the command, which breaks 'sleep' (e.g. it gets '-e cycles').
> -	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" &
> -	rec_pid=$!
> -	sleep 1
> -	kill -INT "${rec_pid}"
> -	wait "${rec_pid}" || true
> +	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" sleep 1

[Severity: Medium]
If this command fails, will the script abort immediately due to set -e being
enabled globally?

The previous implementation guarded against non-zero exit codes by using
wait "${rec_pid}" || true. Without an error-checking conditional or an
appended || true, a failure during this synchronous execution could cause the
script to instantly terminate.

Would this bypass the test's intended logic of recording the failure (err=1)
and accidentally skip subsequent tests like test_kvm_buildid_list?

>  
>  	echo "Reporting kvm profile..."
>  	# Check for some standard output from report

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

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

[sashiko-bot]

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

Re: [PATCH 1/4] perf kvm: Factor out kvm_need_default_arch_event()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `__cmd_top()` unconditionally appends the default architecture event, breaking user-provided event overrides.
- [Low] Missing explicit POSIX <stdlib.h> header inclusion in kvm-stat.c for calloc() and free().
--

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

perf kvm: Factor out kvm_need_default_arch_event()

This commit factors out the logic for checking existing event command line
options into a generic kvm_need_default_arch_event() function. It removes the
duplicated option checking logic from architecture-specific implementations
of kvm_add_default_arch_event() and updates __cmd_record() to use the new
guard.

> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 0c5e6b3aac74d..c236093eac473 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -2015,9 +2015,11 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
>  
>  	BUG_ON(i + 2 != rec_argc);
>  
> -	ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
> -	if (ret)
> -		goto EXIT;
> +	if (kvm_need_default_arch_event(argc, argv)) {
> +		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
> +		if (ret)
> +			goto EXIT;
> +	}
>  
>  	ret = cmd_record(i, rec_argv);
>  

[Severity: High]
Is it possible that __cmd_top() is missing the kvm_need_default_arch_event()
guard?

Since the option parsing logic was removed from kvm_add_default_arch_event(),
it now unconditionally appends the default event.

__cmd_top() in tools/perf/builtin-kvm.c still calls it unconditionally:

__cmd_top()
        ...
        ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
        if (ret)
                goto EXIT;

Will this cause perf kvm top to ignore user-provided event overrides
(like -e some_event) and blindly append the default event?

[ ... ]

> diff --git a/tools/perf/util/kvm-stat.c b/tools/perf/util/kvm-stat.c
> index 27f16810498ca..dd599dd0e4322 100644
> --- a/tools/perf/util/kvm-stat.c
> +++ b/tools/perf/util/kvm-stat.c
[ ... ]
> @@ -271,3 +272,29 @@ int kvm_add_default_arch_event(uint16_t e_machine, int *argc, const char **argv)
>  		return 0;
>  	}
>  }
> +
> +bool kvm_need_default_arch_event(int argc, const char **argv)
> +{
> +	const char **tmp_argv;
> +	bool event = false;
> +	int i;
> +
> +	const struct option event_options[] = {
> +		OPT_BOOLEAN('e', "event", &event, NULL),
> +		OPT_BOOLEAN(0, "pfm-events", &event, NULL),
> +		OPT_END()
> +	};
> +
> +	/* parse_options() may change the argv, let's make a copy */
> +	tmp_argv = calloc(argc + 1, sizeof(char *));

[Severity: Low]
Should we explicitly include <stdlib.h> in this file to support musl libc
builds?

The new kvm_need_default_arch_event() function relies on calloc() and free(),
but <stdlib.h> isn't explicitly included in tools/perf/util/kvm-stat.c.

Because musl libc enforces strict standard header separation, relying on
implicit includes can cause the build to fail.

> +	if (!tmp_argv)
> +		return false;
> +
> +	for (i = 0; i < argc; i++)
> +		tmp_argv[i] = argv[i];
> +
> +	parse_options(argc, tmp_argv, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
> +	free(tmp_argv);
> +
> +	return !event;
> +}

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

Re: [PATCH 2/4] perf kvm: Check kvm_need_default_arch_event() early

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] BUG_ON() unconditionally triggers and crashes `perf kvm record` on architectures where default arch events are not added (e.g., ARM64, RISC-V, s390).
--

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

perf kvm: Check kvm_need_default_arch_event() early

This commit modifies perf kvm record to check if default options are needed
before allocating arguments and passing the extra command line, preventing
the default event from being incorrectly passed to the external command.

> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index c236093eac473..acc6f2a706b35 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -1996,13 +1996,14 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
>  {
>  	int rec_argc, i = 0, j, ret;
>  	const char **rec_argv;
> +	int need_arch_event = !!kvm_need_default_arch_event(argc, argv);
>  
>  	/*
>  	 * Besides the 2 more options "-o" and "filename",
>  	 * kvm_add_default_arch_event() may add 2 extra options,
> -	 * so allocate 4 more items.
> +	 * so allocate more items conditionally.
>  	 */
> -	rec_argc = argc + 2 + 2;
> +	rec_argc = argc + 2 + (2 * need_arch_event);

[ ... ]

> -	if (kvm_need_default_arch_event(argc, argv)) {
> +	if (need_arch_event) {
>  		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
>  		if (ret)
>  			goto EXIT;
>  	}
>  
> +	for (j = 1; j < argc; j++, i++)
> +		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
> +
> -	BUG_ON(i + 2 != rec_argc);
> +	BUG_ON(i != rec_argc);

[Severity: High]
Could this BUG_ON() cause a regression for perf kvm record on architectures
that do not add default events?

If kvm_need_default_arch_event() returns true, rec_argc is set to argc + 4.

However, on architectures like ARM64, RISC-V, or s390, it appears
kvm_add_default_arch_event() can return 0 without actually appending any
events or incrementing i.

If that happens, i will only equal argc + 2 after the loop, causing this
assertion to unconditionally fail and abort the process.

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

Re: [PATCH 1/4] perf kvm: Factor out kvm_need_default_arch_event()

[Namhyung Kim]

On Tue, Jun 23, 2026 at 07:16:40AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] `__cmd_top()` unconditionally appends the default architecture event, breaking user-provided event overrides.
> - [Low] Missing explicit POSIX <stdlib.h> header inclusion in kvm-stat.c for calloc() and free().
> --
> 
> commit 3e2018b99366e21f6651143f8ed90e3f8257ec72
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf kvm: Factor out kvm_need_default_arch_event()
> 
> This commit factors out the logic for checking existing event command line
> options into a generic kvm_need_default_arch_event() function. It removes the
> duplicated option checking logic from architecture-specific implementations
> of kvm_add_default_arch_event() and updates __cmd_record() to use the new
> guard.
> 
> > diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> > index 0c5e6b3aac74d..c236093eac473 100644
> > --- a/tools/perf/builtin-kvm.c
> > +++ b/tools/perf/builtin-kvm.c
> > @@ -2015,9 +2015,11 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
> >  
> >  	BUG_ON(i + 2 != rec_argc);
> >  
> > -	ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
> > -	if (ret)
> > -		goto EXIT;
> > +	if (kvm_need_default_arch_event(argc, argv)) {
> > +		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
> > +		if (ret)
> > +			goto EXIT;
> > +	}
> >  
> >  	ret = cmd_record(i, rec_argv);
> >  
> 
> [Severity: High]
> Is it possible that __cmd_top() is missing the kvm_need_default_arch_event()
> guard?
> 
> Since the option parsing logic was removed from kvm_add_default_arch_event(),
> it now unconditionally appends the default event.
> 
> __cmd_top() in tools/perf/builtin-kvm.c still calls it unconditionally:
> 
> __cmd_top()
>         ...
>         ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
>         if (ret)
>                 goto EXIT;
> 
> Will this cause perf kvm top to ignore user-provided event overrides
> (like -e some_event) and blindly append the default event?

Right, will fix in v2.

> 
> [ ... ]
> 
> > diff --git a/tools/perf/util/kvm-stat.c b/tools/perf/util/kvm-stat.c
> > index 27f16810498ca..dd599dd0e4322 100644
> > --- a/tools/perf/util/kvm-stat.c
> > +++ b/tools/perf/util/kvm-stat.c
> [ ... ]
> > @@ -271,3 +272,29 @@ int kvm_add_default_arch_event(uint16_t e_machine, int *argc, const char **argv)
> >  		return 0;
> >  	}
> >  }
> > +
> > +bool kvm_need_default_arch_event(int argc, const char **argv)
> > +{
> > +	const char **tmp_argv;
> > +	bool event = false;
> > +	int i;
> > +
> > +	const struct option event_options[] = {
> > +		OPT_BOOLEAN('e', "event", &event, NULL),
> > +		OPT_BOOLEAN(0, "pfm-events", &event, NULL),
> > +		OPT_END()
> > +	};
> > +
> > +	/* parse_options() may change the argv, let's make a copy */
> > +	tmp_argv = calloc(argc + 1, sizeof(char *));
> 
> [Severity: Low]
> Should we explicitly include <stdlib.h> in this file to support musl libc
> builds?
> 
> The new kvm_need_default_arch_event() function relies on calloc() and free(),
> but <stdlib.h> isn't explicitly included in tools/perf/util/kvm-stat.c.
> 
> Because musl libc enforces strict standard header separation, relying on
> implicit includes can cause the build to fail.

Good point, will add.

Thanks,
Namhyung

> 
> > +	if (!tmp_argv)
> > +		return false;
> > +
> > +	for (i = 0; i < argc; i++)
> > +		tmp_argv[i] = argv[i];
> > +
> > +	parse_options(argc, tmp_argv, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
> > +	free(tmp_argv);
> > +
> > +	return !event;
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260623070313.55225-1-namhyung@kernel.org?part=1

Re: [PATCH 2/4] perf kvm: Check kvm_need_default_arch_event() early

[Namhyung Kim]

On Tue, Jun 23, 2026 at 07:16:57AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] BUG_ON() unconditionally triggers and crashes `perf kvm record` on architectures where default arch events are not added (e.g., ARM64, RISC-V, s390).
> --
> 
> commit e180ad0afcd0778bb15a60785a0b08bfd2f76a50
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf kvm: Check kvm_need_default_arch_event() early
> 
> This commit modifies perf kvm record to check if default options are needed
> before allocating arguments and passing the extra command line, preventing
> the default event from being incorrectly passed to the external command.
> 
> > diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> > index c236093eac473..acc6f2a706b35 100644
> > --- a/tools/perf/builtin-kvm.c
> > +++ b/tools/perf/builtin-kvm.c
> > @@ -1996,13 +1996,14 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
> >  {
> >  	int rec_argc, i = 0, j, ret;
> >  	const char **rec_argv;
> > +	int need_arch_event = !!kvm_need_default_arch_event(argc, argv);
> >  
> >  	/*
> >  	 * Besides the 2 more options "-o" and "filename",
> >  	 * kvm_add_default_arch_event() may add 2 extra options,
> > -	 * so allocate 4 more items.
> > +	 * so allocate more items conditionally.
> >  	 */
> > -	rec_argc = argc + 2 + 2;
> > +	rec_argc = argc + 2 + (2 * need_arch_event);
> 
> [ ... ]
> 
> > -	if (kvm_need_default_arch_event(argc, argv)) {
> > +	if (need_arch_event) {
> >  		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
> >  		if (ret)
> >  			goto EXIT;
> >  	}
> >  
> > +	for (j = 1; j < argc; j++, i++)
> > +		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
> > +
> > -	BUG_ON(i + 2 != rec_argc);
> > +	BUG_ON(i != rec_argc);
> 
> [Severity: High]
> Could this BUG_ON() cause a regression for perf kvm record on architectures
> that do not add default events?
> 
> If kvm_need_default_arch_event() returns true, rec_argc is set to argc + 4.
> 
> However, on architectures like ARM64, RISC-V, or s390, it appears
> kvm_add_default_arch_event() can return 0 without actually appending any
> events or incrementing i.
> 
> If that happens, i will only equal argc + 2 after the loop, causing this
> assertion to unconditionally fail and abort the process.

I'll check those arch and make sure they return 0 in
kvm_need_default_arch_event().

Thanks,
Namhyung

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

[Namhyung Kim]

On Tue, Jun 23, 2026 at 07:12:59AM +0000, sashiko-bot@kernel.org wrote:
> 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?

Right, will update powerpc.

Thanks,
Namhyung

> 
> >  	free(rec_argv);
> >  	return ret;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260623070313.55225-1-namhyung@kernel.org?part=3

Re: [PATCH 4/4] perf test: Simplify perf kvm record/report tests

[Namhyung Kim]

On Tue, Jun 23, 2026 at 07:09:14AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The synchronous `perf kvm record` command is not wrapped in an error-checking conditional, causing the script to unexpectedly abort via `set -e` upon failure.
> --
> 
> commit a940fab65237cd8e4c37035fe66af932fcd3f955
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf test: Simplify perf kvm record/report tests
> 
> This commit simplifies the perf kvm tests by passing sleep directly as a
> command line argument to perf kvm record, rather than running it in the
> background and manually killing it.
> 
> > diff --git a/tools/perf/tests/shell/kvm.sh b/tools/perf/tests/shell/kvm.sh
> > index f88e859025c42..4ca10388748de 100755
> > --- a/tools/perf/tests/shell/kvm.sh
> > +++ b/tools/perf/tests/shell/kvm.sh
> [ ... ]
> > @@ -63,13 +63,7 @@ test_kvm_record_report() {
> >  	echo "Recording kvm profile for pid ${qemu_pid}..."
> >  	# Use --host to avoid needing guest symbols/mounts for this simple test
> >  	# We just want to verify the command runs and produces data
> > -	# We run in background and kill it because 'perf kvm record' appends options
> > -	# after the command, which breaks 'sleep' (e.g. it gets '-e cycles').
> > -	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" &
> > -	rec_pid=$!
> > -	sleep 1
> > -	kill -INT "${rec_pid}"
> > -	wait "${rec_pid}" || true
> > +	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" sleep 1
> 
> [Severity: Medium]
> If this command fails, will the script abort immediately due to set -e being
> enabled globally?
> 
> The previous implementation guarded against non-zero exit codes by using
> wait "${rec_pid}" || true. Without an error-checking conditional or an
> appended || true, a failure during this synchronous execution could cause the
> script to instantly terminate.
> 
> Would this bypass the test's intended logic of recording the failure (err=1)
> and accidentally skip subsequent tests like test_kvm_buildid_list?

I think it's ok to fail when the record command failed.

Thanks,
Namhyung

> 
> >  
> >  	echo "Reporting kvm profile..."
> >  	# Check for some standard output from report
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260623070313.55225-1-namhyung@kernel.org?part=4