Re: [Xen-devel] [PATCH] x86/vm_event: fix race between vmx_vmexit_handler() and vm_event_resume()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.04.17 at 08:45, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 04/28/2017 09:25 AM, Jan Beulich wrote:
>>>>> On 27.04.17 at 19:31, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 27/04/17 09:52, Jan Beulich wrote:
>>>>>>> On 27.04.17 at 10:34, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> On 27/04/2017 09:01, Jan Beulich wrote:
>>>>>>>>> On 27.04.17 at 09:22, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>>> --- a/xen/common/vm_event.c
>>>>>>> +++ b/xen/common/vm_event.c
>>>>>>> @@ -357,6 +357,13 @@ void vm_event_resume(struct domain *d, struct 
> vm_event_domain *ved)
>>>>>>>  {
>>>>>>>      vm_event_response_t rsp;
>>>>>>>  
>>>>>>> +    /*
>>>>>>> +     * vm_event_resume() runs either from XEN_DOMCTL_VM_EVENT_OP_*, or
>>>>>>> +     * EVTCHN_send from the introspection consumer.  Both contexts are
>>>>>>> +     * guaranteed not to be the subject of vm_event responses.
>>>>>>> +     */
>>>>>>> +    ASSERT(d != current->domain);
>>>>>> What is this meant to guard against?
>>>>> Documentation, as much as anything else.  It took a long time to
>>>>> untangle the contexts involved here; I'm not convinced the logic is safe
>>>>> to run in context, and it certainly doesn't need to.
>>>> Well, as said - I think it is too strict now: You only need the vCPU
>>>> not be current afaict, and it really matters here which of the two
>>>> "current"-s you actually mean (and it looks to me as if you mean
>>>> the other one, guaranteeing register state to no longer be on the
>>>> stack).
>>>
>>> We don't know the vcpu at this point; that information comes out of the
>>> replies on the ring.
>>>
>>> We also may process multiple replies in this one loop.  In the worse
>>> case, we process one reply for every vcpu in d.
>>>
>>> If the ASSERT() were to be deferred until the middle of the request
>>> loop, we could ASSERT(v != current) for every vcpu in d, but that is
>>> identical to this single assertion.
>> 
>> No, it isn't. It would only be if you iterated over all vCPU-s of that
>> domain (which as you say may or may not happen).
> 
> I will modify to the code to whatever you and Andrew decide is best, but
> just in case this helps decide, in my experience iterating over all
> VCPUs here will happen more often than not - even looking at
> xen-access.c today, it poll()s with a timeout, processes all collected
> events (which, if they are sync events - and they always are with our
> application - there can be several of them only if they correspond to
> different VCPUs), and only then signals the event channel.
> 
> But there are of course cases in which less than all the VCPUs have
> placed events in the ring buffer.

So just to clarify - I'm not entirely opposed to the ASSERT() staying
as is, but then the comment next to it should clarify that it is slightly
more strict than absolutely necessary.

Jan


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