Re: [PATCH RFC] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
>>> ---
>>> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>> CC: Wei Liu <wl@xxxxxxx>
>>> CC: Julien Grall <julien@xxxxxxx>
>>> CC: Juergen Gross <jgross@xxxxxxxx>
>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
>>> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>>>
>>> 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