[PATCH 0/1] Mitigate klp-build data sympos bug

[PATCH 0/1] Mitigate klp-build data sympos bug

[Joe Lawrence]

Hi Josh,

This is an upstream reproducer of a downstream bug report from Ben.  He
encountered a sympos bug on rhel-10 with the same pwq_cache variable.
Here I distilled it down to minimal (upstream) reproducer config and
patch.

From what I can tell, function symbols already have a "HACK" disclaimer
around their workaround -- the attached patch tries to extend the same
idea to data symbols.  It may not have 100% coverage, and it's certainly
brittle as it assumes ordering as per the linker script.  But the idea
is to band-aid the problem until a more robust fix is ready.

The comment in find_sympos() suggests leveraging the checksum mechanism
as a better fix, but perhaps we could also pass the vmlinux file through
and read the ordering from the post-linked object?  I can work on that
for a v2, but in the meantime do you think this bandaid would be
sufficient for at least downstream use?


Config
======

Basic default x86 configuration and livepatch dependencies:

  $ make defconfig
  $ ./scripts/config --file .config \
             --set-val CONFIG_FTRACE y \
             --set-val CONFIG_KALLSYMS_ALL y \
             --set-val CONFIG_FUNCTION_TRACER y \
             --set-val CONFIG_DYNAMIC_FTRACE y \
             --set-val CONFIG_DYNAMIC_DEBUG y \
             --set-val CONFIG_LIVEPATCH y
  $ make olddefconfig


Base kernel and livepatch build
===============================

Build the reference kernel:

  $ make -j$(nproc)

and a minimal patch to fs/eventpoll.c :: ep_unregister_pollwait() to
provoke klp-relocation reference to pwq_cache:

  $ cat test.patch
  diff --git a/fs/eventpoll.c b/fs/eventpoll.c
  index 1234567..abcdefg 100644
  --- a/fs/eventpoll.c
  +++ b/fs/eventpoll.c
  @@ -684,6 +684,7 @@ static void ep_unregister_pollwait(struct eventpoll *ep, struct epitem *epi)
          while ((pwq = *p) != NULL) {
                  *p = pwq->next;
                  ep_remove_wait_queue(pwq);
  +               WARN_ON_ONCE(!pwq_cache);
                  kmem_cache_free(pwq_cache, pwq);
          }
   }

  $ ./scripts/livepatch/klp-build -T test.patch 
  Validating patch(es)
  Building original kernel
  Copying original object files
  Fixing patch(es)
  Building patched kernel
  Copying patched object files
  Generating original checksums
  Generating patched checksums
  Diffing objects
  vmlinux.o: new function: ep_unregister_pollwait.isra.0
  vmlinux.o: changed function: ep_remove
  vmlinux.o: changed function: ep_clear_and_put
  vmlinux.o: changed function: eventpoll_release_file
  Building patch module: livepatch-test.ko
  SUCCESS

sympos inspection
=================

klp-build assigns sympos=2 to the pwq_cache klp-relocation:

  $ readelf --wide --relocs livepatch-test.ko | grep -w '.klp.sym.vmlinux.pwq_cache'
  0000000000000008  0000006600000001 R_X86_64_64            0000000000000000 .klp.sym.vmlinux.pwq_cache,2 - 4
  0000000000000069  0000006600000002 R_X86_64_PC32          0000000000000000 .klp.sym.vmlinux.pwq_cache,2 - 4

but check out the ordering difference between vmlinux.o and the
post-linked vmlinux [sympos denoted]:

  vmlinux.o (symbol table order, what objtool uses):

    $ objdump -t vmlinux.o | awk '/\.c$/{file=$NF}; /pwq_cache/{print file": "$0}' | column -t
    workqueue.c:  0000000000012ae0  l  O  .bss                  0000000000000008  pwq_cache      << sympos=1
    eventpoll.c:  000000000000fb40  l  O  .data..ro_after_init  0000000000000008  pwq_cache      << sympos=2

  vmlinux (address order, what kallsyms uses *):

    $ objdump -t vmlinux | awk '/\.c$/{file=$NF}; /pwq_cache/{print file": "$0}' | column -t | sort -k2
    eventpoll.c:  ffffffff82b77b40  l  O  .rodata  0000000000000008  pwq_cache                   << sympos=1
    workqueue.c:  ffffffff83a85ae0  l  O  .bss     0000000000000008  pwq_cache                   << sympos=2
    * Note: .data..ro_after_init becomes .rodata after linking

With that, I think find_sympos() needs the same considerations for data
symbols as it currently has for function symbols.
 
Joe Lawrence (1):
  objtool/klp: fix symbol ordering for data symbols

 tools/objtool/klp-diff.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

-- 
2.54.0

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

[Joe Lawrence]

The find_sympos() function calculates a sympos (symbol position) used by
livepatch to disambiguate duplicate symbol names.  At runtime, kallsyms
iterates symbols sorted by address, so sympos must reflect that order.

For function symbols, there's already a workaround that counts
.text.unlikely symbols before other .text symbols, matching the linker
script's section ordering.

Data symbols suffer the same problem: the linker script places sections
in this order:

  .data..ro_after_init (lowest address)
  .data* sections
  .bss* sections (highest address)

However, vmlinux.o's symbol table is ordered by object file link order
(kernel/ before fs/, etc.), which can differ from the final address
order.

Extend the existing section ordering workaround to handle data symbols,
counting .data..ro_after_init first, then other .data, then .bss.

Reported-by: Ben Procknow <bprockno@redhat.com>
Fixes: dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 tools/objtool/klp-diff.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
index c2c4e4968bc2..1c6f9f6d7565 100644
--- a/tools/objtool/klp-diff.c
+++ b/tools/objtool/klp-diff.c
@@ -509,6 +509,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.
+		 */
+		for_each_sym(elf, s) {
+			if (strstarts(s->sec->name, ".data..ro_after_init") &&
+			    !strcmp(s->name, sym->name)) {
+				nr_matches++;
+				if (s == sym)
+					sympos = nr_matches;
+				else
+					has_dup = true;
+			}
+		}
+		for_each_sym(elf, s) {
+			if (!strstarts(s->sec->name, ".data..ro_after_init") &&
+			    !strstarts(s->sec->name, ".bss") &&
+			    !strcmp(s->name, sym->name)) {
+				nr_matches++;
+				if (s == sym)
+					sympos = nr_matches;
+				else
+					has_dup = true;
+			}
+		}
+		for_each_sym(elf, s) {
+			if (strstarts(s->sec->name, ".bss") &&
+			    !strcmp(s->name, sym->name)) {
+				nr_matches++;
+				if (s == sym)
+					sympos = nr_matches;
+				else
+					has_dup = true;
+			}
+		}
 	} else {
 		for_each_sym(elf, s) {
 			if (!strcmp(s->name, sym->name)) {
-- 
2.54.0

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

[sashiko-bot]

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