[PATCH] powerpc/kprobes: Fix call trace due to incorrect preempt count

[PATCH] powerpc/kprobes: Fix call trace due to incorrect preempt count

[Naveen N. Rao]

Michael Ellerman reported the following call trace when running
ftracetest:

BUG: using __this_cpu_write() in preemptible [00000000] code: ftracetest/6178
caller is opt_pre_handler+0xc4/0x110
CPU: 1 PID: 6178 Comm: ftracetest Not tainted 4.15.0-rc7-gcc6x-gb2cd1df #1
Call Trace:
[c0000000f9ec39c0] [c000000000ac4304] dump_stack+0xb4/0x100 (unreliable)
[c0000000f9ec3a00] [c00000000061159c] check_preemption_disabled+0x15c/0x170
[c0000000f9ec3a90] [c000000000217e84] opt_pre_handler+0xc4/0x110
[c0000000f9ec3af0] [c00000000004cf68] optimized_callback+0x148/0x170
[c0000000f9ec3b40] [c00000000004d954] optinsn_slot+0xec/0x10000
[c0000000f9ec3e30] [c00000000004bae0] kretprobe_trampoline+0x0/0x10

This is showing up since OPTPROBES is now enabled with CONFIG_PREEMPT.

trampoline_probe_handler() considers itself to be a special kprobe
handler for kretprobes. In doing so, it expects to be called from
kprobe_handler() on a trap, and re-enables preemption before returning a
non-zero return value so as to suppress any subsequent processing of the
trap by the kprobe_handler().

However, with optprobes, we don't deal with special handlers (we ignore
the return code) and just try to re-enable preemption causing the above
trace.

To address this, modify trampoline_probe_handler() to not be special.
The only additional processing done in kprobe_handler() is to emulate
the instruction (in this case, a 'nop'). We adjust the value of
regs->nip for the purpose and delegate the job of re-enabling
preemption and resetting current kprobe to the probe handlers
(kprobe_handler() or optimized_callback()).

Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index ca5d5a081e75..e4c5bf33970b 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -455,29 +455,33 @@ static int trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
 	}
 
 	kretprobe_assert(ri, orig_ret_address, trampoline_address);
-	regs->nip = orig_ret_address;
+
 	/*
-	 * Make LR point to the orig_ret_address.
-	 * When the 'nop' inside the kretprobe_trampoline
-	 * is optimized, we can do a 'blr' after executing the
-	 * detour buffer code.
+	 * We get here through one of two paths:
+	 * 1. by taking a trap -> kprobe_handler() -> here
+	 * 2. by optprobe branch -> optimized_callback() -> opt_pre_handler() -> here
+	 *
+	 * When going back through (1), we need regs->nip to be setup properly
+	 * as it is used to determine the return address from the trap.
+	 * For (2), since nip is not honoured with optprobes, we instead setup
+	 * the link register properly so that the subsequent 'blr' in
+	 * kretprobe_trampoline jumps back to the right instruction.
+	 *
+	 * For nip, we should set the address to the previous instruction since
+	 * we end up emulating it in kprobe_handler(), which increments the nip
+	 * again.
 	 */
+	regs->nip = orig_ret_address - 4;
 	regs->link = orig_ret_address;
 
-	reset_current_kprobe();
 	kretprobe_hash_unlock(current, &flags);
-	preempt_enable_no_resched();
 
 	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
 		hlist_del(&ri->hlist);
 		kfree(ri);
 	}
-	/*
-	 * By returning a non-zero value, we are telling
-	 * kprobe_handler() that we don't want the post_handler
-	 * to run (and have re-enabled preemption)
-	 */
-	return 1;
+
+	return 0;
 }
 NOKPROBE_SYMBOL(trampoline_probe_handler);
 
-- 
2.15.1

Re: [PATCH] powerpc/kprobes: Fix call trace due to incorrect preempt count

[Ananth N Mavinakayanahalli]

