* [PATCH] powerpc64/modules: fix ool-ftrace-stub vs. livepatch relocation corruption
@ 2025-09-04 2:29 Joe Lawrence
2025-09-04 2:37 ` Joe Lawrence
0 siblings, 1 reply; 5+ messages in thread
From: Joe Lawrence @ 2025-09-04 2:29 UTC (permalink / raw)
To: linuxppc-dev, live-patching
Cc: Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao
The powerpc64 module .stubs section holds ppc64_stub_entry[] code
trampolines that are generated at module load time. These stubs are
necessary for function calls to external symbols that are too far away
for a simple relative branch.
Logic for finding an available ppc64_stub_entry has relied on a sentinel
value in the funcdata member to indicate a used slot. Code iterates
through the array, inspecting .funcdata to find the first unused (zeroed)
entry:
for (i = 0; stub_func_addr(stubs[i].funcdata); i++)
To support CONFIG_PPC_FTRACE_OUT_OF_LINE, a new setup_ftrace_ool_stubs()
function extended the .stubs section by adding an array of
ftrace_ool_stub structures for each patchable function. A side effect
of writing these smaller structures is that the funcdata sentinel
convention is not maintained. This is not a problem for an ordinary
kernel module, as this occurs in module_finalize(), after which no
further .stubs updates are needed.
However, when loading a livepatch module that contains klp-relocations,
apply_relocate_add() is executed a second time, after the out-of-line
ftrace stubs have been set up.
When apply_relocate_add() then calls stub_for_addr() to handle a
klp-relocation, its search for the next available ppc64_stub_entry[]
slot may stop prematurely in the middle of the ftrace_ool_stub array. A
full ppc64_stub_entry is then written, corrupting the ftrace stubs.
Fix this by explicitly tracking the count of used ppc64_stub_entrys.
Rather than relying on an inline funcdata sentinel value, a new
stub_count is used as the explicit boundary for searching and allocating
stubs. This simplifies the code, eliminates the "one extra reloc" that
was required for the sentinel check, and resolves the memory corruption.
Fixes: eec37961a56a ("powerpc64/ftrace: Move ftrace sequence out of line")
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
arch/powerpc/include/asm/module.h | 1 +
arch/powerpc/kernel/module_64.c | 26 ++++++++------------------
2 files changed, 9 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index e1ee5026ac4a..864e22deaa2c 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -27,6 +27,7 @@ struct ppc_plt_entry {
struct mod_arch_specific {
#ifdef __powerpc64__
unsigned int stubs_section; /* Index of stubs section in module */
+ unsigned int stub_count; /* Number of stubs used */
#ifdef CONFIG_PPC_KERNEL_PCREL
unsigned int got_section; /* What section is the GOT? */
unsigned int pcpu_section; /* .data..percpu section */
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 126bf3b06ab7..2a44bc8e2439 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -209,8 +209,7 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
char *secstrings,
struct module *me)
{
- /* One extra reloc so it's always 0-addr terminated */
- unsigned long relocs = 1;
+ unsigned long relocs = 0;
unsigned i;
/* Every relocated section... */
@@ -705,7 +704,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
/* Find this stub, or if that fails, the next avail. entry */
stubs = (void *)sechdrs[me->arch.stubs_section].sh_addr;
- for (i = 0; stub_func_addr(stubs[i].funcdata); i++) {
+ for (i = 0; i < me->arch.stub_count; i++) {
if (WARN_ON(i >= num_stubs))
return 0;
@@ -716,6 +715,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
if (!create_stub(sechdrs, &stubs[i], addr, me, name))
return 0;
+ me->arch.stub_count++;
return (unsigned long)&stubs[i];
}
@@ -1118,29 +1118,19 @@ int module_trampoline_target(struct module *mod, unsigned long addr,
static int setup_ftrace_ool_stubs(const Elf64_Shdr *sechdrs, unsigned long addr, struct module *me)
{
#ifdef CONFIG_PPC_FTRACE_OUT_OF_LINE
- unsigned int i, total_stubs, num_stubs;
+ unsigned int total_stubs, num_stubs;
struct ppc64_stub_entry *stub;
total_stubs = sechdrs[me->arch.stubs_section].sh_size / sizeof(*stub);
num_stubs = roundup(me->arch.ool_stub_count * sizeof(struct ftrace_ool_stub),
sizeof(struct ppc64_stub_entry)) / sizeof(struct ppc64_stub_entry);
- /* Find the next available entry */
- stub = (void *)sechdrs[me->arch.stubs_section].sh_addr;
- for (i = 0; stub_func_addr(stub[i].funcdata); i++)
- if (WARN_ON(i >= total_stubs))
- return -1;
-
- if (WARN_ON(i + num_stubs > total_stubs))
+ if (WARN_ON(me->arch.stub_count + num_stubs > total_stubs))
return -1;
- stub += i;
- me->arch.ool_stubs = (struct ftrace_ool_stub *)stub;
-
- /* reserve stubs */
- for (i = 0; i < num_stubs; i++)
- if (patch_u32((void *)&stub->funcdata, PPC_RAW_NOP()))
- return -1;
+ stub = (void *)sechdrs[me->arch.stubs_section].sh_addr;
+ me->arch.ool_stubs = (struct ftrace_ool_stub *)(stub + me->arch.stub_count);
+ me->arch.stub_count += num_stubs;
#endif
return 0;
--
2.50.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc64/modules: fix ool-ftrace-stub vs. livepatch relocation corruption
2025-09-04 2:29 [PATCH] powerpc64/modules: fix ool-ftrace-stub vs. livepatch relocation corruption Joe Lawrence
@ 2025-09-04 2:37 ` Joe Lawrence
2025-09-08 11:03 ` Naveen N Rao
0 siblings, 1 reply; 5+ messages in thread
From: Joe Lawrence @ 2025-09-04 2:37 UTC (permalink / raw)
To: linuxppc-dev, live-patching
Cc: Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao
On Wed, Sep 03, 2025 at 10:29:50PM -0400, Joe Lawrence wrote:
> The powerpc64 module .stubs section holds ppc64_stub_entry[] code
> trampolines that are generated at module load time. These stubs are
> necessary for function calls to external symbols that are too far away
> for a simple relative branch.
>
> Logic for finding an available ppc64_stub_entry has relied on a sentinel
> value in the funcdata member to indicate a used slot. Code iterates
> through the array, inspecting .funcdata to find the first unused (zeroed)
> entry:
>
> for (i = 0; stub_func_addr(stubs[i].funcdata); i++)
>
> To support CONFIG_PPC_FTRACE_OUT_OF_LINE, a new setup_ftrace_ool_stubs()
> function extended the .stubs section by adding an array of
> ftrace_ool_stub structures for each patchable function. A side effect
> of writing these smaller structures is that the funcdata sentinel
> convention is not maintained. This is not a problem for an ordinary
> kernel module, as this occurs in module_finalize(), after which no
> further .stubs updates are needed.
>
> However, when loading a livepatch module that contains klp-relocations,
> apply_relocate_add() is executed a second time, after the out-of-line
> ftrace stubs have been set up.
>
> When apply_relocate_add() then calls stub_for_addr() to handle a
> klp-relocation, its search for the next available ppc64_stub_entry[]
> slot may stop prematurely in the middle of the ftrace_ool_stub array. A
> full ppc64_stub_entry is then written, corrupting the ftrace stubs.
>
> Fix this by explicitly tracking the count of used ppc64_stub_entrys.
> Rather than relying on an inline funcdata sentinel value, a new
> stub_count is used as the explicit boundary for searching and allocating
> stubs. This simplifies the code, eliminates the "one extra reloc" that
> was required for the sentinel check, and resolves the memory corruption.
>
Apologies if this is too wordy, I wrote it as a bit of a braindump to
summarize the longer analysis at the bottom of the reply ...
> Fixes: eec37961a56a ("powerpc64/ftrace: Move ftrace sequence out of line")
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
> arch/powerpc/include/asm/module.h | 1 +
> arch/powerpc/kernel/module_64.c | 26 ++++++++------------------
> 2 files changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
> index e1ee5026ac4a..864e22deaa2c 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -27,6 +27,7 @@ struct ppc_plt_entry {
> struct mod_arch_specific {
> #ifdef __powerpc64__
> unsigned int stubs_section; /* Index of stubs section in module */
> + unsigned int stub_count; /* Number of stubs used */
> #ifdef CONFIG_PPC_KERNEL_PCREL
> unsigned int got_section; /* What section is the GOT? */
> unsigned int pcpu_section; /* .data..percpu section */
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 126bf3b06ab7..2a44bc8e2439 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -209,8 +209,7 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
> char *secstrings,
> struct module *me)
> {
> - /* One extra reloc so it's always 0-addr terminated */
> - unsigned long relocs = 1;
> + unsigned long relocs = 0;
> unsigned i;
>
> /* Every relocated section... */
> @@ -705,7 +704,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
>
> /* Find this stub, or if that fails, the next avail. entry */
> stubs = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> - for (i = 0; stub_func_addr(stubs[i].funcdata); i++) {
> + for (i = 0; i < me->arch.stub_count; i++) {
> if (WARN_ON(i >= num_stubs))
> return 0;
>
> @@ -716,6 +715,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
> if (!create_stub(sechdrs, &stubs[i], addr, me, name))
> return 0;
>
> + me->arch.stub_count++;
> return (unsigned long)&stubs[i];
> }
>
> @@ -1118,29 +1118,19 @@ int module_trampoline_target(struct module *mod, unsigned long addr,
> static int setup_ftrace_ool_stubs(const Elf64_Shdr *sechdrs, unsigned long addr, struct module *me)
> {
> #ifdef CONFIG_PPC_FTRACE_OUT_OF_LINE
> - unsigned int i, total_stubs, num_stubs;
> + unsigned int total_stubs, num_stubs;
> struct ppc64_stub_entry *stub;
>
> total_stubs = sechdrs[me->arch.stubs_section].sh_size / sizeof(*stub);
> num_stubs = roundup(me->arch.ool_stub_count * sizeof(struct ftrace_ool_stub),
> sizeof(struct ppc64_stub_entry)) / sizeof(struct ppc64_stub_entry);
>
> - /* Find the next available entry */
> - stub = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> - for (i = 0; stub_func_addr(stub[i].funcdata); i++)
> - if (WARN_ON(i >= total_stubs))
> - return -1;
> -
> - if (WARN_ON(i + num_stubs > total_stubs))
> + if (WARN_ON(me->arch.stub_count + num_stubs > total_stubs))
> return -1;
>
> - stub += i;
> - me->arch.ool_stubs = (struct ftrace_ool_stub *)stub;
> -
> - /* reserve stubs */
> - for (i = 0; i < num_stubs; i++)
> - if (patch_u32((void *)&stub->funcdata, PPC_RAW_NOP()))
> - return -1;
At first I thought the bug was that this loop was re-writting the same
PPC_RAW_NOP() to the same funcdata (i.e, should have been something
like: patch_u32((void *)stub[i]->funcdata, PPC_RAW_NOP())), but that
didn't work and seemed fragile anyway.
> + stub = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> + me->arch.ool_stubs = (struct ftrace_ool_stub *)(stub + me->arch.stub_count);
> + me->arch.stub_count += num_stubs;
> #endif
>
> return 0;
> --
> 2.50.1
>
Original problem
================
I noticed an issue when loading a livepatch module with a klp-relocation
(see Documentation/livepatch/module-elf-format.rst) and then tried
ftracing one of its functions:
$ SYSFS=/sys/kernel/debug/tracing
$ echo 0 > $SYSFS/tracing_on
$ echo > $SYSFS/set_ftrace_filter
$ echo function > $SYSFS/current_tracer
$ echo 'xfs_stats_format:mod:livepatch_module' >> $SYSFS/set_ftrace_filter
# Double check the ftrace filter
$ cat $SYSFS/set_ftrace_filter
xfs_stats_format [livepatch_module]
# Bombs away
$ echo 1 > $SYSFS/tracing_on
$ grep kpatch /sys/fs/xfs/stats/stats
[ ... sometimes a crash, sometimes nothing at all, but never a
$SYSFS/trace entry ... ]
This module is different from any of the in-kernel livepatching
kselftests as it was created by kpatch-build, and included a
klp-relocation:
$ readelf --wide --relocs livepatch-module.ko | awk '/.klp.rela.xfs..text.xfs_stats_format/' RS="\n\n"
Relocation section '.klp.rela.xfs..text.xfs_stats_format' at offset 0x18e010 contains 1 entry:
Offset Info Type Symbol's Value Symbol's Name + Addend
00000000000000d0 0000008c0000000a R_PPC64_REL24 0000000000000000 .klp.sym.xfs.counter_val,1 + 0
And when looking at the module's out-of-line ftrace stubs on a live system, I
saw the corruption.
crash utility analysis
----------------------
Dump the arch-specific module section to find the OOL-ftrace-sub area:
crash> mod | grep livepatch_module
c008000007d30500 livepatch_module c008000007d70000 262144 (not loaded) [CONFIG_KALLSYMS]
crash> struct module.arch c008000007d30500
arch = {
stubs_section = 72,
toc_section = 50,
toc_fixed = true,
tramp = 0xc008000007d70d80,
tramp_regs = 0xc008000007d70da8,
ool_stubs = 0xc008000007d70dd0 <xfs_stats_format+0x558>,
ool_stub_count = 7,
ool_stub_index = 7
},
Figure out how many instructions fit inside the ftrace_ool_stub[] area:
crash> p/d (sizeof(struct ftrace_ool_stub) * 7) / 4
$5 = 42
The crash utility disassembly below shows the memory corruption. After a
valid ftrace_ool_stub at index 1, a full ppc64_stub_entry for a
livepatch relocation has been incorrectly written at
xfs_stats_format+0x580, overwriting the memory where ftrace_ool_stub[1..3]
were supposed to be:
[ edited for readability: ppc64=ppc64_stub_entry and ftrace=ftrace_ool_stub ]
crash> dis 0xc008000007d70dd0 42
ppc64[ ] ftrace[0] <xfs_stats_format+0x558>: .long 0x0
<xfs_stats_format+0x55c>: .long 0x0
<xfs_stats_format+0x560>: mflr r0
<xfs_stats_format+0x564>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
<xfs_stats_format+0x568>: mtlr r0
<xfs_stats_format+0x56c>: b 0xc008000007d70014 <patch_free_livepatch+0xc>
ftrace[1] <xfs_stats_format+0x570>: .long 0x0
<xfs_stats_format+0x574>: .long 0x0
<xfs_stats_format+0x578>: mflr r0
<xfs_stats_format+0x57c>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
ppc64[ ] <xfs_stats_format+0x580>: addis r11,r2,4 << This looks like a full
<xfs_stats_format+0x584>: addi r11,r11,-29448 << ppc64_stub_entry
ftrace[2] <xfs_stats_format+0x588>: std r2,24(r1) << dropped in the middle
<xfs_stats_format+0x58c>: ld r12,32(r11) << of the ool_stubs array
<xfs_stats_format+0x590>: mtctr r12 << of ftrace_ool_stub[]
<xfs_stats_format+0x594>: bctr <<
<xfs_stats_format+0x598>: mtlr r0 <<
<xfs_stats_format+0x59c>: andi. r20,r27,30050 <<
ftrace[3] <xfs_stats_format+0x5a0>: .long 0x54e92b8 <<
<xfs_stats_format+0x5a4>: lfs f0,0(r8) <<
ppc64[ ] <xfs_stats_format+0x5a8>: mflr r0
<xfs_stats_format+0x5ac>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
<xfs_stats_format+0x5b0>: mtlr r0
<xfs_stats_format+0x5b4>: b 0xc008000007d7037c <add_callbacks_to_patch_objects+0xc>
ftrace[4] <xfs_stats_format+0x5b8>: .long 0x0
<xfs_stats_format+0x5bc>: .long 0x0
<xfs_stats_format+0x5c0>: mflr r0
<xfs_stats_format+0x5c4>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
<xfs_stats_format+0x5c8>: mtlr r0
<xfs_stats_format+0x5cc>: b 0xc008000007d705ac <init_module+0xc>
ppc64[ ] ftrace[5] <xfs_stats_format+0x5d0>: .long 0x0
<xfs_stats_format+0x5d4>: .long 0x0
<xfs_stats_format+0x5d8>: mflr r0
<xfs_stats_format+0x5dc>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
<xfs_stats_format+0x5e0>: mtlr r0
<xfs_stats_format+0x5e4>: b 0xc008000007d70864 <kpatch_string+0xc>
ftrace[6] <xfs_stats_format+0x5e8>: .long 0x0
<xfs_stats_format+0x5ec>: .long 0x0
<xfs_stats_format+0x5f0>: mflr r0
<xfs_stats_format+0x5f4>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
ppc64[ ] <xfs_stats_format+0x5f8>: mtlr r0
<xfs_stats_format+0x5fc>: b 0xc008000007d70884 <xfs_stats_format+0xc>
crash utility analysis (post-fix)
--------------------------------
Get our bearings from the arch-specific section of the module:
crash> mod | grep livepatch_module
c008000009ae0500 livepatch_module c0080000099c0000 262144 (not loaded) [CONFIG_KALLSYMS]
crash> struct module.arch c008000009ae0500
arch = {
stubs_section = 72,
stub_count = 20, << new count of all ppc64_stub_entry[]
toc_section = 50,
toc_fixed = true,
tramp = 0xc0080000099c0d80,
tramp_regs = 0xc0080000099c0da8,
ool_stubs = 0xc0080000099c0dd0 <xfs_stats_format+0x558>, << where ftrace_ool_stub[] begins
ool_stub_count = 7, << count of ftrace_ool_stubs
ool_stub_index = 7
},
Disassmble the ftrace_ool_stub[] area for the livepatch_module. Figure out
how many instructions will fit into the ppc64_stub_entry and ftrace_ool_stub
structures so we can mark them along the way.
crash> p/d sizeof(struct ppc64_stub_entry) / 4
$1 = 10
crash> p/d sizeof(struct ftrace_ool_stub) / 4
$2 = 6
crash> p/d (sizeof(struct ppc64_stub_entry) * 20) / 4
$3 = 200
crash> dis 0xc0080000099c0ba0 200
The new disassembly shows that the regular relocation stubs
ppc64_stub_entry[0..13] are followed by the correctly formatted
ftrace_ool_stub area (ftrace_ool_stub[0..6]). The livepatch-specific
ppc64_stub_entry is now safely placed at the end of the section (at
ppc64_stub_entry[19]), avoiding any collision:
[ edited for readability: ppc64=ppc64_stub_entry and ftrace=ftrace_ool_stub ]
ppc64[0] <xfs_stats_format+0x328>: addis r11,r2,-18
<xfs_stats_format+0x32c>: addi r11,r11,-30048
<xfs_stats_format+0x330>: std r2,24(r1)
<xfs_stats_format+0x334>: ld r12,32(r11)
<xfs_stats_format+0x338>: mtctr r12
<xfs_stats_format+0x33c>: bctr
<xfs_stats_format+0x340>: .long 0x0
<xfs_stats_format+0x344>: andi. r20,r27,30050
<xfs_stats_format+0x348>: .long 0x123fef0
<xfs_stats_format+0x34c>: lfs f0,0(0)
ppc64[1] <xfs_stats_format+0x350>: addis r11,r2,-18
<xfs_stats_format+0x354>: addi r11,r11,-30008
<xfs_stats_format+0x358>: std r2,24(r1)
<xfs_stats_format+0x35c>: ld r12,32(r11)
<xfs_stats_format+0x360>: mtctr r12
<xfs_stats_format+0x364>: bctr
<xfs_stats_format+0x368>: .long 0x0
<xfs_stats_format+0x36c>: andi. r20,r27,30050
<xfs_stats_format+0x370>: .long 0xa7c9f0
<xfs_stats_format+0x374>: lfs f0,0(0)
ppc64[2] <xfs_stats_format+0x378>: addis r11,r2,-18
<xfs_stats_format+0x37c>: addi r11,r11,-29968
<xfs_stats_format+0x380>: std r2,24(r1)
<xfs_stats_format+0x384>: ld r12,32(r11)
<xfs_stats_format+0x388>: mtctr r12
<xfs_stats_format+0x38c>: bctr
<xfs_stats_format+0x390>: .long 0x0
<xfs_stats_format+0x394>: andi. r20,r27,30050
<xfs_stats_format+0x398>: .long 0x2f0a94
<xfs_stats_format+0x39c>: lfs f0,0(0)
ppc64[3] <xfs_stats_format+0x3a0>: addis r11,r2,-18
<xfs_stats_format+0x3a4>: addi r11,r11,-29928
<xfs_stats_format+0x3a8>: std r2,24(r1)
<xfs_stats_format+0x3ac>: ld r12,32(r11)
<xfs_stats_format+0x3b0>: mtctr r12
<xfs_stats_format+0x3b4>: bctr
<xfs_stats_format+0x3b8>: .long 0x0
<xfs_stats_format+0x3bc>: andi. r20,r27,30050
<xfs_stats_format+0x3c0>: .long 0x67e440
<xfs_stats_format+0x3c4>: lfs f0,0(0)
ppc64[4] <xfs_stats_format+0x3c8>: addis r11,r2,-18
<xfs_stats_format+0x3cc>: addi r11,r11,-29888
<xfs_stats_format+0x3d0>: std r2,24(r1)
<xfs_stats_format+0x3d4>: ld r12,32(r11)
<xfs_stats_format+0x3d8>: mtctr r12
<xfs_stats_format+0x3dc>: bctr
<xfs_stats_format+0x3e0>: .long 0x0
<xfs_stats_format+0x3e4>: andi. r20,r27,30050
<xfs_stats_format+0x3e8>: .long 0x683da0
<xfs_stats_format+0x3ec>: lfs f0,0(0)
ppc64[5] <xfs_stats_format+0x3f0>: addis r11,r2,-18
<xfs_stats_format+0x3f4>: addi r11,r11,-29848
<xfs_stats_format+0x3f8>: std r2,24(r1)
<xfs_stats_format+0x3fc>: ld r12,32(r11)
<xfs_stats_format+0x400>: mtctr r12
<xfs_stats_format+0x404>: bctr
<xfs_stats_format+0x408>: .long 0x0
<xfs_stats_format+0x40c>: andi. r20,r27,30050
<xfs_stats_format+0x410>: .long 0xa7c8a0
<xfs_stats_format+0x414>: lfs f0,0(0)
ppc64[6] <xfs_stats_format+0x418>: addis r11,r2,-18
<xfs_stats_format+0x41c>: addi r11,r11,-29808
<xfs_stats_format+0x420>: std r2,24(r1)
<xfs_stats_format+0x424>: ld r12,32(r11)
<xfs_stats_format+0x428>: mtctr r12
<xfs_stats_format+0x42c>: bctr
<xfs_stats_format+0x430>: .long 0x0
<xfs_stats_format+0x434>: andi. r20,r27,30050
<xfs_stats_format+0x438>: .long 0x32fad0
<xfs_stats_format+0x43c>: lfs f0,0(0)
ppc64[7] <xfs_stats_format+0x440>: addis r11,r2,-18
<xfs_stats_format+0x444>: addi r11,r11,-29768
<xfs_stats_format+0x448>: std r2,24(r1)
<xfs_stats_format+0x44c>: ld r12,32(r11)
<xfs_stats_format+0x450>: mtctr r12
<xfs_stats_format+0x454>: bctr
<xfs_stats_format+0x458>: .long 0x0
<xfs_stats_format+0x45c>: andi. r20,r27,30050
<xfs_stats_format+0x460>: .long 0x59a5b0
<xfs_stats_format+0x464>: lfs f0,0(0)
ppc64[8] <xfs_stats_format+0x468>: addis r11,r2,-18
<xfs_stats_format+0x46c>: addi r11,r11,-29728
<xfs_stats_format+0x470>: std r2,24(r1)
<xfs_stats_format+0x474>: ld r12,32(r11)
<xfs_stats_format+0x478>: mtctr r12
<xfs_stats_format+0x47c>: bctr
<xfs_stats_format+0x480>: .long 0x0
<xfs_stats_format+0x484>: andi. r20,r27,30050
<xfs_stats_format+0x488>: .long 0xaabd70
<xfs_stats_format+0x48c>: lfs f0,0(0)
ppc64[9] <xfs_stats_format+0x490>: addis r11,r2,-18
<xfs_stats_format+0x494>: addi r11,r11,-29688
<xfs_stats_format+0x498>: std r2,24(r1)
<xfs_stats_format+0x49c>: ld r12,32(r11)
<xfs_stats_format+0x4a0>: mtctr r12
<xfs_stats_format+0x4a4>: bctr
<xfs_stats_format+0x4a8>: .long 0x0
<xfs_stats_format+0x4ac>: andi. r20,r27,30050
<xfs_stats_format+0x4b0>: .long 0xa55f20
<xfs_stats_format+0x4b4>: lfs f0,0(0)
ppc64[10] <xfs_stats_format+0x4b8>: addis r11,r2,-18
<xfs_stats_format+0x4bc>: addi r11,r11,-29648
<xfs_stats_format+0x4c0>: std r2,24(r1)
<xfs_stats_format+0x4c4>: ld r12,32(r11)
<xfs_stats_format+0x4c8>: mtctr r12
<xfs_stats_format+0x4cc>: bctr
<xfs_stats_format+0x4d0>: .long 0x0
<xfs_stats_format+0x4d4>: andi. r20,r27,30050
<xfs_stats_format+0x4d8>: .long 0x59a730
<xfs_stats_format+0x4dc>: lfs f0,0(0)
ppc64[11] <xfs_stats_format+0x4e0>: addis r11,r2,-18
<xfs_stats_format+0x4e4>: addi r11,r11,-29608
<xfs_stats_format+0x4e8>: std r2,24(r1)
<xfs_stats_format+0x4ec>: ld r12,32(r11)
<xfs_stats_format+0x4f0>: mtctr r12
<xfs_stats_format+0x4f4>: bctr
<xfs_stats_format+0x4f8>: .long 0x0
<xfs_stats_format+0x4fc>: andi. r20,r27,30050
<xfs_stats_format+0x500>: .long 0x1249840
<xfs_stats_format+0x504>: lfs f0,0(0)
ppc64[12] <xfs_stats_format+0x508>: ld r12,16(r13)
<xfs_stats_format+0x50c>: addis r12,r12,-464
<xfs_stats_format+0x510>: addi r12,r12,3568
<xfs_stats_format+0x514>: mtctr r12
<xfs_stats_format+0x518>: bctr
<xfs_stats_format+0x51c>: .long 0x0
<xfs_stats_format+0x520>: .long 0x0
<xfs_stats_format+0x524>: andi. r20,r27,30050
<xfs_stats_format+0x528>: .long 0x78ef0
<xfs_stats_format+0x52c>: lfs f0,0(0)
ppc64[13] <xfs_stats_format+0x530>: ld r12,16(r13)
<xfs_stats_format+0x534>: addis r12,r12,-464
<xfs_stats_format+0x538>: addi r12,r12,3104
<xfs_stats_format+0x53c>: mtctr r12
<xfs_stats_format+0x540>: bctr
<xfs_stats_format+0x544>: .long 0x0
<xfs_stats_format+0x548>: .long 0x0
<xfs_stats_format+0x54c>: andi. r20,r27,30050
<xfs_stats_format+0x550>: .long 0x78d20
<xfs_stats_format+0x554>: lfs f0,0(0)
ppc64[14] ftrace[0] <xfs_stats_format+0x558>: .long 0x0
<xfs_stats_format+0x55c>: .long 0x0
<xfs_stats_format+0x560>: mflr r0
<xfs_stats_format+0x564>: bl 0xc0080000099c0d80 <xfs_stats_format+0x508>
<xfs_stats_format+0x568>: mtlr r0
<xfs_stats_format+0x56c>: b 0xc0080000099c0014 <patch_free_livepatch+0xc>
ftrace[1] <xfs_stats_format+0x570>: .long 0x0
<xfs_stats_format+0x574>: .long 0x0
<xfs_stats_format+0x578>: mflr r0
<xfs_stats_format+0x57c>: bl 0xc0080000099c0d80 <xfs_stats_format+0x508>
ppc64[15] <xfs_stats_format+0x580>: mtlr r0
<xfs_stats_format+0x584>: b 0xc0080000099c0100 <patch_free_scaffold+0xc>
ftrace[2] <xfs_stats_format+0x588>: .long 0x0
<xfs_stats_format+0x58c>: .long 0x0
<xfs_stats_format+0x590>: mflr r0
<xfs_stats_format+0x594>: bl 0xc0080000099c0d80 <xfs_stats_format+0x508>
<xfs_stats_format+0x598>: mtlr r0
<xfs_stats_format+0x59c>: b 0xc0080000099c0244 <patch_find_object_by_name+0xc>
ftrace[3] <xfs_stats_format+0x5a0>: .long 0x0
<xfs_stats_format+0x5a4>: .long 0x0
ppc64[16] <xfs_stats_format+0x5a8>: mflr r0
<xfs_stats_format+0x5ac>: bl 0xc0080000099c0d80 <xfs_stats_format+0x508>
<xfs_stats_format+0x5b0>: mtlr r0
<xfs_stats_format+0x5b4>: b 0xc0080000099c037c <add_callbacks_to_patch_objects+0xc>
ftrace[4] <xfs_stats_format+0x5b8>: .long 0x0
<xfs_stats_format+0x5bc>: .long 0x0
<xfs_stats_format+0x5c0>: mflr r0
<xfs_stats_format+0x5c4>: bl 0xc0080000099c0d80 <xfs_stats_format+0x508>
<xfs_stats_format+0x5c8>: mtlr r0
<xfs_stats_format+0x5cc>: b 0xc0080000099c05ac <init_module+0xc>
ppc64[17] ftrace[5] <xfs_stats_format+0x5d0>: .long 0x0
<xfs_stats_format+0x5d4>: .long 0x0
<xfs_stats_format+0x5d8>: mflr r0
<xfs_stats_format+0x5dc>: bl 0xc0080000099c0d80 <xfs_stats_format+0x508>
<xfs_stats_format+0x5e0>: mtlr r0
<xfs_stats_format+0x5e4>: b 0xc0080000099c0864 <kpatch_string+0xc>
ftrace[6] <xfs_stats_format+0x5e8>: .long 0x0
<xfs_stats_format+0x5ec>: .long 0x0
<xfs_stats_format+0x5f0>: mflr r0
<xfs_stats_format+0x5f4>: bl 0xc0080000099c0d80 <xfs_stats_format+0x508>
ppc64[18] <xfs_stats_format+0x5f8>: mtlr r0
<xfs_stats_format+0x5fc>: b 0xc0080000099c0884 <xfs_stats_format+0xc>
(unused) <xfs_stats_format+0x600>: .long 0x0
.. <xfs_stats_format+0x604>: .long 0x0
.. <xfs_stats_format+0x608>: .long 0x0
.. <xfs_stats_format+0x60c>: .long 0x0
.. <xfs_stats_format+0x610>: .long 0x0
.. <xfs_stats_format+0x614>: .long 0x0
.. <xfs_stats_format+0x618>: .long 0x0
.. <xfs_stats_format+0x61c>: .long 0x0
ppc64[19] full- <xfs_stats_format+0x620>: addis r11,r2,-18 << klp-relocation
klp-reloc <xfs_stats_format+0x624>: addi r11,r11,-29288 << ppc64_stub_entry
stub[0] <xfs_stats_format+0x628>: std r2,24(r1) << now tacked on *after*
<xfs_stats_format+0x62c>: ld r12,32(r11) << the OOL-ftrace
<xfs_stats_format+0x630>: mtctr r12 << stubs
<xfs_stats_format+0x634>: bctr <<
<xfs_stats_format+0x638>: .long 0x0 <<
<xfs_stats_format+0x63c>: andi. r20,r27,30050 <<
<xfs_stats_format+0x640>: .long 0x54e92b8 <<
<xfs_stats_format+0x644>: lfs f0,0(r8) <<
And finally re-test the original problem, ftrace the livepatch-provided
function:
$ SYSFS=/sys/kernel/debug/tracing
$ echo 0 > $SYSFS/tracing_on
$ echo > $SYSFS/set_ftrace_filter
$ echo function > $SYSFS/current_tracer
$ echo 'xfs_stats_format:mod:livepatch_module' >> $SYSFS/set_ftrace_filter
# Double check the ftrace filter
$ cat $SYSFS/set_ftrace_filter
xfs_stats_format [livepatch_module]
# Bombs away
$ echo 1 > $SYSFS/tracing_on
$ grep kpatch /sys/fs/xfs/stats/stats
kpatch
$ cat $SYSFS/trace
# tracer: function
#
# entries-in-buffer/entries-written: 1/1 #P:8
#
# _-----=> irqs-off/BH-disabled
# / _----=> need-resched
# | / _---=> hardirq/softirq
# || / _--=> preempt-depth
# ||| / _-=> migrate-disable
# |||| / delay
# TASK-PID CPU# ||||| TIMESTAMP FUNCTION
# | | | ||||| | |
grep-71247 [001] ..... 53756.730711: xfs_stats_format <-livepatch_handler
An entry in the trace buffer and no crash :)
--
Joe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc64/modules: fix ool-ftrace-stub vs. livepatch relocation corruption
2025-09-04 2:37 ` Joe Lawrence
@ 2025-09-08 11:03 ` Naveen N Rao
2025-09-10 18:57 ` Joe Lawrence
0 siblings, 1 reply; 5+ messages in thread
From: Naveen N Rao @ 2025-09-08 11:03 UTC (permalink / raw)
To: Joe Lawrence
Cc: linuxppc-dev, live-patching, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy
On Wed, Sep 03, 2025 at 10:37:39PM -0400, Joe Lawrence wrote:
> On Wed, Sep 03, 2025 at 10:29:50PM -0400, Joe Lawrence wrote:
> > The powerpc64 module .stubs section holds ppc64_stub_entry[] code
> > trampolines that are generated at module load time. These stubs are
> > necessary for function calls to external symbols that are too far away
> > for a simple relative branch.
> >
> > Logic for finding an available ppc64_stub_entry has relied on a sentinel
> > value in the funcdata member to indicate a used slot. Code iterates
> > through the array, inspecting .funcdata to find the first unused (zeroed)
> > entry:
> >
> > for (i = 0; stub_func_addr(stubs[i].funcdata); i++)
> >
> > To support CONFIG_PPC_FTRACE_OUT_OF_LINE, a new setup_ftrace_ool_stubs()
> > function extended the .stubs section by adding an array of
> > ftrace_ool_stub structures for each patchable function. A side effect
> > of writing these smaller structures is that the funcdata sentinel
> > convention is not maintained.
There is clearly a bug in how we are reserving the stubs as you point
out further below, but once that is properly initialized, I don't think
the smaller structure size for ftrace_ool_stub matters (in so far as
stub->funcdata being non-NULL). We end up writing four valid powerpc
instructions, along with a ftrace_ops pointer before those instructions
which should also be non-zero (there is a bug here too, more on that
below). The whole function descriptor dance does complicate matters a
bit though.
> > This is not a problem for an ordinary
> > kernel module, as this occurs in module_finalize(), after which no
> > further .stubs updates are needed.
> >
> > However, when loading a livepatch module that contains klp-relocations,
> > apply_relocate_add() is executed a second time, after the out-of-line
> > ftrace stubs have been set up.
> >
> > When apply_relocate_add() then calls stub_for_addr() to handle a
> > klp-relocation, its search for the next available ppc64_stub_entry[]
> > slot may stop prematurely in the middle of the ftrace_ool_stub array. A
> > full ppc64_stub_entry is then written, corrupting the ftrace stubs.
> >
> > Fix this by explicitly tracking the count of used ppc64_stub_entrys.
> > Rather than relying on an inline funcdata sentinel value, a new
> > stub_count is used as the explicit boundary for searching and allocating
> > stubs. This simplifies the code, eliminates the "one extra reloc" that
> > was required for the sentinel check, and resolves the memory corruption.
> >
>
> Apologies if this is too wordy, I wrote it as a bit of a braindump to
> summarize the longer analysis at the bottom of the reply ...
This was a good read, thanks for all the details. It did help spot
another issue.
>
> > Fixes: eec37961a56a ("powerpc64/ftrace: Move ftrace sequence out of line")
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > ---
> > arch/powerpc/include/asm/module.h | 1 +
> > arch/powerpc/kernel/module_64.c | 26 ++++++++------------------
> > 2 files changed, 9 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
> > index e1ee5026ac4a..864e22deaa2c 100644
> > --- a/arch/powerpc/include/asm/module.h
> > +++ b/arch/powerpc/include/asm/module.h
> > @@ -27,6 +27,7 @@ struct ppc_plt_entry {
> > struct mod_arch_specific {
> > #ifdef __powerpc64__
> > unsigned int stubs_section; /* Index of stubs section in module */
> > + unsigned int stub_count; /* Number of stubs used */
> > #ifdef CONFIG_PPC_KERNEL_PCREL
> > unsigned int got_section; /* What section is the GOT? */
> > unsigned int pcpu_section; /* .data..percpu section */
> > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> > index 126bf3b06ab7..2a44bc8e2439 100644
> > --- a/arch/powerpc/kernel/module_64.c
> > +++ b/arch/powerpc/kernel/module_64.c
> > @@ -209,8 +209,7 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
> > char *secstrings,
> > struct module *me)
> > {
> > - /* One extra reloc so it's always 0-addr terminated */
> > - unsigned long relocs = 1;
> > + unsigned long relocs = 0;
> > unsigned i;
> >
> > /* Every relocated section... */
> > @@ -705,7 +704,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
> >
> > /* Find this stub, or if that fails, the next avail. entry */
> > stubs = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> > - for (i = 0; stub_func_addr(stubs[i].funcdata); i++) {
> > + for (i = 0; i < me->arch.stub_count; i++) {
> > if (WARN_ON(i >= num_stubs))
> > return 0;
> >
> > @@ -716,6 +715,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
> > if (!create_stub(sechdrs, &stubs[i], addr, me, name))
> > return 0;
> >
> > + me->arch.stub_count++;
> > return (unsigned long)&stubs[i];
> > }
> >
> > @@ -1118,29 +1118,19 @@ int module_trampoline_target(struct module *mod, unsigned long addr,
> > static int setup_ftrace_ool_stubs(const Elf64_Shdr *sechdrs, unsigned long addr, struct module *me)
> > {
> > #ifdef CONFIG_PPC_FTRACE_OUT_OF_LINE
> > - unsigned int i, total_stubs, num_stubs;
> > + unsigned int total_stubs, num_stubs;
> > struct ppc64_stub_entry *stub;
> >
> > total_stubs = sechdrs[me->arch.stubs_section].sh_size / sizeof(*stub);
> > num_stubs = roundup(me->arch.ool_stub_count * sizeof(struct ftrace_ool_stub),
> > sizeof(struct ppc64_stub_entry)) / sizeof(struct ppc64_stub_entry);
> >
> > - /* Find the next available entry */
> > - stub = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> > - for (i = 0; stub_func_addr(stub[i].funcdata); i++)
> > - if (WARN_ON(i >= total_stubs))
> > - return -1;
> > -
> > - if (WARN_ON(i + num_stubs > total_stubs))
> > + if (WARN_ON(me->arch.stub_count + num_stubs > total_stubs))
> > return -1;
> >
> > - stub += i;
> > - me->arch.ool_stubs = (struct ftrace_ool_stub *)stub;
> > -
> > - /* reserve stubs */
> > - for (i = 0; i < num_stubs; i++)
> > - if (patch_u32((void *)&stub->funcdata, PPC_RAW_NOP()))
> > - return -1;
>
> At first I thought the bug was that this loop was re-writting the same
> PPC_RAW_NOP() to the same funcdata (i.e, should have been something
> like: patch_u32((void *)stub[i]->funcdata, PPC_RAW_NOP())), but that
> didn't work and seemed fragile anyway.
D'uh - this path was clearly never tested. I suppose this should have
been something like this:
patch_ulong((void *)&stub[i]->funcdata, func_desc(local_paca))
Still convoluted, but I think that should hopefully fix the problem you
are seeing.
>
> > + stub = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> > + me->arch.ool_stubs = (struct ftrace_ool_stub *)(stub + me->arch.stub_count);
> > + me->arch.stub_count += num_stubs;
> > #endif
Regardless of the above, your proposed change looks good to me and
simplifies the logic. So:
Acked-by: Naveen N Rao (AMD) <naveen@kernel.org>
> crash> dis 0xc008000007d70dd0 42
> ppc64[ ] ftrace[0] <xfs_stats_format+0x558>: .long 0x0
> <xfs_stats_format+0x55c>: .long 0x0
> <xfs_stats_format+0x560>: mflr r0
> <xfs_stats_format+0x564>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
> <xfs_stats_format+0x568>: mtlr r0
> <xfs_stats_format+0x56c>: b 0xc008000007d70014 <patch_free_livepatch+0xc>
> ftrace[1] <xfs_stats_format+0x570>: .long 0x0
> <xfs_stats_format+0x574>: .long 0x0
> <xfs_stats_format+0x578>: mflr r0
> <xfs_stats_format+0x57c>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
> ppc64[ ] <xfs_stats_format+0x580>: addis r11,r2,4 << This looks like a full
> <xfs_stats_format+0x584>: addi r11,r11,-29448 << ppc64_stub_entry
> ftrace[2] <xfs_stats_format+0x588>: std r2,24(r1) << dropped in the middle
> <xfs_stats_format+0x58c>: ld r12,32(r11) << of the ool_stubs array
> <xfs_stats_format+0x590>: mtctr r12 << of ftrace_ool_stub[]
> <xfs_stats_format+0x594>: bctr <<
> <xfs_stats_format+0x598>: mtlr r0 <<
> <xfs_stats_format+0x59c>: andi. r20,r27,30050 <<
> ftrace[3] <xfs_stats_format+0x5a0>: .long 0x54e92b8 <<
> <xfs_stats_format+0x5a4>: lfs f0,0(r8) <<
> ppc64[ ] <xfs_stats_format+0x5a8>: mflr r0
> <xfs_stats_format+0x5ac>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
> <xfs_stats_format+0x5b0>: mtlr r0
> <xfs_stats_format+0x5b4>: b 0xc008000007d7037c <add_callbacks_to_patch_objects+0xc>
> ftrace[4] <xfs_stats_format+0x5b8>: .long 0x0
> <xfs_stats_format+0x5bc>: .long 0x0
These NULL values are also problematic. I think those are NULL since we
are not "reserving" the stubs properly, but those should point to some
ftrace_op. I think we are missing a call to ftrace_rec_set_nop_ops() in
ftrace_init_nop(), which would be good to do separately.
- Naveen
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc64/modules: fix ool-ftrace-stub vs. livepatch relocation corruption
2025-09-08 11:03 ` Naveen N Rao
@ 2025-09-10 18:57 ` Joe Lawrence
2025-09-11 6:25 ` Naveen N Rao
0 siblings, 1 reply; 5+ messages in thread
From: Joe Lawrence @ 2025-09-10 18:57 UTC (permalink / raw)
To: Naveen N Rao
Cc: linuxppc-dev, live-patching, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy
On Mon, Sep 08, 2025 at 04:33:24PM +0530, Naveen N Rao wrote:
> On Wed, Sep 03, 2025 at 10:37:39PM -0400, Joe Lawrence wrote:
> > On Wed, Sep 03, 2025 at 10:29:50PM -0400, Joe Lawrence wrote:
> > > The powerpc64 module .stubs section holds ppc64_stub_entry[] code
> > > trampolines that are generated at module load time. These stubs are
> > > necessary for function calls to external symbols that are too far away
> > > for a simple relative branch.
> > >
> > > Logic for finding an available ppc64_stub_entry has relied on a sentinel
> > > value in the funcdata member to indicate a used slot. Code iterates
> > > through the array, inspecting .funcdata to find the first unused (zeroed)
> > > entry:
> > >
> > > for (i = 0; stub_func_addr(stubs[i].funcdata); i++)
> > >
> > > To support CONFIG_PPC_FTRACE_OUT_OF_LINE, a new setup_ftrace_ool_stubs()
> > > function extended the .stubs section by adding an array of
> > > ftrace_ool_stub structures for each patchable function. A side effect
> > > of writing these smaller structures is that the funcdata sentinel
> > > convention is not maintained.
>
> There is clearly a bug in how we are reserving the stubs as you point
> out further below, but once that is properly initialized, I don't think
> the smaller structure size for ftrace_ool_stub matters (in so far as
> stub->funcdata being non-NULL). We end up writing four valid powerpc
> instructions, along with a ftrace_ops pointer before those instructions
> which should also be non-zero (there is a bug here too, more on that
> below). The whole function descriptor dance does complicate matters a
> bit though.
>
Hi Naveen,
Ah hah, I see now, given the other bug that you mention, we should have
had seen non-NULL entries in either ftrace_ool_stub.insn[] or .ftrace_op
fields such that when read as ppc64_stub_entry, .funcdata would indicate
that it's in use:
ppc64_stub_entry[] ftrace_ool_stub[]
0x00 [0].jump[0] [0].ftrace_op
0x04 [0].jump[1] [0].ftrace_op
0x08 [0].jump[2] [0].insn[0]
0x0C [0].jump[3] [0].insn[1]
0x10 [0].jump[4] [0].insn[2]
0x14 [0].jump[5] [0].insn[3]
0x18 [0].jump[6] [1].ftrace_op
0x1C [0].magic [1].ftrace_op
0x20 [0].funcdata [1].insn[0] <<
0x24 [0].funcdata [1].insn[1] <<
0x28 [1].jump[0] [1].insn[2]
0x2C [1].jump[1] [1].insn[3]
0x30 [1].jump[2] [2].ftrace_op
0x34 [1].jump[3] [2].ftrace_op
0x38 [1].jump[4] [2].insn[0]
0x3C [1].jump[5] [2].insn[1]
0x40 [1].jump[6] [2].insn[2]
0x44 [1].magic [2].insn[3]
0x48 [1].funcdata [3].ftrace_op <<
0x4C [1].funcdata [3].ftrace_op <<
If the commit msg for this patch would be clearer by rewording anything,
I'm happy to update. (My understanding at the time of writing was that
the NULL funcdata vs. insn[]/ftrace_op was a valid sequence.)
> > > This is not a problem for an ordinary
> > > kernel module, as this occurs in module_finalize(), after which no
> > > further .stubs updates are needed.
> > >
> > > However, when loading a livepatch module that contains klp-relocations,
> > > apply_relocate_add() is executed a second time, after the out-of-line
> > > ftrace stubs have been set up.
> > >
> > > When apply_relocate_add() then calls stub_for_addr() to handle a
> > > klp-relocation, its search for the next available ppc64_stub_entry[]
> > > slot may stop prematurely in the middle of the ftrace_ool_stub array. A
> > > full ppc64_stub_entry is then written, corrupting the ftrace stubs.
> > >
> > > Fix this by explicitly tracking the count of used ppc64_stub_entrys.
> > > Rather than relying on an inline funcdata sentinel value, a new
> > > stub_count is used as the explicit boundary for searching and allocating
> > > stubs. This simplifies the code, eliminates the "one extra reloc" that
> > > was required for the sentinel check, and resolves the memory corruption.
> > >
> >
> > Apologies if this is too wordy, I wrote it as a bit of a braindump to
> > summarize the longer analysis at the bottom of the reply ...
>
> This was a good read, thanks for all the details. It did help spot
> another issue.
>
> >
> > > Fixes: eec37961a56a ("powerpc64/ftrace: Move ftrace sequence out of line")
> > > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > > ---
> > > arch/powerpc/include/asm/module.h | 1 +
> > > arch/powerpc/kernel/module_64.c | 26 ++++++++------------------
> > > 2 files changed, 9 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
> > > index e1ee5026ac4a..864e22deaa2c 100644
> > > --- a/arch/powerpc/include/asm/module.h
> > > +++ b/arch/powerpc/include/asm/module.h
> > > @@ -27,6 +27,7 @@ struct ppc_plt_entry {
> > > struct mod_arch_specific {
> > > #ifdef __powerpc64__
> > > unsigned int stubs_section; /* Index of stubs section in module */
> > > + unsigned int stub_count; /* Number of stubs used */
> > > #ifdef CONFIG_PPC_KERNEL_PCREL
> > > unsigned int got_section; /* What section is the GOT? */
> > > unsigned int pcpu_section; /* .data..percpu section */
> > > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> > > index 126bf3b06ab7..2a44bc8e2439 100644
> > > --- a/arch/powerpc/kernel/module_64.c
> > > +++ b/arch/powerpc/kernel/module_64.c
> > > @@ -209,8 +209,7 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
> > > char *secstrings,
> > > struct module *me)
> > > {
> > > - /* One extra reloc so it's always 0-addr terminated */
> > > - unsigned long relocs = 1;
> > > + unsigned long relocs = 0;
> > > unsigned i;
> > >
> > > /* Every relocated section... */
> > > @@ -705,7 +704,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
> > >
> > > /* Find this stub, or if that fails, the next avail. entry */
> > > stubs = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> > > - for (i = 0; stub_func_addr(stubs[i].funcdata); i++) {
> > > + for (i = 0; i < me->arch.stub_count; i++) {
> > > if (WARN_ON(i >= num_stubs))
> > > return 0;
> > >
> > > @@ -716,6 +715,7 @@ static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
> > > if (!create_stub(sechdrs, &stubs[i], addr, me, name))
> > > return 0;
> > >
> > > + me->arch.stub_count++;
> > > return (unsigned long)&stubs[i];
> > > }
> > >
> > > @@ -1118,29 +1118,19 @@ int module_trampoline_target(struct module *mod, unsigned long addr,
> > > static int setup_ftrace_ool_stubs(const Elf64_Shdr *sechdrs, unsigned long addr, struct module *me)
> > > {
> > > #ifdef CONFIG_PPC_FTRACE_OUT_OF_LINE
> > > - unsigned int i, total_stubs, num_stubs;
> > > + unsigned int total_stubs, num_stubs;
> > > struct ppc64_stub_entry *stub;
> > >
> > > total_stubs = sechdrs[me->arch.stubs_section].sh_size / sizeof(*stub);
> > > num_stubs = roundup(me->arch.ool_stub_count * sizeof(struct ftrace_ool_stub),
> > > sizeof(struct ppc64_stub_entry)) / sizeof(struct ppc64_stub_entry);
> > >
> > > - /* Find the next available entry */
> > > - stub = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> > > - for (i = 0; stub_func_addr(stub[i].funcdata); i++)
> > > - if (WARN_ON(i >= total_stubs))
> > > - return -1;
> > > -
> > > - if (WARN_ON(i + num_stubs > total_stubs))
> > > + if (WARN_ON(me->arch.stub_count + num_stubs > total_stubs))
> > > return -1;
> > >
> > > - stub += i;
> > > - me->arch.ool_stubs = (struct ftrace_ool_stub *)stub;
> > > -
> > > - /* reserve stubs */
> > > - for (i = 0; i < num_stubs; i++)
> > > - if (patch_u32((void *)&stub->funcdata, PPC_RAW_NOP()))
> > > - return -1;
> >
> > At first I thought the bug was that this loop was re-writting the same
> > PPC_RAW_NOP() to the same funcdata (i.e, should have been something
> > like: patch_u32((void *)stub[i]->funcdata, PPC_RAW_NOP())), but that
> > didn't work and seemed fragile anyway.
>
> D'uh - this path was clearly never tested. I suppose this should have
> been something like this:
> patch_ulong((void *)&stub[i]->funcdata, func_desc(local_paca))
>
> Still convoluted, but I think that should hopefully fix the problem you
> are seeing.
>
I can still try something like this if you prefer that solution (though
I think there may be some type massaging required in the second argument
to patch_ulong().) LMK ...
> >
> > > + stub = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> > > + me->arch.ool_stubs = (struct ftrace_ool_stub *)(stub + me->arch.stub_count);
> > > + me->arch.stub_count += num_stubs;
> > > #endif
>
> Regardless of the above, your proposed change looks good to me and
> simplifies the logic. So:
> Acked-by: Naveen N Rao (AMD) <naveen@kernel.org>
>
> > crash> dis 0xc008000007d70dd0 42
> > ppc64[ ] ftrace[0] <xfs_stats_format+0x558>: .long 0x0
> > <xfs_stats_format+0x55c>: .long 0x0
> > <xfs_stats_format+0x560>: mflr r0
> > <xfs_stats_format+0x564>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
> > <xfs_stats_format+0x568>: mtlr r0
> > <xfs_stats_format+0x56c>: b 0xc008000007d70014 <patch_free_livepatch+0xc>
> > ftrace[1] <xfs_stats_format+0x570>: .long 0x0
> > <xfs_stats_format+0x574>: .long 0x0
> > <xfs_stats_format+0x578>: mflr r0
> > <xfs_stats_format+0x57c>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
> > ppc64[ ] <xfs_stats_format+0x580>: addis r11,r2,4 << This looks like a full
> > <xfs_stats_format+0x584>: addi r11,r11,-29448 << ppc64_stub_entry
> > ftrace[2] <xfs_stats_format+0x588>: std r2,24(r1) << dropped in the middle
> > <xfs_stats_format+0x58c>: ld r12,32(r11) << of the ool_stubs array
> > <xfs_stats_format+0x590>: mtctr r12 << of ftrace_ool_stub[]
> > <xfs_stats_format+0x594>: bctr <<
> > <xfs_stats_format+0x598>: mtlr r0 <<
> > <xfs_stats_format+0x59c>: andi. r20,r27,30050 <<
> > ftrace[3] <xfs_stats_format+0x5a0>: .long 0x54e92b8 <<
> > <xfs_stats_format+0x5a4>: lfs f0,0(r8) <<
> > ppc64[ ] <xfs_stats_format+0x5a8>: mflr r0
> > <xfs_stats_format+0x5ac>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
> > <xfs_stats_format+0x5b0>: mtlr r0
> > <xfs_stats_format+0x5b4>: b 0xc008000007d7037c <add_callbacks_to_patch_objects+0xc>
> > ftrace[4] <xfs_stats_format+0x5b8>: .long 0x0
> > <xfs_stats_format+0x5bc>: .long 0x0
>
> These NULL values are also problematic. I think those are NULL since we
> are not "reserving" the stubs properly, but those should point to some
> ftrace_op. I think we are missing a call to ftrace_rec_set_nop_ops() in
> ftrace_init_nop(), which would be good to do separately.
>
Very lightly tested, but were you thinking of something like:
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 6dca92d5a..687371c64 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -488,8 +488,12 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
return ret;
/* Set up out-of-line stub */
- if (IS_ENABLED(CONFIG_PPC_FTRACE_OUT_OF_LINE))
- return ftrace_init_ool_stub(mod, rec);
+ if (IS_ENABLED(CONFIG_PPC_FTRACE_OUT_OF_LINE)) {
+ ret = ftrace_init_ool_stub(mod, rec);
+ if (ret)
+ return ret;
+ return ftrace_rec_set_nop_ops(rec);
+ }
/* Nop-out the ftrace location */
new = ppc_inst(PPC_RAW_NOP());
@@ -520,7 +524,7 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
return -EINVAL;
}
- return ret;
+ return ftrace_rec_set_nop_ops(rec);
}
int ftrace_update_ftrace_func(ftrace_func_t func)
In which case the ftrace-ool area looks like:
crash> mod | grep livepatch_module
c008000006350500 livepatch_module c008000009b90000 262144 (not loaded) [CONFIG_KALLSYMS]
crash> struct module.arch.ool_stubs c008000006350500
arch.ool_stubs = 0xc008000009b90dd0 <xfs_stats_format+1368>,
crash> struct module.arch.ool_stub_count c008000006350500
arch.ool_stub_count = 7,
crash> struct ftrace_ool_stub 0xc008000009b90dd0 7
struct ftrace_ool_stub {
ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
insn = {0x7c0802a6, 0x4bffffa5, 0x7c0803a6, 0x4bfff230}
}
struct ftrace_ool_stub {
ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
insn = {0x7c0802a6, 0x4bffff8d, 0x7c0803a6, 0x4bfff304}
}
struct ftrace_ool_stub {
ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
insn = {0x7c0802a6, 0x4bffff75, 0x7c0803a6, 0x4bfff430}
}
struct ftrace_ool_stub {
ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
insn = {0x7c0802a6, 0x4bffff5d, 0x7c0803a6, 0x4bfff550}
}
struct ftrace_ool_stub {
ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
insn = {0x7c0802a6, 0x4bffff45, 0x7c0803a6, 0x4bfff768}
}
struct ftrace_ool_stub {
ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
insn = {0x7c0802a6, 0x4bffff2d, 0x7c0803a6, 0x4bfffa08}
}
struct ftrace_ool_stub {
ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
insn = {0x7c0802a6, 0x4bffff15, 0x7c0803a6, 0x4bfffa10}
}
--
Joe
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc64/modules: fix ool-ftrace-stub vs. livepatch relocation corruption
2025-09-10 18:57 ` Joe Lawrence
@ 2025-09-11 6:25 ` Naveen N Rao
0 siblings, 0 replies; 5+ messages in thread
From: Naveen N Rao @ 2025-09-11 6:25 UTC (permalink / raw)
To: Joe Lawrence
Cc: linuxppc-dev, live-patching, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy
On Wed, Sep 10, 2025 at 02:57:19PM -0400, Joe Lawrence wrote:
> On Mon, Sep 08, 2025 at 04:33:24PM +0530, Naveen N Rao wrote:
> > On Wed, Sep 03, 2025 at 10:37:39PM -0400, Joe Lawrence wrote:
> > > On Wed, Sep 03, 2025 at 10:29:50PM -0400, Joe Lawrence wrote:
> > > > The powerpc64 module .stubs section holds ppc64_stub_entry[] code
> > > > trampolines that are generated at module load time. These stubs are
> > > > necessary for function calls to external symbols that are too far away
> > > > for a simple relative branch.
> > > >
> > > > Logic for finding an available ppc64_stub_entry has relied on a sentinel
> > > > value in the funcdata member to indicate a used slot. Code iterates
> > > > through the array, inspecting .funcdata to find the first unused (zeroed)
> > > > entry:
> > > >
> > > > for (i = 0; stub_func_addr(stubs[i].funcdata); i++)
> > > >
> > > > To support CONFIG_PPC_FTRACE_OUT_OF_LINE, a new setup_ftrace_ool_stubs()
> > > > function extended the .stubs section by adding an array of
> > > > ftrace_ool_stub structures for each patchable function. A side effect
> > > > of writing these smaller structures is that the funcdata sentinel
> > > > convention is not maintained.
> >
> > There is clearly a bug in how we are reserving the stubs as you point
> > out further below, but once that is properly initialized, I don't think
> > the smaller structure size for ftrace_ool_stub matters (in so far as
> > stub->funcdata being non-NULL). We end up writing four valid powerpc
> > instructions, along with a ftrace_ops pointer before those instructions
> > which should also be non-zero (there is a bug here too, more on that
> > below). The whole function descriptor dance does complicate matters a
> > bit though.
> >
>
> Hi Naveen,
>
> Ah hah, I see now, given the other bug that you mention, we should have
> had seen non-NULL entries in either ftrace_ool_stub.insn[] or .ftrace_op
> fields such that when read as ppc64_stub_entry, .funcdata would indicate
> that it's in use:
>
> ppc64_stub_entry[] ftrace_ool_stub[]
> 0x00 [0].jump[0] [0].ftrace_op
> 0x04 [0].jump[1] [0].ftrace_op
> 0x08 [0].jump[2] [0].insn[0]
> 0x0C [0].jump[3] [0].insn[1]
> 0x10 [0].jump[4] [0].insn[2]
> 0x14 [0].jump[5] [0].insn[3]
> 0x18 [0].jump[6] [1].ftrace_op
> 0x1C [0].magic [1].ftrace_op
> 0x20 [0].funcdata [1].insn[0] <<
> 0x24 [0].funcdata [1].insn[1] <<
> 0x28 [1].jump[0] [1].insn[2]
> 0x2C [1].jump[1] [1].insn[3]
> 0x30 [1].jump[2] [2].ftrace_op
> 0x34 [1].jump[3] [2].ftrace_op
> 0x38 [1].jump[4] [2].insn[0]
> 0x3C [1].jump[5] [2].insn[1]
> 0x40 [1].jump[6] [2].insn[2]
> 0x44 [1].magic [2].insn[3]
> 0x48 [1].funcdata [3].ftrace_op <<
> 0x4C [1].funcdata [3].ftrace_op <<
>
> If the commit msg for this patch would be clearer by rewording anything,
> I'm happy to update. (My understanding at the time of writing was that
> the NULL funcdata vs. insn[]/ftrace_op was a valid sequence.)
>
Yes, please. But only just to point out the bug in how we are reserving
the stubs.
> > > > @@ -1118,29 +1118,19 @@ int module_trampoline_target(struct
> > > > module *mod, unsigned long addr,
> > > > static int setup_ftrace_ool_stubs(const Elf64_Shdr *sechdrs, unsigned long addr, struct module *me)
> > > > {
> > > > #ifdef CONFIG_PPC_FTRACE_OUT_OF_LINE
> > > > - unsigned int i, total_stubs, num_stubs;
> > > > + unsigned int total_stubs, num_stubs;
> > > > struct ppc64_stub_entry *stub;
> > > >
> > > > total_stubs = sechdrs[me->arch.stubs_section].sh_size / sizeof(*stub);
> > > > num_stubs = roundup(me->arch.ool_stub_count * sizeof(struct ftrace_ool_stub),
> > > > sizeof(struct ppc64_stub_entry)) / sizeof(struct ppc64_stub_entry);
> > > >
> > > > - /* Find the next available entry */
> > > > - stub = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> > > > - for (i = 0; stub_func_addr(stub[i].funcdata); i++)
> > > > - if (WARN_ON(i >= total_stubs))
> > > > - return -1;
> > > > -
> > > > - if (WARN_ON(i + num_stubs > total_stubs))
> > > > + if (WARN_ON(me->arch.stub_count + num_stubs > total_stubs))
> > > > return -1;
> > > >
> > > > - stub += i;
> > > > - me->arch.ool_stubs = (struct ftrace_ool_stub *)stub;
> > > > -
> > > > - /* reserve stubs */
> > > > - for (i = 0; i < num_stubs; i++)
> > > > - if (patch_u32((void *)&stub->funcdata, PPC_RAW_NOP()))
> > > > - return -1;
> > >
> > > At first I thought the bug was that this loop was re-writting the same
> > > PPC_RAW_NOP() to the same funcdata (i.e, should have been something
> > > like: patch_u32((void *)stub[i]->funcdata, PPC_RAW_NOP())), but that
> > > didn't work and seemed fragile anyway.
> >
> > D'uh - this path was clearly never tested. I suppose this should have
> > been something like this:
> > patch_ulong((void *)&stub[i]->funcdata, func_desc(local_paca))
> >
> > Still convoluted, but I think that should hopefully fix the problem you
> > are seeing.
> >
>
> I can still try something like this if you prefer that solution (though
> I think there may be some type massaging required in the second argument
> to patch_ulong().) LMK ...
That's alright -- it is better to rip this out and replace with the
changes in your patch.
>
> > >
> > > > + stub = (void *)sechdrs[me->arch.stubs_section].sh_addr;
> > > > + me->arch.ool_stubs = (struct ftrace_ool_stub *)(stub + me->arch.stub_count);
> > > > + me->arch.stub_count += num_stubs;
> > > > #endif
> >
> > Regardless of the above, your proposed change looks good to me and
> > simplifies the logic. So:
> > Acked-by: Naveen N Rao (AMD) <naveen@kernel.org>
> >
>
>
>
> > > crash> dis 0xc008000007d70dd0 42
> > > ppc64[ ] ftrace[0] <xfs_stats_format+0x558>: .long 0x0
> > > <xfs_stats_format+0x55c>: .long 0x0
> > > <xfs_stats_format+0x560>: mflr r0
> > > <xfs_stats_format+0x564>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
> > > <xfs_stats_format+0x568>: mtlr r0
> > > <xfs_stats_format+0x56c>: b 0xc008000007d70014 <patch_free_livepatch+0xc>
> > > ftrace[1] <xfs_stats_format+0x570>: .long 0x0
> > > <xfs_stats_format+0x574>: .long 0x0
> > > <xfs_stats_format+0x578>: mflr r0
> > > <xfs_stats_format+0x57c>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
> > > ppc64[ ] <xfs_stats_format+0x580>: addis r11,r2,4 << This looks like a full
> > > <xfs_stats_format+0x584>: addi r11,r11,-29448 << ppc64_stub_entry
> > > ftrace[2] <xfs_stats_format+0x588>: std r2,24(r1) << dropped in the middle
> > > <xfs_stats_format+0x58c>: ld r12,32(r11) << of the ool_stubs array
> > > <xfs_stats_format+0x590>: mtctr r12 << of ftrace_ool_stub[]
> > > <xfs_stats_format+0x594>: bctr <<
> > > <xfs_stats_format+0x598>: mtlr r0 <<
> > > <xfs_stats_format+0x59c>: andi. r20,r27,30050 <<
> > > ftrace[3] <xfs_stats_format+0x5a0>: .long 0x54e92b8 <<
> > > <xfs_stats_format+0x5a4>: lfs f0,0(r8) <<
> > > ppc64[ ] <xfs_stats_format+0x5a8>: mflr r0
> > > <xfs_stats_format+0x5ac>: bl 0xc008000007d70d80 <xfs_stats_format+0x508>
> > > <xfs_stats_format+0x5b0>: mtlr r0
> > > <xfs_stats_format+0x5b4>: b 0xc008000007d7037c <add_callbacks_to_patch_objects+0xc>
> > > ftrace[4] <xfs_stats_format+0x5b8>: .long 0x0
> > > <xfs_stats_format+0x5bc>: .long 0x0
> >
> > These NULL values are also problematic. I think those are NULL since we
> > are not "reserving" the stubs properly, but those should point to some
> > ftrace_op. I think we are missing a call to ftrace_rec_set_nop_ops() in
> > ftrace_init_nop(), which would be good to do separately.
> >
>
> Very lightly tested, but were you thinking of something like:
>
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index 6dca92d5a..687371c64 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -488,8 +488,12 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> return ret;
>
> /* Set up out-of-line stub */
> - if (IS_ENABLED(CONFIG_PPC_FTRACE_OUT_OF_LINE))
> - return ftrace_init_ool_stub(mod, rec);
> + if (IS_ENABLED(CONFIG_PPC_FTRACE_OUT_OF_LINE)) {
> + ret = ftrace_init_ool_stub(mod, rec);
> + if (ret)
> + return ret;
> + return ftrace_rec_set_nop_ops(rec);
> + }
Minor nit: since ftrace_rec_set_nop_ops() has to be called regardless, I
would prefer to add a goto here. See below.
>
> /* Nop-out the ftrace location */
> new = ppc_inst(PPC_RAW_NOP());
> @@ -520,7 +524,7 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> return -EINVAL;
> }
>
> - return ret;
> + return ftrace_rec_set_nop_ops(rec);
> }
We will need to still check for ret here, so something like this?
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 6dca92d5a6e8..841d077e2825 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -488,8 +488,10 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
return ret;
/* Set up out-of-line stub */
- if (IS_ENABLED(CONFIG_PPC_FTRACE_OUT_OF_LINE))
- return ftrace_init_ool_stub(mod, rec);
+ if (IS_ENABLED(CONFIG_PPC_FTRACE_OUT_OF_LINE)) {
+ ret = ftrace_init_ool_stub(mod, rec);
+ goto out;
+ }
/* Nop-out the ftrace location */
new = ppc_inst(PPC_RAW_NOP());
@@ -520,6 +522,10 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
return -EINVAL;
}
+out:
+ if (!ret)
+ ret = ftrace_rec_set_nop_ops(rec);
+
return ret;
}
>
> int ftrace_update_ftrace_func(ftrace_func_t func)
>
>
> In which case the ftrace-ool area looks like:
>
> crash> mod | grep livepatch_module
> c008000006350500 livepatch_module c008000009b90000 262144 (not loaded) [CONFIG_KALLSYMS]
> crash> struct module.arch.ool_stubs c008000006350500
> arch.ool_stubs = 0xc008000009b90dd0 <xfs_stats_format+1368>,
> crash> struct module.arch.ool_stub_count c008000006350500
> arch.ool_stub_count = 7,
>
> crash> struct ftrace_ool_stub 0xc008000009b90dd0 7
> struct ftrace_ool_stub {
> ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
> insn = {0x7c0802a6, 0x4bffffa5, 0x7c0803a6, 0x4bfff230}
> }
>
> struct ftrace_ool_stub {
> ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
> insn = {0x7c0802a6, 0x4bffff8d, 0x7c0803a6, 0x4bfff304}
> }
>
> struct ftrace_ool_stub {
> ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
> insn = {0x7c0802a6, 0x4bffff75, 0x7c0803a6, 0x4bfff430}
> }
>
> struct ftrace_ool_stub {
> ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
> insn = {0x7c0802a6, 0x4bffff5d, 0x7c0803a6, 0x4bfff550}
> }
>
> struct ftrace_ool_stub {
> ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
> insn = {0x7c0802a6, 0x4bffff45, 0x7c0803a6, 0x4bfff768}
> }
>
> struct ftrace_ool_stub {
> ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
> insn = {0x7c0802a6, 0x4bffff2d, 0x7c0803a6, 0x4bfffa08}
> }
>
> struct ftrace_ool_stub {
> ftrace_op = 0xc00000000131d140 <ftrace_nop_ops>,
> insn = {0x7c0802a6, 0x4bffff15, 0x7c0803a6, 0x4bfffa10}
> }
LGTM.
Thanks,
- Naveen
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-09-11 6:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-04 2:29 [PATCH] powerpc64/modules: fix ool-ftrace-stub vs. livepatch relocation corruption Joe Lawrence
2025-09-04 2:37 ` Joe Lawrence
2025-09-08 11:03 ` Naveen N Rao
2025-09-10 18:57 ` Joe Lawrence
2025-09-11 6:25 ` Naveen N Rao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).