diff mbox series

powerpc/kprobes: Pass ppc_inst as a pointer to emulate_step() on ppc32

Message ID 20210520072909.2901326-1-naveen.n.rao@linux.vnet.ibm.com (mailing list archive)
State Superseded
Headers show
Series powerpc/kprobes: Pass ppc_inst as a pointer to emulate_step() on ppc32 | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (3a81c0495fdb91fd9a9b4f617098c283131eeae1)
snowpatch_ozlabs/build-ppc64le success Build succeeded
snowpatch_ozlabs/build-ppc64be success Build succeeded
snowpatch_ozlabs/build-ppc64e success Build succeeded
snowpatch_ozlabs/build-pmac32 success Build succeeded
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 14 lines checked
snowpatch_ozlabs/needsstable success Patch fixes a commit that hasn't been released yet

Commit Message

Naveen N. Rao May 20, 2021, 7:29 a.m. UTC 
Trying to use a kprobe on ppc32 results in the below splat:
    BUG: Unable to handle kernel data access on read at 0x7c0802a6
    Faulting instruction address: 0xc002e9f0
    Oops: Kernel access of bad area, sig: 11 [#1]
    BE PAGE_SIZE=4K PowerPC 44x Platform
    Modules linked in:
    CPU: 0 PID: 89 Comm: sh Not tainted 5.13.0-rc1-01824-g3a81c0495fdb #7
    NIP:  c002e9f0 LR: c0011858 CTR: 00008a47
    REGS: c292fd50 TRAP: 0300   Not tainted  (5.13.0-rc1-01824-g3a81c0495fdb)
    MSR:  00009000 <EE,ME>  CR: 24002002  XER: 20000000
    DEAR: 7c0802a6 ESR: 00000000
    <snip>
    NIP [c002e9f0] emulate_step+0x28/0x324
    LR [c0011858] optinsn_slot+0x128/0x10000
    Call Trace:
     opt_pre_handler+0x7c/0xb4 (unreliable)
     optinsn_slot+0x128/0x10000
     ret_from_syscall+0x0/0x28

The offending instruction is:
    81 24 00 00     lwz     r9,0(r4)

Here, we are trying to load the second argument to emulate_step():
struct ppc_inst, which is the instruction to be emulated. On ppc64,
structures are passed in registers when passed by value. However, per
the ppc32 ABI, structures are always passed to functions as pointers.
This isn't being adhered to when setting up the call to emulate_step()
in the optprobe trampoline. Fix the same.

Fixes: eacf4c0202654a ("powerpc: Enable OPTPROBES on PPC32")
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/optprobes.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


base-commit: 3a81c0495fdb91fd9a9b4f617098c283131eeae1

Comments

Christophe Leroy May 20, 2021, 10:17 a.m. UTC | #1 
Le 20/05/2021 à 09:29, Naveen N. Rao a écrit :
> Trying to use a kprobe on ppc32 results in the below splat:
>      BUG: Unable to handle kernel data access on read at 0x7c0802a6
>      Faulting instruction address: 0xc002e9f0
>      Oops: Kernel access of bad area, sig: 11 [#1]
>      BE PAGE_SIZE=4K PowerPC 44x Platform
>      Modules linked in:
>      CPU: 0 PID: 89 Comm: sh Not tainted 5.13.0-rc1-01824-g3a81c0495fdb #7
>      NIP:  c002e9f0 LR: c0011858 CTR: 00008a47
>      REGS: c292fd50 TRAP: 0300   Not tainted  (5.13.0-rc1-01824-g3a81c0495fdb)
>      MSR:  00009000 <EE,ME>  CR: 24002002  XER: 20000000
>      DEAR: 7c0802a6 ESR: 00000000
>      <snip>
>      NIP [c002e9f0] emulate_step+0x28/0x324
>      LR [c0011858] optinsn_slot+0x128/0x10000
>      Call Trace:
>       opt_pre_handler+0x7c/0xb4 (unreliable)
>       optinsn_slot+0x128/0x10000
>       ret_from_syscall+0x0/0x28

I remember running some kprobe tests before submitting the patch, how did I miss that ?
Is there anything special to do to activate the use of optprobes and/or to hit this bug ?

> 
> The offending instruction is:
>      81 24 00 00     lwz     r9,0(r4)
> 
> Here, we are trying to load the second argument to emulate_step():
> struct ppc_inst, which is the instruction to be emulated. On ppc64,
> structures are passed in registers when passed by value. However, per
> the ppc32 ABI, structures are always passed to functions as pointers.

Yes, and that was one of the reasons I was reluctant to that new 'struct ppc_inst', as it means 
stack frames, copies, etc ... whereas we could have just changed it from 'unsigned int' to 'unsigned 
long'.
Ok, now we have it, so let's use it, but use it correctly, see my clean-up series 
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=244719&state=* (v2 on the way).

> This isn't being adhered to when setting up the call to emulate_step()
> in the optprobe trampoline. Fix the same.
> 
> Fixes: eacf4c0202654a ("powerpc: Enable OPTPROBES on PPC32")
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>   arch/powerpc/kernel/optprobes.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> index cdf87086fa33a0..2bc53fa48a1b33 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -281,8 +281,12 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
>   	/*
>   	 * 3. load instruction to be emulated into relevant register, and
>   	 */
> -	temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
> -	patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
> +	if (IS_ENABLED(CONFIG_PPC64)) {
> +		temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
> +		patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
> +	} else {
> +		patch_imm_load_insns((unsigned long)p->ainsn.insn, 4, buff + TMPL_INSN_IDX);
> +	}

It means commit https://github.com/linuxppc/linux/commit/693557ebf407a85ea400a0b501bb97687d8f4856 
was not necessary and may be reverted.

Christophe
Naveen N. Rao May 20, 2021, 10:54 a.m. UTC | #2 
Christophe Leroy wrote:
> 
> 
> Le 20/05/2021 à 09:29, Naveen N. Rao a écrit :
>> Trying to use a kprobe on ppc32 results in the below splat:
>>      BUG: Unable to handle kernel data access on read at 0x7c0802a6
>>      Faulting instruction address: 0xc002e9f0
>>      Oops: Kernel access of bad area, sig: 11 [#1]
>>      BE PAGE_SIZE=4K PowerPC 44x Platform
>>      Modules linked in:
>>      CPU: 0 PID: 89 Comm: sh Not tainted 5.13.0-rc1-01824-g3a81c0495fdb #7
>>      NIP:  c002e9f0 LR: c0011858 CTR: 00008a47
>>      REGS: c292fd50 TRAP: 0300   Not tainted  (5.13.0-rc1-01824-g3a81c0495fdb)
>>      MSR:  00009000 <EE,ME>  CR: 24002002  XER: 20000000
>>      DEAR: 7c0802a6 ESR: 00000000
>>      <snip>
>>      NIP [c002e9f0] emulate_step+0x28/0x324
>>      LR [c0011858] optinsn_slot+0x128/0x10000
>>      Call Trace:
>>       opt_pre_handler+0x7c/0xb4 (unreliable)
>>       optinsn_slot+0x128/0x10000
>>       ret_from_syscall+0x0/0x28
> 
> I remember running some kprobe tests before submitting the patch, how did I miss that ?
> Is there anything special to do to activate the use of optprobes and/or to hit this bug ?

Yeah, I was surprised when I hit this. One of the requirements we have 
for optprobes on powerpc is that the instruction should be a compute 
instruction (no load/store -- emulate_update_regs() should be enough) 
with the exception of conditional branches. It's possible that you ended 
up probing an instruction that couldn't be optimized.

An easy way to confirm if a probe has been optimized is to look at 
kprobes/list in debugfs, and to watch out for [OPTIMIZED] flag there.

>> diff --git a/arch/powerpc/kernel/optprobes.c 
>> b/arch/powerpc/kernel/optprobes.c
>> index cdf87086fa33a0..2bc53fa48a1b33 100644
>> --- a/arch/powerpc/kernel/optprobes.c
>> +++ b/arch/powerpc/kernel/optprobes.c
>> @@ -281,8 +281,12 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
>>   	/*
>>   	 * 3. load instruction to be emulated into relevant register, and
>>   	 */
>> -	temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
>> -	patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
>> +	if (IS_ENABLED(CONFIG_PPC64)) {
>> +		temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
>> +		patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
>> +	} else {
>> +		patch_imm_load_insns((unsigned long)p->ainsn.insn, 4, buff + TMPL_INSN_IDX);
>> +	}
> 
> It means commit https://github.com/linuxppc/linux/commit/693557ebf407a85ea400a0b501bb97687d8f4856 
> was not necessary and may be reverted.

Indeed, I will send a revert for it.


- Naveen
Christophe Leroy May 20, 2021, 12:55 p.m. UTC | #3 
Le 20/05/2021 à 12:54, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>>
>>
>> Le 20/05/2021 à 09:29, Naveen N. Rao a écrit :
>>> Trying to use a kprobe on ppc32 results in the below splat:
>>>      BUG: Unable to handle kernel data access on read at 0x7c0802a6
>>>      Faulting instruction address: 0xc002e9f0
>>>      Oops: Kernel access of bad area, sig: 11 [#1]
>>>      BE PAGE_SIZE=4K PowerPC 44x Platform
>>>      Modules linked in:
>>>      CPU: 0 PID: 89 Comm: sh Not tainted 5.13.0-rc1-01824-g3a81c0495fdb #7
>>>      NIP:  c002e9f0 LR: c0011858 CTR: 00008a47
>>>      REGS: c292fd50 TRAP: 0300   Not tainted  (5.13.0-rc1-01824-g3a81c0495fdb)
>>>      MSR:  00009000 <EE,ME>  CR: 24002002  XER: 20000000
>>>      DEAR: 7c0802a6 ESR: 00000000
>>>      <snip>
>>>      NIP [c002e9f0] emulate_step+0x28/0x324
>>>      LR [c0011858] optinsn_slot+0x128/0x10000
>>>      Call Trace:
>>>       opt_pre_handler+0x7c/0xb4 (unreliable)
>>>       optinsn_slot+0x128/0x10000
>>>       ret_from_syscall+0x0/0x28
>>
>> I remember running some kprobe tests before submitting the patch, how did I miss that ?
>> Is there anything special to do to activate the use of optprobes and/or to hit this bug ?
> 
> Yeah, I was surprised when I hit this. One of the requirements we have for optprobes on powerpc is 
> that the instruction should be a compute instruction (no load/store -- emulate_update_regs() should 
> be enough) with the exception of conditional branches. It's possible that you ended up probing an 
> instruction that couldn't be optimized.
> 
> An easy way to confirm if a probe has been optimized is to look at kprobes/list in debugfs, and to 
> watch out for [OPTIMIZED] flag there.
> 
>>> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
>>> index cdf87086fa33a0..2bc53fa48a1b33 100644
>>> --- a/arch/powerpc/kernel/optprobes.c
>>> +++ b/arch/powerpc/kernel/optprobes.c
>>> @@ -281,8 +281,12 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe 
>>> *p)
>>>       /*
>>>        * 3. load instruction to be emulated into relevant register, and
>>>        */
>>> -    temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
>>> -    patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
>>> +    if (IS_ENABLED(CONFIG_PPC64)) {
>>> +        temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
>>> +        patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
>>> +    } else {
>>> +        patch_imm_load_insns((unsigned long)p->ainsn.insn, 4, buff + TMPL_INSN_IDX);
>>> +    }
>>
>> It means commit https://github.com/linuxppc/linux/commit/693557ebf407a85ea400a0b501bb97687d8f4856 
>> was not necessary and may be reverted.
> 
> Indeed, I will send a revert for it.
> 

I'm not completely sure it is worth reverting, on an other hand it is pointless anyway to have 
something to convert to a u64 something that cannot be more than 32 bits on a PPC32, so now that we 
have ppc_inst_as_ulong() it is as good I think.

Christophe
Naveen N. Rao May 21, 2021, 7:01 a.m. UTC | #4 
Christophe Leroy wrote:
> 
> 
> Le 20/05/2021 à 12:54, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>>
>>>
>>> Le 20/05/2021 à 09:29, Naveen N. Rao a écrit :
>>>> diff --git a/arch/powerpc/kernel/optprobes.c 
>>>> b/arch/powerpc/kernel/optprobes.c
>>>> index cdf87086fa33a0..2bc53fa48a1b33 100644
>>>> --- a/arch/powerpc/kernel/optprobes.c
>>>> +++ b/arch/powerpc/kernel/optprobes.c
>>>> @@ -281,8 +281,12 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe 
>>>> *p)
>>>>       /*
>>>>        * 3. load instruction to be emulated into relevant register, and
>>>>        */
>>>> -    temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
>>>> -    patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
>>>> +    if (IS_ENABLED(CONFIG_PPC64)) {
>>>> +        temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
>>>> +        patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
>>>> +    } else {
>>>> +        patch_imm_load_insns((unsigned long)p->ainsn.insn, 4, buff + TMPL_INSN_IDX);
>>>> +    }
>>>
>>> It means commit https://github.com/linuxppc/linux/commit/693557ebf407a85ea400a0b501bb97687d8f4856 
>>> was not necessary and may be reverted.
>> 
>> Indeed, I will send a revert for it.
>> 
> 
> I'm not completely sure it is worth reverting, on an other hand it is pointless anyway to have 
> something to convert to a u64 something that cannot be more than 32 bits on a PPC32, so now that we 
> have ppc_inst_as_ulong() it is as good I think.

Sure. If necessary, the revert can go in separately as part of your 
other cleanup series.

- Naveen
Naveen N. Rao June 7, 2021, 11:34 a.m. UTC | #5 
Naveen N. Rao wrote:
> Trying to use a kprobe on ppc32 results in the below splat:
>     BUG: Unable to handle kernel data access on read at 0x7c0802a6
>     Faulting instruction address: 0xc002e9f0
>     Oops: Kernel access of bad area, sig: 11 [#1]
>     BE PAGE_SIZE=4K PowerPC 44x Platform
>     Modules linked in:
>     CPU: 0 PID: 89 Comm: sh Not tainted 5.13.0-rc1-01824-g3a81c0495fdb #7
>     NIP:  c002e9f0 LR: c0011858 CTR: 00008a47
>     REGS: c292fd50 TRAP: 0300   Not tainted  (5.13.0-rc1-01824-g3a81c0495fdb)
>     MSR:  00009000 <EE,ME>  CR: 24002002  XER: 20000000
>     DEAR: 7c0802a6 ESR: 00000000
>     <snip>
>     NIP [c002e9f0] emulate_step+0x28/0x324
>     LR [c0011858] optinsn_slot+0x128/0x10000
>     Call Trace:
>      opt_pre_handler+0x7c/0xb4 (unreliable)
>      optinsn_slot+0x128/0x10000
>      ret_from_syscall+0x0/0x28
> 
> The offending instruction is:
>     81 24 00 00     lwz     r9,0(r4)
> 
> Here, we are trying to load the second argument to emulate_step():
> struct ppc_inst, which is the instruction to be emulated. On ppc64,
> structures are passed in registers when passed by value. However, per
> the ppc32 ABI, structures are always passed to functions as pointers.
> This isn't being adhered to when setting up the call to emulate_step()
> in the optprobe trampoline. Fix the same.
> 
> Fixes: eacf4c0202654a ("powerpc: Enable OPTPROBES on PPC32")
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/optprobes.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Christophe,
Can you confirm if this patch works for you? It would be good if this 
can go in v5.13.


Thanks,
Naveen
Christophe Leroy June 7, 2021, 2:31 p.m. UTC | #6 
Le 07/06/2021 à 13:34, Naveen N. Rao a écrit :
> Naveen N. Rao wrote:
>> Trying to use a kprobe on ppc32 results in the below splat:
>>     BUG: Unable to handle kernel data access on read at 0x7c0802a6
>>     Faulting instruction address: 0xc002e9f0
>>     Oops: Kernel access of bad area, sig: 11 [#1]
>>     BE PAGE_SIZE=4K PowerPC 44x Platform
>>     Modules linked in:
>>     CPU: 0 PID: 89 Comm: sh Not tainted 5.13.0-rc1-01824-g3a81c0495fdb #7
>>     NIP:  c002e9f0 LR: c0011858 CTR: 00008a47
>>     REGS: c292fd50 TRAP: 0300   Not tainted  (5.13.0-rc1-01824-g3a81c0495fdb)
>>     MSR:  00009000 <EE,ME>  CR: 24002002  XER: 20000000
>>     DEAR: 7c0802a6 ESR: 00000000
>>     <snip>
>>     NIP [c002e9f0] emulate_step+0x28/0x324
>>     LR [c0011858] optinsn_slot+0x128/0x10000
>>     Call Trace:
>>      opt_pre_handler+0x7c/0xb4 (unreliable)
>>      optinsn_slot+0x128/0x10000
>>      ret_from_syscall+0x0/0x28
>>
>> The offending instruction is:
>>     81 24 00 00     lwz     r9,0(r4)
>>
>> Here, we are trying to load the second argument to emulate_step():
>> struct ppc_inst, which is the instruction to be emulated. On ppc64,
>> structures are passed in registers when passed by value. However, per
>> the ppc32 ABI, structures are always passed to functions as pointers.
>> This isn't being adhered to when setting up the call to emulate_step()
>> in the optprobe trampoline. Fix the same.
>>
>> Fixes: eacf4c0202654a ("powerpc: Enable OPTPROBES on PPC32")
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/kernel/optprobes.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> Christophe,
> Can you confirm if this patch works for you? It would be good if this can go in v5.13.
> 

I'm trying to use kprobes, but I must be missing something. I have tried to follow the exemple in 
kernel's documentation:

  # echo 'p:myprobe do_sys_open dfd=%r3' > /sys/kernel/debug/tracing/kprobe_events

  # echo 1 > /sys/kernel/debug/tracing/events/kprobes/myprobe/enable

  # cat /sys/kernel/debug/kprobes/list

  c00122e4  k  kretprobe_trampoline+0x0    [OPTIMIZED]
  c018a1b4  k  do_sys_open+0x0    [OPTIMIZED]

  # cat /sys/kernel/debug/tracing/tracing_on

  1

  # cat /sys/kernel/debug/tracing/trace

# tracer: nop
#
# entries-in-buffer/entries-written: 0/0   #P:1
#
#                                _-----=> irqs-off
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| /     delay
#           TASK-PID     CPU#  ||||   TIMESTAMP  FUNCTION
#              | |         |   ||||      |         |



So it looks like I get no event. I can't believe that do_sys_open() is never hit.

This is without your patch, so it should Oops ?


Then it looks like something is locked up somewhere, because I can't do anything else:

  # echo 'p:myprobe2 do_sys_openat2 dfd=%r3' >/sys/kernel/debug/tracing/kprobe_events

  -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy

  # echo '-:myprobe' > /sys/kernel/debug/tracing/kprobe_events

  -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy

  # echo > /sys/kernel/debug/tracing/kprobe_events

  -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy


Christophe
Christophe Leroy June 7, 2021, 5:36 p.m. UTC | #7 
Le 07/06/2021 à 16:31, Christophe Leroy a écrit :
> 
> 
> Le 07/06/2021 à 13:34, Naveen N. Rao a écrit :
>> Naveen N. Rao wrote:
>>> Trying to use a kprobe on ppc32 results in the below splat:
>>>     BUG: Unable to handle kernel data access on read at 0x7c0802a6
>>>     Faulting instruction address: 0xc002e9f0
>>>     Oops: Kernel access of bad area, sig: 11 [#1]
>>>     BE PAGE_SIZE=4K PowerPC 44x Platform
>>>     Modules linked in:
>>>     CPU: 0 PID: 89 Comm: sh Not tainted 5.13.0-rc1-01824-g3a81c0495fdb #7
>>>     NIP:  c002e9f0 LR: c0011858 CTR: 00008a47
>>>     REGS: c292fd50 TRAP: 0300   Not tainted  (5.13.0-rc1-01824-g3a81c0495fdb)
>>>     MSR:  00009000 <EE,ME>  CR: 24002002  XER: 20000000
>>>     DEAR: 7c0802a6 ESR: 00000000
>>>     <snip>
>>>     NIP [c002e9f0] emulate_step+0x28/0x324
>>>     LR [c0011858] optinsn_slot+0x128/0x10000
>>>     Call Trace:
>>>      opt_pre_handler+0x7c/0xb4 (unreliable)
>>>      optinsn_slot+0x128/0x10000
>>>      ret_from_syscall+0x0/0x28
>>>
>>> The offending instruction is:
>>>     81 24 00 00     lwz     r9,0(r4)
>>>
>>> Here, we are trying to load the second argument to emulate_step():
>>> struct ppc_inst, which is the instruction to be emulated. On ppc64,
>>> structures are passed in registers when passed by value. However, per
>>> the ppc32 ABI, structures are always passed to functions as pointers.
>>> This isn't being adhered to when setting up the call to emulate_step()
>>> in the optprobe trampoline. Fix the same.
>>>
>>> Fixes: eacf4c0202654a ("powerpc: Enable OPTPROBES on PPC32")
>>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>> ---
>>>  arch/powerpc/kernel/optprobes.c | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> Christophe,
>> Can you confirm if this patch works for you? It would be good if this can go in v5.13.
>>
> 
> I'm trying to use kprobes, but I must be missing something. I have tried to follow the exemple in 
> kernel's documentation:
> 
>   # echo 'p:myprobe do_sys_open dfd=%r3' > /sys/kernel/debug/tracing/kprobe_events
> 
>   # echo 1 > /sys/kernel/debug/tracing/events/kprobes/myprobe/enable
> 
>   # cat /sys/kernel/debug/kprobes/list
> 
>   c00122e4  k  kretprobe_trampoline+0x0    [OPTIMIZED]
>   c018a1b4  k  do_sys_open+0x0    [OPTIMIZED]
> 
>   # cat /sys/kernel/debug/tracing/tracing_on
> 
>   1
> 
>   # cat /sys/kernel/debug/tracing/trace
> 
> # tracer: nop
> #
> # entries-in-buffer/entries-written: 0/0   #P:1
> #
> #                                _-----=> irqs-off
> #                               / _----=> need-resched
> #                              | / _---=> hardirq/softirq
> #                              || / _--=> preempt-depth
> #                              ||| /     delay
> #           TASK-PID     CPU#  ||||   TIMESTAMP  FUNCTION
> #              | |         |   ||||      |         |
> 
> 
> 
> So it looks like I get no event. I can't believe that do_sys_open() is never hit.
> 
> This is without your patch, so it should Oops ?
> 
> 
> Then it looks like something is locked up somewhere, because I can't do anything else:
> 
>   # echo 'p:myprobe2 do_sys_openat2 dfd=%r3' >/sys/kernel/debug/tracing/kprobe_events
> 
>   -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy
> 
>   # echo '-:myprobe' > /sys/kernel/debug/tracing/kprobe_events
> 
>   -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy
> 
>   # echo > /sys/kernel/debug/tracing/kprobe_events
> 
>   -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy
> 
> 

Ok, did a new test. Seems like do_sys_open() is really never called.
I set the test at do_sys_openat2 , it was not optimised and was working.
I set the test at do_sys_openat2+0x10 , it was optimised and crashed.
Now I'm going to test the patch.

When I set an event, is that normal that it removes the previous one ? Then we can have only one 
event at a time ? And then when that event is enabled we get 'Device or resource busy' when trying 
to add a new one ?

Christophe
Christophe Leroy June 8, 2021, 4:58 a.m. UTC | #8 
Le 07/06/2021 à 19:36, Christophe Leroy a écrit :
> 
> 
> Le 07/06/2021 à 16:31, Christophe Leroy a écrit :
>>
>>
>> Le 07/06/2021 à 13:34, Naveen N. Rao a écrit :
>>> Naveen N. Rao wrote:
>>>> Trying to use a kprobe on ppc32 results in the below splat:
>>>>     BUG: Unable to handle kernel data access on read at 0x7c0802a6
>>>>     Faulting instruction address: 0xc002e9f0
>>>>     Oops: Kernel access of bad area, sig: 11 [#1]
>>>>     BE PAGE_SIZE=4K PowerPC 44x Platform
>>>>     Modules linked in:
>>>>     CPU: 0 PID: 89 Comm: sh Not tainted 5.13.0-rc1-01824-g3a81c0495fdb #7
>>>>     NIP:  c002e9f0 LR: c0011858 CTR: 00008a47
>>>>     REGS: c292fd50 TRAP: 0300   Not tainted  (5.13.0-rc1-01824-g3a81c0495fdb)
>>>>     MSR:  00009000 <EE,ME>  CR: 24002002  XER: 20000000
>>>>     DEAR: 7c0802a6 ESR: 00000000
>>>>     <snip>
>>>>     NIP [c002e9f0] emulate_step+0x28/0x324
>>>>     LR [c0011858] optinsn_slot+0x128/0x10000
>>>>     Call Trace:
>>>>      opt_pre_handler+0x7c/0xb4 (unreliable)
>>>>      optinsn_slot+0x128/0x10000
>>>>      ret_from_syscall+0x0/0x28
>>>>
>>>> The offending instruction is:
>>>>     81 24 00 00     lwz     r9,0(r4)
>>>>
>>>> Here, we are trying to load the second argument to emulate_step():
>>>> struct ppc_inst, which is the instruction to be emulated. On ppc64,
>>>> structures are passed in registers when passed by value. However, per
>>>> the ppc32 ABI, structures are always passed to functions as pointers.
>>>> This isn't being adhered to when setting up the call to emulate_step()
>>>> in the optprobe trampoline. Fix the same.
>>>>
>>>> Fixes: eacf4c0202654a ("powerpc: Enable OPTPROBES on PPC32")
>>>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>>> ---
>>>>  arch/powerpc/kernel/optprobes.c | 8 ++++++--
>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> Christophe,
>>> Can you confirm if this patch works for you? It would be good if this can go in v5.13.
>>>
>>
>> I'm trying to use kprobes, but I must be missing something. I have tried to follow the exemple in 
>> kernel's documentation:
>>
>>   # echo 'p:myprobe do_sys_open dfd=%r3' > /sys/kernel/debug/tracing/kprobe_events
>>
>>   # echo 1 > /sys/kernel/debug/tracing/events/kprobes/myprobe/enable
>>
>>   # cat /sys/kernel/debug/kprobes/list
>>
>>   c00122e4  k  kretprobe_trampoline+0x0    [OPTIMIZED]
>>   c018a1b4  k  do_sys_open+0x0    [OPTIMIZED]
>>
>>   # cat /sys/kernel/debug/tracing/tracing_on
>>
>>   1
>>
>>   # cat /sys/kernel/debug/tracing/trace
>>
>> # tracer: nop
>> #
>> # entries-in-buffer/entries-written: 0/0   #P:1
>> #
>> #                                _-----=> irqs-off
>> #                               / _----=> need-resched
>> #                              | / _---=> hardirq/softirq
>> #                              || / _--=> preempt-depth
>> #                              ||| /     delay
>> #           TASK-PID     CPU#  ||||   TIMESTAMP  FUNCTION
>> #              | |         |   ||||      |         |
>>
>>
>>
>> So it looks like I get no event. I can't believe that do_sys_open() is never hit.
>>
>> This is without your patch, so it should Oops ?
>>
>>
>> Then it looks like something is locked up somewhere, because I can't do anything else:
>>
>>   # echo 'p:myprobe2 do_sys_openat2 dfd=%r3' >/sys/kernel/debug/tracing/kprobe_events
>>
>>   -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy
>>
>>   # echo '-:myprobe' > /sys/kernel/debug/tracing/kprobe_events
>>
>>   -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy
>>
>>   # echo > /sys/kernel/debug/tracing/kprobe_events
>>
>>   -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy
>>
>>
> 
> Ok, did a new test. Seems like do_sys_open() is really never called.
> I set the test at do_sys_openat2 , it was not optimised and was working.
> I set the test at do_sys_openat2+0x10 , it was optimised and crashed.
> Now I'm going to test the patch.
> 
> When I set an event, is that normal that it removes the previous one ? Then we can have only one 
> event at a time ? And then when that event is enabled we get 'Device or resource busy' when trying 
> to add a new one ?
> 

I confirm it doesn't crash anymore and it now works with optimised probes.

Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Naveen N. Rao June 8, 2021, 11:34 a.m. UTC | #9 
Christophe Leroy wrote:
> 
> 
> Le 07/06/2021 à 19:36, Christophe Leroy a écrit :
>> 
>> 
>> Le 07/06/2021 à 16:31, Christophe Leroy a écrit :
>>>
>>>
>>> Le 07/06/2021 à 13:34, Naveen N. Rao a écrit :
>>>> Naveen N. Rao wrote:
>>>>> Trying to use a kprobe on ppc32 results in the below splat:
>>>>>     BUG: Unable to handle kernel data access on read at 0x7c0802a6
>>>>>     Faulting instruction address: 0xc002e9f0
>>>>>     Oops: Kernel access of bad area, sig: 11 [#1]
>>>>>     BE PAGE_SIZE=4K PowerPC 44x Platform
>>>>>     Modules linked in:
>>>>>     CPU: 0 PID: 89 Comm: sh Not tainted 5.13.0-rc1-01824-g3a81c0495fdb #7
>>>>>     NIP:  c002e9f0 LR: c0011858 CTR: 00008a47
>>>>>     REGS: c292fd50 TRAP: 0300   Not tainted  (5.13.0-rc1-01824-g3a81c0495fdb)
>>>>>     MSR:  00009000 <EE,ME>  CR: 24002002  XER: 20000000
>>>>>     DEAR: 7c0802a6 ESR: 00000000
>>>>>     <snip>
>>>>>     NIP [c002e9f0] emulate_step+0x28/0x324
>>>>>     LR [c0011858] optinsn_slot+0x128/0x10000
>>>>>     Call Trace:
>>>>>      opt_pre_handler+0x7c/0xb4 (unreliable)
>>>>>      optinsn_slot+0x128/0x10000
>>>>>      ret_from_syscall+0x0/0x28
>>>>>
>>>>> The offending instruction is:
>>>>>     81 24 00 00     lwz     r9,0(r4)
>>>>>
>>>>> Here, we are trying to load the second argument to emulate_step():
>>>>> struct ppc_inst, which is the instruction to be emulated. On ppc64,
>>>>> structures are passed in registers when passed by value. However, per
>>>>> the ppc32 ABI, structures are always passed to functions as pointers.
>>>>> This isn't being adhered to when setting up the call to emulate_step()
>>>>> in the optprobe trampoline. Fix the same.
>>>>>
>>>>> Fixes: eacf4c0202654a ("powerpc: Enable OPTPROBES on PPC32")
>>>>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>>>> ---
>>>>>  arch/powerpc/kernel/optprobes.c | 8 ++++++--
>>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> Christophe,
>>>> Can you confirm if this patch works for you? It would be good if this can go in v5.13.
>>>>
>>>
>>> I'm trying to use kprobes, but I must be missing something. I have tried to follow the exemple in 
>>> kernel's documentation:
>>>
>>>   # echo 'p:myprobe do_sys_open dfd=%r3' > /sys/kernel/debug/tracing/kprobe_events
>>>
>>>   # echo 1 > /sys/kernel/debug/tracing/events/kprobes/myprobe/enable
>>>
>>>   # cat /sys/kernel/debug/kprobes/list
>>>
>>>   c00122e4  k  kretprobe_trampoline+0x0    [OPTIMIZED]
>>>   c018a1b4  k  do_sys_open+0x0    [OPTIMIZED]
>>>
>>>   # cat /sys/kernel/debug/tracing/tracing_on
>>>
>>>   1
>>>
>>>   # cat /sys/kernel/debug/tracing/trace
>>>
>>> # tracer: nop
>>> #
>>> # entries-in-buffer/entries-written: 0/0   #P:1
>>> #
>>> #                                _-----=> irqs-off
>>> #                               / _----=> need-resched
>>> #                              | / _---=> hardirq/softirq
>>> #                              || / _--=> preempt-depth
>>> #                              ||| /     delay
>>> #           TASK-PID     CPU#  ||||   TIMESTAMP  FUNCTION
>>> #              | |         |   ||||      |         |
>>>
>>>
>>>
>>> So it looks like I get no event. I can't believe that do_sys_open() is never hit.
>>>
>>> This is without your patch, so it should Oops ?
>>>
>>>
>>> Then it looks like something is locked up somewhere, because I can't do anything else:
>>>
>>>   # echo 'p:myprobe2 do_sys_openat2 dfd=%r3' >/sys/kernel/debug/tracing/kprobe_events
>>>
>>>   -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy
>>>
>>>   # echo '-:myprobe' > /sys/kernel/debug/tracing/kprobe_events
>>>
>>>   -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy
>>>
>>>   # echo > /sys/kernel/debug/tracing/kprobe_events
>>>
>>>   -sh: can't create /sys/kernel/debug/tracing/kprobe_events: Device or resource busy

These should work if you disable the event. See below...

>>>
>>>
>> 
>> Ok, did a new test. Seems like do_sys_open() is really never called.
>> I set the test at do_sys_openat2 , it was not optimised and was working.
>> I set the test at do_sys_openat2+0x10 , it was optimised and crashed.
>> Now I'm going to test the patch.
>> 
>> When I set an event, is that normal that it removes the previous one ? Then we can have only one 
>> event at a time ? And then when that event is enabled we get 'Device or resource busy' when trying 
>> to add a new one ?

You should append to kprobe_events (i.e., use '>>') when _adding_ an 
event, otherwise it is considered a write and it tries to remove the 
existing event, which can't be done if the event is enabled.

kprobe_events allows events to be removed only after they are disabled.

>> 
> 
> I confirm it doesn't crash anymore and it now works with optimised probes.
> 
> Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Thanks!
- Naveen
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index cdf87086fa33a0..2bc53fa48a1b33 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -281,8 +281,12 @@  int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
 	/*
 	 * 3. load instruction to be emulated into relevant register, and
 	 */
-	temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
-	patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
+	if (IS_ENABLED(CONFIG_PPC64)) {
+		temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
+		patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
+	} else {
+		patch_imm_load_insns((unsigned long)p->ainsn.insn, 4, buff + TMPL_INSN_IDX);
+	}
 
 	/*
 	 * 4. branch back from trampoline