Messages in this thread | | | Date | Mon, 4 Jun 2018 18:08:44 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH -tip v4 24/27] bpf: error-inject: kprobes: Clear current_kprobe and enable preempt in kprobe |
| |
On Sat, 02 Jun 2018 17:28:05 +0530 "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> 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.
I see your concern. Yeah, that's why I added [24/27] for clearly stating it.
Thanks,
-- Masami Hiramatsu <mhiramat@kernel.org>
| |