[PATCH 0/2] kprobes/x86: Allow "boost" for 10- and 11-byte instructions

[PATCH 0/2] kprobes/x86: Allow "boost" for 10- and 11-byte instructions

[Eugene Shatokhin]

Kprobes' "boost" feature allows to avoid single-stepping in some cases, along with its overhead. It is useful for the Kprobes that cannot be optimized for some reason.

Currently, "boost" cannot be applied to the instructions of 10 and 11 bytes in size, including some rather commonly used kinds of MOV.

The first of the two patches in this series fixes the code that checks if the jump needed for the boost fits in the insn slot (the conditional is too strict). This allows to apply "boost" to 10-byte instructions.

As a side effect of commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction size in the insn decoder"), the size of the instruction slot became 1 byte smaller, 15 bytes VS 16 bytes before that change. The second patch makes the size of each insn slot 16 bytes again (while keeping MAX_INSN_SIZE as 15). This allows to apply "boost" to 11-byte instructions as well.

I have checked that "boost" does happen for at least "movq $0x1,0x100(%rbx)" (48 c7 83 00 01 00 00 01 00 00 00) in the kernel 4.1-rc6 after these changes.

arch/x86/include/asm/kprobes.h | 1 +
arch/x86/kernel/kprobes/core.c | 2 +-
kernel/kprobes.c               | 8 ++++++--
3 files changed, 8 insertions(+), 3 deletions(-)

Regards,

Eugene

[PATCH 1/2] kprobes/x86: boost: Fix checking if there is enough room for a jump

[Eugene Shatokhin]

Kprobes' "boost" feature allows to avoid single-stepping in some cases,
along with its overhead. It needs a relative jump placed in the insn
slot right after the instruction. So the length of the instruction plus
5 (the length of the near relative unconditional jump) must not exceed
the length of the slot.

However, the code currently checks if that sum is strictly less than
the length of the slot. So "boost" cannot be used for the instructions
of 10 bytes in size (with MAX_INSN_SIZE == 15), like
"movabs $0x45525f4b434f4c43,%rax" (48 b8 43 4c 4f 43 4b 5f 52 45),
"movl $0x1,0xf8dd(%rip)"          (c7 05 dd f8 00 00 01 00 00 00), etc.

This patch fixes that conditional.

