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 <[email protected]> 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/[email protected]?part=4
