> On Jun 5, 2019, at 6:08 AM, Peter Zijlstra <pet...@infradead.org> 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 <bris...@redhat.com>
> Cc: Nadav Amit <na...@vmware.com>
> Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
> ---
> 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.
