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 )
         {

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.

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

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.