lkml.org 
[lkml]   [2018]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -tip v4 24/27] bpf: error-inject: kprobes: Clear current_kprobe and enable preempt in kprobe
Masami Hiramatsu wrote:
> On Thu, 31 May 2018 16:25:38 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>
>> Masami Hiramatsu wrote:
>> > Clear current_kprobe and enable preemption in kprobe
>> > even if pre_handler returns !0.
>> >
>> > This simplifies function override using kprobes.
>> >
>> > Jprobe used to require to keep the preemption disabled and
>> > keep current_kprobe until it returned to original function
>> > entry. For this reason kprobe_int3_handler() and similar
>> > arch dependent kprobe handers checks pre_handler result
>> > and exit without enabling preemption if the result is !0.
>> >
>> > After removing the jprobe, Kprobes does not need to
>> > keep preempt disabled even if user handler returns !0
>> > anymore.
>>
>> I think the reason jprobes did it that way is to address architecture
>> specific requirements when changing a function. So, without that
>> infrastructure, I am not sure if we will be able to claim support for
>> over-riding functions with kprobes. I am not sure if we want to claim
>> that, but this is something we need to be clear on.
>
> Really? as far as I can see, there seems no such architecture.
> The keeping preempt disabled is corresponding to keeping current_kprobe
> since the current_kprobe is per-cpu.

Right, and the reason for not resetting current_kprobe after kprobe
handling is done is primarily for jprobes.

> This means if it is preempted
> before hitting break_handler and changed cpu core, we missed to
> handle current_kprobe and goes to panic. But if we don't need
> such "break back" (removing break_handler), we don't need to
> keep current_kprobe (because it is not handled afterwards).

Agreed.

>
> Anyway, changing function execution path is a "one-way" change.

This is the problem. With jprobes, over-riding a function was not a
"one-way" change because it involves more than just changing the [n]ip.
That is the reason we had setjmp/longjmp (aka break_handler).

> We don't have a chance to fixup that disabled preemption and current_kprobe
> after returning to the new function. So current error-inject clears
> current_kprobe and enable preemption before returning !0 from its
> kprobe pre_handler.
>
> This is just moving such needless operation from user-pre_handler to
> kprobes itself.
>
>> For powerpc, the current function override in error-inject works fine
>> since the new function does nothing. But, if anyone wants to do more
>> work in the replacement function, it won't work with the current
>> approach.
>
> If you are considering about TOC change etc. yes, it depends on
> the archtecture. As far as I know IA64 and powerpc will not allow
> to support changing execution path without special care.
> Other "flat and simple" function call architectures like x86, arm
> can change execution path without special care.

Yes, that's the concern. As I stated earlier, the only user seems to be
error-injection where this is not a concern. I wanted this to be made
clear.

I've since noticed that you are updating Documentation/kprobes.txt to
make this clear in patch 24/27 in this series. So, I'm ok with the
changes in this series.


Thanks,
Naveen


\
 
 \ /
  Last update: 2018-06-02 13:58    [W:0.132 / U:0.676 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site