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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Apr 27, 2021 at 05:31:25PM +0200, Jan Beulich wrote:
> 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.

Sure, the & is a leftover from when bufhead wasn't an array, or else I
wouldn't have added it.

Would you be OK to drop the '&' and adjust the message mentioning
Linux <= 5.11 on commit?

If not I don't mind sending an updated version.

Thanks, Roger.