Subject: Re: [PATCH][v5] uprobes/x86: emulate push insns for uprobe on x86
From: Yonghong Song <yhs@fb.com>
To: <mingo@kernel.org>, <peterz@infradead.org>, <oleg@redhat.com>,
        <linux-kernel@vger.kernel.org>, <x86@kernel.org>
CC: <kernel-team@fb.com>
References: <20171201001202.3706564-1-yhs@fb.com>
Message-ID: <93935c11-c185-dd4d-912e-3be437781bb9@fb.com>
Date: Tue, 5 Dec 2017 16:33:46 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0)
 Gecko/20100101 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <20171201001202.3706564-1-yhs@fb.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2017 00:33:56.9853 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: ea0a2d48-4820-4e71-f8a6-08d53c4109e0
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR15MB1827
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-12-05_09:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 9311
Lines: 289

Hi, Ingo and Peter,

Could you take a look at this patch and if no objection
merge it into tip? This patch has been reviewed by
Oleg Nesterov.

Thanks!

Yonghong

On 11/30/17 4:12 PM, Yonghong Song wrote:
> 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>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> ---
>   arch/x86/include/asm/uprobes.h |   4 ++
>   arch/x86/kernel/uprobes.c      | 107 +++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 107 insertions(+), 4 deletions(-)
> 
> Changelogs:
> v4 -> v5:
>    . No code change from v4.
>    . Rebased on top of tip and added Reviewed-by from Oleg.
> v3 -> v4:
>    . Revert most of v3 change as 32bit emulation is not really working
>      on x86_64 platform as 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.
>