Re: [PATCH v3] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>

Technically
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
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, &copyback);
>          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