Signed-off-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
---
 arch/x86/kernel/kprobes/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 1deffe6..0a42b76 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -881,7 +881,7 @@ static void resume_execution(struct kprobe *p, struct pt_regs *regs,
 
 	if (p->ainsn.boostable == 0) {
 		if ((regs->ip > copy_ip) &&
-		    (regs->ip - copy_ip) + 5 < MAX_INSN_SIZE) {
+		    (regs->ip - copy_ip) + 5 <= MAX_INSN_SIZE) {
 			/*
 			 * These instructions can be executed directly if it
 			 * jumps back to correct address.
-- 
2.3.2

[PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Eugene Shatokhin]

Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
on x86.

As a side effect, the slots Kprobes use to store the instructions became
1 byte shorter. This is unfortunate because, for example, the Kprobes'
"boost" feature can not be used now for the instructions of length 11,
like a quite common kind of MOV:
* movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
* movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
and so on.

This patch makes the insn slots 16 bytes long, like they were before while
keeping MAX_INSN_SIZE intact.

Other tools may benefit from this change as well.

Signed-off-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
---
 arch/x86/include/asm/kprobes.h | 1 +
 arch/x86/kernel/kprobes/core.c | 2 +-
 kernel/kprobes.c               | 8 ++++++--
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
index 4421b5d..f3f0b4e 100644
--- a/arch/x86/include/asm/kprobes.h
+++ b/arch/x86/include/asm/kprobes.h
@@ -27,6 +27,7 @@
 #include <asm/insn.h>
 
 #define  __ARCH_WANT_KPROBES_INSN_SLOT
+#define KPROBE_INSN_SLOT_SIZE 16
 
 struct pt_regs;
 struct kprobe;
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 0a42b76..1067f90 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -881,7 +881,7 @@ static void resume_execution(struct kprobe *p, struct pt_regs *regs,
 
 	if (p->ainsn.boostable == 0) {
 		if ((regs->ip > copy_ip) &&
-		    (regs->ip - copy_ip) + 5 <= MAX_INSN_SIZE) {
+		    (regs->ip - copy_ip) + 5 <= KPROBE_INSN_SLOT_SIZE) {
 			/*
 			 * These instructions can be executed directly if it
 			 * jumps back to correct address.
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index c90e417..1dc074d 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -57,7 +57,6 @@
 #define KPROBE_HASH_BITS 6
 #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)
 
-
 /*
  * Some oddball architectures like 64bit powerpc have function descriptors
  * so this must be overridable.
@@ -90,6 +89,11 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
 static LIST_HEAD(kprobe_blacklist);
 
 #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
+
+#ifndef KPROBE_INSN_SLOT_SIZE
+#define KPROBE_INSN_SLOT_SIZE MAX_INSN_SIZE
+#endif
+
 /*
  * kprobe->ainsn.insn points to the copy of the instruction to be
  * single-stepped. x86_64, POWER4 and above have no-exec support and
@@ -135,7 +139,7 @@ struct kprobe_insn_cache kprobe_insn_slots = {
 	.alloc = alloc_insn_page,
 	.free = free_insn_page,
 	.pages = LIST_HEAD_INIT(kprobe_insn_slots.pages),
-	.insn_size = MAX_INSN_SIZE,
+	.insn_size = KPROBE_INSN_SLOT_SIZE,
 	.nr_garbage = 0,
 };
 static int collect_garbage_slots(struct kprobe_insn_cache *c);
-- 
2.3.2

Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Andy Lutomirski]

On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
<eugene.shatokhin@rosalab.ru> wrote:
> Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
> size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
> on x86.
>
> As a side effect, the slots Kprobes use to store the instructions became
> 1 byte shorter. This is unfortunate because, for example, the Kprobes'
> "boost" feature can not be used now for the instructions of length 11,
> like a quite common kind of MOV:
> * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
> * movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
> and so on.
>
> This patch makes the insn slots 16 bytes long, like they were before while
> keeping MAX_INSN_SIZE intact.
>
> Other tools may benefit from this change as well.

What is a "slot" and why does this patch make sense?  Naively, I'd
expect that the check you're patching is entirely unnecessary -- I
don't see what the size of the instruction being probed has to do with
the safety of executing it out of line and then jumping back.

Is there another magic 16 somewhere that this is enforcing that we
don't overrun?

--Andy

Re: [PATCH 0/2] kprobes/x86: Allow "boost" for 10- and 11-byte instructions

[Masami Hiramatsu]

On 2015/06/02 1:32, Eugene Shatokhin wrote:
> Kprobes' "boost" feature allows to avoid single-stepping in some cases, along with its overhead.
> It is useful for the Kprobes that cannot be optimized for some reason.
> 
> Currently, "boost" cannot be applied to the instructions of 10 and 11 bytes in size, including 
> some rather commonly used kinds of MOV.
> 
> The first of the two patches in this series fixes the code that checks if the jump needed for
> the boost fits in the insn slot (the conditional is too strict). This allows to apply "boost"
> to 10-byte instructions.
> 
> As a side effect of commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
> size in the insn decoder"), the size of the instruction slot became 1 byte smaller, 15 bytes
> VS 16 bytes before that change. The second patch makes the size of each insn slot 16 bytes
> again (while keeping MAX_INSN_SIZE as 15). This allows to apply "boost" to 11-byte
> instructions as well.
> 
> I have checked that "boost" does happen for at least "movq $0x1,0x100(%rbx)"
> (48 c7 83 00 01 00 00 01 00 00 00) in the kernel 4.1-rc6 after these changes.

Ah, I didn't expected that such long instruction existed without redundant prefixes.
I have some comment on that, but basically agree to support this.

Thank you!

-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Masami Hiramatsu]

On 2015/06/02 2:04, Andy Lutomirski wrote:
> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
> <eugene.shatokhin@rosalab.ru> wrote:
>> Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
>> size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
>> on x86.
>>
>> As a side effect, the slots Kprobes use to store the instructions became
>> 1 byte shorter. This is unfortunate because, for example, the Kprobes'
>> "boost" feature can not be used now for the instructions of length 11,
>> like a quite common kind of MOV:
>> * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
>> * movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
>> and so on.
>>
>> This patch makes the insn slots 16 bytes long, like they were before while
>> keeping MAX_INSN_SIZE intact.
>>
>> Other tools may benefit from this change as well.
> 
> What is a "slot" and why does this patch make sense?  Naively, I'd
> expect that the check you're patching is entirely unnecessary -- I
> don't see what the size of the instruction being probed has to do with
> the safety of executing it out of line and then jumping back.
> 
> Is there another magic 16 somewhere that this is enforcing that we
> don't overrun?

The kprobe-"booster" adds a jump back code (jmp <probed address + insn length>)
right after the instruction in the out-of-code buffer(slot). So we need at least
the insn-length + 5 bytes for the slot, it's the trick of the magic :)

Thank you,


> 
> --Andy
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 1/2] kprobes/x86: boost: Fix checking if there is enough room for a jump

[Masami Hiramatsu]

On 2015/06/02 1:32, Eugene Shatokhin wrote:
> Kprobes' "boost" feature allows to avoid single-stepping in some cases,
> along with its overhead. It needs a relative jump placed in the insn
> slot right after the instruction. So the length of the instruction plus
> 5 (the length of the near relative unconditional jump) must not exceed
> the length of the slot.
> 
> However, the code currently checks if that sum is strictly less than
> the length of the slot. So "boost" cannot be used for the instructions
> of 10 bytes in size (with MAX_INSN_SIZE == 15), like
> "movabs $0x45525f4b434f4c43,%rax" (48 b8 43 4c 4f 43 4b 5f 52 45),
> "movl $0x1,0xf8dd(%rip)"          (c7 05 dd f8 00 00 01 00 00 00), etc.
> 
> This patch fixes that conditional.

Looks good to me,

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

Thanks!

> 
> Signed-off-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> ---
>  arch/x86/kernel/kprobes/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 1deffe6..0a42b76 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -881,7 +881,7 @@ static void resume_execution(struct kprobe *p, struct pt_regs *regs,
>  
>  	if (p->ainsn.boostable == 0) {
>  		if ((regs->ip > copy_ip) &&
> -		    (regs->ip - copy_ip) + 5 < MAX_INSN_SIZE) {
> +		    (regs->ip - copy_ip) + 5 <= MAX_INSN_SIZE) {
>  			/*
>  			 * These instructions can be executed directly if it
>  			 * jumps back to correct address.
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Andy Lutomirski]

On Mon, Jun 1, 2015 at 2:49 PM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
> On 2015/06/02 2:04, Andy Lutomirski wrote:
>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
>> <eugene.shatokhin@rosalab.ru> wrote:
>>> Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
>>> size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
>>> on x86.
>>>
>>> As a side effect, the slots Kprobes use to store the instructions became
>>> 1 byte shorter. This is unfortunate because, for example, the Kprobes'
>>> "boost" feature can not be used now for the instructions of length 11,
>>> like a quite common kind of MOV:
>>> * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
>>> * movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
>>> and so on.
>>>
>>> This patch makes the insn slots 16 bytes long, like they were before while
>>> keeping MAX_INSN_SIZE intact.
>>>
>>> Other tools may benefit from this change as well.
>>
>> What is a "slot" and why does this patch make sense?  Naively, I'd
>> expect that the check you're patching is entirely unnecessary -- I
>> don't see what the size of the instruction being probed has to do with
>> the safety of executing it out of line and then jumping back.
>>
>> Is there another magic 16 somewhere that this is enforcing that we
>> don't overrun?
>
> The kprobe-"booster" adds a jump back code (jmp <probed address + insn length>)
> right after the instruction in the out-of-code buffer(slot). So we need at least
> the insn-length + 5 bytes for the slot, it's the trick of the magic :)

This still doesn't explain what a "slot" is.

I broke (?) something because I didn't see anything that looked
relevant that I was changing.  But now I see it:

-       .insn_size = MAX_INSN_SIZE,
+       .insn_size = KPROBE_INSN_SLOT_SIZE,

Would it make sense to clean this up?  insn_size isn't the size of an
instruction at all -- it's the size of a kprobe jump target in units
of sizeof(kprobe_opcode_t).

How about renaming insn_size to something sensible (and maybe
specifying the size in *bytes*)?

--Andy

Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Masami Hiramatsu]

On 2015/06/02 1:32, Eugene Shatokhin wrote:
> Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
> size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
> on x86.
> 
> As a side effect, the slots Kprobes use to store the instructions became
> 1 byte shorter. This is unfortunate because, for example, the Kprobes'
> "boost" feature can not be used now for the instructions of length 11,
> like a quite common kind of MOV:
> * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
> * movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
> and so on.
> 
> This patch makes the insn slots 16 bytes long, like they were before while
> keeping MAX_INSN_SIZE intact.
> 
> Other tools may benefit from this change as well.
> 
> Signed-off-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> ---
>  arch/x86/include/asm/kprobes.h | 1 +
>  arch/x86/kernel/kprobes/core.c | 2 +-
>  kernel/kprobes.c               | 8 ++++++--
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
> index 4421b5d..f3f0b4e 100644
> --- a/arch/x86/include/asm/kprobes.h
> +++ b/arch/x86/include/asm/kprobes.h
> @@ -27,6 +27,7 @@
>  #include <asm/insn.h>
>  
>  #define  __ARCH_WANT_KPROBES_INSN_SLOT

Please add a comment here why the slot size is bigger than MAX_INSN_SIZE.

> +#define KPROBE_INSN_SLOT_SIZE 16

And make sure that KPROBE_INSN_SLOT_SIZE >= MAX_INSN_SIZE.

Other parts are OK for me.

Thanks!

>  
>  struct pt_regs;
>  struct kprobe;
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 0a42b76..1067f90 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -881,7 +881,7 @@ static void resume_execution(struct kprobe *p, struct pt_regs *regs,
>  
>  	if (p->ainsn.boostable == 0) {
>  		if ((regs->ip > copy_ip) &&
> -		    (regs->ip - copy_ip) + 5 <= MAX_INSN_SIZE) {
> +		    (regs->ip - copy_ip) + 5 <= KPROBE_INSN_SLOT_SIZE) {
>  			/*
>  			 * These instructions can be executed directly if it
>  			 * jumps back to correct address.
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index c90e417..1dc074d 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -57,7 +57,6 @@
>  #define KPROBE_HASH_BITS 6
>  #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)
>  
> -
>  /*
>   * Some oddball architectures like 64bit powerpc have function descriptors
>   * so this must be overridable.
> @@ -90,6 +89,11 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
>  static LIST_HEAD(kprobe_blacklist);
>  
>  #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
> +
> +#ifndef KPROBE_INSN_SLOT_SIZE
> +#define KPROBE_INSN_SLOT_SIZE MAX_INSN_SIZE
> +#endif
> +
>  /*
>   * kprobe->ainsn.insn points to the copy of the instruction to be
>   * single-stepped. x86_64, POWER4 and above have no-exec support and
> @@ -135,7 +139,7 @@ struct kprobe_insn_cache kprobe_insn_slots = {
>  	.alloc = alloc_insn_page,
>  	.free = free_insn_page,
>  	.pages = LIST_HEAD_INIT(kprobe_insn_slots.pages),
> -	.insn_size = MAX_INSN_SIZE,
> +	.insn_size = KPROBE_INSN_SLOT_SIZE,
>  	.nr_garbage = 0,
>  };
>  static int collect_garbage_slots(struct kprobe_insn_cache *c);
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Ingo Molnar]

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

> On 2015/06/02 2:04, Andy Lutomirski wrote:
> > On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
> > <eugene.shatokhin@rosalab.ru> wrote:
> >> Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
> >> size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
> >> on x86.
> >>
> >> As a side effect, the slots Kprobes use to store the instructions became
> >> 1 byte shorter. This is unfortunate because, for example, the Kprobes'
> >> "boost" feature can not be used now for the instructions of length 11,
> >> like a quite common kind of MOV:
> >> * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
> >> * movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
> >> and so on.
> >>
> >> This patch makes the insn slots 16 bytes long, like they were before while
> >> keeping MAX_INSN_SIZE intact.
> >>
> >> Other tools may benefit from this change as well.
> > 
> > What is a "slot" and why does this patch make sense?  Naively, I'd
> > expect that the check you're patching is entirely unnecessary -- I
> > don't see what the size of the instruction being probed has to do with
> > the safety of executing it out of line and then jumping back.
> > 
> > Is there another magic 16 somewhere that this is enforcing that we
> > don't overrun?
> 
> The kprobe-"booster" adds a jump back code (jmp <probed address + insn length>) 
> right after the instruction in the out-of-code buffer(slot). So we need at least 
> the insn-length + 5 bytes for the slot, it's the trick of the magic :)

Please at minimum rename it to 'dynamic code buffer' or some other sensible name - 
the name 'slot' is pretty meaningless at best and misleading at worst.

Thanks,

	Ingo

Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Ingo Molnar]

* Eugene Shatokhin <eugene.shatokhin@rosalab.ru> wrote:

> Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
> size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
> on x86.
> 
> As a side effect, the slots Kprobes use to store the instructions became
> 1 byte shorter. This is unfortunate because, for example, the Kprobes'
> "boost" feature can not be used now for the instructions of length 11,
> like a quite common kind of MOV:
> * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
> * movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
> and so on.
> 
> This patch makes the insn slots 16 bytes long, like they were before while
> keeping MAX_INSN_SIZE intact.
> 
> Other tools may benefit from this change as well.
> 
> Signed-off-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
> ---
>  arch/x86/include/asm/kprobes.h | 1 +
>  arch/x86/kernel/kprobes/core.c | 2 +-
>  kernel/kprobes.c               | 8 ++++++--
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
> index 4421b5d..f3f0b4e 100644
> --- a/arch/x86/include/asm/kprobes.h
> +++ b/arch/x86/include/asm/kprobes.h
> @@ -27,6 +27,7 @@
>  #include <asm/insn.h>
>  
>  #define  __ARCH_WANT_KPROBES_INSN_SLOT
> +#define KPROBE_INSN_SLOT_SIZE 16

Naming aside, it's totally unacceptable to add a define like this with no 
explanation in the code at all.

> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -90,6 +89,11 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
>  static LIST_HEAD(kprobe_blacklist);
>  
>  #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
> +
> +#ifndef KPROBE_INSN_SLOT_SIZE
> +#define KPROBE_INSN_SLOT_SIZE MAX_INSN_SIZE
> +#endif

Ditto. There's no explanation, no way for an arch maintainer to find out whether 
to redefine this or not.

This adds some magic logic that is just as likely to break in the future as the 
old code did. This series needs to fix the primary cause (==poorly documented, 
illogical code), not just the symptom/bug.

Thanks,

	Ingo

Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Masami Hiramatsu]

On 2015/06/02 6:57, Andy Lutomirski wrote:
> On Mon, Jun 1, 2015 at 2:49 PM, Masami Hiramatsu
> <masami.hiramatsu.pt@hitachi.com> wrote:
>> On 2015/06/02 2:04, Andy Lutomirski wrote:
>>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
>>> <eugene.shatokhin@rosalab.ru> wrote:
>>>> Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
>>>> size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
>>>> on x86.
>>>>
>>>> As a side effect, the slots Kprobes use to store the instructions became
>>>> 1 byte shorter. This is unfortunate because, for example, the Kprobes'
>>>> "boost" feature can not be used now for the instructions of length 11,
>>>> like a quite common kind of MOV:
>>>> * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
>>>> * movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
>>>> and so on.
>>>>
>>>> This patch makes the insn slots 16 bytes long, like they were before while
>>>> keeping MAX_INSN_SIZE intact.
>>>>
>>>> Other tools may benefit from this change as well.
>>>
>>> What is a "slot" and why does this patch make sense?  Naively, I'd
>>> expect that the check you're patching is entirely unnecessary -- I
>>> don't see what the size of the instruction being probed has to do with
>>> the safety of executing it out of line and then jumping back.
>>>
>>> Is there another magic 16 somewhere that this is enforcing that we
>>> don't overrun?
>>
>> The kprobe-"booster" adds a jump back code (jmp <probed address + insn length>)
>> right after the instruction in the out-of-code buffer(slot). So we need at least
>> the insn-length + 5 bytes for the slot, it's the trick of the magic :)
> 
> This still doesn't explain what a "slot" is.
> 
> I broke (?) something because I didn't see anything that looked
> relevant that I was changing.  But now I see it:
> 
> -       .insn_size = MAX_INSN_SIZE,
> +       .insn_size = KPROBE_INSN_SLOT_SIZE,
> 
> Would it make sense to clean this up?  insn_size isn't the size of an
> instruction at all -- it's the size of a kprobe jump target in units
> of sizeof(kprobe_opcode_t).
> 
> How about renaming insn_size to something sensible (and maybe
> specifying the size in *bytes*)?

Ah, I see what you meant. Indeed, ".insn_size" is very easy to mislead, which
is the size of code buffer. At least it should be "insn_slot_size".
Since the code on the buffer(slot) must be executable, we need to use
module_alloc. That is why I introduced new allocation logic, and named it
"slot" for each part of the buffer, since module_alloc allocates pages not
a slab object.

Anyway, I'll rename it.

Thank you!

-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Masami Hiramatsu]

On 2015/06/02 14:44, Ingo Molnar wrote:
> 
> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> On 2015/06/02 2:04, Andy Lutomirski wrote:
>>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
>>> <eugene.shatokhin@rosalab.ru> wrote:
>>>> Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
>>>> size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
>>>> on x86.
>>>>
>>>> As a side effect, the slots Kprobes use to store the instructions became
>>>> 1 byte shorter. This is unfortunate because, for example, the Kprobes'
>>>> "boost" feature can not be used now for the instructions of length 11,
>>>> like a quite common kind of MOV:
>>>> * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
>>>> * movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
>>>> and so on.
>>>>
>>>> This patch makes the insn slots 16 bytes long, like they were before while
>>>> keeping MAX_INSN_SIZE intact.
>>>>
>>>> Other tools may benefit from this change as well.
>>>
>>> What is a "slot" and why does this patch make sense?  Naively, I'd
>>> expect that the check you're patching is entirely unnecessary -- I
>>> don't see what the size of the instruction being probed has to do with
>>> the safety of executing it out of line and then jumping back.
>>>
>>> Is there another magic 16 somewhere that this is enforcing that we
>>> don't overrun?
>>
>> The kprobe-"booster" adds a jump back code (jmp <probed address + insn length>) 
>> right after the instruction in the out-of-code buffer(slot). So we need at least 
>> the insn-length + 5 bytes for the slot, it's the trick of the magic :)
> 
> Please at minimum rename it to 'dynamic code buffer' or some other sensible name - 
> the name 'slot' is pretty meaningless at best and misleading at worst.

OK, would 'exec_buffer' is sensible? or just a 'code_buffer' is better?

Thank you,

-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Andy Lutomirski]

On Tue, Jun 2, 2015 at 2:46 PM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
> On 2015/06/02 14:44, Ingo Molnar wrote:
>>
>> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>>
>>> On 2015/06/02 2:04, Andy Lutomirski wrote:
>>>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
>>>> <eugene.shatokhin@rosalab.ru> wrote:
>>>>> Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
>>>>> size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
>>>>> on x86.
>>>>>
>>>>> As a side effect, the slots Kprobes use to store the instructions became
>>>>> 1 byte shorter. This is unfortunate because, for example, the Kprobes'
>>>>> "boost" feature can not be used now for the instructions of length 11,
>>>>> like a quite common kind of MOV:
>>>>> * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
>>>>> * movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
>>>>> and so on.
>>>>>
>>>>> This patch makes the insn slots 16 bytes long, like they were before while
>>>>> keeping MAX_INSN_SIZE intact.
>>>>>
>>>>> Other tools may benefit from this change as well.
>>>>
>>>> What is a "slot" and why does this patch make sense?  Naively, I'd
>>>> expect that the check you're patching is entirely unnecessary -- I
>>>> don't see what the size of the instruction being probed has to do with
>>>> the safety of executing it out of line and then jumping back.
>>>>
>>>> Is there another magic 16 somewhere that this is enforcing that we
>>>> don't overrun?
>>>
>>> The kprobe-"booster" adds a jump back code (jmp <probed address + insn length>)
>>> right after the instruction in the out-of-code buffer(slot). So we need at least
>>> the insn-length + 5 bytes for the slot, it's the trick of the magic :)
>>
>> Please at minimum rename it to 'dynamic code buffer' or some other sensible name -
>> the name 'slot' is pretty meaningless at best and misleading at worst.
>
> OK, would 'exec_buffer' is sensible? or just a 'code_buffer' is better?

redirected_code_buffer_size?

Anyway, regardless of the exact name, I also think it should be
measured in bytes instead of weird per-arch units.

--Andy

Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Ingo Molnar]

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

> On 2015/06/02 14:44, Ingo Molnar wrote:
> > 
> > * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> > 
> >> On 2015/06/02 2:04, Andy Lutomirski wrote:
> >>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
> >>> <eugene.shatokhin@rosalab.ru> wrote:
> >>>> Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
> >>>> size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
> >>>> on x86.
> >>>>
> >>>> As a side effect, the slots Kprobes use to store the instructions became
> >>>> 1 byte shorter. This is unfortunate because, for example, the Kprobes'
> >>>> "boost" feature can not be used now for the instructions of length 11,
> >>>> like a quite common kind of MOV:
> >>>> * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
> >>>> * movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
> >>>> and so on.
> >>>>
> >>>> This patch makes the insn slots 16 bytes long, like they were before while
> >>>> keeping MAX_INSN_SIZE intact.
> >>>>
> >>>> Other tools may benefit from this change as well.
> >>>
> >>> What is a "slot" and why does this patch make sense?  Naively, I'd
> >>> expect that the check you're patching is entirely unnecessary -- I
> >>> don't see what the size of the instruction being probed has to do with
> >>> the safety of executing it out of line and then jumping back.
> >>>
> >>> Is there another magic 16 somewhere that this is enforcing that we
> >>> don't overrun?
> >>
> >> The kprobe-"booster" adds a jump back code (jmp <probed address + insn length>) 
> >> right after the instruction in the out-of-code buffer(slot). So we need at least 
> >> the insn-length + 5 bytes for the slot, it's the trick of the magic :)
> > 
> > Please at minimum rename it to 'dynamic code buffer' or some other sensible 
> > name - the name 'slot' is pretty meaningless at best and misleading at worst.
> 
> OK, would 'exec_buffer' is sensible? or just a 'code_buffer' is better?

Yeah, 'code buffer' sounds good to me.

Thanks,

	Ingo

[PATCH v2 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Eugene Shatokhin]

Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
on x86.

As a side effect, the slots Kprobes use to store the instructions became
1 byte shorter. This is unfortunate because, for example, the Kprobes'
"boost" feature can not be used now for the instructions of length 11,
like a quite common kind of MOV:
* movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
* movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
and so on.

This patch makes the insn slots 16 bytes long, like they were before
while keeping MAX_INSN_SIZE intact.

Other tools may benefit from this change as well.

Changes in v2:
* Explained in the comments what KPROBE_INSN_SLOT_SIZE is and why it may
  be larger than MAX_INSN_SIZE.
* Added a compile-time check that KPROBE_INSN_SLOT_SIZE is not less than
  MAX_INSN_SIZE.

Signed-off-by: Eugene Shatokhin <eugene.shatokhin@rosalab.ru>
---
 arch/x86/include/asm/kprobes.h | 15 +++++++++++++++
 arch/x86/kernel/kprobes/core.c |  2 +-
 kernel/kprobes.c               | 20 ++++++++++++++++++--
 3 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
index 4421b5d..ab6e6a0 100644
--- a/arch/x86/include/asm/kprobes.h
+++ b/arch/x86/include/asm/kprobes.h
@@ -28,6 +28,21 @@
 
 #define  __ARCH_WANT_KPROBES_INSN_SLOT
 
+/*
+ * The size of the instruction slot is greater than the maximum length of
+ * an instruction (15 bytes) for Kprobes to be able to use "boost" for
+ * longer instructions.
+ *
+ * "Boost" allows to avoid single-stepping over an instruction if the Kprobe
+ * has no post handler. A jump to the next instruction is placed after the
+ * copied instruction in the slot for that to work.
+ *
+ * The length of the relative jump instruction is 5 bytes. With the slot
+ * size of 16, "boost" can be used for the instructions up to 11 bytes long,
+ * including rather common kinds of "MOV r/m64, imm32" (opcode 0xc7).
+ */
+#define KPROBE_INSN_SLOT_SIZE 16
+
 struct pt_regs;
 struct kprobe;
 
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 0a42b76..1067f90 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -881,7 +881,7 @@ static void resume_execution(struct kprobe *p, struct pt_regs *regs,
 
 	if (p->ainsn.boostable == 0) {
 		if ((regs->ip > copy_ip) &&
-		    (regs->ip - copy_ip) + 5 <= MAX_INSN_SIZE) {
+		    (regs->ip - copy_ip) + 5 <= KPROBE_INSN_SLOT_SIZE) {
 			/*
 			 * These instructions can be executed directly if it
 			 * jumps back to correct address.
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index c90e417..92788a4 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -57,7 +57,6 @@
 #define KPROBE_HASH_BITS 6
 #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)
 
-
 /*
  * Some oddball architectures like 64bit powerpc have function descriptors
  * so this must be overridable.
@@ -90,6 +89,23 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
 static LIST_HEAD(kprobe_blacklist);
 
 #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
+
+/*
+ * An instruction slot contains a copy of the probed instruction, relocated
+ * if needed. In some cases, it may be necessary to place additional
+ * instructions into that slot, so, the size of the slot may be larger than
+ * the maximum length of an instruction.
+ * Currently, the slots larger than MAX_INSN_SIZE may only be needed on x86
+ * to implement Kprobe "boost". Other architectures do not need to define
+ * KPROBE_INSN_SLOT_SIZE explicitly.
+ */
+#ifndef KPROBE_INSN_SLOT_SIZE
+#define KPROBE_INSN_SLOT_SIZE MAX_INSN_SIZE
+#endif
+#if (KPROBE_INSN_SLOT_SIZE < MAX_INSN_SIZE)
+#error "Size of an instruction slot must not be less than MAX_INSN_SIZE."
+#endif
+
 /*
  * kprobe->ainsn.insn points to the copy of the instruction to be
  * single-stepped. x86_64, POWER4 and above have no-exec support and
@@ -135,7 +151,7 @@ struct kprobe_insn_cache kprobe_insn_slots = {
 	.alloc = alloc_insn_page,
 	.free = free_insn_page,
 	.pages = LIST_HEAD_INIT(kprobe_insn_slots.pages),
-	.insn_size = MAX_INSN_SIZE,
+	.insn_size = KPROBE_INSN_SLOT_SIZE,
 	.nr_garbage = 0,
 };
 static int collect_garbage_slots(struct kprobe_insn_cache *c);
-- 
2.3.2

Re: [PATCH v2 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Jeff Epler]

On Wed, Jun 03, 2015 at 10:54:11AM +0300, Eugene Shatokhin wrote:
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -57,7 +57,6 @@
>  #define KPROBE_HASH_BITS 6
>  #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)
>  
> -
>  /*
>   * Some oddball architectures like 64bit powerpc have function descriptors
>   * so this must be overridable.

Unintentional whitespace change

Re: Re: [PATCH 2/2] kprobes/x86: Use 16 bytes for each instruction slot again

[Masami Hiramatsu]

On 2015/06/03 6:55, Andy Lutomirski wrote:
> On Tue, Jun 2, 2015 at 2:46 PM, Masami Hiramatsu
> <masami.hiramatsu.pt@hitachi.com> wrote:
>> On 2015/06/02 14:44, Ingo Molnar wrote:
>>>
>>> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>>>
>>>> On 2015/06/02 2:04, Andy Lutomirski wrote:
>>>>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
>>>>> <eugene.shatokhin@rosalab.ru> wrote:
>>>>>> Commit 91e5ed49fca0 ("x86/asm/decoder: Fix and enforce max instruction
>>>>>> size in the insn decoder") has changed MAX_INSN_SIZE from 16 to 15 bytes
>>>>>> on x86.
>>>>>>
>>>>>> As a side effect, the slots Kprobes use to store the instructions became
>>>>>> 1 byte shorter. This is unfortunate because, for example, the Kprobes'
>>>>>> "boost" feature can not be used now for the instructions of length 11,
>>>>>> like a quite common kind of MOV:
>>>>>> * movq $0xffffffffffffffff,-0x3fe8(%rax) (48 c7 80 18 c0 ff ff ff ff ff ff)
>>>>>> * movq $0x0,0x88(%rdi)                   (48 c7 87 88 00 00 00 00 00 00 00)
>>>>>> and so on.
>>>>>>
>>>>>> This patch makes the insn slots 16 bytes long, like they were before while
>>>>>> keeping MAX_INSN_SIZE intact.
>>>>>>
>>>>>> Other tools may benefit from this change as well.
>>>>>
>>>>> What is a "slot" and why does this patch make sense?  Naively, I'd
>>>>> expect that the check you're patching is entirely unnecessary -- I
>>>>> don't see what the size of the instruction being probed has to do with
>>>>> the safety of executing it out of line and then jumping back.
>>>>>
>>>>> Is there another magic 16 somewhere that this is enforcing that we
>>>>> don't overrun?
>>>>
>>>> The kprobe-"booster" adds a jump back code (jmp <probed address + insn length>)
>>>> right after the instruction in the out-of-code buffer(slot). So we need at least
>>>> the insn-length + 5 bytes for the slot, it's the trick of the magic :)
>>>
>>> Please at minimum rename it to 'dynamic code buffer' or some other sensible name -
>>> the name 'slot' is pretty meaningless at best and misleading at worst.
>>
>> OK, would 'exec_buffer' is sensible? or just a 'code_buffer' is better?
> 
> redirected_code_buffer_size?
> 
> Anyway, regardless of the exact name, I also think it should be
> measured in bytes instead of weird per-arch units.

OK, will try to use u8 and void *.

Thanks!

-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com