* [PATCH][v4] uprobes/x86: emulate push insns for uprobe on x86
@ 2017-11-15 17:59 Yonghong Song
2017-11-17 17:25 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2017-11-15 17:59 UTC (permalink / raw)
To: mingo, tglx, oleg, peterz, linux-kernel, x86, netdev, ast; +Cc: kernel-team
Uprobe is a tracing mechanism for userspace programs.
Typical uprobe will incur overhead of two traps.
First trap is caused by replaced trap insn, and
the second trap is to execute the original displaced
insn in user space.
To reduce the overhead, kernel provides hooks
for architectures to emulate the original insn
and skip the second trap. In x86, emulation
is done for certain branch insns.
This patch extends the emulation to "push <reg>"
insns. These insns are typical in the beginning
of the function. For example, bcc
in https://github.com/iovisor/bcc repo provides
tools to measure funclantency, detect memleak, etc.
The tools will place uprobes in the beginning of
function and possibly uretprobes at the end of function.
This patch is able to reduce the trap overhead for
uprobe from 2 to 1.
Without this patch, uretprobe will typically incur
three traps. With this patch, if the function starts
with "push" insn, the number of traps can be
reduced from 3 to 2.
An experiment was conducted on two local VMs,
fedora 26 64-bit VM and 32-bit VM, both 4 processors
and 4GB memory, booted with latest tip repo (and this patch).
The host is MacBook with intel i7 processor.
The test program looks like
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <sys/time.h>
static void test() __attribute__((noinline));
void test() {}
int main() {
struct timeval start, end;
gettimeofday(&start, NULL);
for (int i = 0; i < 1000000; i++) {
test();
}
gettimeofday(&end, NULL);
printf("%ld\n", ((end.tv_sec * 1000000 + end.tv_usec)
- (start.tv_sec * 1000000 + start.tv_usec)));
return 0;
}
The program is compiled without optimization, and
the first insn for function "test" is "push %rbp".
The host is relatively idle.
Before the test run, the uprobe is inserted as below for uprobe:
echo 'p <binary>:<test_func_offset>' > /sys/kernel/debug/tracing/uprobe_events
echo 1 > /sys/kernel/debug/tracing/events/uprobes/enable
and for uretprobe:
echo 'r <binary>:<test_func_offset>' > /sys/kernel/debug/tracing/uprobe_events
echo 1 > /sys/kernel/debug/tracing/events/uprobes/enable
Unit: microsecond(usec) per loop iteration
x86_64 W/ this patch W/O this patch
uprobe 1.55 3.1
uretprobe 2.0 3.6
x86_32 W/ this patch W/O this patch
uprobe 1.41 3.5
uretprobe 1.75 4.0
You can see that this patch significantly reduced the overhead,
50% for uprobe and 44% for uretprobe on x86_64, and even more
on x86_32.
Signed-off-by: Yonghong Song <yhs@fb.com>
---
arch/x86/include/asm/uprobes.h | 4 ++
arch/x86/kernel/uprobes.c | 107 +++++++++++++++++++++++++++++++++++++++--
2 files changed, 107 insertions(+), 4 deletions(-)
Changelogs:
v3 -> v4:
. Revert most of v3 change as 32bit emulation is not really working
on x86_64 platform as among other issues, function emulate_push_stack()
needs to account for 32bit app on 64bit platform.
A separate effort is ongoing to address this issue.
v2 -> v3:
. Do not emulate 32bit application on x86_64 platforms
v1 -> v2:
. Address Oleg's comments
diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 74f4c2f..d8bfa98 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -53,6 +53,10 @@ struct arch_uprobe {
u8 fixups;
u8 ilen;
} defparam;
+ struct {
+ u8 reg_offset; /* to the start of pt_regs */
+ u8 ilen;
+ } push;
};
};
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index a3755d2..85c7ef2 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -528,11 +528,11 @@ static int default_pre_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
return 0;
}
-static int push_ret_address(struct pt_regs *regs, unsigned long ip)
+static int emulate_push_stack(struct pt_regs *regs, unsigned long val)
{
unsigned long new_sp = regs->sp - sizeof_long();
- if (copy_to_user((void __user *)new_sp, &ip, sizeof_long()))
+ if (copy_to_user((void __user *)new_sp, &val, sizeof_long()))
return -EFAULT;
regs->sp = new_sp;
@@ -566,7 +566,7 @@ static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs
regs->ip += correction;
} else if (auprobe->defparam.fixups & UPROBE_FIX_CALL) {
regs->sp += sizeof_long(); /* Pop incorrect return address */
- if (push_ret_address(regs, utask->vaddr + auprobe->defparam.ilen))
+ if (emulate_push_stack(regs, utask->vaddr + auprobe->defparam.ilen))
return -ERESTART;
}
/* popf; tell the caller to not touch TF */
@@ -655,7 +655,7 @@ static bool branch_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
*
* But there is corner case, see the comment in ->post_xol().
*/
- if (push_ret_address(regs, new_ip))
+ if (emulate_push_stack(regs, new_ip))
return false;
} else if (!check_jmp_cond(auprobe, regs)) {
offs = 0;
@@ -665,6 +665,16 @@ static bool branch_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
return true;
}
+static bool push_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+ unsigned long *src_ptr = (void *)regs + auprobe->push.reg_offset;
+
+ if (emulate_push_stack(regs, *src_ptr))
+ return false;
+ regs->ip += auprobe->push.ilen;
+ return true;
+}
+
static int branch_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
BUG_ON(!branch_is_call(auprobe));
@@ -703,6 +713,10 @@ static const struct uprobe_xol_ops branch_xol_ops = {
.post_xol = branch_post_xol_op,
};
+static const struct uprobe_xol_ops push_xol_ops = {
+ .emulate = push_emulate_op,
+};
+
/* Returns -ENOSYS if branch_xol_ops doesn't handle this insn */
static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
{
@@ -750,6 +764,87 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
return 0;
}
+/* Returns -ENOSYS if push_xol_ops doesn't handle this insn */
+static int push_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
+{
+ u8 opc1 = OPCODE1(insn), reg_offset = 0;
+
+ if (opc1 < 0x50 || opc1 > 0x57)
+ return -ENOSYS;
+
+ if (insn->length > 2)
+ return -ENOSYS;
+ if (insn->length == 2) {
+ /* only support rex_prefix 0x41 (x64 only) */
+#ifdef CONFIG_X86_64
+ if (insn->rex_prefix.nbytes != 1 ||
+ insn->rex_prefix.bytes[0] != 0x41)
+ return -ENOSYS;
+
+ switch (opc1) {
+ case 0x50:
+ reg_offset = offsetof(struct pt_regs, r8);
+ break;
+ case 0x51:
+ reg_offset = offsetof(struct pt_regs, r9);
+ break;
+ case 0x52:
+ reg_offset = offsetof(struct pt_regs, r10);
+ break;
+ case 0x53:
+ reg_offset = offsetof(struct pt_regs, r11);
+ break;
+ case 0x54:
+ reg_offset = offsetof(struct pt_regs, r12);
+ break;
+ case 0x55:
+ reg_offset = offsetof(struct pt_regs, r13);
+ break;
+ case 0x56:
+ reg_offset = offsetof(struct pt_regs, r14);
+ break;
+ case 0x57:
+ reg_offset = offsetof(struct pt_regs, r15);
+ break;
+ }
+#else
+ return -ENOSYS;
+#endif
+ } else {
+ switch (opc1) {
+ case 0x50:
+ reg_offset = offsetof(struct pt_regs, ax);
+ break;
+ case 0x51:
+ reg_offset = offsetof(struct pt_regs, cx);
+ break;
+ case 0x52:
+ reg_offset = offsetof(struct pt_regs, dx);
+ break;
+ case 0x53:
+ reg_offset = offsetof(struct pt_regs, bx);
+ break;
+ case 0x54:
+ reg_offset = offsetof(struct pt_regs, sp);
+ break;
+ case 0x55:
+ reg_offset = offsetof(struct pt_regs, bp);
+ break;
+ case 0x56:
+ reg_offset = offsetof(struct pt_regs, si);
+ break;
+ case 0x57:
+ reg_offset = offsetof(struct pt_regs, di);
+ break;
+ }
+ }
+
+ auprobe->push.reg_offset = reg_offset;
+ auprobe->push.ilen = insn->length;
+ auprobe->ops = &push_xol_ops;
+ return 0;
+}
+
/**
* arch_uprobe_analyze_insn - instruction analysis including validity and fixups.
* @mm: the probed address space.
@@ -771,6 +866,10 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
if (ret != -ENOSYS)
return ret;
+ ret = push_setup_xol_ops(auprobe, &insn);
+ if (ret != -ENOSYS)
+ return ret;
+
/*
* Figure out which fixups default_post_xol_op() will need to perform,
* and annotate defparam->fixups accordingly.
--
2.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH][v4] uprobes/x86: emulate push insns for uprobe on x86
2017-11-15 17:59 [PATCH][v4] uprobes/x86: emulate push insns for uprobe on x86 Yonghong Song
@ 2017-11-17 17:25 ` Oleg Nesterov
2017-11-17 18:12 ` Yonghong Song
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2017-11-17 17:25 UTC (permalink / raw)
To: Yonghong Song
Cc: mingo, tglx, peterz, linux-kernel, x86, netdev, ast, kernel-team
On 11/15, Yonghong Song wrote:
>
> v3 -> v4:
> . Revert most of v3 change as 32bit emulation is not really working
> on x86_64 platform as among other issues, function emulate_push_stack()
> needs to account for 32bit app on 64bit platform.
> A separate effort is ongoing to address this issue.
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Please test your patch with the fix below, in this particular case the
TIF_IA32 check should be fine. Although this is not what we really want,
we should probably use user_64bit_mode(regs) which checks ->cs. But this
needs more changes and doesn't solve other problems (get_unmapped_area)
so I still can't decide what should we do right now...
Oleg.
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -516,7 +516,7 @@ struct uprobe_xol_ops {
static inline int sizeof_long(void)
{
- return in_ia32_syscall() ? 4 : 8;
+ return test_thread_flag(TIF_IA32) ? 4 : 8;
}
static int default_pre_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][v4] uprobes/x86: emulate push insns for uprobe on x86
2017-11-17 17:25 ` Oleg Nesterov
@ 2017-11-17 18:12 ` Yonghong Song
2017-11-20 16:41 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2017-11-17 18:12 UTC (permalink / raw)
To: Oleg Nesterov
Cc: mingo, tglx, peterz, linux-kernel, x86, netdev, ast, kernel-team
On 11/17/17 9:25 AM, Oleg Nesterov wrote:
> On 11/15, Yonghong Song wrote:
>>
>> v3 -> v4:
>> . Revert most of v3 change as 32bit emulation is not really working
>> on x86_64 platform as among other issues, function emulate_push_stack()
>> needs to account for 32bit app on 64bit platform.
>> A separate effort is ongoing to address this issue.
>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>
>
>
> Please test your patch with the fix below, in this particular case the
> TIF_IA32 check should be fine. Although this is not what we really want,
> we should probably use user_64bit_mode(regs) which checks ->cs. But this
> needs more changes and doesn't solve other problems (get_unmapped_area)
> so I still can't decide what should we do right now...
I tested the below change with my patch. On x86_64, both 64bit and 32bit
program can be uprobe emulated properly. On x86_32, however, there is a
compilation error like below:
In function ‘check_copy_size’,
inlined from ‘copy_to_user’ at
/home/yhs/work/tip/include/linux/uaccess.h:154:6,
inlined from ‘emulate_push_stack.isra.9’ at
/home/yhs/work/tip/arch/x86/kernel/uprobes.c:535:6:
/home/yhs/work/tip/include/linux/thread_info.h:139:4: error: call to
‘__bad_copy_from’ declared with attribute error: copy source size is too
small
__bad_copy_from();
Basically, test_thread_flag(TIF_IA32) returns 0 on x86_32 system.
>
> Oleg.
>
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -516,7 +516,7 @@ struct uprobe_xol_ops {
>
> static inline int sizeof_long(void)
> {
> - return in_ia32_syscall() ? 4 : 8;
> + return test_thread_flag(TIF_IA32) ? 4 : 8;
> }
>
> static int default_pre_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][v4] uprobes/x86: emulate push insns for uprobe on x86
2017-11-17 18:12 ` Yonghong Song
@ 2017-11-20 16:41 ` Oleg Nesterov
2017-11-20 18:25 ` Yonghong Song
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2017-11-20 16:41 UTC (permalink / raw)
To: Yonghong Song
Cc: mingo, tglx, peterz, linux-kernel, x86, netdev, ast, kernel-team
On 11/17, Yonghong Song wrote:
>
> On 11/17/17 9:25 AM, Oleg Nesterov wrote:
> >On 11/15, Yonghong Song wrote:
> >>
> >>v3 -> v4:
> >> . Revert most of v3 change as 32bit emulation is not really working
> >> on x86_64 platform as among other issues, function emulate_push_stack()
> >> needs to account for 32bit app on 64bit platform.
> >> A separate effort is ongoing to address this issue.
> >
> >Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> >
> >
> >
> >Please test your patch with the fix below, in this particular case the
> >TIF_IA32 check should be fine. Although this is not what we really want,
> >we should probably use user_64bit_mode(regs) which checks ->cs. But this
> >needs more changes and doesn't solve other problems (get_unmapped_area)
> >so I still can't decide what should we do right now...
>
> I tested the below change with my patch. On x86_64, both 64bit and 32bit
> program can be uprobe emulated properly.
Good, so your patch is fine.
> On x86_32, however, there is a
> compilation error like below:
Yes, yes, when I said "in this particular case" I meant x86_64 system only.
Sorry for confusion, I asked you to test this additional change just to
ensure that we didn't miss something and your patch has no problems with
32bit tasks on 64bit system, except those we need to fix anyway.
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][v4] uprobes/x86: emulate push insns for uprobe on x86
2017-11-20 16:41 ` Oleg Nesterov
@ 2017-11-20 18:25 ` Yonghong Song
2017-11-27 18:06 ` Yonghong Song
0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2017-11-20 18:25 UTC (permalink / raw)
To: Oleg Nesterov
Cc: mingo, tglx, peterz, linux-kernel, x86, netdev, ast, kernel-team
On 11/20/17 8:41 AM, Oleg Nesterov wrote:
> On 11/17, Yonghong Song wrote:
>>
>> On 11/17/17 9:25 AM, Oleg Nesterov wrote:
>>> On 11/15, Yonghong Song wrote:
>>>>
>>>> v3 -> v4:
>>>> . Revert most of v3 change as 32bit emulation is not really working
>>>> on x86_64 platform as among other issues, function emulate_push_stack()
>>>> needs to account for 32bit app on 64bit platform.
>>>> A separate effort is ongoing to address this issue.
>>>
>>> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>>>
>>>
>>>
>>> Please test your patch with the fix below, in this particular case the
>>> TIF_IA32 check should be fine. Although this is not what we really want,
>>> we should probably use user_64bit_mode(regs) which checks ->cs. But this
>>> needs more changes and doesn't solve other problems (get_unmapped_area)
>>> so I still can't decide what should we do right now...
>>
>> I tested the below change with my patch. On x86_64, both 64bit and 32bit
>> program can be uprobe emulated properly.
>
> Good, so your patch is fine.
Thanks!
>
>> On x86_32, however, there is a
>> compilation error like below:
>
> Yes, yes, when I said "in this particular case" I meant x86_64 system only.
>
> Sorry for confusion, I asked you to test this additional change just to
> ensure that we didn't miss something and your patch has no problems with
> 32bit tasks on 64bit system, except those we need to fix anyway.
Understood. I actually tried a little to see whether I could have a
simple way to fix 32bit compilation error without using ugly "#ifdef
CONFIG_X86_64". Maybe is_64bit_mm is a good choice. But we could defer
this until you have a comprehensive fix for 32bit app uprobe on 64bit
systems as there are multiple issues for this.
>
> Oleg.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][v4] uprobes/x86: emulate push insns for uprobe on x86
2017-11-20 18:25 ` Yonghong Song
@ 2017-11-27 18:06 ` Yonghong Song
0 siblings, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2017-11-27 18:06 UTC (permalink / raw)
To: Oleg Nesterov, mingo, peterz
Cc: tglx, linux-kernel, x86, netdev, ast, kernel-team
Hi, Ingo and Peter,
This patch has been reviewed by Oleg Nesterov. Could you
take a look and help merge it upstream?
Thanks!
Yonghong
On 11/20/17 10:25 AM, Yonghong Song wrote:
>
>
> On 11/20/17 8:41 AM, Oleg Nesterov wrote:
>> On 11/17, Yonghong Song wrote:
>>>
>>> On 11/17/17 9:25 AM, Oleg Nesterov wrote:
>>>> On 11/15, Yonghong Song wrote:
>>>>>
>>>>> v3 -> v4:
>>>>> . Revert most of v3 change as 32bit emulation is not really working
>>>>> on x86_64 platform as among other issues, function
>>>>> emulate_push_stack()
>>>>> needs to account for 32bit app on 64bit platform.
>>>>> A separate effort is ongoing to address this issue.
>>>>
>>>> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>>>>
>>>>
>>>>
>>>> Please test your patch with the fix below, in this particular case the
>>>> TIF_IA32 check should be fine. Although this is not what we really
>>>> want,
>>>> we should probably use user_64bit_mode(regs) which checks ->cs. But
>>>> this
>>>> needs more changes and doesn't solve other problems (get_unmapped_area)
>>>> so I still can't decide what should we do right now...
>>>
>>> I tested the below change with my patch. On x86_64, both 64bit and 32bit
>>> program can be uprobe emulated properly.
>>
>> Good, so your patch is fine.
>
> Thanks!
>
>>
>>> On x86_32, however, there is a
>>> compilation error like below:
>>
>> Yes, yes, when I said "in this particular case" I meant x86_64 system
>> only.
>>
>> Sorry for confusion, I asked you to test this additional change just to
>> ensure that we didn't miss something and your patch has no problems with
>> 32bit tasks on 64bit system, except those we need to fix anyway.
>
> Understood. I actually tried a little to see whether I could have a
> simple way to fix 32bit compilation error without using ugly "#ifdef
> CONFIG_X86_64". Maybe is_64bit_mm is a good choice. But we could defer
> this until you have a comprehensive fix for 32bit app uprobe on 64bit
> systems as there are multiple issues for this.
>
>>
>> Oleg.
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-27 18:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-15 17:59 [PATCH][v4] uprobes/x86: emulate push insns for uprobe on x86 Yonghong Song
2017-11-17 17:25 ` Oleg Nesterov
2017-11-17 18:12 ` Yonghong Song
2017-11-20 16:41 ` Oleg Nesterov
2017-11-20 18:25 ` Yonghong Song
2017-11-27 18:06 ` Yonghong Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).