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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
<[email protected]> 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
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: [email protected]
On 2015/06/02 2:04, Andy Lutomirski wrote:
> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
> <[email protected]> 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: [email protected]
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 <[email protected]>
Thanks!
>
> Signed-off-by: Eugene Shatokhin <[email protected]>
> ---
> 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: [email protected]
On Mon, Jun 1, 2015 at 2:49 PM, Masami Hiramatsu
<[email protected]> wrote:
> On 2015/06/02 2:04, Andy Lutomirski wrote:
>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
>> <[email protected]> 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
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 <[email protected]>
> ---
> 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: [email protected]
* Masami Hiramatsu <[email protected]> wrote:
> On 2015/06/02 2:04, Andy Lutomirski wrote:
> > On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
> > <[email protected]> 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
* Eugene Shatokhin <[email protected]> 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 <[email protected]>
> ---
> 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
On 2015/06/02 6:57, Andy Lutomirski wrote:
> On Mon, Jun 1, 2015 at 2:49 PM, Masami Hiramatsu
> <[email protected]> wrote:
>> On 2015/06/02 2:04, Andy Lutomirski wrote:
>>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
>>> <[email protected]> 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: [email protected]
On 2015/06/02 14:44, Ingo Molnar wrote:
>
> * Masami Hiramatsu <[email protected]> wrote:
>
>> On 2015/06/02 2:04, Andy Lutomirski wrote:
>>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
>>> <[email protected]> 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: [email protected]
On Tue, Jun 2, 2015 at 2:46 PM, Masami Hiramatsu
<[email protected]> wrote:
> On 2015/06/02 14:44, Ingo Molnar wrote:
>>
>> * Masami Hiramatsu <[email protected]> wrote:
>>
>>> On 2015/06/02 2:04, Andy Lutomirski wrote:
>>>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
>>>> <[email protected]> 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
* Masami Hiramatsu <[email protected]> wrote:
> On 2015/06/02 14:44, Ingo Molnar wrote:
> >
> > * Masami Hiramatsu <[email protected]> wrote:
> >
> >> On 2015/06/02 2:04, Andy Lutomirski wrote:
> >>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
> >>> <[email protected]> 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
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 <[email protected]>
---
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
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
On 2015/06/03 6:55, Andy Lutomirski wrote:
> On Tue, Jun 2, 2015 at 2:46 PM, Masami Hiramatsu
> <[email protected]> wrote:
>> On 2015/06/02 14:44, Ingo Molnar wrote:
>>>
>>> * Masami Hiramatsu <[email protected]> wrote:
>>>
>>>> On 2015/06/02 2:04, Andy Lutomirski wrote:
>>>>> On Mon, Jun 1, 2015 at 9:32 AM, Eugene Shatokhin
>>>>> <[email protected]> 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: [email protected]