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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Aug 10, 2016 05:52, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>
> >>> 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.
>

The vm_event pause count is only decremented with vm_event responses. We only have one ring and one consumer of the ring, so even if you have multiple responses placed on the ring only on gets processed at any time.

Tamas

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