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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 04/14/2016 06:40 PM, 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".

I believe at least part of the issue has to do with the comment on line
1013 from xen/arch/x86/hvm/emulate.c:

 994 static int hvmemul_cmpxchg(
 995     enum x86_segment seg,
 996     unsigned long offset,
 997     void *p_old,
 998     void *p_new,
 999     unsigned int bytes,
1000     struct x86_emulate_ctxt *ctxt)
1001 {
1002     struct hvm_emulate_ctxt *hvmemul_ctxt =
1003         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
1004
1005     if ( unlikely(hvmemul_ctxt->set_context) )
1006     {
1007         int rc = set_context_data(p_new, bytes);
1008
1009         if ( rc != X86EMUL_OKAY )
1010             return rc;
1011     }
1012
1013     /* Fix this in case the guest is really relying on r-m-w
atomicity. */
1014     return hvmemul_write(seg, offset, p_new, bytes, ctxt);
1015 }


Thanks,
Razvan

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