Re: [PATCH v1 14/14] perf test: Remove /usr/bin/cc dependency from Intel PT shell test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing instruction cache flush causes SIGILL on non-x86 architectures.
- [Medium] Corrupted timestamps on non-x86 architectures due to incorrect JIT dump flags.
- [Medium] Failure to propagate `mmap` errors causes cryptic downstream test failures.
- [Low] Dead code: Fallback error message for unsupported architectures is never reached.
- [Low] Insecure file creation allows symlink attacks.
--

commit e93f510895eeeef8fa2a184cec0136d1c167c471
Author: Ian Rogers <irogers@google.com>

perf test: Remove /usr/bin/cc dependency from Intel PT shell test

This commit replaces the runtime compilation of a thread loop and JIT
workload in the Intel PT shell test with a built-in jitdump workload.
It adds multi-architecture compatibility by dynamically encoding
instructions into opcodes for various architectures.

> diff --git a/tools/perf/tests/workloads/jitdump.c b/tools/perf/tests/workloads/jitdump.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/perf/tests/workloads/jitdump.c
> @@ -0,0 +1,165 @@
[ ... ]
> +static FILE *open_jitdump(void)
> +{
> +	struct jitheader header = {
> +		.magic = JITHEADER_MAGIC,
> +		.version = JITHEADER_VERSION,
> +		.total_size = sizeof(header),
> +		.pid = getpid(),
> +		.timestamp = get_timestamp(),
> +		.flags = JITDUMP_FLAGS_ARCH_TIMESTAMP,

Is it correct to unconditionally set JITDUMP_FLAGS_ARCH_TIMESTAMP here?

On non-x86 architectures, get_timestamp() returns a monotonic timestamp from
clock_gettime(CLOCK_MONOTONIC). If this flag tells perf inject --jit that the
timestamps are raw hardware cycles requiring conversion, won't applying the
hardware-time conversion to an already monotonic timestamp corrupt the
injected event timestamps?

> +	};
> +	char filename[256];
> +	FILE *f;
> +	void *m;
> +
> +	snprintf(filename, sizeof(filename), "jit-%d.dump", getpid());
> +	f = fopen(filename, "w+");

Does this file creation pattern allow symlink attacks?

If a user manually executes 'perf test -w jitdump' in a shared directory
like /tmp, an attacker could create a symlink with this predictable name
(jit-<pid>.dump) to overwrite arbitrary files. Would using open() with
O_CREAT | O_EXCL be safer here?

> +	if (!f) {
> +		pr_err("Failed to open jitdump '%s'\n", filename);
> +		return NULL;
> +	}
> +	/* Create an MMAP event for the jitdump file. That is how perf tool finds it. */
> +	m = mmap(0, page_size, PROT_READ | PROT_EXEC, MAP_PRIVATE, fileno(f), 0);
> +	if (m == MAP_FAILED)
> +		pr_err("Error creating jitdump MMAP event\n");
> +	else
> +		munmap(m, page_size);

Should this mmap failure be fatal?

The code logs an error but proceeds to execute the workload and return
success. Without the MMAP event, the perf tool cannot link the JIT dump,
which seems like it might cause the test script to fail later with
confusing errors.

[ ... ]
> +static int write_jitdump(FILE *f, void *addr, const void *dat, size_t sz, uint64_t *idx)
> +{
[ ... ]
> +	if (fwrite(&rec, sizeof(rec), 1, f) != 1 || fwrite(dat, sz, 1, f) != 1)
> +		return -1;
> +	return 0;
> +}
[ ... ]
> +static int jitdump(int argc __maybe_unused, const char **argv __maybe_unused)
> +{
[ ... ]
> +	/* Copy executable code to executable memory page. */
> +	memcpy(addr, dat, sizeof(dat));

Does this code need an explicit cache flush before execution on non-x86
architectures?

Architectures like ARM, RISC-V, PowerPC, and MIPS have separate instruction
and data caches without automatic coherence. Since memcpy() only updates the
data cache, won't the instruction fetch unit read stale data from the I-cache
and trigger an Illegal Instruction (SIGILL) exception?

Could we use __builtin___clear_cache() here?

> +	/* Record it in the jitdump file */
> +	if (write_jitdump(f, addr, dat, sizeof(dat), &idx) == 0) {
> +		if (sizeof(dat) > 0) {
> +			int (*fn)(void) = addr;
> +
> +			/* Call the function. */
> +			ret = fn() - CHK_BYTE;
> +		} else {
> +			pr_err("jitdump workload not supported on this architecture\n");
> +			ret = 1;
> +		}
> +	}

Is the fallback error message for unsupported architectures reachable?

On unsupported architectures, the dat array has a size of 0. In
write_jitdump(), when sz is 0, fwrite(dat, sz, 1, f) returns 0, causing
write_jitdump() to return -1:

	if (fwrite(&rec, sizeof(rec), 1, f) != 1 || fwrite(dat, sz, 1, f) != 1)
		return -1;

Since write_jitdump() returns -1, the outer if (write_jitdump(...) == 0)
check will fail, and the else branch intended to print 'jitdump workload not
supported on this architecture' will be entirely skipped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513230450.529380-1-irogers@google.com?part=14