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