| Message ID | dde8c1e55cfb4c878860f47308a52b273e96ae67.1718008093.git.naveen@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/ftrace: Move ftrace sequence out of line | expand |
On Mon, 10 Jun 2024 14:08:16 +0530 Naveen N Rao <naveen@kernel.org> wrote: > On 32-bit powerpc, gcc generates a three instruction sequence for > function profiling: > mflr r0 > stw r0, 4(r1) > bl _mcount > > On kernel boot, the call to _mcount() is nop-ed out, to be patched back > in when ftrace is actually enabled. The 'stw' instruction therefore is > not necessary unless ftrace is enabled. Nop it out during ftrace init. > > When ftrace is enabled, we want the 'stw' so that stack unwinding works > properly. Perform the same within the ftrace handler, similar to 64-bit > powerpc. > > For 64-bit powerpc, early versions of gcc used to emit a three > instruction sequence for function profiling (with -mprofile-kernel) with > a 'std' instruction to mimic the 'stw' above. Address that scenario also > by nop-ing out the 'std' instruction during ftrace init. > > Signed-off-by: Naveen N Rao <naveen@kernel.org> Isn't there still the race that there's a preemption between the: stw r0, 4(r1) and bl _mcount And if this breaks stack unwinding, couldn't this cause an issue for live kernel patching? I know it's very unlikely, but in theory, I think the race exists. -- Steve
On Mon, Jun 10, 2024 at 04:06:32PM GMT, Steven Rostedt wrote: > On Mon, 10 Jun 2024 14:08:16 +0530 > Naveen N Rao <naveen@kernel.org> wrote: > > > On 32-bit powerpc, gcc generates a three instruction sequence for > > function profiling: > > mflr r0 > > stw r0, 4(r1) > > bl _mcount > > > > On kernel boot, the call to _mcount() is nop-ed out, to be patched back > > in when ftrace is actually enabled. The 'stw' instruction therefore is > > not necessary unless ftrace is enabled. Nop it out during ftrace init. > > > > When ftrace is enabled, we want the 'stw' so that stack unwinding works > > properly. Perform the same within the ftrace handler, similar to 64-bit > > powerpc. > > > > For 64-bit powerpc, early versions of gcc used to emit a three > > instruction sequence for function profiling (with -mprofile-kernel) with > > a 'std' instruction to mimic the 'stw' above. Address that scenario also > > by nop-ing out the 'std' instruction during ftrace init. > > > > Signed-off-by: Naveen N Rao <naveen@kernel.org> > > Isn't there still the race that there's a preemption between the: > > stw r0, 4(r1) > and > bl _mcount > > And if this breaks stack unwinding, couldn't this cause an issue for live > kernel patching? > > I know it's very unlikely, but in theory, I think the race exists. I *think* you are assuming that we will be patching back the 'stw' instruction here? So, there could be an issue if a cpu has executed the nop instead of 'stw' and then sees the call to _mcount(). But, we don't patch back the 'stw' instruction. That is instead done as part of ftrace_caller(), along with setting up an additional stack frame to ensure reliable stack unwinding. Commit 41a506ef71eb ("powerpc/ftrace: Create a dummy stackframe to fix stack unwind") has more details. The primary motivation for this patch is to address differences in the function profile sequence with various toolchains. Since commit 0f71dcfb4aef ("powerpc/ftrace: Add support for -fpatchable-function-entry"), we use the same two-instruction profile sequence across 32-bit and 64-bit powerpc: mflr r0 bl ftrace_caller This has also been true on 64-bit powerpc with -mprofile-kernel, except the very early versions of gcc that supported that option (gcc v5). On 32-bit powerpc, we used to use the three instruction sequence before support for -fpatchable-function-entry was introduced. In this patch, we move all toolchain variants to use the two-instruction sequence for consistency. Thanks, Naveen
On Tue, 11 Jun 2024 20:17:19 +0530 Naveen N Rao <naveen@kernel.org> wrote: > > I know it's very unlikely, but in theory, I think the race exists. > > I *think* you are assuming that we will be patching back the 'stw' Yes, that was what I was assuming :-p > instruction here? So, there could be an issue if a cpu has executed the > nop instead of 'stw' and then sees the call to _mcount(). > > But, we don't patch back the 'stw' instruction. That is instead done as > part of ftrace_caller(), along with setting up an additional stack frame > to ensure reliable stack unwinding. Commit 41a506ef71eb > ("powerpc/ftrace: Create a dummy stackframe to fix stack unwind") has > more details. > > The primary motivation for this patch is to address differences in the > function profile sequence with various toolchains. Since commit > 0f71dcfb4aef ("powerpc/ftrace: Add support for > -fpatchable-function-entry"), we use the same two-instruction profile > sequence across 32-bit and 64-bit powerpc: > mflr r0 > bl ftrace_caller > > This has also been true on 64-bit powerpc with -mprofile-kernel, except > the very early versions of gcc that supported that option (gcc v5). > > On 32-bit powerpc, we used to use the three instruction sequence before > support for -fpatchable-function-entry was introduced. > > In this patch, we move all toolchain variants to use the two-instruction > sequence for consistency. OK, if you are not patching that back, then all should be good. -- Steve
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c index 041be965485e..2e1667a578ff 100644 --- a/arch/powerpc/kernel/trace/ftrace.c +++ b/arch/powerpc/kernel/trace/ftrace.c @@ -266,13 +266,15 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec) /* Expected sequence: 'mflr r0', 'stw r0,4(r1)', 'bl _mcount' */ ret = ftrace_validate_inst(ip - 8, ppc_inst(PPC_RAW_MFLR(_R0))); if (!ret) - ret = ftrace_validate_inst(ip - 4, ppc_inst(PPC_RAW_STW(_R0, _R1, 4))); + ret = ftrace_modify_code(ip - 4, ppc_inst(PPC_RAW_STW(_R0, _R1, 4)), + ppc_inst(PPC_RAW_NOP())); } else if (IS_ENABLED(CONFIG_MPROFILE_KERNEL)) { /* Expected sequence: 'mflr r0', ['std r0,16(r1)'], 'bl _mcount' */ ret = ftrace_read_inst(ip - 4, &old); if (!ret && !ppc_inst_equal(old, ppc_inst(PPC_RAW_MFLR(_R0)))) { ret = ftrace_validate_inst(ip - 8, ppc_inst(PPC_RAW_MFLR(_R0))); - ret |= ftrace_validate_inst(ip - 4, ppc_inst(PPC_RAW_STD(_R0, _R1, 16))); + ret |= ftrace_modify_code(ip - 4, ppc_inst(PPC_RAW_STD(_R0, _R1, 16)), + ppc_inst(PPC_RAW_NOP())); } } else { return -EINVAL; diff --git a/arch/powerpc/kernel/trace/ftrace_entry.S b/arch/powerpc/kernel/trace/ftrace_entry.S index 76dbe9fd2c0f..244a1c7bb1e8 100644 --- a/arch/powerpc/kernel/trace/ftrace_entry.S +++ b/arch/powerpc/kernel/trace/ftrace_entry.S @@ -33,6 +33,8 @@ * and then arrange for the ftrace function to be called. */ .macro ftrace_regs_entry allregs + /* Save the original return address in A's stack frame */ + PPC_STL r0, LRSAVE(r1) /* Create a minimal stack frame for representing B */ PPC_STLU r1, -STACK_FRAME_MIN_SIZE(r1) @@ -44,8 +46,6 @@ SAVE_GPRS(3, 10, r1) #ifdef CONFIG_PPC64 - /* Save the original return address in A's stack frame */ - std r0, LRSAVE+SWITCH_FRAME_SIZE+STACK_FRAME_MIN_SIZE(r1) /* Ok to continue? */ lbz r3, PACA_FTRACE_ENABLED(r13) cmpdi r3, 0
On 32-bit powerpc, gcc generates a three instruction sequence for function profiling: mflr r0 stw r0, 4(r1) bl _mcount On kernel boot, the call to _mcount() is nop-ed out, to be patched back in when ftrace is actually enabled. The 'stw' instruction therefore is not necessary unless ftrace is enabled. Nop it out during ftrace init. When ftrace is enabled, we want the 'stw' so that stack unwinding works properly. Perform the same within the ftrace handler, similar to 64-bit powerpc. For 64-bit powerpc, early versions of gcc used to emit a three instruction sequence for function profiling (with -mprofile-kernel) with a 'std' instruction to mimic the 'stw' above. Address that scenario also by nop-ing out the 'std' instruction during ftrace init. Signed-off-by: Naveen N Rao <naveen@kernel.org> --- arch/powerpc/kernel/trace/ftrace.c | 6 ++++-- arch/powerpc/kernel/trace/ftrace_entry.S | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-)