[PATCH v2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[PATCH v2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Petr Mladek]

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

This patch makes Kprobe to use the ideal 5-byte NOP when the code
can be modified by ftrace. It is the original instruction, see
ftrace_make_nop() and ftrace_nop_replace().

Note that we always need to use the NOP for ftrace locations. Kprobes
do not block ftrace and the instruction might get modified at anytime.
It might even be in an inconsistent state because it is modified step
by step using the int3 breakpoint.

The patch also fixes indentation of the touched comment.

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 | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

Changes against v1:

  + always use 5-byte NOP for ftrace location
  + fix indentation of the touched comment

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 98f654d466e5..2f464b56766a 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -223,27 +223,41 @@ static unsigned long
 __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
 {
 	struct kprobe *kp;
+	unsigned long faddr;
 
 	kp = get_kprobe((void *)addr);
-	/* There is no probe, return original address */
-	if (!kp)
+	faddr = ftrace_location(addr);
+	/*
+	 * Use the current code if it is not modified by Kprobe
+	 * and it cannot be modified by ftrace.
+	 */
+	if (!kp && !faddr)
 		return addr;
 
 	/*
-	 *  Basically, kp->ainsn.insn has an original instruction.
-	 *  However, RIP-relative instruction can not do single-stepping
-	 *  at different place, __copy_instruction() tweaks the displacement of
-	 *  that instruction. In that case, we can't recover the instruction
-	 *  from the kp->ainsn.insn.
+	 * Basically, kp->ainsn.insn has an original instruction.
+	 * However, RIP-relative instruction can not do single-stepping
+	 * at different place, __copy_instruction() tweaks the displacement of
+	 * that instruction. In that case, we can't recover the instruction
+	 * from the kp->ainsn.insn.
 	 *
-	 *  On the other hand, kp->opcode has a copy of the first byte of
-	 *  the probed instruction, which is overwritten by int3. And
-	 *  the instruction at kp->addr is not modified by kprobes except
-	 *  for the first byte, we can recover the original instruction
-	 *  from it and kp->opcode.
+	 * On the other hand, in case on normal Kprobe, kp->opcode has a copy
+	 * of the first byte of the probed instruction, which is overwritten
+	 * by int3. And the instruction at kp->addr is not modified by kprobes
+	 * except for the first byte, we can recover the original instruction
+	 * from it and kp->opcode.
+	 *
+	 * In case of Kprobes using ftrace, we do not have a copy of
+	 * the original instruction. In fact, the ftrace location might
+	 * be modified at anytime and even could be in an inconsistent state.
+	 * Fortunately, we know that the original code is the ideal 5-byte
+	 * long NOP.
 	 */
-	memcpy(buf, kp->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
-	buf[0] = kp->opcode;
+	memcpy(buf, (void *)addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
+	if (faddr)
+		memcpy(buf, ideal_nops[NOP_ATOMIC5], MCOUNT_INSN_SIZE);
+	else
+		buf[0] = kp->opcode;
 	return (unsigned long)buf;
 }
 
-- 
1.8.5.6

Re: [PATCH v2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Masami Hiramatsu]

(2015/02/03 2:48), 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().
> 
> This patch makes Kprobe to use the ideal 5-byte NOP when the code
> can be modified by ftrace. It is the original instruction, see
> ftrace_make_nop() and ftrace_nop_replace().
> 
> Note that we always need to use the NOP for ftrace locations. Kprobes
> do not block ftrace and the instruction might get modified at anytime.
> It might even be in an inconsistent state because it is modified step
> by step using the int3 breakpoint.
> 
> The patch also fixes indentation of the touched comment.
> 
> 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.
> 

This looks good to me :)

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Ingo, could you merge this as an urgent fix?

Thank you!

> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> ---
>  arch/x86/kernel/kprobes/core.c | 42 ++++++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 14 deletions(-)
> 
> Changes against v1:
> 
>   + always use 5-byte NOP for ftrace location
>   + fix indentation of the touched comment
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 98f654d466e5..2f464b56766a 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -223,27 +223,41 @@ static unsigned long
>  __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
>  {
>  	struct kprobe *kp;
> +	unsigned long faddr;
>  
>  	kp = get_kprobe((void *)addr);
> -	/* There is no probe, return original address */
> -	if (!kp)
> +	faddr = ftrace_location(addr);
> +	/*
> +	 * Use the current code if it is not modified by Kprobe
> +	 * and it cannot be modified by ftrace.
> +	 */
> +	if (!kp && !faddr)
>  		return addr;
>  
>  	/*
> -	 *  Basically, kp->ainsn.insn has an original instruction.
> -	 *  However, RIP-relative instruction can not do single-stepping
> -	 *  at different place, __copy_instruction() tweaks the displacement of
> -	 *  that instruction. In that case, we can't recover the instruction
> -	 *  from the kp->ainsn.insn.
> +	 * Basically, kp->ainsn.insn has an original instruction.
> +	 * However, RIP-relative instruction can not do single-stepping
> +	 * at different place, __copy_instruction() tweaks the displacement of
> +	 * that instruction. In that case, we can't recover the instruction
> +	 * from the kp->ainsn.insn.
>  	 *
> -	 *  On the other hand, kp->opcode has a copy of the first byte of
> -	 *  the probed instruction, which is overwritten by int3. And
> -	 *  the instruction at kp->addr is not modified by kprobes except
> -	 *  for the first byte, we can recover the original instruction
> -	 *  from it and kp->opcode.
> +	 * On the other hand, in case on normal Kprobe, kp->opcode has a copy
> +	 * of the first byte of the probed instruction, which is overwritten
> +	 * by int3. And the instruction at kp->addr is not modified by kprobes
> +	 * except for the first byte, we can recover the original instruction
> +	 * from it and kp->opcode.
> +	 *
> +	 * In case of Kprobes using ftrace, we do not have a copy of
> +	 * the original instruction. In fact, the ftrace location might
> +	 * be modified at anytime and even could be in an inconsistent state.
> +	 * Fortunately, we know that the original code is the ideal 5-byte
> +	 * long NOP.
>  	 */
> -	memcpy(buf, kp->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
> -	buf[0] = kp->opcode;
> +	memcpy(buf, (void *)addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
> +	if (faddr)
> +		memcpy(buf, ideal_nops[NOP_ATOMIC5], MCOUNT_INSN_SIZE);
> +	else
> +		buf[0] = kp->opcode;
>  	return (unsigned long)buf;
>  }
>  
> 


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

Re: [PATCH v2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Petr Mladek]

On Tue 2015-02-03 16:41:39, Masami Hiramatsu wrote:
> (2015/02/03 2:48), 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().
> > 
> > This patch makes Kprobe to use the ideal 5-byte NOP when the code
> > can be modified by ftrace. It is the original instruction, see
> > ftrace_make_nop() and ftrace_nop_replace().
> > 
> > Note that we always need to use the NOP for ftrace locations. Kprobes
> > do not block ftrace and the instruction might get modified at anytime.
> > It might even be in an inconsistent state because it is modified step
> > by step using the int3 breakpoint.
> > 
> > The patch also fixes indentation of the touched comment.
> > 
> > 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.
> > 
> 
> This looks good to me :)
> 
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> 
> Ingo, could you merge this as an urgent fix?

Please, wait a bit, see below.
 
> Thank you!
> 
> > Signed-off-by: Petr Mladek <pmladek@suse.cz>
> > ---
> >  arch/x86/kernel/kprobes/core.c | 42 ++++++++++++++++++++++++++++--------------
> >  1 file changed, 28 insertions(+), 14 deletions(-)
> > 
> > Changes against v1:
> > 
> >   + always use 5-byte NOP for ftrace location
> >   + fix indentation of the touched comment
> > 
> > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > index 98f654d466e5..2f464b56766a 100644
> > --- a/arch/x86/kernel/kprobes/core.c
> > +++ b/arch/x86/kernel/kprobes/core.c
> > @@ -223,27 +223,41 @@ static unsigned long
> >  __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
> >  {
> >  	struct kprobe *kp;
> > +	unsigned long faddr;
> >  
> >  	kp = get_kprobe((void *)addr);
> > -	/* There is no probe, return original address */
> > -	if (!kp)
> > +	faddr = ftrace_location(addr);

I have just realized that ftrace_location() might return another
address if the given one points inside the ftrace_location.
This situation is not checked by this patch. I am going to work
on v3.

I knew that I should not have sent the patch just before leaving :-(

Best Regards,
Petr

> > +	/*
> > +	 * Use the current code if it is not modified by Kprobe
> > +	 * and it cannot be modified by ftrace.
> > +	 */
> > +	if (!kp && !faddr)
> >  		return addr;
> >  
> >  	/*
> > -	 *  Basically, kp->ainsn.insn has an original instruction.
> > -	 *  However, RIP-relative instruction can not do single-stepping
> > -	 *  at different place, __copy_instruction() tweaks the displacement of
> > -	 *  that instruction. In that case, we can't recover the instruction
> > -	 *  from the kp->ainsn.insn.
> > +	 * Basically, kp->ainsn.insn has an original instruction.
> > +	 * However, RIP-relative instruction can not do single-stepping
> > +	 * at different place, __copy_instruction() tweaks the displacement of
> > +	 * that instruction. In that case, we can't recover the instruction
> > +	 * from the kp->ainsn.insn.
> >  	 *
> > -	 *  On the other hand, kp->opcode has a copy of the first byte of
> > -	 *  the probed instruction, which is overwritten by int3. And
> > -	 *  the instruction at kp->addr is not modified by kprobes except
> > -	 *  for the first byte, we can recover the original instruction
> > -	 *  from it and kp->opcode.
> > +	 * On the other hand, in case on normal Kprobe, kp->opcode has a copy
> > +	 * of the first byte of the probed instruction, which is overwritten
> > +	 * by int3. And the instruction at kp->addr is not modified by kprobes
> > +	 * except for the first byte, we can recover the original instruction
> > +	 * from it and kp->opcode.
> > +	 *
> > +	 * In case of Kprobes using ftrace, we do not have a copy of
> > +	 * the original instruction. In fact, the ftrace location might
> > +	 * be modified at anytime and even could be in an inconsistent state.
> > +	 * Fortunately, we know that the original code is the ideal 5-byte
> > +	 * long NOP.
> >  	 */
> > -	memcpy(buf, kp->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
> > -	buf[0] = kp->opcode;
> > +	memcpy(buf, (void *)addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
> > +	if (faddr)
> > +		memcpy(buf, ideal_nops[NOP_ATOMIC5], MCOUNT_INSN_SIZE);
> > +	else
> > +		buf[0] = kp->opcode;
> >  	return (unsigned long)buf;
> >  }
> >  
> > 
> 
> 
> -- 
> Masami HIRAMATSU
> Software Platform Research Dept. Linux Technology Research Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu.pt@hitachi.com
> 
> 

Re: Re: [PATCH v2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Masami Hiramatsu]

(2015/02/03 20:38), Petr Mladek wrote:
> On Tue 2015-02-03 16:41:39, Masami Hiramatsu wrote:
>> (2015/02/03 2:48), 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().
>>>
>>> This patch makes Kprobe to use the ideal 5-byte NOP when the code
>>> can be modified by ftrace. It is the original instruction, see
>>> ftrace_make_nop() and ftrace_nop_replace().
>>>
>>> Note that we always need to use the NOP for ftrace locations. Kprobes
>>> do not block ftrace and the instruction might get modified at anytime.
>>> It might even be in an inconsistent state because it is modified step
>>> by step using the int3 breakpoint.
>>>
>>> The patch also fixes indentation of the touched comment.
>>>
>>> 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.
>>>
>>
>> This looks good to me :)
>>
>> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>
>> Ingo, could you merge this as an urgent fix?
> 
> Please, wait a bit, see below.
>  
>> Thank you!
>>
>>> Signed-off-by: Petr Mladek <pmladek@suse.cz>
>>> ---
>>>  arch/x86/kernel/kprobes/core.c | 42 ++++++++++++++++++++++++++++--------------
>>>  1 file changed, 28 insertions(+), 14 deletions(-)
>>>
>>> Changes against v1:
>>>
>>>   + always use 5-byte NOP for ftrace location
>>>   + fix indentation of the touched comment
>>>
>>> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
>>> index 98f654d466e5..2f464b56766a 100644
>>> --- a/arch/x86/kernel/kprobes/core.c
>>> +++ b/arch/x86/kernel/kprobes/core.c
>>> @@ -223,27 +223,41 @@ static unsigned long
>>>  __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
>>>  {
>>>  	struct kprobe *kp;
>>> +	unsigned long faddr;
>>>  
>>>  	kp = get_kprobe((void *)addr);
>>> -	/* There is no probe, return original address */
>>> -	if (!kp)
>>> +	faddr = ftrace_location(addr);
> 
> I have just realized that ftrace_location() might return another
> address if the given one points inside the ftrace_location.
> This situation is not checked by this patch. I am going to work
> on v3.

Ah, I see. But when it happens, something goes wrong, since
__recover_probed_insn() must be called with the address from
where an instruction starts (e.g. function entry or the
instruction boundary.)
So, we just need "BUG_ON(faddr && faddr != addr)" for that
case.

Thank you,

> 
> I knew that I should not have sent the patch just before leaving :-(
> 
> Best Regards,
> Petr
> 
>>> +	/*
>>> +	 * Use the current code if it is not modified by Kprobe
>>> +	 * and it cannot be modified by ftrace.
>>> +	 */
>>> +	if (!kp && !faddr)
>>>  		return addr;
>>>  
>>>  	/*
>>> -	 *  Basically, kp->ainsn.insn has an original instruction.
>>> -	 *  However, RIP-relative instruction can not do single-stepping
>>> -	 *  at different place, __copy_instruction() tweaks the displacement of
>>> -	 *  that instruction. In that case, we can't recover the instruction
>>> -	 *  from the kp->ainsn.insn.
>>> +	 * Basically, kp->ainsn.insn has an original instruction.
>>> +	 * However, RIP-relative instruction can not do single-stepping
>>> +	 * at different place, __copy_instruction() tweaks the displacement of
>>> +	 * that instruction. In that case, we can't recover the instruction
>>> +	 * from the kp->ainsn.insn.
>>>  	 *
>>> -	 *  On the other hand, kp->opcode has a copy of the first byte of
>>> -	 *  the probed instruction, which is overwritten by int3. And
>>> -	 *  the instruction at kp->addr is not modified by kprobes except
>>> -	 *  for the first byte, we can recover the original instruction
>>> -	 *  from it and kp->opcode.
>>> +	 * On the other hand, in case on normal Kprobe, kp->opcode has a copy
>>> +	 * of the first byte of the probed instruction, which is overwritten
>>> +	 * by int3. And the instruction at kp->addr is not modified by kprobes
>>> +	 * except for the first byte, we can recover the original instruction
>>> +	 * from it and kp->opcode.
>>> +	 *
>>> +	 * In case of Kprobes using ftrace, we do not have a copy of
>>> +	 * the original instruction. In fact, the ftrace location might
>>> +	 * be modified at anytime and even could be in an inconsistent state.
>>> +	 * Fortunately, we know that the original code is the ideal 5-byte
>>> +	 * long NOP.
>>>  	 */
>>> -	memcpy(buf, kp->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
>>> -	buf[0] = kp->opcode;
>>> +	memcpy(buf, (void *)addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
>>> +	if (faddr)
>>> +		memcpy(buf, ideal_nops[NOP_ATOMIC5], MCOUNT_INSN_SIZE);
>>> +	else
>>> +		buf[0] = kp->opcode;
>>>  	return (unsigned long)buf;
>>>  }
>>>  
>>>
>>
>>
>> -- 
>> 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/
> 


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

Re: [PATCH v2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Petr Mladek]

On Tue 2015-02-03 12:38:28, Petr Mladek wrote:
> On Tue 2015-02-03 16:41:39, Masami Hiramatsu wrote:
> > (2015/02/03 2:48), 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().
> > > 
> > > This patch makes Kprobe to use the ideal 5-byte NOP when the code
> > > can be modified by ftrace. It is the original instruction, see
> > > ftrace_make_nop() and ftrace_nop_replace().
> > > 
> > > Note that we always need to use the NOP for ftrace locations. Kprobes
> > > do not block ftrace and the instruction might get modified at anytime.
> > > It might even be in an inconsistent state because it is modified step
> > > by step using the int3 breakpoint.
> > > 
> > > The patch also fixes indentation of the touched comment.
> > > 
> > > 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.
> > > 
> > 
> > This looks good to me :)
> > 
> > Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > 
> > Ingo, could you merge this as an urgent fix?
> 
> Please, wait a bit, see below.
>  
> > Thank you!
> > 
> > > Signed-off-by: Petr Mladek <pmladek@suse.cz>
> > > ---
> > >  arch/x86/kernel/kprobes/core.c | 42 ++++++++++++++++++++++++++++--------------
> > >  1 file changed, 28 insertions(+), 14 deletions(-)
> > > 
> > > Changes against v1:
> > > 
> > >   + always use 5-byte NOP for ftrace location
> > >   + fix indentation of the touched comment
> > > 
> > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > > index 98f654d466e5..2f464b56766a 100644
> > > --- a/arch/x86/kernel/kprobes/core.c
> > > +++ b/arch/x86/kernel/kprobes/core.c
> > > @@ -223,27 +223,41 @@ static unsigned long
> > >  __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
> > >  {
> > >  	struct kprobe *kp;
> > > +	unsigned long faddr;
> > >  
> > >  	kp = get_kprobe((void *)addr);
> > > -	/* There is no probe, return original address */
> > > -	if (!kp)
> > > +	faddr = ftrace_location(addr);
> 
> I have just realized that ftrace_location() might return another
> address if the given one points inside the ftrace_location.
> This situation is not checked by this patch. I am going to work
> on v3.

Well, it should not happen after all because __recover_probed_insn() is called
only for already approved Kprobe locations and therefore only for the first
byte of the ftrace location. Any address inside the ftrace location is
refused earlier by check_kprobe_address_safe() that is called from
register_kprobe.

It means that it will never return another address here and the patch
can be used as is unless you want to be paranoid.

I am sorry for the rumor.

Best Regards,
Petr

> > > +	/*
> > > +	 * Use the current code if it is not modified by Kprobe
> > > +	 * and it cannot be modified by ftrace.
> > > +	 */
> > > +	if (!kp && !faddr)
> > >  		return addr;
> > >  
> > >  	/*
> > > -	 *  Basically, kp->ainsn.insn has an original instruction.
> > > -	 *  However, RIP-relative instruction can not do single-stepping
> > > -	 *  at different place, __copy_instruction() tweaks the displacement of
> > > -	 *  that instruction. In that case, we can't recover the instruction
> > > -	 *  from the kp->ainsn.insn.
> > > +	 * Basically, kp->ainsn.insn has an original instruction.
> > > +	 * However, RIP-relative instruction can not do single-stepping
> > > +	 * at different place, __copy_instruction() tweaks the displacement of
> > > +	 * that instruction. In that case, we can't recover the instruction
> > > +	 * from the kp->ainsn.insn.
> > >  	 *
> > > -	 *  On the other hand, kp->opcode has a copy of the first byte of
> > > -	 *  the probed instruction, which is overwritten by int3. And
> > > -	 *  the instruction at kp->addr is not modified by kprobes except
> > > -	 *  for the first byte, we can recover the original instruction
> > > -	 *  from it and kp->opcode.
> > > +	 * On the other hand, in case on normal Kprobe, kp->opcode has a copy
> > > +	 * of the first byte of the probed instruction, which is overwritten
> > > +	 * by int3. And the instruction at kp->addr is not modified by kprobes
> > > +	 * except for the first byte, we can recover the original instruction
> > > +	 * from it and kp->opcode.
> > > +	 *
> > > +	 * In case of Kprobes using ftrace, we do not have a copy of
> > > +	 * the original instruction. In fact, the ftrace location might
> > > +	 * be modified at anytime and even could be in an inconsistent state.
> > > +	 * Fortunately, we know that the original code is the ideal 5-byte
> > > +	 * long NOP.
> > >  	 */
> > > -	memcpy(buf, kp->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
> > > -	buf[0] = kp->opcode;
> > > +	memcpy(buf, (void *)addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
> > > +	if (faddr)
> > > +		memcpy(buf, ideal_nops[NOP_ATOMIC5], MCOUNT_INSN_SIZE);
> > > +	else
> > > +		buf[0] = kp->opcode;
> > >  	return (unsigned long)buf;
> > >  }
> > >  
> > > 
> > 
> > 
> > -- 
> > Masami HIRAMATSU
> > Software Platform Research Dept. Linux Technology Research Center
> > Hitachi, Ltd., Yokohama Research Laboratory
> > E-mail: masami.hiramatsu.pt@hitachi.com
> > 
> > 

Re: [PATCH v2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Ingo Molnar]

* Petr Mladek <pmladek@suse.cz> wrote:

> On Tue 2015-02-03 16:41:39, Masami Hiramatsu wrote:
> > (2015/02/03 2:48), 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().
> > > 
> > > This patch makes Kprobe to use the ideal 5-byte NOP when the code
> > > can be modified by ftrace. It is the original instruction, see
> > > ftrace_make_nop() and ftrace_nop_replace().
> > > 
> > > Note that we always need to use the NOP for ftrace locations. Kprobes
> > > do not block ftrace and the instruction might get modified at anytime.
> > > It might even be in an inconsistent state because it is modified step
> > > by step using the int3 breakpoint.
> > > 
> > > The patch also fixes indentation of the touched comment.
> > > 
> > > 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.
> > > 
> > 
> > This looks good to me :)
> > 
> > Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > 
> > Ingo, could you merge this as an urgent fix?
> 
> Please, wait a bit, see below.

Please [re]send the final patch with Masami's Ack added and 
me Cc:-ed.

Thanks,

	Ingo

Re: [PATCH v2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Petr Mladek]

On Wed 2015-02-18 22:22:05, Ingo Molnar wrote:
> 
> * Petr Mladek <pmladek@suse.cz> wrote:
> 
> > On Tue 2015-02-03 16:41:39, Masami Hiramatsu wrote:
> > > (2015/02/03 2:48), 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().
> > > > 
> > > > This patch makes Kprobe to use the ideal 5-byte NOP when the code
> > > > can be modified by ftrace. It is the original instruction, see
> > > > ftrace_make_nop() and ftrace_nop_replace().
> > > > 
> > > > Note that we always need to use the NOP for ftrace locations. Kprobes
> > > > do not block ftrace and the instruction might get modified at anytime.
> > > > It might even be in an inconsistent state because it is modified step
> > > > by step using the int3 breakpoint.
> > > > 
> > > > The patch also fixes indentation of the touched comment.
> > > > 
> > > > 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.
> > > > 
> > > 
> > > This looks good to me :)
> > > 
> > > Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > > 
> > > Ingo, could you merge this as an urgent fix?
> > 
> > Please, wait a bit, see below.
> 
> Please [re]send the final patch with Masami's Ack added and 
> me Cc:-ed.

The other problem was solved and acked in a separate patch. I'll rather
send it this way. I am sorry for the confusion.

Best Regards,
Petr

[PATCH 1/2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Petr Mladek]

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

This patch makes Kprobe to use the ideal 5-byte NOP when the code
can be modified by ftrace. It is the original instruction, see
ftrace_make_nop() and ftrace_nop_replace().

Note that we always need to use the NOP for ftrace locations. Kprobes
do not block ftrace and the instruction might get modified at anytime.
It might even be in an inconsistent state because it is modified step
by step using the int3 breakpoint.

The patch also fixes indentation of the touched comment.

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:

 static struct kprobe kp = {
 	.symbol_name	= "do_fork",
+	.offset = 5,
 };

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>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 arch/x86/kernel/kprobes/core.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 98f654d466e5..2f464b56766a 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -223,27 +223,41 @@ static unsigned long
 __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
 {
 	struct kprobe *kp;
+	unsigned long faddr;
 
 	kp = get_kprobe((void *)addr);
-	/* There is no probe, return original address */
-	if (!kp)
+	faddr = ftrace_location(addr);
+	/*
+	 * Use the current code if it is not modified by Kprobe
+	 * and it cannot be modified by ftrace.
+	 */
+	if (!kp && !faddr)
 		return addr;
 
 	/*
-	 *  Basically, kp->ainsn.insn has an original instruction.
-	 *  However, RIP-relative instruction can not do single-stepping
-	 *  at different place, __copy_instruction() tweaks the displacement of
-	 *  that instruction. In that case, we can't recover the instruction
-	 *  from the kp->ainsn.insn.
+	 * Basically, kp->ainsn.insn has an original instruction.
+	 * However, RIP-relative instruction can not do single-stepping
+	 * at different place, __copy_instruction() tweaks the displacement of
+	 * that instruction. In that case, we can't recover the instruction
+	 * from the kp->ainsn.insn.
 	 *
-	 *  On the other hand, kp->opcode has a copy of the first byte of
-	 *  the probed instruction, which is overwritten by int3. And
-	 *  the instruction at kp->addr is not modified by kprobes except
-	 *  for the first byte, we can recover the original instruction
-	 *  from it and kp->opcode.
+	 * On the other hand, in case on normal Kprobe, kp->opcode has a copy
+	 * of the first byte of the probed instruction, which is overwritten
+	 * by int3. And the instruction at kp->addr is not modified by kprobes
+	 * except for the first byte, we can recover the original instruction
+	 * from it and kp->opcode.
+	 *
+	 * In case of Kprobes using ftrace, we do not have a copy of
+	 * the original instruction. In fact, the ftrace location might
+	 * be modified at anytime and even could be in an inconsistent state.
+	 * Fortunately, we know that the original code is the ideal 5-byte
+	 * long NOP.
 	 */
-	memcpy(buf, kp->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
-	buf[0] = kp->opcode;
+	memcpy(buf, (void *)addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
+	if (faddr)
+		memcpy(buf, ideal_nops[NOP_ATOMIC5], MCOUNT_INSN_SIZE);
+	else
+		buf[0] = kp->opcode;
 	return (unsigned long)buf;
 }
 
-- 
1.8.5.6

[PATCH 2/2] kprobes/x86: Check for invalid ftrace location in __recover_probed_insn()

[Petr Mladek]

__recover_probed_insn() should always be called from an address where
an instructions starts. The check for ftrace_location() might help to
discover a potential inconsistency. Something goes terribly wrong when
an address inside the ftrace location is checked. Let's BUG() in this case.

Suggested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Petr Mladek <pmladek@suse.cz>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 arch/x86/kernel/kprobes/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 2f464b56766a..124577dcf768 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -228,6 +228,12 @@ __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
 	kp = get_kprobe((void *)addr);
 	faddr = ftrace_location(addr);
 	/*
+	 * Addresses inside the ftrace location are refused by
+	 * arch_check_ftrace_location(). Something went terribly wrong
+	 * if such an address is checked here.
+	 */
+	BUG_ON(faddr && faddr != addr);
+	/*
 	 * Use the current code if it is not modified by Kprobe
 	 * and it cannot be modified by ftrace.
 	 */
-- 
1.8.5.6

Re: [PATCH 2/2] kprobes/x86: Check for invalid ftrace location in __recover_probed_insn()

[Ingo Molnar]

* Petr Mladek <pmladek@suse.cz> wrote:

> __recover_probed_insn() should always be called from an address where
> an instructions starts. The check for ftrace_location() might help to
> discover a potential inconsistency. Something goes terribly wrong when
> an address inside the ftrace location is checked. Let's BUG() in this case.
> 
> Suggested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
>  arch/x86/kernel/kprobes/core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 2f464b56766a..124577dcf768 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -228,6 +228,12 @@ __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
>  	kp = get_kprobe((void *)addr);
>  	faddr = ftrace_location(addr);
>  	/*
> +	 * Addresses inside the ftrace location are refused by
> +	 * arch_check_ftrace_location(). Something went terribly wrong
> +	 * if such an address is checked here.
> +	 */
> +	BUG_ON(faddr && faddr != addr);

Crashing the system with a BUG_ON() makes users very sad. 
Please use a construct like:

	if (WARN_ON(faddr && faddr != addr))
		return gently;

I've picked up your first patch.

Thanks,

	Ingo

Re: [PATCH 1/2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Ingo Molnar]

* Petr Mladek <pmladek@suse.cz> 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.
>
> [...]

Had to drop this patch due to build failures on 32-bit x86:

arch/x86/kernel/kprobes/core.c:258:40: error: ‘MCOUNT_INSN_SIZE’ undeclared (first use in this function)

Thanks,

	Ingo

Re: [PATCH 1/2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Masami Hiramatsu]

(2015/02/20 19:26), Ingo Molnar wrote:
> 
> * Petr Mladek <pmladek@suse.cz> 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.
>>
>> [...]
> 
> Had to drop this patch due to build failures on 32-bit x86:
> 
> arch/x86/kernel/kprobes/core.c:258:40: error: ‘MCOUNT_INSN_SIZE’ undeclared (first use in this function)

Oops, MCOUNT_INSN_SIZE actually depends on CONFIG_FUNCTION_TRACER...
I think we can use 5 instead of that since we are copying NOP_ATOMIC5.

Thank you,

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

Re: [PATCH 1/2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Ingo Molnar]

* Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> (2015/02/20 19:26), Ingo Molnar wrote:
> > 
> > * Petr Mladek <pmladek@suse.cz> 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.
> >>
> >> [...]
> > 
> > Had to drop this patch due to build failures on 32-bit x86:
> > 
> > arch/x86/kernel/kprobes/core.c:258:40: error: ‘MCOUNT_INSN_SIZE’ undeclared (first use in this function)
> 
> Oops, MCOUNT_INSN_SIZE actually depends on 
> CONFIG_FUNCTION_TRACER... I think we can use 5 instead of 
> that since we are copying NOP_ATOMIC5.

Or just make the define more widely available? It's not 
like the size changes from disabling the function tracer.

Thanks,

	Ingo

Re: [PATCH 1/2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Masami Hiramatsu]

(2015/02/20 21:52), Ingo Molnar wrote:
> 
> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> (2015/02/20 19:26), Ingo Molnar wrote:
>>>
>>> * Petr Mladek <pmladek@suse.cz> 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.
>>>>
>>>> [...]
>>>
>>> Had to drop this patch due to build failures on 32-bit x86:
>>>
>>> arch/x86/kernel/kprobes/core.c:258:40: error: ‘MCOUNT_INSN_SIZE’ undeclared (first use in this function)
>>
>> Oops, MCOUNT_INSN_SIZE actually depends on 
>> CONFIG_FUNCTION_TRACER... I think we can use 5 instead of 
>> that since we are copying NOP_ATOMIC5.
> 
> Or just make the define more widely available? It's not 
> like the size changes from disabling the function tracer.

Yeah, it could be.
Actually, to tell the truth, if CONFIG_FUNCTION_TRACER=n,
ftrace_location() always returns 0, so the 2nd memcpy never be
executed (it should be disappeared by optimization).
Thus *this* issue is very local one, and I thought we'd better
fix this locally. :)

Thank you,

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

Re: [PATCH 1/2] kprobes/x86: Use 5-byte NOP when the code might be modified by ftrace

[Petr Mladek]

On Fri 2015-02-20 13:52:23, Ingo Molnar wrote:
> 
> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
> > (2015/02/20 19:26), Ingo Molnar wrote:
> > > 
> > > * Petr Mladek <pmladek@suse.cz> 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.
> > >>
> > >> [...]
> > > 
> > > Had to drop this patch due to build failures on 32-bit x86:
> > > 
> > > arch/x86/kernel/kprobes/core.c:258:40: error: ‘MCOUNT_INSN_SIZE’ undeclared (first use in this function)
> > 
> > Oops, MCOUNT_INSN_SIZE actually depends on 
> > CONFIG_FUNCTION_TRACER... I think we can use 5 instead of 
> > that since we are copying.

I'll use 5 for now.

> Or just make the define more widely available? It's not 
> like the size changes from disabling the function tracer.

I would do this in a separate patch that would consolidate all
NOP_ATOMIC5 users: ftrace, jump_labels, and kprobe. The number
5 is hardcoded more times in arch/x86/kernel/kprobes/core.c.
I'll put it into my TODO list.

Best Regards,
Petr