Re: [PATCH v6 4/4] kallsyms: add KUnit tests for lineinfo feature

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Test failures on architectures using function descriptors
- [Medium] Silently failing tests due to missing Kconfig dependency for module lineinfo
- [Medium] Test flakiness due to incorrect assumption about compiler optimization and function size
- [Medium] Missing NULL pointer check for test buffer allocation leading to potential kernel panic
--

commit 799168a56f108698328a395b2e257d42cad0b5d4
Author: Sasha Levin <sashal@kernel.org>

kallsyms: add KUnit tests for lineinfo feature
    
Add a KUnit test module (CONFIG_LINEINFO_KUNIT_TEST) that verifies the
kallsyms lineinfo feature produces correct source file:line annotations
in stack traces.

> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 1244dcac2294a..257b5ca442f32 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
[ ... ]
> @@ -3092,6 +3092,16 @@ config LONGEST_SYM_KUNIT_TEST
>  
>  	  If unsure, say N.
>  
> +config LINEINFO_KUNIT_TEST
> +	tristate "KUnit tests for kallsyms lineinfo" if !KUNIT_ALL_TESTS
> +	depends on KUNIT && KALLSYMS_LINEINFO

[Severity: Medium]
If this test is built as a loadable module and CONFIG_KALLSYMS_LINEINFO_MODULES
is disabled, will the module lineinfo tests fail at runtime? Should this Kconfig
also depend on KALLSYMS_LINEINFO_MODULES when built as a module, or should the
test suite gracefully skip module-specific tests if that configuration is not
enabled?

> diff --git a/lib/tests/lineinfo_kunit.c b/lib/tests/lineinfo_kunit.c
> new file mode 100644
> index 0000000000000..285d798cb6a3c
> --- /dev/null
> +++ b/lib/tests/lineinfo_kunit.c
[ ... ]
> +static char *alloc_sym_buf(struct kunit *test)
> +{
> +	return kunit_kzalloc(test, KSYM_SYMBOL_LEN, GFP_KERNEL);
> +}

[Severity: Medium]
Can kunit_kzalloc() return NULL here and cause a kernel panic when callers
unconditionally pass the buffer to sprint_with_lineinfo()? Would it be better
to add a KUNIT_ASSERT_NOT_NULL() check here or validate the return value in
the callers?

[ ... ]
> +static noinline int lineinfo_target_many_lines(void)
> +{
> +	int a = 0;
> +
> +	barrier();
> +	a += 1;
> +	a += 2;
[ ... ]
> +	a += 10;
> +	barrier();
> +	return a;
> +}

[Severity: Medium]
Is it possible that standard compiler optimizations will fold the arithmetic
in this function away, causing it to compile to fewer than 8 bytes?

If that happens, wouldn't the test_many_lines_mid_function() test below fail
because it adds 8 bytes to the start address and ends up looking outside the
function bounds? Should WRITE_ONCE() or inline assembly be used here to
prevent the arithmetic from being optimized out?

[ ... ]
> +static void test_normal_function(struct kunit *test)
> +{
> +	char *buf = alloc_sym_buf(test);
> +	unsigned long addr = (unsigned long)lineinfo_target_normal;

[Severity: Medium]
On architectures that use function descriptors (like PowerPC ELFv1 or ia64),
will casting lineinfo_target_normal directly to unsigned long yield a data
descriptor address instead of the instruction pointer?

Could this cause sprint_backtrace() to fail to resolve the symbol, breaking
the tests unconditionally on those architectures? Does this require using
dereference_symbol_descriptor() to get the actual instruction pointer?

[ ... ]
> +static void test_pBb_format(struct kunit *test)
> +{
> +	char *buf = alloc_sym_buf(test);
> +	/*
> +	 * %pBb uses sprint_backtrace_build_id which subtracts 1 from the
> +	 * address, so pass addr+1 to resolve back to the function.
> +	 */
> +	void *addr = (void *)((unsigned long)lineinfo_target_normal + 1);

[Severity: Medium]
Similarly, adding 1 to a function descriptor pointer instead of an instruction
pointer creates a corrupt address. Will this cause snprintf() with %pBb to
fail to format the backtrace correctly on architectures using function
descriptors?

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