Re: [PATCH 1/3] x86/alternatives: Teach text_poke_bp() to emulate instructions

From: Masami Hiramatsu
Date: Fri Oct 04 2019 - 09:45:48 EST


Hi Peter,

On Thu, 3 Oct 2019 13:01:06 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Thu, Oct 03, 2019 at 10:27:51AM +0200, Peter Zijlstra wrote:
> > On Thu, Oct 03, 2019 at 02:00:50PM +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.
> > >
> > > OK, and in this case, I would like to change RELATIVEJUMP_OPCODE
> > > to JMP32_INSN_OPCODE for readability. (or at least
> > > making RELATIVEJUMP_OPCODE as an alias of JMP32_INSN_OPCODE)
> >
> > > > @@ -448,12 +447,18 @@ void arch_optimize_kprobes(struct list_h
> > > > void arch_unoptimize_kprobe(struct optimized_kprobe *op)
> > > > {
> > > > u8 insn_buff[RELATIVEJUMP_SIZE];
> > > > + u8 emulate_buff[RELATIVEJUMP_SIZE];
> > > >
> > > > /* Set int3 to first byte for kprobes */
> > > > insn_buff[0] = BREAKPOINT_INSTRUCTION;
> > > > memcpy(insn_buff + 1, op->optinsn.copied_insn, RELATIVE_ADDR_SIZE);
> > > > +
> > > > + emulate_buff[0] = RELATIVEJUMP_OPCODE;
> > > > + *(s32 *)(&emulate_buff[1]) = (s32)((long)op->optinsn.insn -
> > > > + ((long)op->kp.addr + RELATIVEJUMP_SIZE));
> >
> > I'm halfway through a patch introducing:
> >
> > union text_poke_insn {
> > u8 code[POKE_MAX_OPCODE_SUZE];
> > struct {
> > u8 opcode;
> > s32 disp;
> > } __attribute__((packed));
> > };
> >
> > to text-patching.h to unify all such custom unions we have all over the
> > place. I'll mob up the above in that.

I think it is good to unify such unions, but I meant above was, it was
also important to unify the opcode macro. Since poke_int3_handler()
clasifies the opcode by your *_INSN_OPCODE macro, it is natual to use
those opcode for text_poke_bp() interface.

> > > > +
> > > > text_poke_bp(op->kp.addr, insn_buff, RELATIVEJUMP_SIZE,
> > > > - op->optinsn.insn);
> > > > + emulate_buff);
> > > > }
> >
> > As argued in a previous thread, text_poke_bp() is broken when it changes
> > more than a single instruction at a time.
> >
> > Now, ISTR optimized kprobes does something like:
> >
> > poke INT3
>
> Hmm, it does this using text_poke(), but lacks a
> on_each_cpu(do_sync_core, NULL, 1), which I suppose is OK-ish IFF you do
> that synchronize_rcu_tasks() after it, but less so if you don't.
>
> That is, without either, you can't really tell if the kprobe is in
> effect or not.

Yes, it doesn't wait the change by design at this moment.

> Also, I think text_poke_bp(INT3) is broken, although I don't think
> anybody actually does that. Still, let me fix that.

OK.

>
> > synchronize_rcu_tasks() /* waits for all tasks to schedule
> > guarantees instructions after INT3
> > are unused */
> > install optimized probe /* overwrites multiple instrctions with
> > JMP.d32 */
> >
> > And the above then undoes that by:
> >
> > poke INT3 on top of the optimzed probe
> >
> > poke tail instructions back /* guaranteed safe because the
> > above INT3 poke ensures the
> > JMP.d32 instruction is unused */
> >
> > poke head byte back

Yes, anyway, the last poke should recover another INT3... (for kprobe)

> >
> > Is this correct? If so, we should probably put a comment in there
> > explaining how all this is unusual but safe.

OK.

Thank you,

--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>