[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 4/8] x86/vm_event/monitor/cr: check for vm-event subscriber on domctl
On 7/6/2016 6:52 PM, Corneliu ZUZU wrote: Enforce presence of a monitor vm-event subscriber when the toolstack user calls xc_monitor_write_ctrlreg (XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG domctl). Without this change, ASSERT(v->arch.vm_event) @ hvm_set_cr0 and such would fail if the toolstack user calls xc_monitor_write_ctrlreg(...) w/ enable = true, without first calling xc_monitor_enable(). Also adjust returned error code for similar check from -EINVAL to more descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP). Signed-off-by: Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx> --- xen/arch/x86/monitor.c | 7 +++++++ xen/include/asm-x86/monitor.h | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 42f31bf..4d4db33 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -188,6 +188,13 @@ int arch_monitor_domctl_event(struct domain *d, unsigned int ctrlreg_bitmask; bool_t old_status;+ /*+ * Enabling cr-write vm-events without a vm_event subscriber is + * meaningless. + */ + if ( !vm_event_domain_initialised(d) ) + return -ENOSYS; + /* sanity check: avoid left-shift undefined behavior */ if ( unlikely(mop->u.mov_to_cr.index > 31) ) return -EINVAL; diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 9238ec8..2213124 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -52,7 +52,7 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) if ( vm_event_domain_initialised(d) ) d->arch.mem_access_emulate_each_rep = !!mop->event; else - rc = -EINVAL; + rc = -ENOSYS;domain_unpause(d);break; But this also doesn't guarantee the ASSERT not failing, as I now realize, simply because with this patch v->arch.vm_event can *still be NULL* even with a *non-zero d->monitor.write_ctrlreg_enabled*. Can happen for example if:1) the toolstack user calls *xc_monitor_enable()* to enable domain monitoring (calls *vm_event_init_domain()* along the way) 2) the toolstack user calls *xc_monitor_write_ctrlreg()* to enable *CR0* exiting - this will succeed since the user *first called xc_monitor_enable()* - which *will result in having a non-zero d->monitor.write_ctrlreg_enabled* 3) the toolstack user calls *xc_mem_paging_enable() followed by xc_mem_paging_disable()*; this *will call vm_event_disable()->vm_event_cleanup_domain()* along the way which *will xfree v->arch.vm_event*, but _will not_ call *arch_monitor_cleanup_domain()* (see vm_event_domctl) which zeroes out d->monitor.write_ctrlreg_enabled 4) a CR0-write is intercepted after these operations which will ASSERT(v->arch.vm_event) in hvm_set_cr0(), which will subsequently fail This issue happens because *vm_event_cleanup()* domain *xfrees arch_vm_event along with its write_data field* needed by the monitor subsystem, even a vm-event subsystem other than the monitor one is uninitialized. I'm wondering why vm_event_{init,cleanup}_domain() allocate/free *monitor-specific* resources (arch_vm_event fields) even when they're called as a result of XEN_DOMCTL_VM_EVENT_OP_PAGING/XEN_DOMCTL_VM_EVENT_OP_SHARING domctl. Tamas, Razvan, this seems wrong, shouldn't most operations currently done in *vm_event_{init,cleanup}_domain* actually be done in *arch_monitor_{init,cleanup}_domain* instead? I propose the following refactoring: - define: struct arch_vm_event_monitor { uint32_t emulate_flags; struct vm_event_emul_read_data *emul_read_data; struct monitor_write_data write_data; } - change struct arch_vm_event {struct arch_vm_event_monitor *monitor; // monitor subsystem-stuff! }- allocate *arch_vm_event* (but don't touch its *monitor* field) in *vm_event_init_domain()* - allocate/free *arch_vm_event->monitor* in *arch_monitor_{init,cleanup}_domain()*, but *only free write_data if there are no uncommitted writes* - call arch_monitor_cleanup_domain() *before* vm_event_cleanup_domain() and only free arch_vm_event in the latter if arch_vm_event->monitor was freed in the former - on domain/vcpu destroyal free everything left allocated (arch_vm_event->monitor and arch_vm_event) Sounds good? Thanks, Corneliu. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |