| Message ID | 20210423123509.9354-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/oprofile: remove compat accessors usage from backtrace | expand |
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.
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
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
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
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.
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
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;
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(-)