[PATCH 0/3] x86/PV: '0' debug key stack dumping adjustments

[PATCH 0/3] x86/PV: '0' debug key stack dumping adjustments

[Jan Beulich]

Having played with '0' for PVH quite a bit lately, it just so happened
that I also tried it with PV, finding that it has been broken for quite
some time.

1: make '0' debug key dump Dom0's stacks again
2: replace assertions in '0' debug key stack dumping
3: drop "vcpu" local variable from show_guest_stack()

Jan

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

[Jan Beulich]

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@suse.com>
---
Using get_unsafe() might be an option as well, instead of the added
extra conditional.

--- 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;
+        else if ( __get_guest(addr, stack) )
         {
             if ( i != 0 )
                 printk("\n    ");

[PATCH 2/3] x86/PV: replace assertions in '0' debug key stack dumping

[Jan Beulich]

While it was me to add them, I'm afraid I don't see justification for
the assertions: A vCPU may very well have got preempted while in user
mode. Limit compat guest user mode stack dumps to the containing page
(like is done when using do_page_walk()), and suppress their dumping
altogether for 64-bit Dom0.

Fixes: cc0de53a903c ("x86: improve output resulting from sending '0' over serial")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
An alternative to suppressing the dump for 64-bit would be to make
do_page_fault() guest-user-mode aware.

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -254,7 +254,6 @@ static void compat_show_guest_stack(stru
         struct vcpu *vcpu;
         unsigned long mfn;
 
-        ASSERT(guest_kernel_mode(v, regs));
         mfn = read_cr3() >> PAGE_SHIFT;
         for_each_vcpu( v->domain, vcpu )
             if ( pagetable_get_pfn(vcpu->arch.guest_table) == mfn )
@@ -269,6 +268,8 @@ static void compat_show_guest_stack(stru
             }
             mask = PAGE_SIZE;
         }
+        else if ( !guest_kernel_mode(v, regs) )
+            mask = PAGE_SIZE;
     }
 
     for ( i = 0; i < debug_stack_lines * 8; i++ )
