Re: [PATCH v3] x86/oprofile: remove compat accessors usage from backtrace

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.04.2021 16:21, Roger Pau Monne wrote:
> Remove the unneeded usage of the compat layer to copy frame pointers
> from guest address space. Instead just use raw_copy_from_guest.
> 
> While there change the accessibility check of one frame_head beyond to
> be performed as part of the copy, like it's done in the Linux code.
> Note it's unclear why this is needed.
> 
> Also drop the explicit truncation of the head pointer in the 32bit
> case as all callers already pass a zero extended value. The first
> value being rsp from the guest registers, and further calls will use
> ebp from frame_head_32bit struct.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v2:
>  - Keep accessibility check for one frame_head beyond.
>  - Fix coding style.

I'm indeed more comfortable with this variant, so
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Andrew - can you live with the 2-frame thingy staying around?

> @@ -51,52 +49,35 @@ static inline int is_32bit_vcpu(struct vcpu *vcpu)
>      else
>          return is_pv_32bit_vcpu(vcpu);
>  }
> -#endif
>  
>  static struct frame_head *
>  dump_guest_backtrace(struct vcpu *vcpu, const struct frame_head *head,
>                       int mode)
>  {
> -    frame_head_t bufhead;
> +    /* Also check accessibility of one struct frame_head beyond. */
> +    frame_head_t bufhead[2];
>  
> -#ifdef CONFIG_COMPAT
>      if ( is_32bit_vcpu(vcpu) )
>      {
> -        DEFINE_COMPAT_HANDLE(frame_head32_t);
> -        __compat_handle_const_frame_head32_t guest_head =
> -            { .c = (unsigned long)head };
> -        frame_head32_t bufhead32;
> -
> -        /* Also check accessibility of one struct frame_head beyond */
> -        if (!compat_handle_okay(guest_head, 2))
> -            return 0;
> -        if (__copy_from_compat(&bufhead32, guest_head, 1))
> -            return 0;
> -        bufhead.ebp = (struct frame_head *)(unsigned long)bufhead32.ebp;
> -        bufhead.ret = bufhead32.ret;
> -    }
> -    else
> -#endif
> -    {
> -        XEN_GUEST_HANDLE_PARAM(const_frame_head_t) guest_head =
> -            const_guest_handle_from_ptr(head, frame_head_t);
> +        frame_head32_t bufhead32[2];
>  
> -        /* Also check accessibility of one struct frame_head beyond */
> -        if (!guest_handle_okay(guest_head, 2))
> -            return 0;
> -        if (__copy_from_guest(&bufhead, guest_head, 1))
> +        if ( raw_copy_from_guest(&bufhead32, head, sizeof(bufhead32)) )

As a minor remark, personally I'd prefer the & to be dropped here
and ...

>              return 0;
> +        bufhead[0].ebp = (struct frame_head *)(unsigned 
> long)bufhead32[0].ebp;
> +        bufhead[0].ret = bufhead32[0].ret;
>      }
> +    else if ( raw_copy_from_guest(&bufhead, head, sizeof(bufhead)) )

... here (and doing so while committing would be easy), but I'm
not going to insist.

Jan