Re: [PATCH] powerpc/kprobes: Pass ppc_inst as a pointer to emulate_step() on ppc32

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

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