| Message ID | 20220420155657.32506-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash | expand |
On 20.04.22 17:56, Andrew Cooper wrote: > When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL > pointer. It isn't really correct for processing of XEN_DOMCTL_gdbsx_* to fall > into the default case when compiled out. > > 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> > > RFC, because this has implications across the codebase. The tl;dr is that > case FOO:'s shouldn't be compiled out; we still know what the subops are, even > when the functionality is compiled out. > > There are several ways to express this. Alternatives would be: > > case XEN_DOMCTL_gdbsx_guestmemio: > if ( !IS_ENABLED(CONFIG_GDBSX) ) > { > rc = -EOPNOTSUPP; > break; > } > ...; > > but given my debugger series creating gdbsx.c, I was also considering: > > case XEN_DOMCTL_gdbsx_guestmemio: > case XEN_DOMCTL_gdbsx_pausevcpu: > case XEN_DOMCTL_gdbsx_unpausevcpu: > case XEN_DOMCTL_gdbsx_domstatus: > rc = gdbsx_do_domctl(d, iop); > break; I'd go this route. Juergen
On 20.04.2022 18:03, Juergen Gross wrote: > On 20.04.22 17:56, Andrew Cooper wrote: >> When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL >> pointer. It isn't really correct for processing of XEN_DOMCTL_gdbsx_* to fall >> into the default case when compiled out. >> >> 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> >> >> RFC, because this has implications across the codebase. The tl;dr is that >> case FOO:'s shouldn't be compiled out; we still know what the subops are, even >> when the functionality is compiled out. >> >> There are several ways to express this. Alternatives would be: >> >> case XEN_DOMCTL_gdbsx_guestmemio: >> if ( !IS_ENABLED(CONFIG_GDBSX) ) >> { >> rc = -EOPNOTSUPP; >> break; >> } >> ...; >> >> but given my debugger series creating gdbsx.c, I was also considering: >> >> case XEN_DOMCTL_gdbsx_guestmemio: >> case XEN_DOMCTL_gdbsx_pausevcpu: >> case XEN_DOMCTL_gdbsx_unpausevcpu: >> case XEN_DOMCTL_gdbsx_domstatus: >> rc = gdbsx_do_domctl(d, iop); >> break; > > I'd go this route. +1 if we already start enumerating sub-system domctl-s (as proposed for the IOMMU ones as well). Jan
On 21/04/2022 10:26, Jan Beulich wrote: > On 20.04.2022 18:03, Juergen Gross wrote: >> On 20.04.22 17:56, Andrew Cooper wrote: >>> When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL >>> pointer. It isn't really correct for processing of XEN_DOMCTL_gdbsx_* to fall >>> into the default case when compiled out. >>> >>> 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> >>> >>> RFC, because this has implications across the codebase. The tl;dr is that >>> case FOO:'s shouldn't be compiled out; we still know what the subops are, even >>> when the functionality is compiled out. >>> >>> There are several ways to express this. Alternatives would be: >>> >>> case XEN_DOMCTL_gdbsx_guestmemio: >>> if ( !IS_ENABLED(CONFIG_GDBSX) ) >>> { >>> rc = -EOPNOTSUPP; >>> break; >>> } >>> ...; >>> >>> but given my debugger series creating gdbsx.c, I was also considering: >>> >>> case XEN_DOMCTL_gdbsx_guestmemio: >>> case XEN_DOMCTL_gdbsx_pausevcpu: >>> case XEN_DOMCTL_gdbsx_unpausevcpu: >>> case XEN_DOMCTL_gdbsx_domstatus: >>> rc = gdbsx_do_domctl(d, iop); >>> break; >> I'd go this route. > +1 if we already start enumerating sub-system domctl-s (as proposed > for the IOMMU ones as well). Ok, so that seems like general agreement for "we shouldn't compile out case statements" which is something we should apply across the codebase going forward. For gdbsx_do_domctl() specifically, that's going to want to wait until "[PATCH v5 0/6] Clean up common/arch split for debugger.h" has gone in, but I don't anticipate any issues there. ~Andrew
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index a6aae500a30b..1faa5a49ff3c 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -890,7 +890,14 @@ long arch_do_domctl( copyback = true; break; } -#endif +#else /* CONFIG_GDBSX */ + case XEN_DOMCTL_gdbsx_guestmemio: + case XEN_DOMCTL_gdbsx_pausevcpu: + case XEN_DOMCTL_gdbsx_unpausevcpu: + case XEN_DOMCTL_gdbsx_domstatus: + rc = -EOPNOTSUPP; + break; +#endif /* CONFIG_GDBSX */ case XEN_DOMCTL_setvcpuextstate: case XEN_DOMCTL_getvcpuextstate:
When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL pointer. It isn't really correct for processing of XEN_DOMCTL_gdbsx_* to fall into the default case when compiled out. 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> RFC, because this has implications across the codebase. The tl;dr is that case FOO:'s shouldn't be compiled out; we still know what the subops are, even when the functionality is compiled out. There are several ways to express this. Alternatives would be: case XEN_DOMCTL_gdbsx_guestmemio: if ( !IS_ENABLED(CONFIG_GDBSX) ) { rc = -EOPNOTSUPP; break; } ...; but given my debugger series creating gdbsx.c, I was also considering: case XEN_DOMCTL_gdbsx_guestmemio: case XEN_DOMCTL_gdbsx_pausevcpu: case XEN_DOMCTL_gdbsx_unpausevcpu: case XEN_DOMCTL_gdbsx_domstatus: rc = gdbsx_do_domctl(d, iop); break; when I can rework the callers of domain_pause_for_debugger() slightly, at which point we can conditionally compile the gdbsx variables out struct domain/vcpu, which wouldn't be compatible with the first suggestion. Thoughts? --- xen/arch/x86/domctl.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)