| Message ID | 1470814527-12929-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 10.08.16 at 09:35, <rcojocaru@bitdefender.com> 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). Jan
On 08/10/2016 01:12 PM, Jan Beulich wrote: >>>> On 10.08.16 at 09:35, <rcojocaru@bitdefender.com> 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. If there are unbalanced vcpu_pause() / vcpu_unpause() somewhere else in the code, that surely would be bug. Am I missing a scenario? Thanks, Razvan
>>> On 10.08.16 at 12:55, <rcojocaru@bitdefender.com> wrote: > On 08/10/2016 01:12 PM, Jan Beulich wrote: >>>>> On 10.08.16 at 09:35, <rcojocaru@bitdefender.com> 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
On Aug 10, 2016 05:52, "Jan Beulich" <JBeulich@suse.com> wrote: > > >>> On 10.08.16 at 12:55, <rcojocaru@bitdefender.com> wrote: > > On 08/10/2016 01:12 PM, Jan Beulich wrote: > >>>>> On 10.08.16 at 09:35, <rcojocaru@bitdefender.com> 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
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index 941345b..53cab90 100644 --- 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); + + /* * In some cases the response type needs extra handling, so here * we call the appropriate handlers. */
Vm_event_vcpu_pause() needs to use vcpu_pause_nosync() in order for the current vCPU to not get stuck. A consequence of this is that the custom vm_event response handlers will not always see the real vCPU state in v->arch.user_regs. This patch makes sure that the state is always synchronized in vm_event_resume, before any handlers have been called. This problem especially affects vm_event_set_registers(). Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> --- Changes since V1: - Only call sync_vcpu_execstate() when the vCPU is paused. --- xen/common/vm_event.c | 7 +++++++ 1 file changed, 7 insertions(+)