Xen devel - Jan 2012 - [PATCH 2/3] xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor

xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor

The dump_guest_backtrace() function attempted to walk the stack
based on the assumption that the guest and hypervisor pointer sizes
were the same; thus any 32-bit guest running under 64-bit hypervisor
would have unreliable results.

In 64-bit mode, read the 32-bit stack frame properly.

Signed-off-by: Marcus Granado <marcus.granado@eu.citrix.com>
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>


diff -r f6953e89913f -r 21e7744a52b1 xen/arch/x86/oprofile/backtrace.c
--- a/xen/arch/x86/oprofile/backtrace.c    Wed Jan 18 17:23:02 2012 +0000
+++ b/xen/arch/x86/oprofile/backtrace.c    Wed Jan 18 17:35:06 2012 +0000
@@ -20,6 +20,13 @@ struct frame_head {
      unsigned long ret;
  } __attribute__((packed));

+#ifdef CONFIG_X86_64
+struct frame_head_32bit {
+    uint32_t ebp;
+    unsigned long ret;
+} __attribute__((packed));
+#endif
+
  static struct frame_head *
  dump_hypervisor_backtrace(struct domain *d, struct vcpu *vcpu,
                struct frame_head * head, int mode)
@@ -35,19 +42,46 @@ dump_hypervisor_backtrace(struct domain
      return head->ebp;
  }

