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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/04/16 16:40, Jan Beulich wrote:
>>>> Razvan Cojocaru <rcojocaru@bitdefender.com> 04/14/16 1:43 PM >>>
>> On 04/14/2016 01:35 PM, David Vrabel wrote:
>>> On 13/04/16 13:26, Razvan Cojocaru wrote:
>>>> 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.
>>> Is this sufficient?  What if another VCPU is running on another PCPU and
>>> doing an unemulated LOCK-prefixed instruction to the same memory address?
>>>
>>> This other VCPU could be for another domain (or Xen for that matter).
>> The patch is only sufficient for parallel runs of emulated instructions,
>> as previously stated. It is, however, able to prevent nasty guest lockups.
>>
>> This is what happened in a previous thread where I was hunting down the
>> issue and initially thought that the xen-access.c model was broken when
>> used with emulation, and even proceeded to check that the ring buffer
>> memory accesses are synchronized properly. They were alright, the
>> problem was in fact LOCKed instruction emulation happening in parallel,
>> i.e. a race condition there.
>>
>> This is less obvious if we signal that vm_event responses are available
>> immediately after processing each one (which greatly reduces the chances
>> of a race happening), and more obvious with guests that have 2 (or more)
>> VCPUs where all of them are paused waiting for a vm_event reply, and all
>> of them are woken up at the same time, after processing all of the
>> events, and asked to emulate.
>>
>> I do believe that somewhere in Xen emulating in this manner could occur,
>> so I hope to make emulation generally safer.
>>
>> As for not fixing the _whole_ issue, as Jan has rightly pointed out,
>> that's a rather difficult thing to do.
> To be honest, just having remembered that we do the write back for locked
> instructions using CMPXCHG, I'd first of all like to see a proper description
> of "the _whole_ issue".

All emulated instructions with a lock prefix end up calling into
hvmemul_cmpxchg()

I suspect the issue is to do with the implementation of
hvmemul_cmpxchg(), which contains a TODO from 2008 of

/* Fix this in case the guest is really relying on r-m-w atomicity. */

which, amongst other things, never updates *p_old.


Short of having the instruction emulator convert any locked instruction
into a stub, I can't think of a solution which works for both emulated
and non-emulated instructions.

~Andrew

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