From: George Dunlap <george.dunlap@citrix.com>
To: Corneliu ZUZU <czuzu@bitdefender.com>, xen-devel@lists.xen.org
Cc: Tamas K Lengyel <tamas@tklengyel.com>,
Razvan Cojocaru <rcojocaru@bitdefender.com>,
George Dunlap <george.dunlap@eu.citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Paul Durrant <paul.durrant@citrix.com>,
Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH v2 3/7] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup
Date: Tue, 5 Jul 2016 15:45:46 +0100 [thread overview]
Message-ID: <86db9af7-88b8-d2f2-e435-00346f563338@citrix.com> (raw)
In-Reply-To: <1467728902-13725-1-git-send-email-czuzu@bitdefender.com>
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@bitdefender.com>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
[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@citrix.com>
> 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@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-07-05 14:45 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-05 14:25 [PATCH v2 0/7] x86/vm-event: Adjustments & fixes Corneliu ZUZU
2016-07-05 14:26 ` [PATCH v2 1/7] x86/vmx_update_guest_cr: minor optimization Corneliu ZUZU
2016-07-05 14:27 ` [PATCH v2 2/7] x86/vm-event/monitor: relocate code-motion more appropriately Corneliu ZUZU
2016-07-05 14:28 ` [PATCH v2 3/7] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup Corneliu ZUZU
2016-07-05 14:45 ` George Dunlap [this message]
2016-07-05 14:46 ` George Dunlap
2016-07-05 17:16 ` Razvan Cojocaru
2016-07-06 7:01 ` Corneliu ZUZU
2016-07-05 14:29 ` [PATCH v2 4/7] x86/vm_event_resume: surround VM_EVENT_REASON_MOV_TO_MSR w/ CONFIG_X86 Corneliu ZUZU
2016-07-05 14:29 ` [PATCH v2 5/7] x86/vm-event: minor ASSERT fix, add 'unlikely' Corneliu ZUZU
2016-07-05 14:30 ` [PATCH v2 6/7] minor fixes (formatting, comments, unused includes etc.) Corneliu ZUZU
2016-07-05 15:46 ` Razvan Cojocaru
2016-07-06 9:52 ` Julien Grall
2016-07-05 14:31 ` [PATCH v2 7/7] minor #include change Corneliu ZUZU
2016-07-05 14:39 ` Tamas K Lengyel
2016-07-05 15:25 ` Razvan Cojocaru
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86db9af7-88b8-d2f2-e435-00346f563338@citrix.com \
--to=george.dunlap@citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=czuzu@bitdefender.com \
--cc=george.dunlap@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=paul.durrant@citrix.com \
--cc=rcojocaru@bitdefender.com \
--cc=tamas@tklengyel.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).