Re: [Xen-devel] [PATCH v2 3/7] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 05/07/16 15:28, Corneliu ZUZU wrote:
> The arch_vm_event structure is dynamically allocated and freed @
> vm_event_cleanup_domain. This cleanup is triggered e.g. when the toolstack 
> user
> disables domain monitoring (xc_monitor_disable), which in turn effectively
> discards any information that was in arch_vm_event.write_data.
> 
> But this can yield unexpected behavior since if a CR-write was awaiting to be
> committed on the scheduling tail (hvm_do_resume->arch_monitor_write_data)
> before xc_monitor_disable is called, then the domain CR write is wrongfully
> ignored, which of course, in these cases, can easily render a domain crash.
> 
> To fix the issue, this patch makes arch_vm_event.emul_read_data dynamically
> allocated and only frees that in vm_event_cleanup_domain, instead of the whole
> arch_vcpu.vm_event structure, which with this patch will only be freed on
> vcpu/domain destroyal.
> 
> Signed-off-by: Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>
> Acked-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>

[snip]

> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 16733a4..6616626 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1642,7 +1642,7 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>          v->arch.vm_event->emulate_flags = violation ? rsp->flags : 0;
>  
>          if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
> -            v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
> +            *v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
>      }
>  }
>  

On the basis of Razvan's ack:

Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>

> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> index 80f84d6..0e25e4d 100644
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -30,12 +30,18 @@ int vm_event_init_domain(struct domain *d)
>  
>      for_each_vcpu ( d, v )
>      {
> -        if ( v->arch.vm_event )
> +        if ( likely(!v->arch.vm_event) )
> +        {
> +            v->arch.vm_event = xzalloc(struct arch_vm_event);
> +            if ( !v->arch.vm_event )
> +                return -ENOMEM;
> +        }
> +        else if ( unlikely(v->arch.vm_event->emul_read_data) )
>              continue;
>  
> -        v->arch.vm_event = xzalloc(struct arch_vm_event);
> -
> -        if ( !v->arch.vm_event )
> +        v->arch.vm_event->emul_read_data =
> +                xzalloc(struct vm_event_emul_read_data);
> +        if ( !v->arch.vm_event->emul_read_data )
>              return -ENOMEM;
>      }
>  
> @@ -52,8 +58,12 @@ void vm_event_cleanup_domain(struct domain *d)
>  
>      for_each_vcpu ( d, v )
>      {
> -        xfree(v->arch.vm_event);
> -        v->arch.vm_event = NULL;
> +        if ( likely(!v->arch.vm_event) )
> +            continue;
> +
> +        v->arch.vm_event->emulate_flags = 0;
> +        xfree(v->arch.vm_event->emul_read_data);
> +        v->arch.vm_event->emul_read_data = NULL;
>      }
>  
>      d->arch.mem_access_emulate_each_rep = 0;
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 17d2716..75bbbab 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -534,6 +534,8 @@ static void mem_sharing_notification(struct vcpu *v, 
> unsigned int port)
>  /* Clean up on domain destruction */
>  void vm_event_cleanup(struct domain *d)
>  {
> +    struct vcpu *v;
> +
>  #ifdef CONFIG_HAS_MEM_PAGING
>      if ( d->vm_event->paging.ring_page )
>      {
> @@ -560,6 +562,15 @@ void vm_event_cleanup(struct domain *d)
>          (void)vm_event_disable(d, &d->vm_event->share);
>      }
>  #endif
> +
> +    for_each_vcpu ( d, v )
> +    {
> +        if ( unlikely(v->arch.vm_event) )
> +        {
> +            xfree(v->arch.vm_event);
> +            v->arch.vm_event = NULL;
> +        }
> +    }
>  }
>  
>  int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 0611681..a094c0d 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -26,6 +26,7 @@
>  #include <public/domctl.h>
>  #include <asm/cpufeature.h>
>  #include <asm/hvm/hvm.h>
> +#include <asm/vm_event.h>
>  
>  #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
>  
> @@ -48,7 +49,8 @@ int arch_monitor_domctl_op(struct domain *d, struct 
> xen_domctl_monitor_op *mop)
>           * Enabling mem_access_emulate_each_rep without a vm_event subscriber
>           * is meaningless.
>           */
> -        if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event )
> +        if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event &&
> +             d->vcpu[0]->arch.vm_event->emul_read_data )
>              d->arch.mem_access_emulate_each_rep = !!mop->event;
>          else
>              rc = -EINVAL;
> diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
> index 026f42e..557f353 100644
> --- a/xen/include/asm-x86/vm_event.h
> +++ b/xen/include/asm-x86/vm_event.h
> @@ -28,7 +28,7 @@
>   */
>  struct arch_vm_event {
>      uint32_t emulate_flags;
> -    struct vm_event_emul_read_data emul_read_data;
> +    struct vm_event_emul_read_data *emul_read_data;
>      struct monitor_write_data write_data;
>  };
>  
> 


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