Re: [PATCH v3 0/6] Static calls

[Linus Torvalds <torvalds@linux-foundation.org>]

On Fri, Jan 11, 2019 at 7:15 AM Josh Poimboeuf <jpoimboe@redhat.com> 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://lkml.kernel.org/r/20181129122000.7fb4fb04@gandalf.local.home
>
> 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?

              Linus