Re: [Xen-devel] [Xen-Devel] [PATCH] [GSOC14] refactored mempaging code from xenpaging to libxc.

[Andres Lagar Cavilla <andres@xxxxxxxxxxxxxxxx>]

On Fri, Jun 13, 2014 at 8:55 AM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 13/06/14 16:07, Andres Lagar Cavilla wrote:

On Fri, Jun 13, 2014 at 7:51 AM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 13/06/14 15:16, Dushyant Behl wrote:
> This patch is part of the work done under the gsoc project -
> Lazy Restore Using Memory Paging.
>
> This patch moves the code to initialize mempaging from xenpaging to libxc.
> The code refractored from xenpaging is the code which sets up paging,
> initializes a shared ring and event channel to communicate with xen. This
> communication is done between the hypervisor and the dom0 tool which performs
> the activity of pager. The xenpaging code is changed to use the newly created
> routines and is tested to properly build and work with this code.
>
> The refractoring is done so that any tool which will act as pager in
> lazy restore or use memory paging can use a same routines to initialize mempaging.
> This refactoring will also allow any future (in-tree) tools to use mempaging.
>
> The refractored code in xc_mem_paging_ring_setup is to be compiled into
> libxenguest.
>
> Signed-off-by: Dushyant Behl <myselfdushyantbehl@xxxxxxxxx>
> Reviewed-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
> Acked-by: David Scott <dave.scott@xxxxxxxxxx>
> ---
> Âtools/libxc/Makefile        Â|  2 +
> Âtools/libxc/xc_mem_paging_setup.c  | 135 ++++++++++++++++++++++++++++++++++++
> Âtools/libxc/xenctrl.h        | Â15 ++++
> Âtools/ocaml/libs/xc/xenctrl_stubs.c | Â11 +--
> Âtools/xenpaging/Makefile      Â|  4 +-
> Âtools/xenpaging/xenpaging.c     | Â93 +++----------------------
> Â6 files changed, 172 insertions(+), 88 deletions(-)
> Âcreate mode 100644 tools/libxc/xc_mem_paging_setup.c
>
> diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
> index a74b19e..6cf14f0 100644
> --- a/tools/libxc/Makefile
> +++ b/tools/libxc/Makefile
> @@ -69,6 +69,8 @@ GUEST_SRCS-$(CONFIG_ARM) Â Â += xc_dom_armzimageloader.c
> ÂGUEST_SRCS-y         += xc_dom_binloader.c
> ÂGUEST_SRCS-y         += xc_dom_compat_linux.c
>
> +GUEST_SRCS-y         += xc_mem_paging_setup.c
> +
> ÂGUEST_SRCS-$(CONFIG_X86) Â Â += xc_dom_x86.c
> ÂGUEST_SRCS-$(CONFIG_X86) Â Â += xc_cpuid_x86.c
> ÂGUEST_SRCS-$(CONFIG_X86) Â Â += xc_hvm_build_x86.c
> diff --git a/tools/libxc/xc_mem_paging_setup.c b/tools/libxc/xc_mem_paging_setup.c
> new file mode 100644
> index 0000000..7b3ab38
> --- /dev/null
> +++ b/tools/libxc/xc_mem_paging_setup.c
> @@ -0,0 +1,135 @@
> +/*
> + * tools/libxc/xc_mem_paging_setup.c
> + *
> + * Routines to initialize memory paging. Create shared ring
> + * and event channels to communicate with the hypervisor.
> + *
> + * Copyright (c) 2014 Dushyant Behl
> + * Copyright (c) 2009 by Citrix Systems, Inc. (Patrick Colp)
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ÂSee the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA Â02110-1301 USA
> + */
> +
> +#include "xc_private.h"
> +#include <xen/event_channel.h>
> +#include <xen/mem_event.h>
> +
> +/*
> + * Mem paging ring and event channel setup routine.
> + * Setup a shared ring and an event channel to communicate between
> + * hypervisor and the tool performing mem paging operations.
> + * The function will return zero on successful completion and will
> + * return -1 on failure at any intermediate step setting up errno
> + * properly.
> + */
> +int xc_mem_paging_ring_setup(xc_interface *xch, domid_t domain_id,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âvoid *ring_page, int *port,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âuint32_t *evtchn_port,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âxc_evtchn **xce_handle,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âmem_event_back_ring_t *back_ring)
> +{
> + Â Âint rc;
> + Â Âuint64_t ring_pfn, mmap_pfn;
> +
> + Â Â/* Map the ring page */
> + Â Âxc_get_hvm_param(xch, domain_id, HVM_PARAM_PAGING_RING_PFN, &ring_pfn);
> + Â Âmmap_pfn = ring_pfn;
> + Â Âring_page = xc_map_foreign_batch(xch, domain_id,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂPROT_READ | PROT_WRITE, &mmap_pfn, 1);
> + Â Âif ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
> + Â Â{
> + Â Â Â Â/* Map failed, populate ring page */
> + Â Â Â Ârc = xc_domain_populate_physmap_exact(xch, domain_id,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â1, 0, 0, &ring_pfn);
> + Â Â Â Âif ( rc != 0 )
> + Â Â Â Â{
> + Â Â Â Â Â ÂPERROR("Failed to populate ring gfn\n");
> + Â Â Â Â Â Âreturn -1;
> + Â Â Â Â}
> +
> + Â Â Â Âmmap_pfn = ring_pfn;
> + Â Â Â Âring_page = xc_map_foreign_batch(xch, domain_id,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂPROT_READ | PROT_WRITE, &mmap_pfn, 1);
> +
> + Â Â Â Âif ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
> + Â Â Â Â{
> + Â Â Â Â Â ÂPERROR("Could not map the ring page\n");
> + Â Â Â Â Â Âreturn -1;
> + Â Â Â Â}
> + Â Â}
> +
> + Â Â/* Initialise Xen */
> + Â Ârc = xc_mem_paging_enable(xch, domain_id, evtchn_port);
> + Â Âif ( rc != 0 )
> + Â Â{
> + Â Â Â Âswitch ( errno ) {
> + Â Â Â Â Â Âcase EBUSY:
> + Â Â Â Â Â Â Â ÂERROR("mempaging is (or was) active on this domain");
> + Â Â Â Â Â Â Â Âbreak;
> + Â Â Â Â Â Âcase ENODEV:
> + Â Â Â Â Â Â Â ÂERROR("mempaging requires Hardware Assisted Paging");
> + Â Â Â Â Â Â Â Âbreak;
> + Â Â Â Â Â Âcase EMLINK:
> + Â Â Â Â Â Â Â ÂERROR("mempaging not supported while iommu passthrough is enabled");
> + Â Â Â Â Â Â Â Âbreak;
> + Â Â Â Â Â Âcase EXDEV:
> + Â Â Â Â Â Â Â ÂERROR("mempaging not supported in a PoD guest");
> + Â Â Â Â Â Â Â Âbreak;
> + Â Â Â Â Â Âdefault:
> + Â Â Â Â Â Â Â ÂPERROR("mempaging: error initialising shared page");
> + Â Â Â Â Â Â Â Âbreak;
> + Â Â Â Â}
> + Â Â Â Âreturn -1;
> + Â Â}
> +
> + Â Â/* Open event channel */
> + Â Â*xce_handle = xc_evtchn_open(NULL, 0);
> + Â Âif ( *xce_handle == NULL )
> + Â Â{
> + Â Â Â ÂPERROR("Failed to open event channel");
> + Â Â Â Âreturn -1;
> + Â Â}

This is inappropriate inside libxenguest. ÂThe user of the library
possibly already has open evtchn handle. ÂWhile this is only wasteful of
an fd under linux, I believe it will result in open() failing under some
of the *BSDs

The correct way of doing this is to have the caller of
xc_mem_paging_ring_setup() provide their xce_handle, and require them to
open one if they need to.

I think this is heavy handed. I mean, the idea here is to relieve the caller from doing all the setup work.

calling xc_evtchn_open() is *not* something which should ever be relieved from the caller.

In fact, you could argue that a follow-up patch should encapsulate all the cleanup.

And then consumers of this particular module call setup, teardown, use the intermediate result, no concerns.

I realise I am not a maintainer, so ultimately it is Ian/Ian you have to convince in this matter.


If I were doing this, there would be two patches. The first touches libxc alone and introduces this setup function and a teardown function, both written correctly and bugfree. The second patch modifies xenpaging to use the new setup and teardown functions.

As it currently stands, this is a halfbaked copy and paste of too much setup code, moving into a library without its equivalent teardown, and a promise of a fixup patch.

Ok, so next submit should have both setup and teardown. I'm guessing the Ian's will have final word on the evtchn thing?

Â

LGTM, to be honest.Â

> +
> + Â Â/* Bind event notification */
> + Â Ârc = xc_evtchn_bind_interdomain(*xce_handle, domain_id, *evtchn_port);
> + Â Âif ( rc < 0 )
> + Â Â{
> + Â Â Â ÂPERROR("Failed to bind event channel");
> + Â Â Â Âreturn -1;
> + Â Â}
> + Â Â*port = rc;
> +
> + Â Â/* Initialise ring */
> + Â ÂSHARED_RING_INIT((mem_event_sring_t *)ring_page);
> + Â ÂBACK_RING_INIT(back_ring, (mem_event_sring_t *)ring_page, PAGE_SIZE);
> +
> + Â Â/* Now that the ring is set, remove it from the guest's physmap */
> + Â Âif ( xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, &ring_pfn) )
> + Â Â{
> + Â Â Â ÂPERROR("Failed to remove ring from guest physmap");
> + Â Â Â Âreturn -1;
> + Â Â}

There is a race condition here where the guest can play with the
post-initialised ring state.

Well-known, not the place to fix it here.

I don't know how exactly the security policy applies here. My understanding is that the hypervisor side paging code has been declared stable and therefore subject to XSAs.

The libxc codes is, is far as I can tell, just thin wrappers around the hypercalls. At the moment, the xenpaging utility is just example code, so is probably fine from an XSA point of view.

However, moving code from xenpaging into libxc changes this split.


Speaking now with my XenServer security hat on,

Irrespective of whether it would technically be declared an XSA or not, it is stupid to knowingly put a latent security bug in a library. The best case is that the development tree has a security bug in it for a while. The worst case is that you forget to fix it before the next Xen release and everyone in the world picks up the security bug as part of their stable code.

You do realize all this does is put a ring into the guest addressable memory. Exactly what PV drivers do, and Xen on behalf of qemu does.

So ... ?

AndresÂ

So no - this is absolutely the place to fix it.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel