Re: [Xen-devel] [PATCH V2] common/vm_event: synchronize vCPU state in vm_event_resume()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.08.16 at 12:55, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 08/10/2016 01:12 PM, Jan Beulich wrote:
>>>>> On 10.08.16 at 09:35, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> --- a/xen/common/vm_event.c
>>> +++ b/xen/common/vm_event.c
>>> @@ -388,6 +388,13 @@ void vm_event_resume(struct domain *d, struct 
> vm_event_domain *ved)
>>>          v = d->vcpu[rsp.vcpu_id];
>>>  
>>>          /*
>>> +         * Make sure the vCPU state has been synchronized for the custom
>>> +         * handlers.
>>> +         */
>>> +        if ( atomic_read(&v->vm_event_pause_count) )
>>> +            sync_vcpu_execstate(v);
>> 
>> It seems to me that the latest with this change using a simple
>> atomic_t won't suffice anymore - you'd now really need to
>> make sure the user mode tools can't resume that vCPU behind
>> your back, which aiui can only be achieved by using a suitable
>> lock (perhaps a read/write one if reading the pause count is
>> more common than updating it).
> 
> I'm not sure how that would happen, vm_event_vcpu_pause() increments
> v->vm_event_pause_count, and then calls vcpu_pause_nosync(v), which then
> increments it's own counter.
> 
> So there doesn't seem to be a possibility of v->vm_event_pause_count
> being > 0 while the vCPU is unpaused, and there should also be no
> possibility that vm_event_resume() and vm_event_pause() could happen on
> the same vCPU at the same time.

Here you say "should", which is exactly what worries me: Is it
technically possible or not? If it is, there is the potential for a
race (with a buggy or malicious user mode tool). If there isn't,
calling out what it is that guarantees that in the commit
message (or even a code comment) would be much appreciated.

Jan


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