Message-ID: <54EC64BA.2040908@hitachi.com>
Date:	Tue, 24 Feb 2015 20:47:06 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Petr Mladek <pmladek@...e.cz>
Cc:	Steven Rostedt <rostedt@...dmis.org>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Jiri Kosina <jkosina@...e.cz>,
	Seth Jennings <sjenning@...hat.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Vojtech Pavlik <vojtech@...e.cz>,
	Namhyung Kim <namhyung@...nel.org>,
	Miroslav Benes <mbenes@...e.cz>, Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if
 the probe can change regs->ip

(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:
>>>
>>> 1. Jprobe and LivePatch must be in a hard conflict. They both need
>>>    to change IP and continue another way after ftrace ops finishes.
>>>
>>>    BTW: I wonder a bit why Jprobe handler could not be called directly
>>>    from kprobe_ftrace_handler(). I guess that it is because we want
>>>    to call the kprobe handler in a sane context: preemption and IRQs
>>>    enabled, be able to use traced functions.
>>
>> Right, Jprobe is just a different interface of kprobe handler. It must be
>> called from kprobes.
>> However, I think this is not so hard in practice, since we already have
>> perf-probe which allows us to find which register or stack is assigned to
>> which function parameter. That was the main reason why jprobe is introduced.
>> But now, we have perf-probe or systemtap, we don't(or less) need the hack like
>> jprobe anymore.
> 
> I see. It is called this special way (modifies regs->ip), so that
> registry and stack have the same content as if the probed function was called.

Yes.

>>> 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 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...
And anyway, IPMODIFY should be only for jprobes not kprobes...

> I still want to clean and test it a bit before sending as RFC.
> 
>> BTW, the kprobes on function entry (iow, ftrace-based kprobes) should
>> not be ignored. Even if we patches a function-body, the entrance
>> address should be same.
> 
> Yup
> 
>>> 3. Kretprobe could live with a patch without a problem!
>>>
>>>    The Kretprobe entry handler is called directly in
>>>    kprobe_ftrace_handler() and does not change IP.
>>>    On the other hand the LivePatch ftrace handler does
>>>    not modify the return address because the return address
>>>    is the same for the original and the patched function.
>>
>> Right.
> 
> Thanks for confirmation.
> 
>>>    Or did I miss something?
>>>
>>>    This is where this patch might be useful. The other patches
>>>    from this patch set are already in Linus' tree and I cannot
>>>    find any information about this one.
>>
>> Well, thank you for picking it up!
> 
> 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.

Thank you,

> 
> Best Regards,
> Petr
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists