Re: [PATCH v2 next 06/15] 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: race in set_kallsyms_off_size() causes NULL dereference
- [High] kallsyms: set_kallsyms_off_size() sleeps in atomic context
--

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

kallsyms: Optimise symbol name search

This commit optimizes symbol name lookups by indexing the compressed
names table directly instead of the array of symbol values. It supports
both 24-bit and 32-bit offset tables, selecting between them using a
static branch initialized on the first symbol lookup.

> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index d18d78e626db2..31451feac8221 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c

[ ... ]

> @@ -185,14 +216,19 @@ unsigned long kallsyms_sym_address(int idx)
>  	return (unsigned long)offset_to_ptr(kallsyms_offsets + idx);
>  }
>  
> -static unsigned int get_symbol_seq(int index)
> -{
> -	unsigned int i, seq = 0;
> +static DEFINE_STATIC_KEY_TRUE(kallsyms_off_size_24);
>  
> -	for (i = 0; i < 3; i++)
> -		seq = (seq << 8) | kallsyms_seqs_of_names[3 * index + i];
> +static inline unsigned int get_symbol_name_offset(unsigned long index)
> +{
> +	if (static_branch_likely(&kallsyms_off_size_24))
> +		return kallsyms_off24_of_names[index].v;
> +	return kallsyms_off32_of_names[index];
> +}
>  
> -	return seq;
> +static void set_kallsyms_off_size(void)
> +{
> +	if (kallsyms_off_size != 24)
> +		static_branch_disable(&kallsyms_off_size_24);
>  }

[Severity: High]
Can calling static_branch_disable() in set_kallsyms_off_size() sleep in
an atomic or early-boot context when kallsyms_off_size != 24?

static_branch_disable() calls static_key_disable() in
kernel/jump_label.c, which acquires cpus_read_lock() and
jump_label_mutex:

kernel/jump_label.c:static_key_disable() {
	cpus_read_lock();
	static_key_disable_cpuslocked(key);
	cpus_read_unlock();
}

However, kallsyms_lookup_name() -> kallsyms_lookup_names() can be called
first from contexts where sleeping is not allowed:

- lib/codetag.c:get_symbol() calls kallsyms_lookup_name() under
  preempt_disable():

	preempt_disable();
	ret = mod ?
		(void *)find_kallsyms_symbol_value(mod, buf) :
		(void *)kallsyms_lookup_name(buf);
	preempt_enable();

- mm/alloc_tag.c:alloc_tag_sec_init() calls kallsyms_lookup_name() during
  start_kernel() -> mm_core_init() while interrupts are disabled and
  before poking_init().

- Non-sleepable runtime or debugger callers such as
  bpf_kallsyms_lookup_name(), kdbgetsymval(), and xmon also invoke
  kallsyms_lookup_name().

>  
>  static int kallsyms_lookup_names(const char *name,
> @@ -201,15 +237,16 @@ static int kallsyms_lookup_names(const char *name,
>  {
>  	int ret;
>  	int low, mid, high;
> -	unsigned int seq, off;
> +	unsigned int off;
> +
> +	DO_ONCE_LITE(set_kallsyms_off_size);
>  
>  	low = 0;
>  	high = kallsyms_num_syms - 1;
>  
>  	while (low <= high) {
>  		mid = low + (high - low) / 2;
> -		seq = get_symbol_seq(mid);
> -		off = get_symbol_offset(seq);
> +		off = get_symbol_name_offset(mid);

[Severity: High]
Does DO_ONCE_LITE(set_kallsyms_off_size) allow a concurrent caller to
dereference NULL in get_symbol_name_offset() when kallsyms_off_size == 32?

In scripts/kallsyms.c:write_src(), only kallsyms_off24_of_names or
kallsyms_off32_of_names is emitted, and both are declared __weak in
kernel/kallsyms_internal.h:

extern struct { unsigned int v:24 __packed; } __packed kallsyms_off24_of_names[] __weak;
extern u32 kallsyms_off32_of_names[] __weak;

When kallsyms_off_size == 32, kallsyms_off24_of_names resolves to NULL,
while kallsyms_off_size_24 starts enabled via
DEFINE_STATIC_KEY_TRUE(kallsyms_off_size_24).

In include/linux/once_lite.h, __ONCE_LITE_IF() sets __already_done = true
before executing set_kallsyms_off_size() and does not wait for it to
finish:

		if (unlikely(__ret_cond) && unlikely(!__already_done)) {
			__already_done = true;
			__ret_once = true;
		}

If two CPUs call kallsyms_lookup_names() concurrently for the first time
while the first CPU is still inside set_kallsyms_off_size() ->
static_branch_disable(&kallsyms_off_size_24), the second CPU sees
__already_done == true and proceeds directly to
get_symbol_name_offset(mid). Since kallsyms_off_size_24 has not been
patched yet, get_symbol_name_offset() evaluates
kallsyms_off24_of_names[index].v and dereferences NULL.

[ ... ]

>
>
>
>
>

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