Re: [PATCH v2 1/3] kallsyms: Add test_kallsyms_perf module to benchmark lookup latency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] kallsyms: re-exporting iterators breaks security encapsulation
- [High] kallsyms: missing preemption in tight loops causes soft lockups
- [High] kallsyms: divide by zero in latency calculation via module parameter
- [High] kallsyms: direct 64-bit division breaks 32-bit builds
- [Medium] kallsyms: test module spuriously fails on kernels without IPv4
- [Low] kconfig: test module set to 'default m' pollutes builds
--

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

This commit adds a lightweight microbenchmark module in lib/ to evaluate
optimizations and measure performance regressions across kallsyms lookups.

> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index aec2f06858afd..b9e573e9a10b9 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -261,6 +261,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, unsigned long),
>  	}
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(kallsyms_on_each_symbol);

[Severity: High]
Does exporting this function directly reverse the security protection
introduced by commit 0bd476e6c6719 ("kallsyms: unexport
kallsyms_lookup_name() and kallsyms_on_each_symbol()")?

It appears this allows any GPL-compatible out-of-tree module to bypass
symbol exports and resolve unexported symbols by passing a custom match
function.

>  int kallsyms_on_each_match_symbol(int (*fn)(void *, unsigned long),
>  				  const char *name, void *data)
> @@ -279,6 +280,7 @@ int kallsyms_on_each_match_symbol(int (*fn)(void *, unsigned long),
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(kallsyms_on_each_match_symbol);

[Severity: High]
Can this export similarly be abused by loadable modules to circumvent the
EXPORT_SYMBOL mechanism and resolve unexported symbols natively?

> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 134b15a44625e..2a8b1aaee23b3 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -3122,6 +3122,16 @@ config TEST_STATIC_KEYS
>  
>  	  If unsure, say N.
>  
> +config TEST_KALLSYMS_PERF
> +	tristate "kallsyms performance benchmark test module"
> +	default m

[Severity: Low]
Is it intended to have this test module enabled by default? Setting this
to default m pollutes default configurations and slows down compilation.

[ ... ]

> diff --git a/lib/test_kallsyms_perf.c b/lib/test_kallsyms_perf.c
> new file mode 100644
> index 0000000000000..c649e55dae3b8
> --- /dev/null
> +++ b/lib/test_kallsyms_perf.c
> @@ -0,0 +1,228 @@

[ ... ]

> +static unsigned int num_iters = 100000;
> +module_param(num_iters, uint, 0644);
> +MODULE_PARM_DESC(num_iters, "Number of iterations per microbenchmark");

[Severity: High]
What happens if num_iters is set to 0 via sysfs? 

Since this parameter is globally writable and unchecked, could a user
write 0 and instantly trigger a hardware divide-by-zero exception during
the latency calculations below?

> +static const char * const hit_symbols[] = {
> +	"_printk",
> +	"schedule",
> +	"vfs_read",
> +	"do_sys_openat2",
> +	"kernel_clone",
> +	"tcp_v4_rcv",

[Severity: Medium]
Is it guaranteed that tcp_v4_rcv will be present on all kernel 
configurations? 

If the kernel is built without IPv4 support (CONFIG_INET), won't this valid
symbol be missing, causing the correctness validation in
run_name_lookup_bench() to emit a false failure?

[ ... ]

> +static void run_name_lookup_bench(void)
> +{
> +	u64 t0, t1, dt_hit, dt_miss;

[ ... ]

> +	/* 2. Name search: Non-existent symbols (Misses - 17 bsearch probes) */
> +	t0 = ktime_get_ns();
> +	for (i = 0; i < num_iters; i++) {
> +		const char *sym = miss_symbols[i % nr_misses];
> +
> +		kallsyms_on_each_match_symbol(match_cb, sym, &addr);
> +		OPTIMIZER_HIDE_VAR(addr);
> +	}

[Severity: High]
Could large values of num_iters in this tight loop stall the CPU and 
trigger soft lockups on CONFIG_PREEMPT_NONE or CONFIG_PREEMPT_VOLUNTARY
kernels, since cond_resched() is not invoked?

> +	t1 = ktime_get_ns();
> +	dt_miss = t1 - t0;
> +
> +	pr_info("Name Search Hit:  %llu ns/lookup (%llu ms total, %u iters)\n",
> +		dt_hit / num_iters, dt_hit / 1000000, num_iters);

[Severity: High]
Does this direct 64-bit division of dt_hit (a u64) by num_iters break 
32-bit builds? 

This typically causes link-time failures due to an undefined reference to
__udivdi3 on 32-bit architectures. Even with a transition to div_u64() in
later commits, the divisor would still need to be checked against zero.

[ ... ]

> +static void run_address_lookup_bench(void)
> +{
> +	u64 t0, t1, dt_sprint, dt_bldid;

[ ... ]

> +	/* 1. Address-to-name resolution (sprint_symbol) */
> +	t0 = ktime_get_ns();
> +	for (i = 0; i < num_iters; i++) {
> +		unsigned long addr = addrs[i % nr_addrs];
> +
> +		sprint_symbol(symname, addr);
> +		barrier_data(symname);
> +	}

[Severity: High]
Similar to the name search loop, could this tight loop executing millions 
of operations without yielding the CPU trigger soft lockup panics?

[ ... ]

> +	pr_info("sprint_symbol:           %llu ns/lookup (%llu ms total, %u iters)\n",
> +		dt_sprint / num_iters, dt_sprint / 1000000, num_iters);

[Severity: High]
Could this also trigger a divide-by-zero hardware exception if num_iters
is set to 0?

[ ... ]

> +static void run_table_walk_bench(void)
> +{
> +	u64 t0, t1, dt_walk;
> +	unsigned long total_symbols = 0;
> +	int iter = 50;
> +	int i;
> +
> +	t0 = ktime_get_ns();
> +	for (i = 0; i < iter; i++) {
> +		total_symbols = 0;
> +		kallsyms_on_each_symbol(count_cb, &total_symbols);
> +	}
> +	t1 = ktime_get_ns();
> +	dt_walk = t1 - t0;
> +
> +	pr_info("Table Full Walk:  %llu us/pass (%lu symbols scanned, %d passes)\n",
> +		(dt_walk / iter) / 1000, total_symbols, iter);
> +}

[Severity: High]
Does the 64-bit division here (dt_walk / iter) also break 32-bit builds
with an undefined reference to __udivdi3?

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