Re: [PATCH 2/2] kallsyms: Optimise symbol name search

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kallsyms: 32-bit offset truncation in scripts/kallsyms.c causes panic
- [Medium] kallsyms: off-by-one in get_name_address binary search bounds doubles scan time
--

commit aa83959ffd157437ca56938c0da66225c5fc9aaf
Author: David Laight <david.laight.linux@gmail.com>

kallsyms: Optimise symbol name search

Change the alphabetically ordered lookup table (kallsyms_seqs_of_names)
with one that indexes the table of compressed names rather than the
array of symbol values. This removes linear scans during the binary search
for the symbol name.

> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index d18d78e626db2..2f39fa850deae 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
[ ... ]
> @@ -177,6 +177,36 @@ static unsigned int get_symbol_offset(unsigned long pos)
>  	return name - kallsyms_names;
>  }
>  
> +/*
> + * Find the value of a symbol givem the offset in the compressed stream.
> + */
> +static unsigned long get_name_address(unsigned int name_offset)
> +{
> +	unsigned int low, pos, high;
> +
> +	low = 0;
> +	high = kallsyms_num_syms >> 8;
> +
> +	while (high - low > 1) {

[Severity: Medium]
Since high acts as an exclusive upper bound here, doesn't this omit the final
lookup marker from the search space if kallsyms_num_syms is not an exact
multiple of 256?

Any kallsyms lookup for symbols in the last partial block (up to 255 symbols)
might fall back to the second-to-last marker, unnecessarily doubling the
linear scan fallback duration.

> +		pos = low + (high - low) / 2;
> +		if (name_offset >= kallsyms_markers[pos])
> +			low = pos;
> +		else
> +			high = pos;
> +	}
[ ... ]
> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 494852ade6d87..74264ead1efe1 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
[ ... ]
> @@ -448,13 +439,28 @@ static void write_src(void)
>  	printf("\n");
>  
>  	sort_symbols_by_name();
> -	output_label("kallsyms_seqs_of_names");
> -	for (i = 0; i < table_cnt; i++)
> -		printf("\t.byte 0x%02x, 0x%02x, 0x%02x\t/* %s */\n",
> -			(unsigned char)(table[i]->seq >> 16),
> -			(unsigned char)(table[i]->seq >> 8),
> -			(unsigned char)(table[i]->seq >> 0),
> -		       table[i]->sym);
> +	if (table_size < (1u << 24)) {
> +		output_label("kallsyms_off24_of_names");
> +		for (i = 0; i < table_cnt; i++) {
> +			printf("\t.byte 0x%02x, 0x%02x, 0x%02x\t/* %s */\n",
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> +				(unsigned char)(table[i]->seq >> 16),
> +				(unsigned char)(table[i]->seq >> 8),
> +				(unsigned char)(table[i]->seq >> 0),
> +#else
> +				(unsigned char)(table[i]->seq >> 0),
> +				(unsigned char)(table[i]->seq >> 8),
> +				(unsigned char)(table[i]->seq >> 16),
> +#endif
> +			       table[i]->sym);
> +		}
> +	} else {
> +		output_label("kallsyms_off32_of_names");
> +		for (i = 0; i < table_cnt; i++) {
> +			printf("\t.long %#04x\t/* %s */\n",
> +				table[i]->seq >> 16, table[i]->sym);

[Severity: High]
Does this code inadvertently truncate the 32-bit offset?

When the compressed symbol table exceeds 16MB (such as in allyesconfig builds),
this code shifts table[i]->seq right by 16 bits. This unconditionally discards
the lower 16 bits of the sequence offset.

Could this corrupt the lookup table kallsyms_off32_of_names and lead to a
panic during boot when the kernel attempts to look up wild memory addresses?

> +		}
> +	}
>  	printf("\n");
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930132109.260597-1-david.laight.linux@gmail.com?part=2