On Sat, Jun 08, 2019 at 12:47:08AM +0900, Masami Hiramatsu wrote: > > 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. > > Hm, actually it doesn't restores randome text, since the first byte > must always be int3. As the function name means, it just unoptimizes > (jump based optprobe -> int3 based kprobe). > Anyway, that is not an issue. With this patch, optprobe must still work.
I thought it basically restored 5 bytes of original text (with no guarantee it is a single instruction, or even a complete instruction), with the first byte replaced with INT3. > > @@ -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; > > + > > + default: /* assume NOP */ > > Shouldn't we check whether it is actually NOP here? I was/am lazy and didn't want to deal with: arch/x86/include/asm/nops.h:#define GENERIC_NOP5_ATOMIC NOP_DS_PREFIX,GENERIC_NOP4 arch/x86/include/asm/nops.h:#define K8_NOP5_ATOMIC 0x66,K8_NOP4 arch/x86/include/asm/nops.h:#define K7_NOP5_ATOMIC NOP_DS_PREFIX,K7_NOP4 arch/x86/include/asm/nops.h:#define P6_NOP5_ATOMIC P6_NOP5 But maybe we should check for all the various NOP5 variants and BUG() on anything unexpected. > > + int3_emulate_jmp(regs, ip); > > + break; > > + } > > BTW, if we fix the length of patching always 5 bytes and allow user > to apply it only from/to jump/call/nop, we may be better to remove > "len" and rename it, something like "text_poke_branch" etc. I considered it; but was thinking we could still allow patching other instructions, we'd just have to extend the emulation in poke_int3_handler(). Then again, if/when we want to do that, we can also restore the @len argument again.