@@ -328,7 +329,12 @@ static void show_guest_stack(struct vcpu
     {
         struct vcpu *vcpu;
 
-        ASSERT(guest_kernel_mode(v, regs));
+        if ( !guest_kernel_mode(v, regs) )
+        {
+            printk("User mode stack\n");
+            return;
+        }
+
         vcpu = maddr_get_owner(read_cr3()) == v->domain ? v : NULL;
         if ( !vcpu )
         {

[PATCH 3/3] x86/PV: drop "vcpu" local variable from show_guest_stack()

[Jan Beulich]

It's not really needed and has been misleading me more than once to try
and spot its "actual" use(s). It should really have been dropped when
the 32-bit specific logic was purged from here.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -327,16 +327,13 @@ static void show_guest_stack(struct vcpu
 
     if ( v != current )
     {
-        struct vcpu *vcpu;
-
         if ( !guest_kernel_mode(v, regs) )
         {
             printk("User mode stack\n");
             return;
         }
 
-        vcpu = maddr_get_owner(read_cr3()) == v->domain ? v : NULL;
-        if ( !vcpu )
+        if ( maddr_get_owner(read_cr3()) != v->domain )
         {
             stack_page = stack = do_page_walk(v, (unsigned long)stack);
             if ( (unsigned long)stack < PAGE_SIZE )

Ping: [PATCH 0/3] x86/PV: '0' debug key stack dumping adjustments

[Jan Beulich]

On 29.09.2021 11:40, Jan Beulich wrote:
> Having played with '0' for PVH quite a bit lately, it just so happened
> that I also tried it with PV, finding that it has been broken for quite
> some time.
> 
> 1: make '0' debug key dump Dom0's stacks again

The breakage dealt with here can be observed in about every osstest flight.
Therefore I consider this and ...

> 2: replace assertions in '0' debug key stack dumping

... this bugfixes for 4.16, while clearly ...

> 3: drop "vcpu" local variable from show_guest_stack()

... this one is only cleanup.

Jan

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

[Roger Pau Monné]

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@suse.com>
> ---
> 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@citrix.com>

Thanks, Roger.

Re: [PATCH 2/3] x86/PV: replace assertions in '0' debug key stack dumping

[Roger Pau Monné]

On Wed, Sep 29, 2021 at 11:42:54AM +0200, Jan Beulich wrote:
> While it was me to add them, I'm afraid I don't see justification for
> the assertions: A vCPU may very well have got preempted while in user
> mode. Limit compat guest user mode stack dumps to the containing page
> (like is done when using do_page_walk()), and suppress their dumping
> altogether for 64-bit Dom0.

I'm slightly lost by this last sentence...

> Fixes: cc0de53a903c ("x86: improve output resulting from sending '0' over serial")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> An alternative to suppressing the dump for 64-bit would be to make
> do_page_fault() guest-user-mode aware.
> 
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -254,7 +254,6 @@ static void compat_show_guest_stack(stru
>          struct vcpu *vcpu;
>          unsigned long mfn;
>  
> -        ASSERT(guest_kernel_mode(v, regs));
>          mfn = read_cr3() >> PAGE_SHIFT;
>          for_each_vcpu( v->domain, vcpu )
>              if ( pagetable_get_pfn(vcpu->arch.guest_table) == mfn )
> @@ -269,6 +268,8 @@ static void compat_show_guest_stack(stru
>              }
>              mask = PAGE_SIZE;
>          }
> +        else if ( !guest_kernel_mode(v, regs) )
> +            mask = PAGE_SIZE;
>      }
>  
>      for ( i = 0; i < debug_stack_lines * 8; i++ )
> @@ -328,7 +329,12 @@ static void show_guest_stack(struct vcpu
>      {
>          struct vcpu *vcpu;
>  
> -        ASSERT(guest_kernel_mode(v, regs));
> +        if ( !guest_kernel_mode(v, regs) )
> +        {
> +            printk("User mode stack\n");
> +            return;
> +        }

...as you seem to unconditionally prevent the dump regardless of
whether it's dom0 or domU as long as it's not a kernel stack?

I assume when running in PV 64bit mode user-space could be executing a
32bit program and hence Xen could then misprint the stack as a 64bit
one?

Thanks, Roger.

Re: [PATCH 3/3] x86/PV: drop "vcpu" local variable from show_guest_stack()

[Roger Pau Monné]

On Wed, Sep 29, 2021 at 11:43:32AM +0200, Jan Beulich wrote:
> It's not really needed and has been misleading me more than once to try
> and spot its "actual" use(s). It should really have been dropped when
> the 32-bit specific logic was purged from here.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

As it makes the current code clearer. I have one question/concern
below.

> 
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -327,16 +327,13 @@ static void show_guest_stack(struct vcpu
>  
>      if ( v != current )
>      {
> -        struct vcpu *vcpu;
> -
>          if ( !guest_kernel_mode(v, regs) )
>          {
>              printk("User mode stack\n");
>              return;
>          }
>  
> -        vcpu = maddr_get_owner(read_cr3()) == v->domain ? v : NULL;
> -        if ( !vcpu )
> +        if ( maddr_get_owner(read_cr3()) != v->domain )

Wouldn't it be more accurate to check that the current loaded cr3
matches the one used by v?

AFAICT we don't load the cr3 from v, so it's still possible to have
diverging per-vcpu page tables and thus end up dumping wrong data?

Thanks, Roger.

Re: [PATCH 2/3] x86/PV: replace assertions in '0' debug key stack dumping

[Jan Beulich]

On 18.10.2021 16:26, Roger Pau Monné wrote:
> On Wed, Sep 29, 2021 at 11:42:54AM +0200, Jan Beulich wrote:
>> While it was me to add them, I'm afraid I don't see justification for
>> the assertions: A vCPU may very well have got preempted while in user
>> mode. Limit compat guest user mode stack dumps to the containing page
>> (like is done when using do_page_walk()), and suppress their dumping
>> altogether for 64-bit Dom0.
> 
> I'm slightly lost by this last sentence...
> 
>> @@ -328,7 +329,12 @@ static void show_guest_stack(struct vcpu
>>      {
>>          struct vcpu *vcpu;
>>  
>> -        ASSERT(guest_kernel_mode(v, regs));
>> +        if ( !guest_kernel_mode(v, regs) )
>> +        {
>> +            printk("User mode stack\n");
>> +            return;
>> +        }
> 
> ...as you seem to unconditionally prevent the dump regardless of
> whether it's dom0 or domU as long as it's not a kernel stack?

Well, Dom0 comes into play by way of me talking about debug key '0'.
I've replaced "Dom0" by "domains" in the sentence.

> I assume when running in PV 64bit mode user-space could be executing a
> 32bit program and hence Xen could then misprint the stack as a 64bit
> one?

That's not a primary concern, I would think. The real problem is
do_page_walk() doing

    unsigned long mfn = pagetable_get_pfn(v->arch.guest_table);

first thing: No consideration of guest user mode here at all. And
I didn't want to teach it without real need.

Jan

Re: [PATCH 3/3] x86/PV: drop "vcpu" local variable from show_guest_stack()

[Jan Beulich]

On 18.10.2021 16:35, Roger Pau Monné wrote:
> On Wed, Sep 29, 2021 at 11:43:32AM +0200, Jan Beulich wrote:
>> It's not really needed and has been misleading me more than once to try
>> and spot its "actual" use(s). It should really have been dropped when
>> the 32-bit specific logic was purged from here.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> As it makes the current code clearer. I have one question/concern
> below.
> 
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -327,16 +327,13 @@ static void show_guest_stack(struct vcpu
>>  
>>      if ( v != current )
>>      {
>> -        struct vcpu *vcpu;
>> -
>>          if ( !guest_kernel_mode(v, regs) )
>>          {
>>              printk("User mode stack\n");
>>              return;
>>          }
>>  
>> -        vcpu = maddr_get_owner(read_cr3()) == v->domain ? v : NULL;
>> -        if ( !vcpu )
>> +        if ( maddr_get_owner(read_cr3()) != v->domain )
> 
> Wouldn't it be more accurate to check that the current loaded cr3
> matches the one used by v?
> 
> AFAICT we don't load the cr3 from v, so it's still possible to have
> diverging per-vcpu page tables and thus end up dumping wrong data?

I think truly per-CPU page tables in a PV guest would be quite
cumbersome to have. And iirc (it's been over 12 years since introducing
that logic) the check also isn't about the CR3 we're on being the one
associated with the present vCPU, but merely whether we can expect that
page to be mapped. This would also fall apart when a remote vCPU's
stack isn't accessible on the current CPU for whichever other reason.
I'd be inclined to further tighten this (and force more cases through
do_page_walk()) when we see evidence that we need doing so.

Jan

Re: [PATCH 2/3] x86/PV: replace assertions in '0' debug key stack dumping

[Roger Pau Monné]

On Mon, Oct 18, 2021 at 04:37:27PM +0200, Jan Beulich wrote:
> On 18.10.2021 16:26, Roger Pau Monné wrote:
> > On Wed, Sep 29, 2021 at 11:42:54AM +0200, Jan Beulich wrote:
> >> While it was me to add them, I'm afraid I don't see justification for
> >> the assertions: A vCPU may very well have got preempted while in user
> >> mode. Limit compat guest user mode stack dumps to the containing page
> >> (like is done when using do_page_walk()), and suppress their dumping
> >> altogether for 64-bit Dom0.
> > 
> > I'm slightly lost by this last sentence...
> > 
> >> @@ -328,7 +329,12 @@ static void show_guest_stack(struct vcpu
> >>      {
> >>          struct vcpu *vcpu;
> >>  
> >> -        ASSERT(guest_kernel_mode(v, regs));
> >> +        if ( !guest_kernel_mode(v, regs) )
> >> +        {
> >> +            printk("User mode stack\n");
> >> +            return;
> >> +        }
> > 
> > ...as you seem to unconditionally prevent the dump regardless of
> > whether it's dom0 or domU as long as it's not a kernel stack?
> 
> Well, Dom0 comes into play by way of me talking about debug key '0'.
> I've replaced "Dom0" by "domains" in the sentence.

Thanks, with that:

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

> > I assume when running in PV 64bit mode user-space could be executing a
> > 32bit program and hence Xen could then misprint the stack as a 64bit
> > one?
> 
> That's not a primary concern, I would think. The real problem is
> do_page_walk() doing
> 
>     unsigned long mfn = pagetable_get_pfn(v->arch.guest_table);
> 
> first thing: No consideration of guest user mode here at all. And
> I didn't want to teach it without real need.

Oh, indeed. It would need to use guest_table_user at least.

Roger.