Date: Tue, 24 Feb 2015 09:52:39 +0100
From: Petr Mladek <pmladek@suse.cz>
To: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>, Josh Poimboeuf <jpoimboe@redhat.com>,
        Ananth N Mavinakayanahalli <ananth@in.ibm.com>,
        Jiri Kosina <jkosina@suse.cz>, Seth Jennings <sjenning@redhat.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Vojtech Pavlik <vojtech@suse.cz>, Namhyung Kim <namhyung@kernel.org>,
        Miroslav Benes <mbenes@suse.cz>, Ingo Molnar <mingo@kernel.org>
Subject: Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if
 the probe can change regs->ip
Message-ID: <20150224085239.GA2358@dhcp128.suse.cz>
References: <20141121102502.11844.82696.stgit@localhost.localdomain>
 <20141121102530.11844.41626.stgit@localhost.localdomain>
 <20150126161436.GC8244@dhcp128.suse.cz>
 <54EC2A6A.2010007@hitachi.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <54EC2A6A.2010007@hitachi.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4764
Lines: 114

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.


> > 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.


> >    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.

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.

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