Re: [PATCH v3 0/6] Static calls

From: Nadav Amit
Date: Fri Jan 11 2019 - 14:20:21 EST


> On Jan 11, 2019, at 11:03 AM, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Jan 11, 2019 at 7:15 AM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>>> Now, in the int3 handler can you take the faulting RIP and search for it in
>>> the âstatic-callsâ table, writing the RIP+5 (offset) into R10 (return
>>> address) and the target into R11. You make the int3 handler to divert the
>>> code execution by changing pt_regs->rip to point to a new function that does:
>>>
>>> push R10
>>> jmp __x86_indirect_thunk_r11
>>>
>>> And then you are done. No?
>>
>> IIUC, that sounds pretty much like what Steven proposed:
>>
>> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2F20181129122000.7fb4fb04%40gandalf.local.home&amp;data=02%7C01%7Cnamit%40vmware.com%7Cd90f2aee03854a8da5dd08d677f7866e%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636828302371417454&amp;sdata=KEGVjCZbl5z7cCQX7F0%2FOaWfU7l%2Bvd2YDwZMXuOhpGI%3D&amp;reserved=0
>>
>> I liked the idea, BUT, how would it work for callee-saved PV ops? In
>> that case there's only one clobbered register to work with (rax).
>
> Actually, there's a much simpler model now that I think about it.
>
> The BP fixup just fixes up %rip to to point to "bp_int3_handler".
>
> And that's just a random text address set up by "text_poke_bp()".
>
> So how about the static call rewriting simply do this:
>
> - for each static call:
>
> 1) create a fixup code stub that does
>
> push $returnaddressforTHIScall
> jmp targetforTHIScall
>
> 2) do
>
> on_each_cpu(do_sync_core, NULL, 1);
>
> to make sure all CPU's see this generated code
>
> 3) do
>
> text_poke_bp(addr, newcode, newlen, generatedcode);
>
> Ta-daa! Done.
>
> In fact, it turns out that even the extra "do_sync_core()" in #2 is
> unnecessary, because taking the BP will be serializing on the CPU that
> takes it, so we can skip it.
>
> End result: the text_poke_bp() function will do the two do_sync_core
> IPI's that guarantee that by the time it returns, no other CPU is
> using the generated code any more, so it can be re-used for the next
> static call fixup.
>
> Notice? No odd emulation, no need to adjust the stack in the BP
> handler, just the regular "return to a different IP".
>
> Now, there is a nasty special case with that stub, though.
>
> So nasty thing with the whole "generate a stub for each call" case:
> because it's dynamic and because of the re-use of the stub, you could
> be in the situation where:
>
> CPU1 CPU2
> ---- ----
>
> generate a stub
> on_each_cpu(do_sync_core..)
> text_poke_bp()
> ...
>
> rewrite to BP
> trigger the BP
> return to the stub
> fun the first instruction of the stub
> *INTERRUPT causes rescheduling*
>
> on_each_cpu(do_sync_core..)
> rewrite to good instruction
> on_each_cpu(do_sync_core..)
>
> free or re-generate the stub
>
> !! The stub is still in use !!
>
> So that simple "just generate the stub dynamically" isn't so simple after all.
>
> But it turns out that that is really simple to handle too. How do we do that?
>
> We do that by giving the BP handler *two* code sequences, and we make
> the BP handler pick one depending on whether it is returning to a
> "interrupts disabled" or "interrupts enabled" case.
>
> So the BP handler does this:
>
> - if we're returning with interrupts disabled, pick the simple stub
>
> - if we're returning with interrupts enabled, clkear IF in the return
> %rflags, and pick a *slightly* more complex stub:
>
> push $returnaddressforTHIScall
> sti
> jmp targetforTHIScall
>
> and now the STI shadow will mean that this sequence is uninterruptible.
>
> So we'd not do complex emulation of the call instruction at BP time,
> but we'd do that *trivial* change at BP time.
>
> This seems simple, doesn't need any temporary registers at all, and
> doesn't need any extra stack magic. It literally needs just a trivial
> sequence in poke_int3_handler().
>
> The we'd change the end of poke_int3_handler() to do something like
> this instead:
>
> void *newip = bp_int3_handler;
> ..
> if (new == magic_static_call_bp_int3_handler) {
> if (regs->flags &X86_FLAGS_IF) {
> newip = magic_static_call_bp_int3_handler_sti;
> regs->flags &= ~X86_FLAGS_IF;
> }
> regs->ip = (unsigned long) newip;
> return 1;
>
> AAND now we're *really* done.
>
> Does anybody see any issues in this?

I think itâs a better articulated, more detailed version to the solution I
proposed in this thread (which also used a patched trampoline with STI+JMP).

So obviously I like it. ;-)