| Message ID | 20220425100642.14383-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash | expand |
On 25.04.2022 12:06, Andrew Cooper wrote: > @@ -178,6 +179,71 @@ void domain_pause_for_debugger(void) > send_global_virq(VIRQ_DEBUGGER); > } > > +long gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback) Is there anything that requires "long" (and not just "int") here and ... > +{ > + struct vcpu *v; > + long ret = 0; ... here? > + 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: > + 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; > + } Just as a remark: It's never really clear to me whether we actually want to permit omitting "break" in cases like this one. It always feels slightly risky towards someone subsequently adding another case label below here without adding the suddenly necessary "break". While for sentinel code like this doing so may be okay, it would seem to me that we might be better off not allowing omission of "break" anywhere. > --- a/xen/arch/x86/include/asm/gdbsx.h > +++ b/xen/arch/x86/include/asm/gdbsx.h > @@ -2,18 +2,27 @@ > #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); > > +long gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback); > + > #else > > +#include <xen/errno.h> > + > static inline void domain_pause_for_debugger(void) {} > > +long gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback) static inline? Jan
On 25/04/2022 12:04, Jan Beulich wrote: > On 25.04.2022 12:06, Andrew Cooper wrote: >> @@ -178,6 +179,71 @@ void domain_pause_for_debugger(void) >> send_global_virq(VIRQ_DEBUGGER); >> } >> >> +long gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback) > Is there anything that requires "long" (and not just "int") here and ... > >> +{ >> + struct vcpu *v; >> + long ret = 0; > ... here? Consistency with its caller. Although I can't actually see a good reason for arch_do_domctl() to use long's either, and that would at least mean we've only got one place doing extension of 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: >> + 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; >> + } > Just as a remark: It's never really clear to me whether we actually want > to permit omitting "break" in cases like this one. That was an oversight. I'd intended to include one. >> --- a/xen/arch/x86/include/asm/gdbsx.h >> +++ b/xen/arch/x86/include/asm/gdbsx.h >> @@ -2,18 +2,27 @@ >> #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); >> >> +long gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback); >> + >> #else >> >> +#include <xen/errno.h> >> + >> static inline void domain_pause_for_debugger(void) {} >> >> +long gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback) > static inline? Oops yes. I clearly need more coffee. ~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..d8067ec90fd4 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,71 @@ void domain_pause_for_debugger(void) send_global_virq(VIRQ_DEBUGGER); } +long gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback) +{ + struct vcpu *v; + long ret = 0; + + 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: + 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; + } + + 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..8d357e5c9102 100644 --- a/xen/arch/x86/include/asm/gdbsx.h +++ b/xen/arch/x86/include/asm/gdbsx.h @@ -2,18 +2,27 @@ #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); +long gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool *copyback); + #else +#include <xen/errno.h> + static inline void domain_pause_for_debugger(void) {} +long 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. --- xen/arch/x86/domctl.c | 61 +---------------------------------- xen/arch/x86/gdbsx.c | 68 +++++++++++++++++++++++++++++++++++++++- xen/arch/x86/include/asm/gdbsx.h | 15 +++++++-- 3 files changed, 80 insertions(+), 64 deletions(-)