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

From: Corneliu ZUZU <czuzu@bitdefender.com>
To: Razvan Cojocaru <rcojocaru@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: Wed, 6 Jul 2016 10:01:04 +0300	[thread overview]
Message-ID: <bd89c3cc-2cd7-286a-46b9-00be8b75a9b2@bitdefender.com> (raw)
In-Reply-To: <c888df27-3b99-8c8a-e669-4b1555697369@bitdefender.com>

On 7/5/2016 8:16 PM, Razvan Cojocaru wrote:
> 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

At some point I actually defined that exact macro while constructing 
this patch, but I decided to delete it afterwards because I thought the 
code would still be clear without it (i.e. only check emul_read_data 
when we actually need _that_ to be non-NULL) and because it seemed a bit 
strange, being possible to have _arch_vm_event allocated_ but having the 
monitor vm-events subsystem _uninitialized_ (because of .write_data 
being treated specially). Since the write_data field is also part of the 
monitor subsystem, conceptually one could say the monitor subsystem is 
at least _partially_ initialized when that's non-NULL, not uninitialized 
at all. I'll think of a resolution around this, but in the meantime I 
there's something more important to settle:

I only now notice, it seems to me that the ASSERT(v->arch.vm_event) @ 
hvm_set_cr0 & such (the one just before calling hvm_monitor_crX) might 
fail. That's because from what I see in the code the following can happen:
- v.arch.vm_event = NULL (vm-event _not_ initialized)
- toolstack user calls e.g. xc_moLRnitor_write_ctrlreg(xch, domid, 
VM_EVENT_X86_CR0, true, true, false) -> do_domctl(XEN_DOMCTL_monitor_op) 
-> monitor_domctl(XEN_DOMCTL_MONITOR_OP_ENABLE) -> 
arch_monitor_domctl_event(XEN_DOMCTL_MONITOR_EVENT_WRITE_CTREG) which in 
turn sets the CR0 bit of d.arch.monitor.write_ctrlreg_enabled _without 
ever checking if v.arch.vm_event is non-NULL_
- afterwards a CR0 write happens on one of the domain vCPUs and we 
arrive at that assert without having v.arch.vm_event allocated!

I can't test this @ the moment, am I missing something here? I remember 
taking a look on this code path at some point and idk how I didn't 
notice something like this, it seems obviously wrong..has something 
changed recently?
I think we need to take a second look over this code-path, I'm also 
seeing that the proper check _is done_ @ 
arch_monitor_domctl_op(XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP) (though I 
think a more proper error return value there would be ENODEV instead of 
EINVAL).

Corneliu.

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