| Message ID | 52492e7b44f311b09e9a433f2e5a2ba607a85c78.1590028160.git.tamas@tklengyel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vm_event: fix race-condition when disabling monitor events | expand |
On Wed, May 20, 2020 at 08:31:53PM -0600, Tamas K Lengyel wrote: > Perform sanity checking when shutting vm_event down to determine whether > it is safe to do so. Error out with -EAGAIN in case pending operations > have been found for the domain. > > Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com> > --- > xen/arch/x86/vm_event.c | 23 +++++++++++++++++++++++ > xen/common/vm_event.c | 17 ++++++++++++++--- > xen/include/asm-arm/vm_event.h | 7 +++++++ > xen/include/asm-x86/vm_event.h | 2 ++ > 4 files changed, 46 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c > index 848d69c1b0..a23aadc112 100644 > --- a/xen/arch/x86/vm_event.c > +++ b/xen/arch/x86/vm_event.c > @@ -297,6 +297,29 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp) > }; > } > > +bool vm_event_check_pending_op(const struct vcpu *v) > +{ > + struct monitor_write_data *w = &v->arch.vm_event->write_data; const > + > + if ( !v->arch.vm_event->sync_event ) > + return false; > + > + if ( w->do_write.cr0 ) > + return true; > + if ( w->do_write.cr3 ) > + return true; > + if ( w->do_write.cr4 ) > + return true; > + if ( w->do_write.msr ) > + return true; > + if ( v->arch.vm_event->set_gprs ) > + return true; > + if ( v->arch.vm_event->emulate_flags ) > + return true; Can you please group this into a single if, ie: if ( w->do_write.cr0 || w->do_write.cr3 || ... ) return true; > + > + return false; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c > index 127f2d58f1..2df327a42c 100644 > --- a/xen/common/vm_event.c > +++ b/xen/common/vm_event.c > @@ -183,6 +183,7 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) > if ( vm_event_check_ring(ved) ) > { > struct vcpu *v; > + bool pending_op = false; > > spin_lock(&ved->lock); > > @@ -192,9 +193,6 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) > return -EBUSY; > } > > - /* Free domU's event channel and leave the other one unbound */ > - free_xen_event_channel(d, ved->xen_port); > - > /* Unblock all vCPUs */ > for_each_vcpu ( d, v ) > { > @@ -203,8 +201,21 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) > vcpu_unpause(v); > ved->blocked--; > } > + > + if ( vm_event_check_pending_op(v) ) > + pending_op = true; You could just do: pending_op |= vm_event_check_pending_op(v); and avoid the initialization of pending_op above. Or alternatively: if ( !pending_op && vm_event_check_pending_op(v) ) pending_op = true; Which avoid repeated calls to vm_event_check_pending_op when at least one vCPU is known to be busy. > } > > + /* vm_event ops are still pending until vCPUs get scheduled */ > + if ( pending_op ) > + { > + spin_unlock(&ved->lock); > + return -EAGAIN; What happens when this gets called from vm_event_cleanup? AFAICT the result there is ignored, and could leak the vm_event allocated memory? Thanks, Roger.
On 02.06.2020 13:47, Roger Pau Monné wrote: > On Wed, May 20, 2020 at 08:31:53PM -0600, Tamas K Lengyel wrote: >> Perform sanity checking when shutting vm_event down to determine whether >> it is safe to do so. Error out with -EAGAIN in case pending operations >> have been found for the domain. >> >> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com> >> --- >> xen/arch/x86/vm_event.c | 23 +++++++++++++++++++++++ >> xen/common/vm_event.c | 17 ++++++++++++++--- >> xen/include/asm-arm/vm_event.h | 7 +++++++ >> xen/include/asm-x86/vm_event.h | 2 ++ >> 4 files changed, 46 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c >> index 848d69c1b0..a23aadc112 100644 >> --- a/xen/arch/x86/vm_event.c >> +++ b/xen/arch/x86/vm_event.c >> @@ -297,6 +297,29 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp) >> }; >> } >> >> +bool vm_event_check_pending_op(const struct vcpu *v) >> +{ >> + struct monitor_write_data *w = &v->arch.vm_event->write_data; > > const > >> + >> + if ( !v->arch.vm_event->sync_event ) >> + return false; >> + >> + if ( w->do_write.cr0 ) >> + return true; >> + if ( w->do_write.cr3 ) >> + return true; >> + if ( w->do_write.cr4 ) >> + return true; >> + if ( w->do_write.msr ) >> + return true; >> + if ( v->arch.vm_event->set_gprs ) >> + return true; >> + if ( v->arch.vm_event->emulate_flags ) >> + return true; > > Can you please group this into a single if, ie: > > if ( w->do_write.cr0 || w->do_write.cr3 || ... ) > return true; > >> + >> + return false; >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c >> index 127f2d58f1..2df327a42c 100644 >> --- a/xen/common/vm_event.c >> +++ b/xen/common/vm_event.c >> @@ -183,6 +183,7 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) >> if ( vm_event_check_ring(ved) ) >> { >> struct vcpu *v; >> + bool pending_op = false; >> >> spin_lock(&ved->lock); >> >> @@ -192,9 +193,6 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) >> return -EBUSY; >> } >> >> - /* Free domU's event channel and leave the other one unbound */ >> - free_xen_event_channel(d, ved->xen_port); >> - >> /* Unblock all vCPUs */ >> for_each_vcpu ( d, v ) >> { >> @@ -203,8 +201,21 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) >> vcpu_unpause(v); >> ved->blocked--; >> } >> + >> + if ( vm_event_check_pending_op(v) ) >> + pending_op = true; > > You could just do: > > pending_op |= vm_event_check_pending_op(v); > > and avoid the initialization of pending_op above. The initialization has to stay, or it couldn't be |= afaict. > Or alternatively: > > if ( !pending_op && vm_event_check_pending_op(v) ) > pending_op = true; > > Which avoid repeated calls to vm_event_check_pending_op when at least > one vCPU is known to be busy. if ( !pending_op ) pending_op = vm_event_check_pending_op(v); ? Jan
On Tue, Jun 2, 2020 at 5:47 AM Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Wed, May 20, 2020 at 08:31:53PM -0600, Tamas K Lengyel wrote: > > Perform sanity checking when shutting vm_event down to determine whether > > it is safe to do so. Error out with -EAGAIN in case pending operations > > have been found for the domain. > > > > Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com> > > --- > > xen/arch/x86/vm_event.c | 23 +++++++++++++++++++++++ > > xen/common/vm_event.c | 17 ++++++++++++++--- > > xen/include/asm-arm/vm_event.h | 7 +++++++ > > xen/include/asm-x86/vm_event.h | 2 ++ > > 4 files changed, 46 insertions(+), 3 deletions(-) > > > > diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c > > index 848d69c1b0..a23aadc112 100644 > > --- a/xen/arch/x86/vm_event.c > > +++ b/xen/arch/x86/vm_event.c > > @@ -297,6 +297,29 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp) > > }; > > } > > > > +bool vm_event_check_pending_op(const struct vcpu *v) > > +{ > > + struct monitor_write_data *w = &v->arch.vm_event->write_data; > > const > > > + > > + if ( !v->arch.vm_event->sync_event ) > > + return false; > > + > > + if ( w->do_write.cr0 ) > > + return true; > > + if ( w->do_write.cr3 ) > > + return true; > > + if ( w->do_write.cr4 ) > > + return true; > > + if ( w->do_write.msr ) > > + return true; > > + if ( v->arch.vm_event->set_gprs ) > > + return true; > > + if ( v->arch.vm_event->emulate_flags ) > > + return true; > > Can you please group this into a single if, ie: > > if ( w->do_write.cr0 || w->do_write.cr3 || ... ) > return true; > > > + > > + return false; > > +} > > + > > /* > > * Local variables: > > * mode: C > > diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c > > index 127f2d58f1..2df327a42c 100644 > > --- a/xen/common/vm_event.c > > +++ b/xen/common/vm_event.c > > @@ -183,6 +183,7 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) > > if ( vm_event_check_ring(ved) ) > > { > > struct vcpu *v; > > + bool pending_op = false; > > > > spin_lock(&ved->lock); > > > > @@ -192,9 +193,6 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) > > return -EBUSY; > > } > > > > - /* Free domU's event channel and leave the other one unbound */ > > - free_xen_event_channel(d, ved->xen_port); > > - > > /* Unblock all vCPUs */ > > for_each_vcpu ( d, v ) > > { > > @@ -203,8 +201,21 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) > > vcpu_unpause(v); > > ved->blocked--; > > } > > + > > + if ( vm_event_check_pending_op(v) ) > > + pending_op = true; > > You could just do: > > pending_op |= vm_event_check_pending_op(v); > > and avoid the initialization of pending_op above. Or alternatively: > > if ( !pending_op && vm_event_check_pending_op(v) ) > pending_op = true; > > Which avoid repeated calls to vm_event_check_pending_op when at least > one vCPU is known to be busy. > > > } > > > > + /* vm_event ops are still pending until vCPUs get scheduled */ > > + if ( pending_op ) > > + { > > + spin_unlock(&ved->lock); > > + return -EAGAIN; > > What happens when this gets called from vm_event_cleanup? > > AFAICT the result there is ignored, and could leak the vm_event > allocated memory? Thanks for the feedback. I'm going to drop this patch at let Bitdefender pick it up if they feel like fixing their buggy feature. As things stand for my use-case I only need patch 1 from this series. Tamas
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index 848d69c1b0..a23aadc112 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -297,6 +297,29 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp) }; } +bool vm_event_check_pending_op(const struct vcpu *v) +{ + struct monitor_write_data *w = &v->arch.vm_event->write_data; + + if ( !v->arch.vm_event->sync_event ) + return false; + + if ( w->do_write.cr0 ) + return true; + if ( w->do_write.cr3 ) + return true; + if ( w->do_write.cr4 ) + return true; + if ( w->do_write.msr ) + return true; + if ( v->arch.vm_event->set_gprs ) + return true; + if ( v->arch.vm_event->emulate_flags ) + return true; + + return false; +} + /* * Local variables: * mode: C diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index 127f2d58f1..2df327a42c 100644 --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -183,6 +183,7 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) if ( vm_event_check_ring(ved) ) { struct vcpu *v; + bool pending_op = false; spin_lock(&ved->lock); @@ -192,9 +193,6 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) return -EBUSY; } - /* Free domU's event channel and leave the other one unbound */ - free_xen_event_channel(d, ved->xen_port); - /* Unblock all vCPUs */ for_each_vcpu ( d, v ) { @@ -203,8 +201,21 @@ static int vm_event_disable(struct domain *d, struct vm_event_domain **p_ved) vcpu_unpause(v); ved->blocked--; } + + if ( vm_event_check_pending_op(v) ) + pending_op = true; } + /* vm_event ops are still pending until vCPUs get scheduled */ + if ( pending_op ) + { + spin_unlock(&ved->lock); + return -EAGAIN; + } + + /* Free domU's event channel and leave the other one unbound */ + free_xen_event_channel(d, ved->xen_port); + destroy_ring_for_helper(&ved->ring_page, ved->ring_pg_struct); vm_event_cleanup_domain(d); diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h index 14d1d341cc..978b224dc3 100644 --- a/xen/include/asm-arm/vm_event.h +++ b/xen/include/asm-arm/vm_event.h @@ -58,4 +58,11 @@ void vm_event_sync_event(struct vcpu *v, bool value) /* Not supported on ARM. */ } +static inline +bool vm_event_check_pending_op(const struct vcpu *v) +{ + /* Not supported on ARM. */ + return false; +} + #endif /* __ASM_ARM_VM_EVENT_H__ */ diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h index 785e741fba..97860d0d99 100644 --- a/xen/include/asm-x86/vm_event.h +++ b/xen/include/asm-x86/vm_event.h @@ -54,4 +54,6 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp); void vm_event_sync_event(struct vcpu *v, bool value); +bool vm_event_check_pending_op(const struct vcpu *v); + #endif /* __ASM_X86_VM_EVENT_H__ */
Perform sanity checking when shutting vm_event down to determine whether it is safe to do so. Error out with -EAGAIN in case pending operations have been found for the domain. Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com> --- xen/arch/x86/vm_event.c | 23 +++++++++++++++++++++++ xen/common/vm_event.c | 17 ++++++++++++++--- xen/include/asm-arm/vm_event.h | 7 +++++++ xen/include/asm-x86/vm_event.h | 2 ++ 4 files changed, 46 insertions(+), 3 deletions(-)