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

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

On Fri, Apr 23, 2021 at 02:53:14PM +0200, Jan Beulich wrote:
> On 23.04.2021 14:35, 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.
> > 
> > Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Just build tested. Note sure I'm missing something, since using the
> > compat layer here was IMO much more complicated than just using the
> > raw accessors.
> 
> The main reason, I suppose, was that raw_copy_*() aren't supposed to
> be used directly.
> 
> > @@ -59,34 +56,17 @@ dump_guest_backtrace(struct vcpu *vcpu, const struct 
> > frame_head *head,
> >  {
> >      frame_head_t bufhead;
> >  
> > -#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 };
> 
> You're losing the truncation to 32 bits here.

I didn't think it was relevant here, as value should already have the
high bits zeroed?

One being the rbp from the guest cpu registers and the other a
recursive call using bufhead.ebp.

> >          frame_head32_t bufhead32;
> >  
> > -        /* Also check accessibility of one struct frame_head beyond */
> > -        if (!compat_handle_okay(guest_head, 2))
> > -            return 0;
> 
> If you intentionally remove this and ...
> 
> > -        if (__copy_from_compat(&bufhead32, guest_head, 1))
> > +        if (raw_copy_from_guest(&bufhead32, head, sizeof(bufhead32)))
> >              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);
> > -
> > -        /* Also check accessibility of one struct frame_head beyond */
> > -        if (!guest_handle_okay(guest_head, 2))
> > -            return 0;
> 
> ... this, then you should justify why these aren't needed anymore
> (or maybe were never really needed). They've been put there for a
> purpose, I'm sure, even if I'm unclear about what one it was/is.

I've been doing some archaeology on Linux and I'm still not sure why
this is required. Linux copies two frame_head{32,}_t elements, so we
could follow suit and do the same - albeit I won't be able to provide
a reasoning myself as to why this is required, the second element
seems to be completely unused.

Thanks, Roger.