Re: [PATCH 1/3] x86/PV: make '0' debug key dump Dom0's stacks again

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Sep 29, 2021 at 11:42:34AM +0200, Jan Beulich wrote:
> The conversion to __get_guest() failed to account for the fact that for
> remote vCPU-s dumping gets done through a pointer obtained from
> map_domain_page(): __get_guest() arranges for (apparent) accesses to
> hypervisor space to cause #GP(0).
> 
> Fixes: 6a1d72d3739e ('x86: split __{get,put}_user() into "guest" and "unsafe" 
> variants')
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Using get_unsafe() might be an option as well, instead of the added
> extra conditional.

A third option might be to place them in a non Xen private VA range,
like we do for the compat argument translation. That's however too
much IMO.

> 
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -275,7 +275,9 @@ static void compat_show_guest_stack(stru
>      {
>          if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask )
>              break;
> -        if ( __get_guest(addr, stack) )
> +        if ( stack_page )
> +            addr = *stack;
> +        else if ( __get_guest(addr, stack) )
>          {
>              if ( i != 0 )
>                  printk("\n    ");
> @@ -344,7 +346,9 @@ static void show_guest_stack(struct vcpu
>      {
>          if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask )
>              break;
> -        if ( __get_guest(addr, stack) )
> +        if ( stack_page )
> +            addr = *stack;

I don't really have a strong opinion regarding this or the usage of
get_unsafe. When v == current we have the extra protection of being
sure no private Xen mappings are accessed, but that's only for a
single vCPU at most, at which point we might just use get_unsafe
uniformly. I guess I have a slight preference for get_unsafe because
using get_guest doesn't really get us anything.

If you keep it in the current form, or decide to switch to
get_unsafe:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.