Re: [PATCH v3] x86/oprofile: remove compat accessors usage from backtrace

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxx>
>>
>> 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