* [PATCH 1/2] powerpc/ftrace: Separate the heuristics for checking call sites
@ 2016-07-19 4:48 Michael Ellerman
2016-07-19 4:48 ` [PATCH 2/2] powerpc/modules: Never restore r2 for a mprofile-kernel style mcount() call Michael Ellerman
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Michael Ellerman @ 2016-07-19 4:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: aneesh.kumar, Anton Blanchard, duwe
In __ftrace_make_nop() (the 64-bit version), we have code to deal with
two ftrace ABIs. There is the original ABI, which looks mostly like a
function call, and then the mprofile-kernel ABI which is just a branch.
The code tries to handle both cases, by looking for the presence of a
load to restore the TOC pointer (PPC_INST_LD_TOC). If we detect the TOC
load, we assume the call site is for an mcount() call using the old ABI.
That means we patch the mcount() call with a b +8, to branch over the
TOC load.
However if the kernel was built with mprofile-kernel, then there will
never be a call site using the original ftrace ABI. If for some reason
we do see a TOC load, then it's there for a good reason, and we should
not jump over it.
So split the code, using the existing CC_USING_MPROFILE_KERNEL. Kernels
built with mprofile-kernel will only look for, and expect, the new ABI,
and similarly for the original ABI.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/ftrace.c | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 7af6c4de044b..438442dac44c 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -144,6 +144,21 @@ __ftrace_make_nop(struct module *mod,
return -EINVAL;
}
+#ifdef CC_USING_MPROFILE_KERNEL
+ /* When using -mkernel_profile there is no load to jump over */
+ pop = PPC_INST_NOP;
+
+ if (probe_kernel_read(&op, (void *)(ip - 4), 4)) {
+ pr_err("Fetching instruction at %lx failed.\n", ip - 4);
+ return -EFAULT;
+ }
+
+ /* We expect either a mlfr r0, or a std r0, LRSAVE(r1) */
+ if (op != PPC_INST_MFLR && op != PPC_INST_STD_LR) {
+ pr_err("Unexpected instruction %08x around bl _mcount\n", op);
+ return -EINVAL;
+ }
+#else
/*
* Our original call site looks like:
*
@@ -170,24 +185,10 @@ __ftrace_make_nop(struct module *mod,
}
if (op != PPC_INST_LD_TOC) {
- unsigned int inst;
-
- if (probe_kernel_read(&inst, (void *)(ip - 4), 4)) {
- pr_err("Fetching instruction at %lx failed.\n", ip - 4);
- return -EFAULT;
- }
-
- /* We expect either a mlfr r0, or a std r0, LRSAVE(r1) */
- if (inst != PPC_INST_MFLR && inst != PPC_INST_STD_LR) {
- pr_err("Unexpected instructions around bl _mcount\n"
- "when enabling dynamic ftrace!\t"
- "(%08x,bl,%08x)\n", inst, op);
- return -EINVAL;
- }
-
- /* When using -mkernel_profile there is no load to jump over */
- pop = PPC_INST_NOP;
+ pr_err("Expected %08x found %08x\n", PPC_INST_LD_TOC, op);
+ return -EINVAL;
}
+#endif /* CC_USING_MPROFILE_KERNEL */
if (patch_instruction((unsigned int *)ip, pop)) {
pr_err("Patching NOP failed.\n");
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] powerpc/modules: Never restore r2 for a mprofile-kernel style mcount() call
2016-07-19 4:48 [PATCH 1/2] powerpc/ftrace: Separate the heuristics for checking call sites Michael Ellerman
@ 2016-07-19 4:48 ` Michael Ellerman
2016-07-22 5:50 ` [2/2] " Michael Ellerman
2016-07-19 5:44 ` [PATCH 1/2] powerpc/ftrace: Separate the heuristics for checking call sites Madhavan Srinivasan
2016-07-22 5:50 ` [1/2] " Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Michael Ellerman @ 2016-07-19 4:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: aneesh.kumar, Anton Blanchard, duwe
In the module loader we process relocations, and for long jumps we
generate trampolines (aka stubs). At the call site for one of these
trampolines we usually need to generate a load instruction to restore
the TOC pointer into r2.
There is one exception however, which is calls to mcount() using the
mprofile-kernel ABI, they handle the TOC inside the stub, and so for
them we do not generate a TOC load.
The bug is in how the code in restore_r2() decides if it needs to
generate the TOC load. It does so by looking for a nop following the
branch, and if it sees a nop, it replaces it with the load. In general
the compiler has no reason to generate a nop following the mcount()
call and so that check works OK.
However if we combine a jump label at the start of a function, with an
early return, such that GCC applies the shrink-wrapping optimisation, we
can then end up with an mcount call followed immediately by a nop.
However the nop is not there for a TOC load, it is for the jump label.
That confuses restore_r2() into replacing the jump label nop with a TOC
load, which in turn confuses ftrace into replacing the mcount call with
a b +8 (fixed in the previous commit). The end result is we jump over
the jump label, which if it was supposed to return means we incorrectly
run the body of the function.
We have seen this in practice with some yet-to-be-merged patches that
use jump labels more extensively.
The fix is relatively simple, in restore_r2() we check for an
mprofile-kernel style mcount() call first, before looking for the
presence of a nop.
Fixes: 153086644fd1 ("powerpc/ftrace: Add support for -mprofile-kernel ftrace ABI")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/module_64.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index f703f343358e..183368e008cf 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -494,9 +494,10 @@ static bool is_early_mcount_callsite(u32 *instruction)
restore r2. */
static int restore_r2(u32 *instruction, struct module *me)
{
+ if (is_early_mcount_callsite(instruction - 1))
+ return 1;
+
if (*instruction != PPC_INST_NOP) {
- if (is_early_mcount_callsite(instruction - 1))
- return 1;
pr_err("%s: Expect noop after relocate, got %08x\n",
me->name, *instruction);
return 0;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] powerpc/ftrace: Separate the heuristics for checking call sites
2016-07-19 4:48 [PATCH 1/2] powerpc/ftrace: Separate the heuristics for checking call sites Michael Ellerman
2016-07-19 4:48 ` [PATCH 2/2] powerpc/modules: Never restore r2 for a mprofile-kernel style mcount() call Michael Ellerman
@ 2016-07-19 5:44 ` Madhavan Srinivasan
2016-07-21 10:36 ` Michael Ellerman
2016-07-22 5:50 ` [1/2] " Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Madhavan Srinivasan @ 2016-07-19 5:44 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: duwe, aneesh.kumar, Anton Blanchard
On Tuesday 19 July 2016 10:18 AM, Michael Ellerman wrote:
> In __ftrace_make_nop() (the 64-bit version), we have code to deal with
> two ftrace ABIs. There is the original ABI, which looks mostly like a
> function call, and then the mprofile-kernel ABI which is just a branch.
>
> The code tries to handle both cases, by looking for the presence of a
> load to restore the TOC pointer (PPC_INST_LD_TOC). If we detect the TOC
> load, we assume the call site is for an mcount() call using the old ABI.
> That means we patch the mcount() call with a b +8, to branch over the
> TOC load.
>
> However if the kernel was built with mprofile-kernel, then there will
> never be a call site using the original ftrace ABI. If for some reason
> we do see a TOC load, then it's there for a good reason, and we should
> not jump over it.
>
> So split the code, using the existing CC_USING_MPROFILE_KERNEL. Kernels
> built with mprofile-kernel will only look for, and expect, the new ABI,
> and similarly for the original ABI.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/kernel/ftrace.c | 35 ++++++++++++++++++-----------------
> 1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
> index 7af6c4de044b..438442dac44c 100644
> --- a/arch/powerpc/kernel/ftrace.c
> +++ b/arch/powerpc/kernel/ftrace.c
> @@ -144,6 +144,21 @@ __ftrace_make_nop(struct module *mod,
> return -EINVAL;
> }
>
> +#ifdef CC_USING_MPROFILE_KERNEL
> + /* When using -mkernel_profile there is no load to jump over */
> + pop = PPC_INST_NOP;
> +
> + if (probe_kernel_read(&op, (void *)(ip - 4), 4)) {
> + pr_err("Fetching instruction at %lx failed.\n", ip - 4);
> + return -EFAULT;
> + }
> +
> + /* We expect either a mlfr r0, or a std r0, LRSAVE(r1) */
nit.. "mflr" and not "mlfr"
> + if (op != PPC_INST_MFLR && op != PPC_INST_STD_LR) {
> + pr_err("Unexpected instruction %08x around bl _mcount\n", op);
> + return -EINVAL;
> + }
> +#else
> /*
> * Our original call site looks like:
> *
> @@ -170,24 +185,10 @@ __ftrace_make_nop(struct module *mod,
> }
>
> if (op != PPC_INST_LD_TOC) {
> - unsigned int inst;
> -
> - if (probe_kernel_read(&inst, (void *)(ip - 4), 4)) {
> - pr_err("Fetching instruction at %lx failed.\n", ip - 4);
> - return -EFAULT;
> - }
> -
> - /* We expect either a mlfr r0, or a std r0, LRSAVE(r1) */
> - if (inst != PPC_INST_MFLR && inst != PPC_INST_STD_LR) {
> - pr_err("Unexpected instructions around bl _mcount\n"
> - "when enabling dynamic ftrace!\t"
> - "(%08x,bl,%08x)\n", inst, op);
> - return -EINVAL;
> - }
> -
> - /* When using -mkernel_profile there is no load to jump over */
> - pop = PPC_INST_NOP;
> + pr_err("Expected %08x found %08x\n", PPC_INST_LD_TOC, op);
> + return -EINVAL;
> }
> +#endif /* CC_USING_MPROFILE_KERNEL */
>
> if (patch_instruction((unsigned int *)ip, pop)) {
> pr_err("Patching NOP failed.\n");
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] powerpc/ftrace: Separate the heuristics for checking call sites
2016-07-19 5:44 ` [PATCH 1/2] powerpc/ftrace: Separate the heuristics for checking call sites Madhavan Srinivasan
@ 2016-07-21 10:36 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2016-07-21 10:36 UTC (permalink / raw)
To: Madhavan Srinivasan, linuxppc-dev; +Cc: duwe, aneesh.kumar, Anton Blanchard
Madhavan Srinivasan <maddy@linux.vnet.ibm.com> writes:
> On Tuesday 19 July 2016 10:18 AM, Michael Ellerman wrote:
>> In __ftrace_make_nop() (the 64-bit version), we have code to deal with
>> two ftrace ABIs. There is the original ABI, which looks mostly like a
>> function call, and then the mprofile-kernel ABI which is just a branch.
>>
...
>> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
>> index 7af6c4de044b..438442dac44c 100644
>> --- a/arch/powerpc/kernel/ftrace.c
>> +++ b/arch/powerpc/kernel/ftrace.c
>> @@ -144,6 +144,21 @@ __ftrace_make_nop(struct module *mod,
>> return -EINVAL;
>> }
>>
>> +#ifdef CC_USING_MPROFILE_KERNEL
>> + /* When using -mkernel_profile there is no load to jump over */
>> + pop = PPC_INST_NOP;
>> +
>> + if (probe_kernel_read(&op, (void *)(ip - 4), 4)) {
>> + pr_err("Fetching instruction at %lx failed.\n", ip - 4);
>> + return -EFAULT;
>> + }
>> +
>> + /* We expect either a mlfr r0, or a std r0, LRSAVE(r1) */
>
> nit.. "mflr" and not "mlfr"
Yep, copied from the old comment, but I'll fix it up. Thanks.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [1/2] powerpc/ftrace: Separate the heuristics for checking call sites
2016-07-19 4:48 [PATCH 1/2] powerpc/ftrace: Separate the heuristics for checking call sites Michael Ellerman
2016-07-19 4:48 ` [PATCH 2/2] powerpc/modules: Never restore r2 for a mprofile-kernel style mcount() call Michael Ellerman
2016-07-19 5:44 ` [PATCH 1/2] powerpc/ftrace: Separate the heuristics for checking call sites Madhavan Srinivasan
@ 2016-07-22 5:50 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2016-07-22 5:50 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: duwe, aneesh.kumar, Anton Blanchard
On Tue, 2016-19-07 at 04:48:30 UTC, Michael Ellerman wrote:
> In __ftrace_make_nop() (the 64-bit version), we have code to deal with
> two ftrace ABIs. There is the original ABI, which looks mostly like a
> function call, and then the mprofile-kernel ABI which is just a branch.
>
> The code tries to handle both cases, by looking for the presence of a
> load to restore the TOC pointer (PPC_INST_LD_TOC). If we detect the TOC
> load, we assume the call site is for an mcount() call using the old ABI.
> That means we patch the mcount() call with a b +8, to branch over the
> TOC load.
>
> However if the kernel was built with mprofile-kernel, then there will
> never be a call site using the original ftrace ABI. If for some reason
> we do see a TOC load, then it's there for a good reason, and we should
> not jump over it.
>
> So split the code, using the existing CC_USING_MPROFILE_KERNEL. Kernels
> built with mprofile-kernel will only look for, and expect, the new ABI,
> and similarly for the original ABI.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Applied to powerpc next.
https://git.kernel.org/powerpc/c/9d636109511a000882f8dff4ea
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-22 5:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-19 4:48 [PATCH 1/2] powerpc/ftrace: Separate the heuristics for checking call sites Michael Ellerman
2016-07-19 4:48 ` [PATCH 2/2] powerpc/modules: Never restore r2 for a mprofile-kernel style mcount() call Michael Ellerman
2016-07-22 5:50 ` [2/2] " Michael Ellerman
2016-07-19 5:44 ` [PATCH 1/2] powerpc/ftrace: Separate the heuristics for checking call sites Madhavan Srinivasan
2016-07-21 10:36 ` Michael Ellerman
2016-07-22 5:50 ` [1/2] " Michael Ellerman
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).