Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if the probe can change regs->ip

[Petr Mladek <pmladek@suse.cz>]

On Tue 2015-02-24 20:47:06, Masami Hiramatsu wrote:
> (2015/02/24 17:52), Petr Mladek wrote:
> > On Tue 2015-02-24 16:38:18, Masami Hiramatsu wrote:
> >> Hi Petr,
> >>
> >> Sorry I missed this mail.
> > 
> > Thanks a lot for answering it with many valuable information.
> >  
> >> (2015/01/27 1:14), Petr Mladek wrote:> On Fri 2014-11-21 05:25:30, Masami Hiramatsu wrote:
> >>>> Set FTRACE_OPS_FL_IPMODIFY flag only for the probes which can change
> >>>> regs->ip, which has kprobe->break_handler.
> >>>> Currently we can not put jprobe and another ftrace handler which
> >>>> changes regs->ip on the same function because all kprobes have
> >>>> FTRACE_OPS_FL_IPMODIFY flag. This removes FTRACE_OPS_FL_IPMODIFY
> >>>> flag from kprobes and only when the user uses jprobe (or the
> >>>> kprobe.break_handler != NULL) we add additinal ftrace_ops with
> >>>> FTRACE_OPS_FL_IPMODIFY on target function.
> >>>
> >>> Please, what are the plans with this patch?
> >>
> >> Well, I'll revise this for newer kernel.
> >>>
> >>> I have checked the interference between Kprobes and LivePatching and
> >>> here is my observation:
[...]
> >>> 2. Normal Kprobe for the original function is ignored if the function
> >>>    is patched.
> >>>
> >>>    I am working on a code that will print warning in both
> >>>    cases. First, when we add a patch and the function has
> >>>    a Kprobe registered. Second, the function is patched and
> >>>    we want to add Kprobe for the original version.
> >>
> >> Thanks! Maybe we can add "Ignored" flag for those kprobes so that users
> >> can check it is working or not via debugfs.
> > 
> > Great idea. Well, it will solve only already existing Kprobes.
> 
> Yeah, just changing the kprobe state is easy and needed.
> And for newer kprobes, perhaps we need to add
> bool klp_patched_function(void *func_addr); to check the
> function is patched. (this will need to be done with
> locking kpatch...)

I like this idea and will try to use it once I get time again.

> >>>    I want to make it generic and make it dependent on the
> >>>    IPMODIFY flag. IMHO, it just could be a handshake between
> >>>    kprobe and ftrace code. I am still trying to understand
> >>>    the needed parts of the code ;-)
> > 
> > I have played with it and realized that only Kprobes framework has
> > information about all existing and newly created Kprobes. Therefore
> > we need to somehow inform it that there is a patch and that the code
> > is redirected. I have a prototype that is introducing a new fake
> > Kprobe, so called Patch Probe. It has new flag KPROBE_FLAG_PATCH
> > and no handlers. Conflicts with existing Kprobes are checked when
> > this special probe is added. Also conflicts with these Patch probes
> > are checked when new normal Kprobe is added.
> 
> No, you don't need that. I can make kprobes_location() or
> kprobe_for_each_on(kp, start, end) {} iterator. Since the livepatch
> is in-tree feature now, we can change kprobes for it...

You are right, the in-tree live patch code brings more possibilities. 

> And anyway, IPMODIFY should be only for jprobes not kprobes...

Yup.

> > I have one more patch set in the queue. It better handle errors when
> > kprobe_ftrace_ops could not be registered in arm_kprobe_ftrace()
> > and disarm_kprobe_ftrace(). This one is nearly done. Unfortunately,
> > I had to interupt it because my wife got sick and I had to take care
> > of babies. And then there is the big activity around life patching
> > that we need to somehow handle.
> 
> Ah, thanks, and hope your wife to get better soon.

Thanks a lot. Fortunately, she already is better.

Best Regards,
Petr