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

From: Razvan Cojocaru <rcojocaru@bitdefender.com>
To: Corneliu ZUZU <czuzu@bitdefender.com>, xen-devel@lists.xen.org
Cc: George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Paul Durrant <paul.durrant@citrix.com>,
	Tamas K Lengyel <tamas@tklengyel.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 20:16:28 +0300	[thread overview]
Message-ID: <c888df27-3b99-8c8a-e669-4b1555697369@bitdefender.com> (raw)
In-Reply-To: <1467728902-13725-1-git-send-email-czuzu@bitdefender.com>

On 07/05/16 17: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>
> ---
> Changed since v1:
>   * arch_vcpu.vm_event made pointer again to avoid eating memory from arch_vcpu
>     structure

I believe that all acks should be presumed lost on non-trivial changes
in a new version (which I believe this qualifies as being, with all the
new logic of allocating / deallocating only part of vm_event).

Unfortunately I'm out of office until early next week so I can't
properly test / thoroughly parse this until then, but we should be extra
careful that there are several places in the code where it is assumed
that v->arch.vm_event != NULL is the same thing as monitoring being
enabled. I'm not saying that they're not treated in this patch (the
proper change has certainly been made in emulate.c), I'm just saying
that we should be careful that they are.

Having said that, I propose a special macro to make this all clearer,
something like:

#define is_monitor_enabled_for_vcpu(v) \
    ( v->arch.vm_event && v->arch.vm_event->emul_read_data )

or equivalent inline functions returning a bool_t. Just a thought.

Thanks,
Razvan

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