On Wed, Jan 17, 2018 at 05:52:24PM +0530, Naveen N. Rao wrote:
> Michael Ellerman reported the following call trace when running
> ftracetest:
> 
> BUG: using __this_cpu_write() in preemptible [00000000] code: ftracetest/6178
> caller is opt_pre_handler+0xc4/0x110
> CPU: 1 PID: 6178 Comm: ftracetest Not tainted 4.15.0-rc7-gcc6x-gb2cd1df #1
> Call Trace:
> [c0000000f9ec39c0] [c000000000ac4304] dump_stack+0xb4/0x100 (unreliable)
> [c0000000f9ec3a00] [c00000000061159c] check_preemption_disabled+0x15c/0x170
> [c0000000f9ec3a90] [c000000000217e84] opt_pre_handler+0xc4/0x110
> [c0000000f9ec3af0] [c00000000004cf68] optimized_callback+0x148/0x170
> [c0000000f9ec3b40] [c00000000004d954] optinsn_slot+0xec/0x10000
> [c0000000f9ec3e30] [c00000000004bae0] kretprobe_trampoline+0x0/0x10
> 
> This is showing up since OPTPROBES is now enabled with CONFIG_PREEMPT.
> 
> trampoline_probe_handler() considers itself to be a special kprobe
> handler for kretprobes. In doing so, it expects to be called from
> kprobe_handler() on a trap, and re-enables preemption before returning a
> non-zero return value so as to suppress any subsequent processing of the
> trap by the kprobe_handler().
> 
> However, with optprobes, we don't deal with special handlers (we ignore
> the return code) and just try to re-enable preemption causing the above
> trace.
> 
> To address this, modify trampoline_probe_handler() to not be special.
> The only additional processing done in kprobe_handler() is to emulate
> the instruction (in this case, a 'nop'). We adjust the value of
> regs->nip for the purpose and delegate the job of re-enabling
> preemption and resetting current kprobe to the probe handlers
> (kprobe_handler() or optimized_callback()).
> 
> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>

Re: [PATCH] powerpc/kprobes: Fix call trace due to incorrect preempt count

[Michael Ellerman]

Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com> writes:

> On Wed, Jan 17, 2018 at 05:52:24PM +0530, Naveen N. Rao wrote:
>> Michael Ellerman reported the following call trace when running
>> ftracetest:
>> 
>> BUG: using __this_cpu_write() in preemptible [00000000] code: ftracetest/6178
>> caller is opt_pre_handler+0xc4/0x110
>> CPU: 1 PID: 6178 Comm: ftracetest Not tainted 4.15.0-rc7-gcc6x-gb2cd1df #1
>> Call Trace:
>> [c0000000f9ec39c0] [c000000000ac4304] dump_stack+0xb4/0x100 (unreliable)
>> [c0000000f9ec3a00] [c00000000061159c] check_preemption_disabled+0x15c/0x170
>> [c0000000f9ec3a90] [c000000000217e84] opt_pre_handler+0xc4/0x110
>> [c0000000f9ec3af0] [c00000000004cf68] optimized_callback+0x148/0x170
>> [c0000000f9ec3b40] [c00000000004d954] optinsn_slot+0xec/0x10000
>> [c0000000f9ec3e30] [c00000000004bae0] kretprobe_trampoline+0x0/0x10
>> 
>> This is showing up since OPTPROBES is now enabled with CONFIG_PREEMPT.
>> 
>> trampoline_probe_handler() considers itself to be a special kprobe
>> handler for kretprobes. In doing so, it expects to be called from
>> kprobe_handler() on a trap, and re-enables preemption before returning a
>> non-zero return value so as to suppress any subsequent processing of the
>> trap by the kprobe_handler().
>> 
>> However, with optprobes, we don't deal with special handlers (we ignore
>> the return code) and just try to re-enable preemption causing the above
>> trace.
>> 
>> To address this, modify trampoline_probe_handler() to not be special.
>> The only additional processing done in kprobe_handler() is to emulate
>> the instruction (in this case, a 'nop'). We adjust the value of
>> regs->nip for the purpose and delegate the job of re-enabling
>> preemption and resetting current kprobe to the probe handlers
>> (kprobe_handler() or optimized_callback()).
>> 
>> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>
> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>

Fixes: 51c9c0843993 ("powerpc/kprobes: Implement Optprobes")

??

cheers

Re: [PATCH] powerpc/kprobes: Fix call trace due to incorrect preempt count

[Naveen N. Rao]

Michael Ellerman wrote:
> Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com> writes:
>=20
>> On Wed, Jan 17, 2018 at 05:52:24PM +0530, Naveen N. Rao wrote:
>>> Michael Ellerman reported the following call trace when running
>>> ftracetest:
>>>=20
>>> BUG: using __this_cpu_write() in preemptible [00000000] code: ftracetes=
t/6178
>>> caller is opt_pre_handler+0xc4/0x110
>>> CPU: 1 PID: 6178 Comm: ftracetest Not tainted 4.15.0-rc7-gcc6x-gb2cd1df=
 #1