+#ifdef CONFIG_X86_64
+static inline int is_32bit_vcpu(struct vcpu *vcpu)
+{
+    if (is_hvm_vcpu(vcpu))
+        return !hvm_long_mode_enabled(vcpu);
+    else
+        return is_pv_32bit_vcpu(vcpu);
+}
+#endif
+
  static struct frame_head *
  dump_guest_backtrace(struct domain *d, struct vcpu *vcpu,
               struct frame_head * head, int mode)
  {
      struct frame_head bufhead[2];
      XEN_GUEST_HANDLE(char) guest_head = guest_handle_from_ptr(head, char);
-
-    /* Also check accessibility of one struct frame_head beyond */
-    if (!guest_handle_okay(guest_head, sizeof(bufhead)))
-        return 0;
-    if (__copy_from_guest_offset((char *)bufhead, guest_head, 0,
-                                 sizeof(bufhead)))
-        return 0;
+
+#ifdef CONFIG_X86_64
+    if ( is_32bit_vcpu(vcpu) )
+    {
+        struct frame_head_32bit bufhead32[2];
+        /* Also check accessibility of one struct frame_head beyond */
+        if (!guest_handle_okay(guest_head, sizeof(bufhead32)))
+            return 0;
+        if (__copy_from_guest_offset((char *)bufhead32, guest_head, 0,
+                                     sizeof(bufhead32)))
+            return 0;
+        bufhead[0].ebp=(struct frame_head *)(unsigned 
long)bufhead32[0].ebp;
+        bufhead[0].ret=bufhead32[0].ret;
+    }
+    else
+#endif
+    {
+        /* Also check accessibility of one struct frame_head beyond */
+        if (!guest_handle_okay(guest_head, sizeof(bufhead)))
+            return 0;
+        if (__copy_from_guest_offset((char *)bufhead, guest_head, 0,
+                                     sizeof(bufhead)))
+            return 0;
+    }

      if (!xenoprof_add_trace(d, vcpu, bufhead[0].ret, mode))
          return 0;



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 20.01.12 at 19:45, Marcus Granado
<marcus.granado@citrix.com> wrote:
> xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor
> 
> The dump_guest_backtrace() function attempted to walk the stack
> based on the assumption that the guest and hypervisor pointer sizes
> were the same; thus any 32-bit guest running under 64-bit hypervisor
> would have unreliable results.
> 
> In 64-bit mode, read the 32-bit stack frame properly.
> 
> Signed-off-by: Marcus Granado <marcus.granado@eu.citrix.com>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> 
> diff -r f6953e89913f -r 21e7744a52b1 xen/arch/x86/oprofile/backtrace.c
> --- a/xen/arch/x86/oprofile/backtrace.c    Wed Jan 18 17:23:02 2012 +0000
> +++ b/xen/arch/x86/oprofile/backtrace.c    Wed Jan 18 17:35:06 2012 +0000
> @@ -20,6 +20,13 @@ struct frame_head {
>       unsigned long ret;
>   } __attribute__((packed));
> 
> +#ifdef CONFIG_X86_64
> +struct frame_head_32bit {
> +    uint32_t ebp;
> +    unsigned long ret;
unsigned long (i.e. 64 bits)?
> +} __attribute__((packed));
> +#endif
> +
>   static struct frame_head *
>   dump_hypervisor_backtrace(struct domain *d, struct vcpu *vcpu,
>                 struct frame_head * head, int mode)
> @@ -35,19 +42,46 @@ dump_hypervisor_backtrace(struct domain
>       return head->ebp;
>   }
> 
> +#ifdef CONFIG_X86_64
> +static inline int is_32bit_vcpu(struct vcpu *vcpu)
> +{
> +    if (is_hvm_vcpu(vcpu))
> +        return !hvm_long_mode_enabled(vcpu);
> +    else
> +        return is_pv_32bit_vcpu(vcpu);
> +}
> +#endif
> +
>   static struct frame_head *
>   dump_guest_backtrace(struct domain *d, struct vcpu *vcpu,
>                struct frame_head * head, int mode)
>   {
>       struct frame_head bufhead[2];
>       XEN_GUEST_HANDLE(char) guest_head = guest_handle_from_ptr(head,
char);
> -
> -    /* Also check accessibility of one struct frame_head beyond */
> -    if (!guest_handle_okay(guest_head, sizeof(bufhead)))
> -        return 0;
> -    if (__copy_from_guest_offset((char *)bufhead, guest_head, 0,
> -                                 sizeof(bufhead)))
> -        return 0;
> +
> +#ifdef CONFIG_X86_64
> +    if ( is_32bit_vcpu(vcpu) )
> +    {
> +        struct frame_head_32bit bufhead32[2];
> +        /* Also check accessibility of one struct frame_head beyond */
> +        if (!guest_handle_okay(guest_head, sizeof(bufhead32)))
> +            return 0;
> +        if (__copy_from_guest_offset((char *)bufhead32, guest_head, 0,
If you''re adding a compat mode guest case here, then you should
also use compat mode accessors (compat_handle_okay(),
__copy_from_compat_offset()), implying that you also have a local
handle variable of the appropriate type (and perhaps moving the
native one down into the ''else'' body).

Also, as you''re touching this code anyway, the
__copy_from_..._offset() here and below, as they''re passing literal
zero for the offset, can be abbreviated by using __copy_from_...()
(i.e. without the offset).

Jan
> +                                     sizeof(bufhead32)))
> +            return 0;
> +        bufhead[0].ebp=(struct frame_head *)(unsigned
long)bufhead32[0].ebp;
> +        bufhead[0].ret=bufhead32[0].ret;
> +    }
> +    else
> +#endif
> +    {
> +        /* Also check accessibility of one struct frame_head beyond */
> +        if (!guest_handle_okay(guest_head, sizeof(bufhead)))
> +            return 0;
> +        if (__copy_from_guest_offset((char *)bufhead, guest_head, 0,
> +                                     sizeof(bufhead)))
> +            return 0;
> +    }
> 
>       if (!xenoprof_add_trace(d, vcpu, bufhead[0].ret, mode))
>           return 0;

On 23/01/12 09:50, Jan Beulich wrote:
> If you''re adding a compat mode guest case here, then you should
> also use compat mode accessors (compat_handle_okay(),
> __copy_from_compat_offset()), implying that you also have a local
> handle variable of the appropriate type (and perhaps moving the
> native one down into the ''else'' body).
I''m trying to understand the compat handle. It is not clear to me how
to
map one from head (a 64-bit pointer), since COMPAT_HANDLE seems to store 
a 32-bit compat_ptr_t value in its structure. Ideally, what I would like 
to do is

COMPAT_HANDLE(char) guest_head = map_guest_handle_to_compat_handle 
(guest_handle_from_ptr(head, char));
or
COMPAT_HANDLE(char) guest_head = compat_handle_from_ptr(head, char));
but I can''t find any equivalent functions in any header.

The following line compiles,
COMPAT_HANDLE(char) guest_head = { (full_ptr_t)head };
but it looks like, in this case, the compat handle structure in compat.h 
will truncate the most significant bits from the head pointer, so 
compat_handle_okay(guest_head,...) and 
__copy_from_compat(...,guest_head,...) below will be using a truncated 
pointer:

      56 static struct frame_head *
      57 dump_guest_backtrace(struct domain *d, struct vcpu *vcpu,
      58                      struct frame_head * head, int mode)
      59 {
      60     struct frame_head bufhead[2];
      61
      62 #ifdef CONFIG_X86_64
      63     if ( is_32bit_vcpu(vcpu) )
      64     {
      65         COMPAT_HANDLE(char) guest_head = { (full_ptr_t)head };
      66         struct frame_head_32bit bufhead32[2];
      67         /* Also check accessibility of one struct frame_head 
beyond */
      68         if (!compat_handle_okay(guest_head, sizeof(bufhead32)))
      69             return 0;
      70         if (__copy_from_compat((char *)bufhead32, guest_head,
      71                                      sizeof(bufhead32)))
      72             return 0;
      73         bufhead[0].ebp=(struct frame_head 
*)(full_ptr_t)bufhead32[0].ebp;
      74         bufhead[0].ret=bufhead32[0].ret;
      75     }
      76     else
      77 #endif

Any advice? Maybe the best option in this case is to avoid the compat* 
functions and to use the original guest* functions instead.

Thanks,
Marcus

>>> On 24.01.12 at 20:27, Marcus Granado
<marcus.granado@citrix.com> wrote:
> I''m trying to understand the compat handle. It is not clear to me
how to
> map one from head (a 64-bit pointer), since COMPAT_HANDLE seems to store 
That cannot generally be done, as a 64-bit pointer can never be
represented as a compat handle.

Proper conversion has to start at where ''head'' is first
generated (i.e.
the line

    head = (struct frame_head *)regs->ebp;

in xenoprof_backtrace() (the more that here you really *want* to
drop the upper 32 bits in the compat case. Working with a union is
a possible approach, but it may also be acceptable to actually do
the truncation in dump_guest_backtrace(), properly explaining why
the dropping of the upper half is valid and intended there.

(I''ve already put fixing up of this already committed patch on my
todo list, so feel free to drop further attempts; once I''m done
I''d
appreciate review/testing of the code though.)

Jan
> a 32-bit compat_ptr_t value in its structure. Ideally, what I would like 
> to do is
> 
> COMPAT_HANDLE(char) guest_head = map_guest_handle_to_compat_handle 
> (guest_handle_from_ptr(head, char));
> or
> COMPAT_HANDLE(char) guest_head = compat_handle_from_ptr(head, char));
> but I can''t find any equivalent functions in any header.
> 
> The following line compiles,
> COMPAT_HANDLE(char) guest_head = { (full_ptr_t)head };
> but it looks like, in this case, the compat handle structure in compat.h 
> will truncate the most significant bits from the head pointer, so 
> compat_handle_okay(guest_head,...) and 
> __copy_from_compat(...,guest_head,...) below will be using a truncated 
> pointer:
> 
>       56 static struct frame_head *
>       57 dump_guest_backtrace(struct domain *d, struct vcpu *vcpu,
>       58                      struct frame_head * head, int mode)
>       59 {
>       60     struct frame_head bufhead[2];
>       61
>       62 #ifdef CONFIG_X86_64
>       63     if ( is_32bit_vcpu(vcpu) )
>       64     {
>       65         COMPAT_HANDLE(char) guest_head = { (full_ptr_t)head };
>       66         struct frame_head_32bit bufhead32[2];
>       67         /* Also check accessibility of one struct frame_head 
> beyond */
>       68         if (!compat_handle_okay(guest_head, sizeof(bufhead32)))
>       69             return 0;
>       70         if (__copy_from_compat((char *)bufhead32, guest_head,
>       71                                      sizeof(bufhead32)))
>       72             return 0;
>       73         bufhead[0].ebp=(struct frame_head 
> *)(full_ptr_t)bufhead32[0].ebp;
>       74         bufhead[0].ret=bufhead32[0].ret;
>       75     }
>       76     else
>       77 #endif
> 
> Any advice? Maybe the best option in this case is to avoid the compat* 
> functions and to use the original guest* functions instead.
> 
> Thanks,
> Marcus