Date: Mon, 2 Feb 2015 15:57:20 +0100
From: Petr Mladek <pmladek@suse.cz>
To: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: "David S. Miller" <davem@davemloft.net>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
        Ananth N Mavinakayanahalli <ananth@in.ibm.com>,
        Frederic Weisbecker <fweisbec@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>, Jiri Kosina <jkosina@suse.cz>,
        linux-kernel@vger.kernel.org, Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH] kprobes/x86: Use the current code when modified by
 Kprobes using ftrace
Message-ID: <20150202145720.GA1576@dhcp128.suse.cz>
References: <1422632700-4342-1-git-send-email-pmladek@suse.cz>
 <54CF37EA.50000@hitachi.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <54CF37EA.50000@hitachi.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4038
Lines: 108

On Mon 2015-02-02 17:40:10, Masami Hiramatsu wrote:
> (2015/01/31 0:45), Petr Mladek wrote:
> > can_probe() checks if the given address points to the beginning of
> > an instruction. It analyzes all the instructions from the beginning
> > of the function until the given address. The code might be modified
> > by another Kprobe. In this case, the current code is read into a buffer,
> > int3 breakpoint is replaced by the saved opcode in the buffer, and
> > can_probe() analyzes the buffer instead.
> > 
> > There is a bug that __recover_probed_insn() tries to restore
> > the original code even for Kprobes using the ftrace framework.
> > But in this case, the opcode is not stored. See the difference
> > between arch_prepare_kprobe() and arch_prepare_kprobe_ftrace().
> > The opcode is stored by arch_copy_kprobe() only from
> > arch_prepare_kprobe().
> 
> Good catch!
> 
> > This patch makes Kprobe to use the current code when it is modified
> > by ftrace. It is not the original one but it is a valid instruction
> > of the same length. Therefore it is perfectly fine for the check.
> 
> Hmm, but there is a chance we can hit it in the period of replacing code.
> This means we'll see 5byte nop but started with int3. (see P6_NOP5 in
> arch/x86/include/asm/nops.h)
> So, I think we'd better return a special buffer which has a 5byte NOP.

Good point! I though that we were synchronized against ftrace by
text_mutex and kprobe_mutex but we were not. Ftrace does not take
text_mutex and it can modify the ftrace location also from other
reasons, e.g. to enable tracing.

Therefore we need to always check if the ftrace location is included
in the helper buffer and replace it by the 5-byte nop. It need not be
on the beginning of the instruction if mcount is used.

I am going to prepare v2 of the patch.

Best Regards,
Petr


> Thank you for reporting!
> 
> > 
> > Note that I found this problem when playing with Kprobes. I did it
> > on x86_64 with gcc-4.8.3 that supported -mfentry. I modified
> > samples/kprobes/kprobe_example.c and added offset 5 to put
> > the probe right after the fentry area:
> > 
> > --- cut ---
> >  static struct kprobe kp = {
> >  	.symbol_name	= "do_fork",
> > +	.offset = 5,
> >  };
> > --- cut ---
> > 
> > Then I was able to load kprobe_example before jprobe_example
> > but not the other way around:
> > 
> > $> modprobe jprobe_example
> > $> modprobe kprobe_example
> > modprobe: ERROR: could not insert 'kprobe_example': Invalid or incomplete multibyte or wide character
> > 
> > It did not make much sense and debugging pointed to the bug
> > described above.
> > 
> > Signed-off-by: Petr Mladek <pmladek@suse.cz>
> > ---
> >  arch/x86/kernel/kprobes/core.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > index 98f654d466e5..ef321caae3ba 100644
> > --- a/arch/x86/kernel/kprobes/core.c
> > +++ b/arch/x86/kernel/kprobes/core.c
> > @@ -225,8 +225,14 @@ __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
> >  	struct kprobe *kp;
> >  
> >  	kp = get_kprobe((void *)addr);
> > -	/* There is no probe, return original address */
> > -	if (!kp)
> > +	/*
> > +	 * Use the current code if it is not modified by Kprobe
> > +	 * or when the Kprobe is using ftrace. In the second case
> > +	 * we do not have any information about the original code
> > +	 * but it is not a problem. Ftrace has put there a valid
> > +	 * instruction of the same length.
> > +	 */
> > +	if (!kp || kprobe_ftrace(kp))
> >  		return addr;
> >  
> >  	/*
> > 
> 
> 
> -- 
> Masami HIRAMATSU
> Software Platform Research Dept. Linux Technology Research Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu.pt@hitachi.com
> 
> 
--
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/