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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/04/2021 13:53, 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.
>
>>          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 don't see what purpose they serve at all.  The OK checks only look for
encroachment into the Xen virtual range.

The two cases where this does anything, is userspace with a stack
immediately adjacent to the lower canonical boundary (which doesn't
happen in practice because of the astounding number of errata there), or
for PV32 kernels with a stack at the legacy Xen boundary.

~Andrew