On Wed, Sep 29, 2021 at 11:59:51AM +0200, Peter Zijlstra wrote:
> On Wed, Sep 29, 2021 at 09:50:45AM +0100, Mark Rutland wrote:
> > On Wed, Sep 29, 2021 at 09:39:47AM +0200, Peter Zijlstra wrote:
> > > On Tue, Sep 28, 2021 at 06:36:37PM -0700, Josh Poimboeuf wrote:
>
> > > > + asm volatile("417: rdmsr\n"
> > > > + : EAX_EDX_RET(val, low, high)
> > > > + : "c" (msr));
> > > > + asm_volatile_goto(_ASM_EXTABLE(417b, %l[Efault]) :::: Efault);
> > >
> > > That's terrible :-) Could probably do with a comment, but might just
> > > work..
> >
> > The compiler is well within its rights to spill/restore/copy/shuffle
> > registers or modify memory between the two asm blocks (which it's liable
> > to do that when optimizing this after a few layers of inlining), and
> > skipping that would cause all sorts of undefined behaviour.
>
> Ah, but in this case it'll work irrespective of that (which is why we
> needs a comment!).
>
> This is because _ASM_EXTABLE only generates data for another section.
> There doesn't need to be code continuity between these two asm
> statements.
I think you've missed my point. It doesn't matter that the
asm_volatile_goto() doesn't contain code, and this is solely about the
*state* expected at entry/exit from each asm block being different.
The problem is that when the compiler encounters the
asm_volatile_goto(), it will generate a target for `Efault` expecting
the state of registers/stack/etc to be consistent with the state at
entry to the asm_volatile_goto() block. So if the compiler places any
register/memory manipulation between the asm volatile and the
asm_volatile_goto block, that expectation will be violated, since we
effectively branch from the first asm volatile block directly to the
label handed to the asm_volatile_goto block.
Consider the following pseudo asm example:
inline unsigned long read_magic_asm_thing(void)
{
// asm constraints allocates this into x3 for now
unsigned long ret = 3;
asm volatile(
"magic_insn_that_can_only_read_into x3\n"
"fault_insn: some_faulting_insn x3\n"
: [x3] "x3" (ret)
);
// compiler moves x3 into x0 because that's simpler for later
// code (in both the fall-through and branch case of the
// asm_volatile_goto()).
// Maybe it shuffles other things too, e.g. moving another
// variable into x3.
// This is generated expecting the register allocation at this
// instant in the code
asm_volatile_goto(extable_from_to(fault_isn, Efault));
// When not faulting, x0 is used here; this works correctly.
return ret;
Efault:
// When we take a fault from the first asm, the `ret` value is
// in x3, and we skipped the moves between the two asm blocks.
// This code was generated assuming those had happened (since
// that was the case at the start of the asm_volatile_goto(),
// and consumes x0 here, which contains garbage.
do_something_with(ret);
// Maybe this uses something that was moved into x3, but we have
// `ret` there instead.
something_else();
// Who knows if we even got here safely.
return whatever;
}
Thanks,
Mark.
