| Message ID | 20210423143755.12189-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86/oprofile: remove compat accessors usage from backtrace | expand |
On 23.04.2021 16:37, 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 drop the checks for the accessibility of one struct > frame_head beyond the current one: it's not clear why it's needed and > all the hypnoses point to dropping such check being harmless. The DYM "hypotheses"? > worse that could happen is that a failure happens later if data past > frame_head is attempted to be fetched, albeit I'm not able to spot any > such access. > > 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, While I know I'm guilty of splitting hair saying so, I'd like to point out that I'm unaware of guarantees that the upper halves of GPRs are zero after a switch from compat to 64-bit mode. With this I'm also unconvinced there are guarantees that the %rsp stored into a stack frame is actually guaranteed to be zero-extended. Nevertheless I'm not meaning this remark to keep the change from going in as is - for all practical purposes what you say is presumably true. What I would consider nice though is if the two remaining if() could be corrected for coding style: Adjacent code is already inconsistent, so taking the opportunity to move it a little in the right direction would seem desirable to me. (I would suggest doing so myself while committing, but because I don't fully agree with dropping the 2-frame checks described further up without properly understanding why they're there, I'd like to not put my name on this change in any way, not even just as committer. But I guess Andrew or Wei or whoever ends up committing this could do so, as long as they agree of course.) Jan > 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> > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > Changes since v2: > - Expand commit message. > --- > 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; >
On Mon, Apr 26, 2021 at 09:49:13AM +0200, Jan Beulich wrote: > On 23.04.2021 16:37, 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 drop the checks for the accessibility of one struct > > frame_head beyond the current one: it's not clear why it's needed and > > all the hypnoses point to dropping such check being harmless. The > > DYM "hypotheses"? Yes, sorry, selected the wrong spell checker suggestion I guess. > > > worse that could happen is that a failure happens later if data past > > frame_head is attempted to be fetched, albeit I'm not able to spot any > > such access. > > > > 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, > > While I know I'm guilty of splitting hair saying so, I'd like to point > out that I'm unaware of guarantees that the upper halves of GPRs are > zero after a switch from compat to 64-bit mode. With this I'm also > unconvinced there are guarantees that the %rsp stored into a stack > frame is actually guaranteed to be zero-extended. Nevertheless I'm not > meaning this remark to keep the change from going in as is - for all > practical purposes what you say is presumably true. Also, given the context of this code (oprofile backtrace generation), I'm unsure what issues could arise from not using a zero extended value for a guest in 32bit mode apart from a failure to obtain the backtrace itself. > What I would consider nice though is if the two remaining if() could > be corrected for coding style: Adjacent code is already inconsistent, > so taking the opportunity to move it a little in the right direction > would seem desirable to me. (I would suggest doing so myself while > committing, but because I don't fully agree with dropping the 2-frame > checks described further up without properly understanding why they're > there, I'd like to not put my name on this change in any way, not even > just as committer. But I guess Andrew or Wei or whoever ends up > committing this could do so, as long as they agree of course.) OK, I can add the 2-frame checks back in, I certainly don't have that strong opinion and the resulting code will be better just by dropping the usage of the compat layer even if we decide to keep those checks. Let me prepare a new version. Thanks, Roger.
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;