| Message ID | 1467820378-13448-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 06.07.16 at 17:52, <czuzu@bitdefender.com> wrote: > Also adjust returned error code for similar check from -EINVAL to more > descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP). I'm not sure that's more descriptive, and we really try to not use ENOSYS for other than unimplemented hypercalls. EOPNOTSUPP perhaps? Jan
On 7/6/2016 7:01 PM, Jan Beulich wrote: >>>> On 06.07.16 at 17:52, <czuzu@bitdefender.com> wrote: >> Also adjust returned error code for similar check from -EINVAL to more >> descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP). > I'm not sure that's more descriptive, and we really try to not use > ENOSYS for other than unimplemented hypercalls. EOPNOTSUPP > perhaps? > > Jan Well, it's not quite an 'unsupported operation' and neither does the toolstack user communicate an 'invalid value', he must just be noticed that something (the vm-event subsystem) needs to be initialised before the operation can be done. But I agree ENOSYS is not acceptable either (I only now see it signifies "Function not implemented", my bad for not peeking at that before using it, I expected differently). What about ENODEV, i.e. "No such device."? We need an error code saying "Device not initialised"... Thanks, Corneliu.
On 7/6/2016 7:15 PM, Corneliu ZUZU wrote: > On 7/6/2016 7:01 PM, Jan Beulich wrote: >>>>> On 06.07.16 at 17:52, <czuzu@bitdefender.com> wrote: >>> Also adjust returned error code for similar check from -EINVAL to more >>> descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP). >> I'm not sure that's more descriptive, and we really try to not use >> ENOSYS for other than unimplemented hypercalls. EOPNOTSUPP >> perhaps? >> >> Jan > > Well, it's not quite an 'unsupported operation' and neither does the > toolstack user communicate an 'invalid value', he must just be noticed > that something (the vm-event subsystem) needs to be initialised before > the operation can be done. > But I agree ENOSYS is not acceptable either (I only now see it > signifies "Function not implemented", my bad for not peeking at that > before using it, I expected differently). > > What about ENODEV, i.e. "No such device."? We need an error code > saying "Device not initialised"... > > Thanks, > Corneliu. Or ENOTCONN..? Corneliu.
>>> On 06.07.16 at 18:15, <czuzu@bitdefender.com> wrote: > On 7/6/2016 7:01 PM, Jan Beulich wrote: >>>>> On 06.07.16 at 17:52, <czuzu@bitdefender.com> wrote: >>> Also adjust returned error code for similar check from -EINVAL to more >>> descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP). >> I'm not sure that's more descriptive, and we really try to not use >> ENOSYS for other than unimplemented hypercalls. EOPNOTSUPP >> perhaps? > > Well, it's not quite an 'unsupported operation' and neither does the > toolstack user communicate an 'invalid value', he must just be noticed > that something (the vm-event subsystem) needs to be initialised before > the operation can be done. > But I agree ENOSYS is not acceptable either (I only now see it signifies > "Function not implemented", my bad for not peeking at that before using > it, I expected differently). > > What about ENODEV, i.e. "No such device."? We need an error code saying > "Device not initialised"... Fine with me (almost anything other than ENOSYS would be). Jan
On 7/7/2016 10:30 AM, Jan Beulich wrote: >>>> On 06.07.16 at 18:15, <czuzu@bitdefender.com> wrote: >> On 7/6/2016 7:01 PM, Jan Beulich wrote: >>>>>> On 06.07.16 at 17:52, <czuzu@bitdefender.com> wrote: >>>> Also adjust returned error code for similar check from -EINVAL to more >>>> descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP). >>> I'm not sure that's more descriptive, and we really try to not use >>> ENOSYS for other than unimplemented hypercalls. EOPNOTSUPP >>> perhaps? >> Well, it's not quite an 'unsupported operation' and neither does the >> toolstack user communicate an 'invalid value', he must just be noticed >> that something (the vm-event subsystem) needs to be initialised before >> the operation can be done. >> But I agree ENOSYS is not acceptable either (I only now see it signifies >> "Function not implemented", my bad for not peeking at that before using >> it, I expected differently). >> >> What about ENODEV, i.e. "No such device."? We need an error code saying >> "Device not initialised"... > Fine with me (almost anything other than ENOSYS would be). > > Jan > > Ack, thanks. Corneliu.
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@bitdefender.com> > --- > 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.
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;
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@bitdefender.com> --- xen/arch/x86/monitor.c | 7 +++++++ xen/include/asm-x86/monitor.h | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-)