Re: [PATCH 1/2] kallsyms: show function parameter info in oops/WARN dumps

[sashiko-bot@kernel.org]

Sashiko AI review found 8 potential issue(s):
- [High] Build Tool Infinite Loop / Hash Table Exhaustion
- [Medium] Incorrect Parameter Attribution due to Missing Validation
- [Medium] Missing Architecture Dependency in Kconfig
- [Medium] Misleading Fault Address Highlighting for Non-Page-Fault Exceptions
- [Medium] Conceptual Flaw: Statically Mapping Parameters to Clobbered Registers
- [Low] Build Error on x86_64 due to Incorrect pt_regs Fields
- [Low] KUnit Test Module Linkage Failure
- [Low] Unused Static Variable Causes Build Warning/Error
--

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

kallsyms: show function parameter info in oops/WARN dumps

This commit embeds DWARF-derived function parameter name and type
information in the kernel image so that oops and WARN dumps display the
crashing function's register-passed arguments. It adds a build tool to
parse DWARF and stores it in .rodata, gated behind CONFIG_KALLSYMS_PARAMINFO.

> diff --git a/init/Kconfig b/init/Kconfig
> index 3e3acfc37be7e..bc1b38458029d 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -2105,6 +2105,27 @@ config KALLSYMS_LINEINFO_MODULES
>  
>  	  If unsure, say N.
>  
> +config KALLSYMS_PARAMINFO
> +	bool "Show function parameter info in oops/WARN dumps"
> +	depends on KALLSYMS_LINEINFO

Should this have an architecture dependency (e.g., depends on X86_64)?

The feature appears to be heavily x86-64 specific, hardcoding x86-64
registers and exclusively hooking into arch/x86/kernel/dumpstack.c. Enabling
it on unsupported architectures could inflate the kernel image with unused
tables.

[ ... ]

> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index e6f796d43dd70..af8de3d8e3ba3 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -501,6 +501,174 @@ bool kallsyms_lookup_lineinfo(unsigned long addr,
>  	return lineinfo_search(&tbl, (unsigned int)raw_offset, file, line);
>  }
>  
> +#ifdef CONFIG_KALLSYMS_PARAMINFO
> +
> +#include <linux/ptrace.h>
> +#ifdef CONFIG_X86
> +#include <asm/special_insns.h>
> +#endif
> +
> +#define MAX_PARAMINFO_PARAMS 6
> +
> +/*
> + * x86-64 calling convention: arguments are passed in registers
> + * RDI, RSI, RDX, RCX, R8, R9 (in that order).
> + */
> +#ifdef CONFIG_X86_64
> +
> +static const char * const paraminfo_reg_names[] = {
> +	"RDI", "RSI", "RDX", "RCX", "R8", "R9"
> +};

Is this variable used anywhere? It doesn't seem to be referenced in the file,
which could trigger a -Wunused-variable compiler warning.

> +
> +static unsigned long paraminfo_get_reg(const struct pt_regs *regs,
> +				       unsigned int idx)
> +{
> +	switch (idx) {
> +	case 0: return regs->di;
> +	case 1: return regs->si;
> +	case 2: return regs->dx;

Will this cause a build error on x86_64? In the 64-bit kernel, the struct
pt_regs defines these fields as rdi, rsi, rdx, etc. rather than di, si, dx.

[ ... ]

> +	func_idx = paraminfo_find_func(offset);
> +	if (func_idx < 0)
> +		return;
> +
> +	/*
> +	 * Verify the IP is within a reasonable range of the function
> +	 * start.  paraminfo_func_addrs[] contains function start offsets;
> +	 * check that we're not too far past the start.  Use kallsyms to
> +	 * verify we're in the right function.
> +	 */
> +	if (!kallsyms_lookup_size_offset(ip, &sym_size, &sym_offset))
> +		return;

Does this code verify that the matched paraminfo function actually aligns with
the faulting function? 

If the fault occurs in a function without parameters, func_idx will point to 
the closest preceding function with parameter data. Should there be a check 
such as offset - sym_offset == paraminfo_func_addrs[func_idx] to prevent 
printing parameters of the preceding function?

[ ... ]

> +	/* Look up function name for the header */
> +	if (lookup_symbol_name(ip - sym_offset, sym_name))
> +		return;
> +
> +	/*
> +	 * Read the fault address for highlighting.  On x86, CR2 holds
> +	 * the page fault linear address.  On other architectures this
> +	 * would need a different mechanism.
> +	 */
> +#ifdef CONFIG_X86
> +	fault_addr = read_cr2();
> +#else

Can this produce misleading highlighting for non-page-fault exceptions?

The x86 CPU updates the CR2 register during a Page Fault, but for other
exceptions such as WARN() or General Protection Faults, CR2 retains the
stale address from the last unrelated page fault.

[ ... ]

> +	for (i = 0; i < num_params; i++) {
> +		u32 name_off, type_off;
> +		const char *pname, *ptype;
> +		unsigned long val;
> +		bool is_fault_addr;
> +
> +		memcpy(&name_off, data, sizeof(u32));
> +		data += sizeof(u32);
> +		memcpy(&type_off, data, sizeof(u32));
> +		data += sizeof(u32);
> +
> +		pname = paraminfo_strings + name_off;
> +		ptype = paraminfo_strings + type_off;
> +		val = paraminfo_get_reg(regs, i);

Could these caller-saved registers contain clobbered values?

Since exceptions typically occur mid-function, the compiler might have freely
clobbered these registers to hold local variables or intermediate state,
which could lead to garbage values being displayed instead of the original
parameters.

[ ... ]

> diff --git a/lib/tests/paraminfo_kunit.c b/lib/tests/paraminfo_kunit.c
> new file mode 100644
> index 0000000000000..e09efc4ddeb0e
> --- /dev/null
> +++ b/lib/tests/paraminfo_kunit.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0

[ ... ]

> +/*
> + * These access the raw paraminfo tables to verify correctness.
> + * The tables are defined in kernel/kallsyms_internal.h.
> + */
> +extern const u32 paraminfo_num_funcs;
> +extern const u32 paraminfo_func_addrs[];
> +extern const u32 paraminfo_func_offsets[];
> +extern const u8  paraminfo_func_data[];
> +extern const char paraminfo_strings[];

Can this test be built as a loadable module? 

CONFIG_PARAMINFO_KUNIT_TEST is a tristate config, but the test directly 
references internal unexported tables. Building it as a module might fail 
with undefined symbol errors during the modpost stage.

[ ... ]

> diff --git a/scripts/gen_paraminfo.c b/scripts/gen_paraminfo.c
> new file mode 100644
> index 0000000000000..ea1d23f3ddd9a
> --- /dev/null
> +++ b/scripts/gen_paraminfo.c

[ ... ]

> +static unsigned int find_or_add_string(const char *s)
> +{
> +	unsigned int h = hash_str(s);
> +
> +	while (str_hash[h].str) {
> +		if (!strcmp(str_hash[h].str, s))
> +			return str_hash[h].idx;
> +		h = (h + 1) & (STR_HASH_SIZE - 1);
> +	}

Could this loop permanently hang the kernel build process?

The STR_HASH_SIZE is 16,384, but a typical kernel has around 58k functions.
If the number of unique parameter names and types exceeds 16,384, the hash
table will become completely full and this while loop will never find an 
empty slot.

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