Re: [PATCH 1/1] objtool/klp: fix symbol ordering for data symbols

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The new multi-pass sympos calculation for data symbols hardcodes an incorrect section ordering that prioritizes `.data..ro_after_init` over `.rodata`, and lumps other distinct data sections together, failing to match kallsyms address ordering.

Pre-existing issues:
- [High] Calculating `sympos` for cross-type duplicates (e.g., a function and a variable sharing the same name) yields contradictory results because the iteration passes diverge based on the queried symbol's type without filtering the counted symbols.
--

--- Patch [1]: [PATCH 1/1] objtool/klp: fix symbol ordering for data symbols ---
commit c98091d5ce8ba2d91968d3bbc6e227ed2bf27934
Author: Joe Lawrence <joe.lawrence@redhat.com>

objtool/klp: fix symbol ordering for data symbols

The find_sympos() function calculates a sympos (symbol position) used by
livepatch to disambiguate duplicate symbol names. This commit extends the
existing section ordering workaround to handle data symbols, counting
.data..ro_after_init first, then other .data, then .bss.

> diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> index f8787d7d14547..034778885e922 100644
> --- a/tools/objtool/klp-diff.c
> +++ b/tools/objtool/klp-diff.c
> @@ -873,6 +873,8 @@ static unsigned long find_sympos(struct elf *elf, struct symbol *sym)
>  
>  	if (sym->bind != STB_LOCAL)
>  		return 0;
>  
>  	if (vmlinux && is_func_sym(sym)) {

[Severity: High]
This is a pre-existing issue, but does this logic cause collisions when a
function and a variable share the same name?

The branches here decide which sections are prioritized during counting based
on the queried symbol's type (is_func_sym vs is_object_sym). However, the
inner loops count all duplicates blindly by name without filtering the counted
symbols by type.

If there is a cross-type duplicate, the function and the variable could each
be found first during their respective queries because the passes iterate
sections in different orders. Would this result in both getting the exact same
sympos?

Since klp_find_object_symbol doesn't filter by type in the kernel, could a
request to patch the variable erroneously resolve to the function?

[ ... ]

> @@ -905,6 +907,46 @@ static unsigned long find_sympos(struct elf *elf, struct symbol *sym)
>  					has_dup = true;
>  			}
>  		}
> +	} else if (vmlinux && is_object_sym(sym)) {
> +		/*
> +		 * HACK: Similarly, data symbol ordering can differ between
> +		 * vmlinux.o and vmlinux.  The linker script places sections:
> +		 *   .data..ro_after_init (lowest address)
> +		 *   .data* sections
> +		 *   .bss* sections       (highest address)
> +		 * Count in that order.
> +		 */

[Severity: High]
Does this hardcoded section ordering match the actual kallsyms address
ordering?

In the kernel linker script, .rodata is placed before RO_AFTER_INIT_DATA,
meaning .rodata symbols will appear before .data..ro_after_init symbols since
kallsyms sorts strictly by address.

If a livepatch contains duplicate static data symbols where one is in .rodata
and another is in .data..ro_after_init, this patch will count
.data..ro_after_init first.

Could this assign sympos=1 to the .data..ro_after_init symbol and sympos=2 to
the .rodata symbol, causing livepatch to patch the wrong variable when it asks
for sympos=2?

> +		for_each_sym(elf, s) {
> +			if (strstarts(s->sec->name, ".data..ro_after_init") &&
> +			    !strcmp(s->name, sym->name)) {
> +				nr_matches++;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710153042.3156788-1-joe.lawrence@redhat.com?part=1