Re: [for-4.7] x86/emulate: synchronize LOCKed instruction emulation

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 04/14/16 07:35, Jan Beulich wrote:
>>>> Razvan Cojocaru <rcojocaru@bitdefender.com> 04/13/16 7:53 PM >>>
>> LOCK-prefixed instructions are currenly allowed to run in parallel
>> in x86_emulate(), which can lead the guest into an undefined state.
>> This patch fixes the issue.
> 
> ... by ... (read: Too brief a description)

I'll make it more detailed.

>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -25,6 +25,8 @@
>  >#include <asm/hvm/svm/svm.h>
>  >#include <asm/vm_event.h>
>  >
>> +DEFINE_PERCPU_RWLOCK_GLOBAL(emulate_locked_rwlock);
> 
> You should try hard to make this static.

I'll try.

>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -112,6 +112,7 @@
>  >#include <asm/ldt.h>
>  >#include <asm/x86_emulate.h>
>  >#include <asm/e820.h>
>> +#include <asm/hvm/emulate.h>
>  
> This header shouldn't be included here. You need to move the declarations
> elsewhere for them to be usable here.

Understood. Is there any place particularly fitting you could suggest?
Thanks!

>> @@ -1589,6 +1591,8 @@ x86_emulate(
>      >}
>   >done_prefixes:
>  >
>> +    ops->smp_lock(lock_prefix);
>> +
>      >if ( rex_prefix & REX_W )
>          >op_bytes = 8;
>  
> Already from the context it is obvious that the lock can be taken later.

I'll take it later.

>> @@ -2380,7 +2390,12 @@ x86_emulate(
>          >}
>          >/* Write back the memory destination with implicit LOCK prefix. */
>          >dst.val = src.val;
>> -        lock_prefix = 1;
>> +        if ( !lock_prefix )
>> +        {
>> +            ops->smp_unlock(lock_prefix);
>> +            lock_prefix = 1;
>> +            ops->smp_lock(lock_prefix);
>> +        }
>          
> This can't be correct: You need to take the write lock _before_ reading the
> memory location.

Right.

>> @@ -3859,6 +3874,9 @@ x86_emulate(
>   >done:
>      >_put_fpu();
>      >put_stub(stub);
>> +
>> +    ops->smp_unlock(lock_prefix);
> 
> And just like above from the context alone it is clear that the unlock can
> happen earlier. Locked regions should always as small as possible.

I'll move it up. I was just following returns.

>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -272,6 +272,8 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>  >
>      >TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id);
>  >
>> +    percpu_rwlock_resource_init(&d->arch.emulate_lock, emulate_locked_rwlock);
> 
> I cannot see how this would build on ARM.

I'll add separate functions for ARM and x86, with a no-op for ARM.

> Overall I can see why you want this, but what is the performance impact? After
> all you're basically making the emulator act like very old systems using a bus
> lock (i.e. a global one) instead of utilizing cacheline exclusive ownership. Plus
> you still don't (and cannot, afaict) deal with one LOCKed instruction getting
> emulated and another in parallel getting executed by the CPU without emulation
> (granted this shouldn't normally happen, but we also can't exclude that case).

I was under the impression that for reads, with the new percpu_rwlocks
the performance impact is close to zero, with some performance impact
for writes - but writes should, for one, be more infrequent, and then
the added safety factor should make up for that.

This indeed doesn't guard against LOCKed instructions being run in
parallel with and without emulation, however that is a case that should
almost never occur - at least not with introspection, where currently
all emulation happens as a result of EPT faults - so either all
instructions hitting a restricted page are emulated, or all ar run
directly. As long as all emulation can safely run in parallel and all
parallel non-emulation is also safe, it should be alright. But, yes,
this patch doesn't cover the case you're mentioning.

> With the above I'm also not convinced this is an issue that absolutely needs to
> be addressed in 4.7 - it's not a regression, and I suppose not a problem for
> guests that aren't under introspection.

The issue can also affect the pagetable and mmio hooks. Since Xen does
emulate outside of introspection, I thought that this has some immediate
importance. But obviously you know more about the code, so if you prefer
I can drop the "for-4.7" tag.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel