| Message ID | 20210427142113.31961-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] x86/oprofile: remove compat accessors usage from backtrace | expand |
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
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
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.
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.
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
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
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; } /*
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(-)