Re: [PATCH 08/15] x86/alternatives: Teach text_poke_bp() to emulate instructions

[Nadav Amit]

> On Jun 5, 2019, at 6:08 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> 
> In preparation for static_call support, teach text_poke_bp() to
> emulate instructions, including CALL.
> 
> The current text_poke_bp() takes a @handler argument which is used as
> a jump target when the temporary INT3 is hit by a different CPU.
> 
> When patching CALL instructions, this doesn't work because we'd miss
> the PUSH of the return address. Instead, teach poke_int3_handler() to
> emulate an instruction, typically the instruction we're patching in.
> 
> This fits almost all text_poke_bp() users, except
> arch_unoptimize_kprobe() which restores random text, and for that site
> we have to build an explicit emulate instruction.
> 
> Cc: Daniel Bristot de Oliveira <bristot@xxxxxxxxxx>
> Cc: Nadav Amit <namit@xxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
> arch/x86/include/asm/text-patching.h |    2 -
> arch/x86/kernel/alternative.c        |   47 ++++++++++++++++++++++++++---------
> arch/x86/kernel/jump_label.c         |    3 --
> arch/x86/kernel/kprobes/opt.c        |   11 +++++---
> 4 files changed, 46 insertions(+), 17 deletions(-)
> 
> --- a/arch/x86/include/asm/text-patching.h
> +++ b/arch/x86/include/asm/text-patching.h
> @@ -37,7 +37,7 @@ extern void text_poke_early(void *addr,
> extern void *text_poke(void *addr, const void *opcode, size_t len);
> extern void *text_poke_kgdb(void *addr, const void *opcode, size_t len);
> extern int poke_int3_handler(struct pt_regs *regs);
> -extern void text_poke_bp(void *addr, const void *opcode, size_t len, void *handler);
> +extern void text_poke_bp(void *addr, const void *opcode, size_t len, const void *emulate);
> extern int after_bootmem;
> extern __ro_after_init struct mm_struct *poking_mm;
> extern __ro_after_init unsigned long poking_addr;
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -921,19 +921,25 @@ static void do_sync_core(void *info)
> }
> 
> static bool bp_patching_in_progress;
> -static void *bp_int3_handler, *bp_int3_addr;
> +static const void *bp_int3_opcode, *bp_int3_addr;
> 
> int poke_int3_handler(struct pt_regs *regs)
> {
> +	long ip = regs->ip - INT3_INSN_SIZE + CALL_INSN_SIZE;
> +	struct opcode {
> +		u8 insn;
> +		s32 rel;
> +	} __packed opcode;
> +
> 	/*
> 	 * Having observed our INT3 instruction, we now must observe
> 	 * bp_patching_in_progress.
> 	 *
> -	 * 	in_progress = TRUE		INT3
> -	 * 	WMB				RMB
> -	 * 	write INT3			if (in_progress)
> +	 *	in_progress = TRUE		INT3
> +	 *	WMB				RMB
> +	 *	write INT3			if (in_progress)

I donât see what has changed in this chunkâ Whitespaces?

> 	 *
> -	 * Idem for bp_int3_handler.
> +	 * Idem for bp_int3_opcode.
> 	 */
> 	smp_rmb();
> 
> @@ -943,8 +949,21 @@ int poke_int3_handler(struct pt_regs *re
> 	if (user_mode(regs) || regs->ip != (unsigned long)bp_int3_addr)
> 		return 0;
> 
> -	/* set up the specified breakpoint handler */
> -	regs->ip = (unsigned long) bp_int3_handler;
> +	opcode = *(struct opcode *)bp_int3_opcode;
> +
> +	switch (opcode.insn) {
> +	case 0xE8: /* CALL */
> +		int3_emulate_call(regs, ip + opcode.rel);
> +		break;
> +
> +	case 0xE9: /* JMP */
> +		int3_emulate_jmp(regs, ip + opcode.rel);
> +		break;

Consider using RELATIVECALL_OPCODE and RELATIVEJUMP_OPCODE instead of the
constants (0xE8, 0xE9), just as you do later in the patch.