Re: [PATCH v3 1/4] kallsyms: Add test_kallsyms_perf module to benchmark lookup latency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] kallsyms: NULL pointer dereference in early boot via run_test parameter
- [High] kallsyms: divide-by-zero kernel panic via sysfs num_iters TOCTOU
- [Medium] kallsyms: multi-second boot stall on every boot when config is enabled
- [Low] kallsyms: benchmark latency skewed by cond_resched() preemption
--

commit 1f88a15d1a75733470768014a06f1b4c5dac957f
Author: Jim Cromie <jim.cromie@gmail.com>
Subject: kallsyms: Add test_kallsyms_perf module to benchmark lookup latency

This commit adds a microbenchmark module for the kallsyms subsystem to
evaluate name-to-address, address-to-name, and full table walk performance.
It is configured as a built-in boolean test and includes a sysfs trigger
as well as an automatic run during late boot.

> diff --git a/lib/test_kallsyms_perf.c b/lib/test_kallsyms_perf.c
> index 0000000000000..4d0c39c0a94f7
> --- /dev/null
> +++ b/lib/test_kallsyms_perf.c
> @@ -0,0 +1,241 @@
[ ... ]
> +static void run_name_lookup_bench(void)
> +{
> +	u64 t0, t1, dt_hit, dt_miss;
> +	unsigned long addr = 0;
> +	unsigned int i, nr_hits, nr_misses;
[ ... ]
> +	/* 1. Name search: Existing symbols (Hits) */
> +	t0 = ktime_get_ns();
> +	for (i = 0; i < num_iters; i++) {
> +		const char *sym = hit_symbols[i % nr_hits];
> +
> +		if (unlikely(!(i & 0x3fff)))
> +			cond_resched();

[Severity: Low]
Because this is inside the ktime_get_ns() timing loop, if preemption occurs,
will the elapsed wall-clock time include the sleep time? Could this heavily
inflate the measured latency per iteration, especially since
kallsyms_on_each_match_symbol() already calls cond_resched() internally?

> +
> +		kallsyms_on_each_match_symbol(match_cb, sym, &addr);
> +		OPTIMIZER_HIDE_VAR(addr);
> +	}
> +	t1 = ktime_get_ns();
> +	dt_hit = t1 - t0;
[ ... ]
> +	pr_info("Name Search Hit:  %llu ns/lookup (%llu ms total, %u iters)\n",
> +		div_u64(dt_hit, num_iters), div_u64(dt_hit, 1000000), num_iters);

[Severity: High]
Since num_iters is exposed via sysfs with 0644 permissions, could a concurrent
write change num_iters to 0 while this benchmark is running? Does this dynamic
read of the global variable risk a divide-by-zero kernel panic here, despite
the initial check in run_kallsyms_benchmark()?

> +	pr_info("Name Search Miss: %llu ns/lookup (%llu ms total, %u iters)\n",
> +		div_u64(dt_miss, num_iters), div_u64(dt_miss, 1000000), num_iters);
> +}
[ ... ]
> +static int param_set_trigger(const char *val, const struct kernel_param *kp)
> +{
> +	return run_kallsyms_benchmark();
> +}
> +
> +static const struct kernel_param_ops param_ops_trigger = {
> +	.set = param_set_trigger,
> +};
> +module_param_cb(run_test, &param_ops_trigger, NULL, 0200);

[Severity: High]
If this module parameter is passed on the kernel command line during early
boot, parse_args() will invoke this callback before timekeeping_init() has
run. Will this cause a NULL pointer dereference when the benchmark
subsequently calls ktime_get_ns() and tries to read the uninitialized
tk_core timekeeper?

> +MODULE_PARM_DESC(run_test, "Write 1 to trigger kallsyms benchmark run");
> +
> +static int __init test_kallsyms_init(void)
> +{
> +	return run_kallsyms_benchmark();
> +}
> +late_initcall(test_kallsyms_init);

[Severity: Medium]
Because CONFIG_TEST_KALLSYMS_PERF is a built-in bool, this late_initcall
will execute the heavy benchmark unconditionally on every boot. Will this
introduce a multi-second boot stall for any kernel built with this config
enabled?

> +
> +MODULE_DESCRIPTION("Microbenchmark test module for kallsyms subsystem");
> +MODULE_AUTHOR("Jim Cromie <jim.cromie@gmail.com>");
> +MODULE_LICENSE("GPL");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922-ksyms-tune-v3-0-681a34ea05d9@gmail.com?part=1