* [PATCH] x86/oprofile: remove compat accessors usage from backtrace
@ 2021-04-23 12:35 Roger Pau Monne
2021-04-23 12:51 ` Andrew Cooper
2021-04-23 12:53 ` Jan Beulich
0 siblings, 2 replies; 7+ messages in thread
From: Roger Pau Monne @ 2021-04-23 12:35 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper, Wei Liu
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/oprofile: remove compat accessors usage from backtrace
2021-04-23 12:35 [PATCH] x86/oprofile: remove compat accessors usage from backtrace Roger Pau Monne
@ 2021-04-23 12:51 ` Andrew Cooper
2021-04-23 12:54 ` Jan Beulich
2021-04-23 12:53 ` Jan Beulich
1 sibling, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2021-04-23 12:51 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Jan Beulich, Wei Liu
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/oprofile: remove compat accessors usage from backtrace
2021-04-23 12:51 ` Andrew Cooper
@ 2021-04-23 12:54 ` Jan Beulich
0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2021-04-23 12:54 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, xen-devel, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/oprofile: remove compat accessors usage from backtrace
2021-04-23 12:35 [PATCH] x86/oprofile: remove compat accessors usage from backtrace Roger Pau Monne
2021-04-23 12:51 ` Andrew Cooper
@ 2021-04-23 12:53 ` Jan Beulich
2021-04-23 13:40 ` Andrew Cooper
2021-04-23 13:51 ` Roger Pau Monné
1 sibling, 2 replies; 7+ messages in thread
From: Jan Beulich @ 2021-04-23 12:53 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/oprofile: remove compat accessors usage from backtrace
2021-04-23 12:53 ` Jan Beulich
@ 2021-04-23 13:40 ` Andrew Cooper
2021-04-23 13:51 ` Roger Pau Monné
1 sibling, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2021-04-23 13:40 UTC (permalink / raw)
To: Jan Beulich, Roger Pau Monne; +Cc: Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/oprofile: remove compat accessors usage from backtrace
2021-04-23 12:53 ` Jan Beulich
2021-04-23 13:40 ` Andrew Cooper
@ 2021-04-23 13:51 ` Roger Pau Monné
2021-04-23 13:55 ` Jan Beulich
1 sibling, 1 reply; 7+ messages in thread
From: Roger Pau Monné @ 2021-04-23 13:51 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, xen-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/oprofile: remove compat accessors usage from backtrace
2021-04-23 13:51 ` Roger Pau Monné
@ 2021-04-23 13:55 ` Jan Beulich
0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2021-04-23 13:55 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Andrew Cooper, Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-04-23 13:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-23 12:35 [PATCH] x86/oprofile: remove compat accessors usage from backtrace Roger Pau Monne
2021-04-23 12:51 ` Andrew Cooper
2021-04-23 12:54 ` Jan Beulich
2021-04-23 12:53 ` Jan Beulich
2021-04-23 13:40 ` Andrew Cooper
2021-04-23 13:51 ` Roger Pau Monné
2021-04-23 13:55 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).