[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>, Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Fri, 23 Apr 2021 14:40:03 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=YMAidUQ0DrttpQtCJRf4LM3szzqXRUunIkHby37Sh9g=; b=D9czFeRnVHbNiKZDWSC5XGbNOGpPQeVfGlrgVQQSYqG4kAZvUbqW8QiPhp0kqXnZctw12EVlZNdCXqAfzyzP4RP/ndRzWEeft3Lj+qFbOL8FJDy1OBGePFRQRTj7KKh96Ghw/IpLNRU2Y/CuDeDOj6vBzDwrzFgSXSUVRkXRTO46FBodTnIP0u9W208eoVgfdwLICvu/tF7V4ifxPJORpss90cjOJGQlyfsSHbnJ6jbL2oKLNa8Fe1kCV5yLxvpMpCB+enSzMs7qTfia38QWj/Q3A44BTaOSUUcIm9FUTYJNaxg7FJU/q542RSezJ6q3S6TMXDa1fP5Egd0hHrFLGQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=lQOJJ3qJlflyySO1t4/A+RVpWknxfUijVUK2pSPxp8aa8vDjcUBve6h+AxATyhxxVbE411bijifo6Hz21bA7Wy9HaMHwck7SH7UK5+BwlLWm049j4WsBzgq9W7YAiFFaW35XFXOXUU+2InB4lP0cK33IS0LYtSyazqCDuoXTERCc79n9lx5LcBRtL8EAzbKdy2X45TcN+KpjsWVnEWh1YZY/v+BaWGasaV/K5RI4HDL4jVDoD98lsn7S2QDfyrt1x++5VfG8VS1Ln/DcS/Zh9hiU8rVcU6m8x1itwwpXoKVbenKES/B2b+XroJ1N+FcTW1Mj+PdWsU2UNSaeeJ+5hQ==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Wei Liu <wl@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 23 Apr 2021 13:40:35 +0000
  • Ironport-hdrordr: A9a23:udVpEKmptTkuIgBvBFxOVCCKyivpDfOBj2dD5ilNYBxZY6Wkvu iUtrAyyQL0hDENWHsphNCHP+26TWnB8INuiLN/AZ6LZyOjnGezNolt4c/ZwzPmEzDj7eI178 tdWoBEIpnLAVB+5PyW3CCRD8sgzN6b8KqhmOfZyDNXQRt3brx7hj0ZNi+wOCRNNWx7LLA+E4 eR4dcCgjKmd2geYMjTPAh7Y8HoodrXmJX6JSMcDxk85wWUyR+u4rj2Ex+Xty1uLw9n67Ek7G TDjkjF9ryu2svLtyP0+k3yy9BtmNXnwsZeH8DksKYoAxjllwrAXvUYZ5SspzYwydvfj2oCsN 6JmBs4OtQ21nW5RBDOnTLI+y3NlAkj8GXjz1jwuwqRneXcSCghA8RMwaJ1GyGpknYIh9133K JV02/xjfM+Znmh7UeNkuTgbB1kmlG5pnAvi4co/hhieLATdaNLqsgn9F5Vea1wbB7S0pwtE+ VlEajnlZBrWG6dBkqp3FVH/MahRTAaEBuAXyE5y7eo+gkTtnV4w0wE/dcYj3cN+bksIqM0lt jsA+BGkqpDQdQRar84LOAdQdGvAmiIeh7UNnmOSG6XW50vCjbokdra8b817OaldNghy4Yzoo 3IVBd9uXQpc0zjJMWS1PRwg17waVT4eQ6o5tBV5pB/tLG5bqHsKze/RFcnlNbli+kDA+XAMs zDe65+MrvGFy/DCIxJ1wrxV915Mn8FSvAYvd49Rhanvt/LEIv3rebWGcyjZIbFIHIBYCfSE3 EDVD/8KIFr9UawQEL1hxDXRjfDYUr60ZVsELXL3uQaxYQXX7c89zQ9uBCc3IWmODdCuqs5cA 9VO7X8iJ62omGw4CLp4gxSS15gJ3cQxI+lf2JBpAcMPU+xW60Eoc+jdWdb22bCAhd+SsjRAT NOvlgfw9PxE7WggQQZT/63OGOTiHUe4FiQSY0Hp6GF7cD5PrQ1E4ghQ640MQnQDRR6lUJLpQ 54GU45b36aMgmrpbSujZQSCu2aXcJ7mh2XLcldrm+ak16dq8EpTn4yRCWvTsaTvAYrS1Nv9x 9M2p5apIDFtSekKGM5juh9GkZLcn6rDLVPCxnAWJ9ZgYnxeAZ7TX6DgBuTjx1bQButy2wiwk jaaQGEc/DCBVRQ/lRVyLzj/l9PemKBRE5ocXxhvYphFWPJh2Zr3YawF9iO+lrUTmFH7vAWMT nDbzdXGA9oytyt/DO+mTqJFxwdt98TF92YKI5mX6DY23urJoHNqLoPGOVM+o15cPr0tPUQbO 6ZcwiJDT/xBu8zwTaJrnI9NCQckgh8rdrYnDneqESo1n82BvTfZGl8T7YAOteG8izKQe2L3J gRt6N9gcKAdkHKLviIxqHcY2Qddlf9oWuqQ/oprp4Rl6Qor7d3F4TaVzyN9Hwv5mRIEO7E0G clBIJ86/T9H6UqWeo4USdQ5EAom9SCN1FDiH28PsYOOXUWy0bGNNaI6YfSobUhAke9tBL9UG PvhxF1zrPgZW+/zrYUBKI7HHROZGU94Hpk+vmed4e4MnTdS8hzuH67OGS6arlTVeysHqgRtA 9z57iz7qKqXhu9/ADbpj1gJK1St06hXMOpGQqJXcpF6cazN1jJoqyk5qeI/XvKYAr+T0QTno tec0MMKuxFlzk5lYUylhGIdZafmDNsr3JupRd9llDs3YC64GDUWWF+WDep/Kl+bH10KXiHjc PM7O6C8m/yiQI1gaX+KA==
  • Ironport-sdr: 3VEhdjWMKy2BGor2x5M3+d2rNgPPsuHHq5j0nXIPg5bHIAN4HZq4Cz/Y8xLEQkxiI1x8bnTVb1 cOIHrRTePaVIuNdjZMMxejI3EGo9PwSoo7wcVxYBeuiFPxJ35oe2jPGaMNbWUwC8DaPx/mFIjw utA+M/rwZaYuJ0ObvxGttF8dcjigb4ZuooMs89OkJoV1cpHB/f7vQsBZ/Id0BKCeImiBHA4y3+ 4yWA9a9nybZFBkQR3S6bcvX3F7CkGllG3YHwJ1HEUROZYdyAKiG0sWQDTHoai2Is1N3ftDDGLb H1o=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.