| Message ID | 20220425123717.18876-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash | expand |
On 25.04.2022 14:37, Andrew Cooper wrote: > When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL > pointer. One of several bugs here is known-but-compiled-out subops falling > into the default chain and hitting unrelated logic. > > Remove the CONFIG_GDBSX ifdefary in arch_do_domctl() by implementing > gdbsx_domctl() and moving the logic across. > > As minor cleanup, > * gdbsx_guest_mem_io() can become static > * Remove opencoding of domain_vcpu() and %pd > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Technically Reviewed-by: Jan Beulich <jbeulich@suse.com> Yet as mentioned before, ... > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -816,71 +816,12 @@ long arch_do_domctl( > } > #endif > > -#ifdef CONFIG_GDBSX > case XEN_DOMCTL_gdbsx_guestmemio: > - ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio); > - if ( !ret ) > - copyback = true; > - break; > - > case XEN_DOMCTL_gdbsx_pausevcpu: > - { > - struct vcpu *v; > - > - ret = -EBUSY; > - if ( !d->controller_pause_count ) > - break; > - ret = -EINVAL; > - if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus || > - (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL ) > - break; > - ret = vcpu_pause_by_systemcontroller(v); > - break; > - } > - > case XEN_DOMCTL_gdbsx_unpausevcpu: > - { > - struct vcpu *v; > - > - ret = -EBUSY; > - if ( !d->controller_pause_count ) > - break; > - ret = -EINVAL; > - if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus || > - (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL ) > - break; > - ret = vcpu_unpause_by_systemcontroller(v); > - if ( ret == -EINVAL ) > - printk(XENLOG_G_WARNING > - "WARN: d%d attempting to unpause %pv which is not paused\n", > - currd->domain_id, v); > - break; > - } > - > case XEN_DOMCTL_gdbsx_domstatus: > - { > - struct vcpu *v; > - > - domctl->u.gdbsx_domstatus.vcpu_id = -1; > - domctl->u.gdbsx_domstatus.paused = d->controller_pause_count > 0; > - if ( domctl->u.gdbsx_domstatus.paused ) > - { > - for_each_vcpu ( d, v ) > - { > - if ( v->arch.gdbsx_vcpu_event ) > - { > - domctl->u.gdbsx_domstatus.vcpu_id = v->vcpu_id; > - domctl->u.gdbsx_domstatus.vcpu_ev = > - v->arch.gdbsx_vcpu_event; > - v->arch.gdbsx_vcpu_event = 0; > - break; > - } > - } > - } > - copyback = true; > + ret = gdbsx_domctl(d, domctl, ©back); > break; > - } > -#endif ... I'm not overly happy with the retaining of the case labels here (and the knock on effect it'll have for other subsystem domctl-s), so unlike usually this R-b isn't implicitly an A-b. Which doesn't matter in practice, aiui ... Jan
On 25/04/2022 2:36 pm, Jan Beulich wrote: > On 25.04.2022 14:37, Andrew Cooper wrote: >> When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL >> pointer. One of several bugs here is known-but-compiled-out subops >> falling >> into the default chain and hitting unrelated logic. >> >> Remove the CONFIG_GDBSX ifdefary in arch_do_domctl() by implementing >> gdbsx_domctl() and moving the logic across. >> >> As minor cleanup, >> * gdbsx_guest_mem_io() can become static >> * Remove opencoding of domain_vcpu() and %pd >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Technically > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. I'll tweak the commit message now that the IOMMU fix has gone in. > Yet as mentioned before, ... > >> --- a/xen/arch/x86/domctl.c >> +++ b/xen/arch/x86/domctl.c >> @@ -816,71 +816,12 @@ long arch_do_domctl( >> } >> #endif >> >> -#ifdef CONFIG_GDBSX >> case XEN_DOMCTL_gdbsx_guestmemio: >> - ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio); >> - if ( !ret ) >> - copyback = true; >> - break; >> - >> case XEN_DOMCTL_gdbsx_pausevcpu: >> - { >> - struct vcpu *v; >> - >> - ret = -EBUSY; >> - if ( !d->controller_pause_count ) >> - break; >> - ret = -EINVAL; >> - if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus || >> - (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == >> NULL ) >> - break; >> - ret = vcpu_pause_by_systemcontroller(v); >> - break; >> - } >> - >> case XEN_DOMCTL_gdbsx_unpausevcpu: >> - { >> - struct vcpu *v; >> - >> - ret = -EBUSY; >> - if ( !d->controller_pause_count ) >> - break; >> - ret = -EINVAL; >> - if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus || >> - (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == >> NULL ) >> - break; >> - ret = vcpu_unpause_by_systemcontroller(v); >> - if ( ret == -EINVAL ) >> - printk(XENLOG_G_WARNING >> - "WARN: d%d attempting to unpause %pv which is not >> paused\n", >> - currd->domain_id, v); >> - break; >> - } >> - >> case XEN_DOMCTL_gdbsx_domstatus: >> - { >> - struct vcpu *v; >> - >> - domctl->u.gdbsx_domstatus.vcpu_id = -1; >> - domctl->u.gdbsx_domstatus.paused = d->controller_pause_count >> > 0; >> - if ( domctl->u.gdbsx_domstatus.paused ) >> - { >> - for_each_vcpu ( d, v ) >> - { >> - if ( v->arch.gdbsx_vcpu_event ) >> - { >> - domctl->u.gdbsx_domstatus.vcpu_id = v->vcpu_id; >> - domctl->u.gdbsx_domstatus.vcpu_ev = >> - v->arch.gdbsx_vcpu_event; >> - v->arch.gdbsx_vcpu_event = 0; >> - break; >> - } >> - } >> - } >> - copyback = true; >> + ret = gdbsx_domctl(d, domctl, ©back); >> break; >> - } >> -#endif > > ... I'm not overly happy with the retaining of the case labels here > (and the knock on effect it'll have for other subsystem domctl-s), The crash in do_iommu_op() happened because things which weren't iommu subops ended up in a function only expecting iommu subops, *because* unrelated case labels got compiled out. We've also had multiple XSAs elsewhere created by related "just chain everything through default:" patterns. The legacy MSR paths are still especially guilty of doing this. The case labels need to never ever get compiled out, even if their subsystem has been. So you have a choice between this patch, or a pattern of the form: case XEN_DOMCTL_gdbsx_domstatus: if ( !IS_ENABLED(CONFIG_GDBSX) ) { ret = -Exxx; break; } ... but the top level case labels need to stay one way or another. For this patch, it moves a chunk of logic out of a fairly generic file into its proper subsystem file, and a few extra case labels is a very cheap price to pay. ~Andrew
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index c20ab4352715..9131acb8a230 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -816,71 +816,12 @@ long arch_do_domctl( } #endif -#ifdef CONFIG_GDBSX case XEN_DOMCTL_gdbsx_guestmemio: - ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio); - if ( !ret ) - copyback = true; - break; - case XEN_DOMCTL_gdbsx_pausevcpu: - { - struct vcpu *v; - - ret = -EBUSY; - if ( !d->controller_pause_count ) - break; - ret = -EINVAL; - if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus || - (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL ) - break; - ret = vcpu_pause_by_systemcontroller(v); - break; - } - case XEN_DOMCTL_gdbsx_unpausevcpu: - { - struct vcpu *v; - - ret = -EBUSY; - if ( !d->controller_pause_count ) - break; - ret = -EINVAL; - if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus || - (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL ) - break; - ret = vcpu_unpause_by_systemcontroller(v); - if ( ret == -EINVAL ) - printk(XENLOG_G_WARNING - "WARN: d%d attempting to unpause %pv which is not paused\n", - currd->domain_id, v); - break; - } - case XEN_DOMCTL_gdbsx_domstatus: - { - struct vcpu *v; - - domctl->u.gdbsx_domstatus.vcpu_id = -1; - domctl->u.gdbsx_domstatus.paused = d->controller_pause_count > 0; - if ( domctl->u.gdbsx_domstatus.paused ) - { - for_each_vcpu ( d, v ) - { - if ( v->arch.gdbsx_vcpu_event ) - { - domctl->u.gdbsx_domstatus.vcpu_id = v->vcpu_id; - domctl->u.gdbsx_domstatus.vcpu_ev = - v->arch.gdbsx_vcpu_event; - v->arch.gdbsx_vcpu_event = 0; - break; - } - } - } - copyback = true; + ret = gdbsx_domctl(d, domctl, ©back); break; - } -#endif case XEN_DOMCTL_setvcpuextstate: case XEN_DOMCTL_getvcpuextstate: diff --git a/xen/arch/x86/gdbsx.c b/xen/arch/x86/gdbsx.c index 6ef46e8ea77d..21442f5dff1a 100644 --- a/xen/arch/x86/gdbsx.c +++ b/xen/arch/x86/gdbsx.c @@ -152,7 +152,8 @@ static unsigned int dbg_rw_guest_mem(struct domain *dp, unsigned long addr, return len; } -int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop) +static int gdbsx_guest_mem_io( + struct domain *d, struct xen_domctl_gdbsx_memio *iop) { if ( d && !d->is_dying ) { @@ -178,6 +179,73 @@ void domain_pause_for_debugger(void) send_global_virq(VIRQ_DEBUGGER); } +int gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback) +{ + struct vcpu *v; + int ret; + + switch ( domctl->cmd ) + { + case XEN_DOMCTL_gdbsx_guestmemio: + ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio); + if ( !ret ) + *copyback = true; + break; + + case XEN_DOMCTL_gdbsx_pausevcpu: + ret = -EBUSY; + if ( !d->controller_pause_count ) + break; + ret = -EINVAL; + if ( (v = domain_vcpu(d, domctl->u.gdbsx_pauseunp_vcpu.vcpu)) == NULL ) + break; + ret = vcpu_pause_by_systemcontroller(v); + break; + + case XEN_DOMCTL_gdbsx_unpausevcpu: + ret = -EBUSY; + if ( !d->controller_pause_count ) + break; + ret = -EINVAL; + if ( (v = domain_vcpu(d, domctl->u.gdbsx_pauseunp_vcpu.vcpu)) == NULL ) + break; + ret = vcpu_unpause_by_systemcontroller(v); + if ( ret == -EINVAL ) + printk(XENLOG_G_WARNING + "WARN: %pd attempting to unpause %pv which is not paused\n", + current->domain, v); + break; + + case XEN_DOMCTL_gdbsx_domstatus: + ret = 0; + domctl->u.gdbsx_domstatus.vcpu_id = -1; + domctl->u.gdbsx_domstatus.paused = d->controller_pause_count > 0; + if ( domctl->u.gdbsx_domstatus.paused ) + { + for_each_vcpu ( d, v ) + { + if ( v->arch.gdbsx_vcpu_event ) + { + domctl->u.gdbsx_domstatus.vcpu_id = v->vcpu_id; + domctl->u.gdbsx_domstatus.vcpu_ev = + v->arch.gdbsx_vcpu_event; + v->arch.gdbsx_vcpu_event = 0; + break; + } + } + } + *copyback = true; + break; + + default: + ASSERT_UNREACHABLE(); + ret = -ENOSYS; + break; + } + + return ret; +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/include/asm/gdbsx.h b/xen/arch/x86/include/asm/gdbsx.h index 938eb74e2e25..e906be9ea318 100644 --- a/xen/arch/x86/include/asm/gdbsx.h +++ b/xen/arch/x86/include/asm/gdbsx.h @@ -2,18 +2,28 @@ #ifndef __X86_GDBX_H__ #define __X86_GDBX_H__ -#ifdef CONFIG_GDBSX +#include <xen/stdbool.h> struct domain; -struct xen_domctl_gdbsx_memio; +struct xen_domctl; -int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop); +#ifdef CONFIG_GDBSX void domain_pause_for_debugger(void); +int gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback); + #else +#include <xen/errno.h> + static inline void domain_pause_for_debugger(void) {} +static inline int gdbsx_domctl( + struct domain *d, struct xen_domctl *domctl, bool *copyback) +{ + return -ENOSYS; +} + #endif /* CONFIG_GDBSX */ #endif /* __X86_GDBX_H__ */
When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL pointer. One of several bugs here is known-but-compiled-out subops falling into the default chain and hitting unrelated logic. Remove the CONFIG_GDBSX ifdefary in arch_do_domctl() by implementing gdbsx_domctl() and moving the logic across. As minor cleanup, * gdbsx_guest_mem_io() can become static * Remove opencoding of domain_vcpu() and %pd Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Julien Grall <julien@xen.org> CC: Juergen Gross <jgross@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> v2: * Implement the "split into new function" approach from the RFC. v3: * Switch to int. * Insert missing break. * static inline for stub. --- xen/arch/x86/domctl.c | 61 +--------------------------------- xen/arch/x86/gdbsx.c | 70 +++++++++++++++++++++++++++++++++++++++- xen/arch/x86/include/asm/gdbsx.h | 16 +++++++-- 3 files changed, 83 insertions(+), 64 deletions(-)