>>> Call Trace:
>>> [c0000000f9ec39c0] [c000000000ac4304] dump_stack+0xb4/0x100 (unreliable=
)
>>> [c0000000f9ec3a00] [c00000000061159c] check_preemption_disabled+0x15c/0=
x170
>>> [c0000000f9ec3a90] [c000000000217e84] opt_pre_handler+0xc4/0x110
>>> [c0000000f9ec3af0] [c00000000004cf68] optimized_callback+0x148/0x170
>>> [c0000000f9ec3b40] [c00000000004d954] optinsn_slot+0xec/0x10000
>>> [c0000000f9ec3e30] [c00000000004bae0] kretprobe_trampoline+0x0/0x10
>>>=20
>>> This is showing up since OPTPROBES is now enabled with CONFIG_PREEMPT.
>>>=20
>>> trampoline_probe_handler() considers itself to be a special kprobe
>>> handler for kretprobes. In doing so, it expects to be called from
>>> kprobe_handler() on a trap, and re-enables preemption before returning =
a
>>> non-zero return value so as to suppress any subsequent processing of th=
e
>>> trap by the kprobe_handler().
>>>=20
>>> However, with optprobes, we don't deal with special handlers (we ignore
>>> the return code) and just try to re-enable preemption causing the above
>>> trace.
>>>=20
>>> To address this, modify trampoline_probe_handler() to not be special.
>>> The only additional processing done in kprobe_handler() is to emulate
>>> the instruction (in this case, a 'nop'). We adjust the value of
>>> regs->nip for the purpose and delegate the job of re-enabling
>>> preemption and resetting current kprobe to the probe handlers
>>> (kprobe_handler() or optimized_callback()).
>>>=20
>>> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
>>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>
>> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
>=20
> Fixes: 51c9c0843993 ("powerpc/kprobes: Implement Optprobes")

Fixes: 8a2d71a3f2737e ("powerpc/kprobes: Disable preemption before=20
invoking probe handler for optprobes")

I think this is more appropriate. I should have caught this issue with=20
kretprobes, but I am fairly certain that I ran ftracetest at that point,
but didn't see any call traces.

Regards,
Naveen

=

Re: powerpc/kprobes: Fix call trace due to incorrect preempt count

[Michael Ellerman]

On Wed, 2018-01-17 at 12:22:24 UTC, "Naveen N. Rao" wrote:
> Michael Ellerman reported the following call trace when running
> ftracetest:
> 
> BUG: using __this_cpu_write() in preemptible [00000000] code: ftracetest/6178
> caller is opt_pre_handler+0xc4/0x110
> CPU: 1 PID: 6178 Comm: ftracetest Not tainted 4.15.0-rc7-gcc6x-gb2cd1df #1
> Call Trace:
> [c0000000f9ec39c0] [c000000000ac4304] dump_stack+0xb4/0x100 (unreliable)
> [c0000000f9ec3a00] [c00000000061159c] check_preemption_disabled+0x15c/0x170
> [c0000000f9ec3a90] [c000000000217e84] opt_pre_handler+0xc4/0x110
> [c0000000f9ec3af0] [c00000000004cf68] optimized_callback+0x148/0x170
> [c0000000f9ec3b40] [c00000000004d954] optinsn_slot+0xec/0x10000
> [c0000000f9ec3e30] [c00000000004bae0] kretprobe_trampoline+0x0/0x10
> 
> This is showing up since OPTPROBES is now enabled with CONFIG_PREEMPT.
> 
> trampoline_probe_handler() considers itself to be a special kprobe
> handler for kretprobes. In doing so, it expects to be called from
> kprobe_handler() on a trap, and re-enables preemption before returning a
> non-zero return value so as to suppress any subsequent processing of the
> trap by the kprobe_handler().
> 
> However, with optprobes, we don't deal with special handlers (we ignore
> the return code) and just try to re-enable preemption causing the above
> trace.
> 
> To address this, modify trampoline_probe_handler() to not be special.
> The only additional processing done in kprobe_handler() is to emulate
> the instruction (in this case, a 'nop'). We adjust the value of
> regs->nip for the purpose and delegate the job of re-enabling
> preemption and resetting current kprobe to the probe handlers
> (kprobe_handler() or optimized_callback()).
> 
> Reported-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/e6e133c47e6bd4d5dac05b35d06634

cheers