Re: [BUGFIX PATCH] kprobes/x86: Remove IRQ disabling from jprobe handlers

[Masami Hiramatsu]

On Tue, 3 Oct 2017 19:11:00 +0200
Ingo Molnar <mingo@xxxxxxxxxx> wrote:

> 
> * Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> 
> > On Tue, 3 Oct 2017 11:33:44 +0200
> > Ingo Molnar <mingo@xxxxxxxxxx> wrote:
> > 
> > > 
> > > * Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> > > 
> > > > Jprobe actually doesn't need to disable IRQs while calling
> > > > handlers, because Documentation/kprobes.txt says:
> > > > 
> > > > -----
> > > >  Probe handlers are run with preemption disabled.  Depending on the
> > > >  architecture and optimization state, handlers may also run with
> > > >  interrupts disabled (e.g., kretprobe handlers and optimized kprobe
> > > >  handlers run without interrupt disabled on x86/x86-64).
> > > > -----
> > > > 
> > > > So let's remove IRQ disabling from jprobes too.
> > > > 
> > > > Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > > > ---
> > > >  arch/x86/kernel/kprobes/core.c |    2 --
> > > >  1 file changed, 2 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > > > index b48e0efd668e..bd36f3c33cd0 100644
> > > > --- a/arch/x86/kernel/kprobes/core.c
> > > > +++ b/arch/x86/kernel/kprobes/core.c
> > > > @@ -1099,8 +1099,6 @@ int setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
> > > >  	 * raw stack chunk with redzones:
> > > >  	 */
> > > >  	__memcpy(kcb->jprobes_stack, (kprobe_opcode_t *)addr, MIN_STACK_SIZE(addr));
> > > > -	regs->flags &= ~X86_EFLAGS_IF;
> > > > -	trace_hardirqs_off();
> > > >  	regs->ip = (unsigned long)(jp->entry);
> > > >  
> > > >  	/*
> > > 
> > > Could you please also do a patch that disables jprobes permanently, in preparation 
> > > of eventually removing them later?
> > 
> > Would you mean introducing CONFIG_JPROBE_API and cut APIs out,
> > or just adding "__deprecated" for each API?
> 
> No, I'd suggest removing in-kernel uses and disabling jprobes in the Kconfig 
> altogether, but without removing the code and its integration for the time being.

Hmm, actually we can not disable jprobe, that has no separate Kconfig.
So we need to introduce new kconfig for that.

And, there are several network protocols using jprobe to trace events.
(e.g. NET_DCCPPROBE and NET_TCPPROBE)
I think they need to migrate to trace-event at first.

So, how about below idea?

1. Introduce CONFIG_JPROBE_API which only separate jprobe general parts
     (no arch dependent code involves) and make it default n.
2. Mark break_handler and jprobe APIs deprecated so that no new user comes up.
3. migrate in-kernel jprobe user to trace-event or ftrace.
   (may take some time)
4. after that, we can completely remove jprobe which will be a series for
   all archs. (or just one big patch?)

Thank you,

-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>