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

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 a=
n 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 a=
nd a variable sharing the same name) yields contradictory results because t=
he iteration passes diverge based on the queried symbol's type without filt=
ering the counted symbols.
--

--- Patch [1]: [PATCH 1/1] objtool/klp: fix symbol ordering for data symbol=
s ---
commit c98091d5ce8ba2d91968d3bbc6e227ed2bf27934
Author: Joe Lawrence <[email protected]>

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, str=
uct symbol *sym)
> =20
>  	if (sym->bind !=3D STB_LOCAL)
>  		return 0;
> =20
>  	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 bas=
ed
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 coun=
ted
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 s=
ame
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, st=
ruct symbol *sym)
>  					has_dup =3D 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 sin=
ce
kallsyms sorts strictly by address.

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

Could this assign sympos=3D1 to the .data..ro_after_init symbol and sympos=
=3D2 to
the .rodata symbol, causing livepatch to patch the wrong variable when it a=
sks
for sympos=3D2?

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

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260710153042.3156=
[email protected]?part=3D1