* [PATCH v2] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash
@ 2022-04-25 10:06 Andrew Cooper
2022-04-25 11:04 ` Jan Beulich
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Cooper @ 2022-04-25 10:06 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Jan Beulich, Stefano Stabellini,
Wei Liu, Julien Grall, Juergen Gross, Roger Pau Monné,
Volodymyr Babchuk, Bertrand Marquis
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(-)
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__ */
--
2.11.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash
2022-04-25 10:06 [PATCH v2] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash Andrew Cooper
@ 2022-04-25 11:04 ` Jan Beulich
2022-04-25 12:25 ` Andrew Cooper
0 siblings, 1 reply; 3+ messages in thread
From: Jan Beulich @ 2022-04-25 11:04 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Stefano Stabellini, Wei Liu, Julien Grall,
Juergen Gross, Roger Pau Monné,
Volodymyr Babchuk, Bertrand Marquis, Xen-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash
2022-04-25 11:04 ` Jan Beulich
@ 2022-04-25 12:25 ` Andrew Cooper
0 siblings, 0 replies; 3+ messages in thread
From: Andrew Cooper @ 2022-04-25 12:25 UTC (permalink / raw)
To: Jan Beulich
Cc: George Dunlap, Stefano Stabellini, Wei Liu, Julien Grall,
Juergen Gross, Roger Pau Monne, Volodymyr Babchuk,
Bertrand Marquis, Xen-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-04-25 12:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-25 10:06 [PATCH v2] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash Andrew Cooper
2022-04-25 11:04 ` Jan Beulich
2022-04-25 12:25 ` Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.