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

[Jan Beulich <jbeulich@xxxxxxxx>]

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