On Fri, 7 Jun 2019 10:20:13 +0200
Peter Zijlstra <pet...@infradead.org> wrote:
> On Fri, Jun 07, 2019 at 05:41:42AM +0000, Nadav Amit wrote:
>
> > > 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?
>
> Yep, my editor kept marking that stuff red (space before tab), which
> annoyed me enough to fix it.
>
> > > *
> > > - * 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.
>
> Those are private to kprobes..
>
> but I can do something like so:
>
> --- a/arch/x86/include/asm/text-patching.h
> +++ b/arch/x86/include/asm/text-patching.h
> @@ -48,8 +48,14 @@ static inline void int3_emulate_jmp(stru
> regs->ip = ip;
> }
>
> -#define INT3_INSN_SIZE 1
> -#define CALL_INSN_SIZE 5
> +#define INT3_INSN_SIZE 1
> +#define INT3_INSN_OPCODE 0xCC
> +
> +#define CALL_INSN_SIZE 5
> +#define CALL_INSN_OPCODE 0xE8
> +
> +#define JMP_INSN_SIZE 5
> +#define JMP_INSN_OPCODE 0xE9
>
> static inline void int3_emulate_push(struct pt_regs *regs, unsigned long val)
> {
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -952,11 +952,11 @@ int poke_int3_handler(struct pt_regs *re
> opcode = *(struct opcode *)bp_int3_opcode;
>
> switch (opcode.insn) {
> - case 0xE8: /* CALL */
> + case CALL_INSN_OPCODE:
> int3_emulate_call(regs, ip + opcode.rel);
> break;
>
> - case 0xE9: /* JMP */
> + case JMP_INSN_OPCODE:
> int3_emulate_jmp(regs, ip + opcode.rel);
> break;
>
This looks good. I don't want to make those opcode as private.
I would like to share it.
Thank you,
--
Masami Hiramatsu <mhira...@kernel.org>
