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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.01.12 at 19:45, Marcus Granado <marcus.granado@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> 
> 
> 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;




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