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

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

[Roger Pau Monne]

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@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
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.
---
 xen/arch/x86/oprofile/backtrace.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/oprofile/backtrace.c b/xen/arch/x86/oprofile/backtrace.c
index bd5d1b0f6ce..45f7fb65fa2 100644
--- a/xen/arch/x86/oprofile/backtrace.c
+++ b/xen/arch/x86/oprofile/backtrace.c
@@ -20,7 +20,6 @@ struct __packed frame_head {
     unsigned long ret;
 };
 typedef struct frame_head frame_head_t;
-DEFINE_XEN_GUEST_HANDLE(frame_head_t);
 
 struct __packed frame_head_32bit {
     uint32_t ebp;
@@ -43,7 +42,6 @@ dump_hypervisor_backtrace(struct vcpu *vcpu, const struct frame_head *head,
     return head->ebp;
 }
 
-#ifdef CONFIG_COMPAT
 static inline int is_32bit_vcpu(struct vcpu *vcpu)
 {
     if (is_hvm_vcpu(vcpu))
@@ -51,7 +49,6 @@ 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,
@@ -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 };
         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))
+        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;
-        if (__copy_from_guest(&bufhead, guest_head, 1))
-            return 0;
-    }
+    else if (raw_copy_from_guest(&bufhead, head, sizeof(bufhead)))
+        return 0;
     
     if (!xenoprof_add_trace(vcpu, bufhead.ret, mode))
         return 0;
-- 
2.30.1

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

[Andrew Cooper]

On 23/04/2021 13: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@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> 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.

Thankyou, and yes - I agree.  The logic was unnecessarily complicated.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Needs a fixes tag, but can be sorted on commit.

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

[Jan Beulich]

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@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> 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.

>          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.

Jan

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

[Jan Beulich]

On 23.04.2021 14:51, Andrew Cooper wrote:
> On 23/04/2021 13: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@citrix.com>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> 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.
> 
> Thankyou, and yes - I agree.  The logic was unnecessarily complicated.
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Needs a fixes tag, but can be sorted on commit.

Fixes what?

Jan

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

[Andrew Cooper]

On 23/04/2021 13:53, 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@citrix.com>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> 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.
>
>>          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 don't see what purpose they serve at all.  The OK checks only look for
encroachment into the Xen virtual range.

The two cases where this does anything, is userspace with a stack
immediately adjacent to the lower canonical boundary (which doesn't
happen in practice because of the astounding number of errata there), or
for PV32 kernels with a stack at the legacy Xen boundary.

~Andrew

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

[Roger Pau Monné]

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@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > 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.

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

[Jan Beulich]

On 23.04.2021 15:51, Roger Pau Monné wrote:
> 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@citrix.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> 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.

Would you mind stating this (i.e. what guarantees the zero-extension)
in the description?

>>>          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.

Well, my guess has always been that this is an attempt at making sure
the stack with the frame popped is still a valid one. This would still
seem to me to be a somewhat questionable reasoning for the two-element
access, but I couldn't think of any other possible thoughts behind
this.

Jan