Re: [PATCH 15/15] 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 3 potential issue(s) to consider:
- [Medium] Malformed JIT_CODE_LOAD record omits the mandatory null-terminated symbol name, causing an out-of-bounds read in `perf inject --jit`.
- [Low] Implicit function declaration of `gettid()` causes build failures on systems with older glibc.
- [Low] Missing file unlink on the mmap failure path in `open_jitdump` leaves a stale zero-byte file on disk.
--

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

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

This commit converts the external C program workloads in the Intel PT shell
test into built-in workloads. It creates a built-in 'jitdump' workload and
adds multi-architecture compatibility to remove the external C compiler
dependency.

> diff --git a/tools/perf/tests/workloads/jitdump.c b/tools/perf/tests/workloads/jitdump.c
> new file mode 100644
> index 0000000000000..225d3f3dc6e3b
> --- /dev/null
> +++ b/tools/perf/tests/workloads/jitdump.c

[ ... ]

> +static FILE *open_jitdump(void)
> +{

[ ... ]

> +	/* Create an MMAP event for the jitdump file. That is how perf tool finds it. */
> +	m = mmap(0, getpagesize(), PROT_READ | PROT_EXEC, MAP_PRIVATE, fileno(f), 0);
> +	if (m == MAP_FAILED) {
> +		pr_err("mmap failed: %s\n", strerror(errno));
> +		fclose(f);

[Severity: Low]
Does this leave a stale zero-byte file on disk if mmap fails?

The other error paths in open_jitdump() call unlink(filename) before
returning, but it seems to be omitted here.

> +		return NULL;
> +	}
> +	munmap(m, getpagesize());

[ ... ]

> +static int write_jitdump(FILE *f, void *addr, const void *dat, size_t sz, uint64_t *idx)
> +{
> +	struct jr_code_load rec = {
> +		.p.id = JIT_CODE_LOAD,
> +		.p.total_size = sizeof(rec) + sz,
> +		.p.timestamp = get_timestamp(),
> +		.pid = getpid(),
> +		.tid = gettid(),

[Severity: Low]
Is there a missing feature guard or fallback for gettid() here?

On older systems with glibc < 2.30, gettid() is not natively declared in
<unistd.h>. Because the perf tool typically compiles with -Werror, this
implicit declaration can cause a build failure. The perf subsystem usually
provides syscall(__NR_gettid) as a fallback or uses a feature test.

> +		.vma = (unsigned long)addr,
> +		.code_addr = (unsigned long)addr,
> +		.code_size = sz,
> +		.code_index = ++*idx,
> +	};
> +
> +	if (fwrite(&rec, sizeof(rec), 1, f) != 1 || fwrite(dat, sz, 1, f) != 1)

[Severity: Medium]
Can this cause an out-of-bounds read in perf inject --jit?

It looks like write_jitdump() writes the record header and directly follows it
with the raw instruction bytes, omitting the required null-terminated string
for the symbol name.

While the original shell script also omitted this, the original x86 payload
happened to contain null bytes that safely terminated strlen(). The newly
added non-x86 payloads (e.g., ARM64, RISC-V) contain no null bytes.

When perf inject calculates the symbol name pointer in jit_repipe_code_load()
and calls strlen(sym) in jit_write_elf(), could it read out of bounds into
subsequent records or unmapped memory?

> +		return -1;
> +	return 0;
> +}

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