1. Patchew
  2. Xen
  3. View series

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

Roger Pau Monne posted 1 patch 2 years, 12 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210427142113.31961-1-roger.pau@citrix.com
xen/arch/x86/oprofile/backtrace.c | 41 +++++++++----------------------
1 file changed, 11 insertions(+), 30 deletions(-)
[PATCH v3] x86/oprofile: remove compat accessors usage from backtrace
Posted by Roger Pau Monne 2 years, 12 months ago 
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@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Keep accessibility check for one frame_head beyond.
 - Fix coding style.

Changes since v1:
 - Expand commit message.
---
 xen/arch/x86/oprofile/backtrace.c | 41 +++++++++----------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/oprofile/backtrace.c b/xen/arch/x86/oprofile/backtrace.c
index bd5d1b0f6ce..21b14f63bdb 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,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)) )
             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)) )
+        return 0;
     
-    if (!xenoprof_add_trace(vcpu, bufhead.ret, mode))
+    if ( !xenoprof_add_trace(vcpu, bufhead[0].ret, mode) )
         return 0;
     
     /* frame pointers should strictly progress back up the stack
      * (towards higher addresses) */
-    if (head >= bufhead.ebp)
+    if ( head >= bufhead[0].ebp )
         return NULL;
     
-    return bufhead.ebp;
+    return bufhead[0].ebp;
 }
 
 /*
-- 
2.30.1
Re: [PATCH v3] x86/oprofile: remove compat accessors usage from backtrace
Posted by Jan Beulich 2 years, 12 months ago 
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@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> 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@suse.com>

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
Re: [PATCH v3] x86/oprofile: remove compat accessors usage from backtrace
Posted by Roger Pau Monné 2 years, 12 months ago 
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@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > 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@suse.com>
> 
> 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.
Re: [PATCH v3] x86/oprofile: remove compat accessors usage from backtrace
Posted by Jan Beulich 2 years, 12 months ago 
On 27.04.2021 20:09, Roger Pau Monné wrote:
> 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@citrix.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> 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@suse.com>
>>
>> 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?

Of course - there's no reason at all for you to bother re-sending.
Assuming of course Andrew can live with this effectively halfway
simplification. (If not, as said, I wouldn't object to the earlier
version going in, but I wouldn't want to commit it myself.)

Jan
Re: [PATCH v3] x86/oprofile: remove compat accessors usage from backtrace
Posted by Jan Beulich 2 years, 12 months ago 
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.

Oh, one further question: I suppose you mean historic Linux here? I
can't find anything like this anymore in current one. If so, I'm
inclined to suggest "..., like used to be done in the Linux code."
Or something substantially similar.

Jan
Re: [PATCH v3] x86/oprofile: remove compat accessors usage from backtrace
Posted by Roger Pau Monné 2 years, 12 months ago 
On Tue, Apr 27, 2021 at 05:37:22PM +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.
> 
> Oh, one further question: I suppose you mean historic Linux here? I
> can't find anything like this anymore in current one. If so, I'm
> inclined to suggest "..., like used to be done in the Linux code."
> Or something substantially similar.

Oh, so my local copy of Linux seems to be circa 5.11, and AFAICT the
code was removed in 5.12. I can reword as: "..., like it's done in the
Linux code in 5.11 and earlier versions".

Thanks, Roger.
Re: [PATCH v3] x86/oprofile: remove compat accessors usage from backtrace
Posted by Jan Beulich 2 years, 12 months ago 
On 27.04.2021 20:07, Roger Pau Monné wrote:
> On Tue, Apr 27, 2021 at 05:37:22PM +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.
>>
>> Oh, one further question: I suppose you mean historic Linux here? I
>> can't find anything like this anymore in current one. If so, I'm
>> inclined to suggest "..., like used to be done in the Linux code."
>> Or something substantially similar.
> 
> Oh, so my local copy of Linux seems to be circa 5.11, and AFAICT the
> code was removed in 5.12. I can reword as: "..., like it's done in the
> Linux code in 5.11 and earlier versions".

Oh, I will admit I didn't look at the date of the removal commit.
I just double checked that it didn't get moved elsewhere.

Jan

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.