[PATCH 11/20] xen/domctl: Merge set_gnttab_limits into createdomain

[Andrew Cooper <andrew.cooper3@citrix.com>]

XEN_DOMCTL_set_gnttab_limits is a fairly new hypercall, and is strictly
mandatory.  Adding support for it introduced a state where a domain has a
mostly un-constructed grant table, and there were cases where mis-ordering of
toolstack hypercalls could cause a NULL pointer deference in the hypervisor.
In fixing this, the grant table initialisation code became very tangled.

As the settings are mandatory, delete XEN_DOMCTL_set_gnttab_limits (including
XSM hooks and libxc wrappers) and retain the functionality in
XEN_DOMCTL_createdomain.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Christian Lindig <christian.lindig@citrix.com>
CC: David Scott <dave@recoil.org>
CC: Jon Ludlam <jonathan.ludlam@eu.citrix.com>
CC: Rob Hoes <Rob.Hoes@citrix.com>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Hypervisor side cleanup is present in later patchs
---
 tools/flask/policy/modules/dom0.te   |  2 +-
 tools/flask/policy/modules/xen.if    |  2 +-
 tools/helpers/init-xenstore-domain.c | 19 ++++++++-----------
 tools/libxc/include/xenctrl.h        | 13 -------------
 tools/libxc/xc_domain.c              | 13 -------------
 tools/libxl/libxl_create.c           |  2 ++
 tools/libxl/libxl_dom.c              |  6 ------
 tools/ocaml/libs/xc/xenctrl.ml       |  2 ++
 tools/ocaml/libs/xc/xenctrl.mli      |  2 ++
 tools/ocaml/libs/xc/xenctrl_stubs.c  |  8 +++++++-
 tools/python/xen/lowlevel/xc/xc.c    |  2 ++
 xen/common/domctl.c                  | 34 ++++++++++++++++++++++++++--------
 xen/include/public/domctl.h          | 10 +++-------
 xen/xsm/flask/hooks.c                |  3 ---
 xen/xsm/flask/policy/access_vectors  |  2 --
 15 files changed, 54 insertions(+), 66 deletions(-)

diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index 4eb3843..dfdcdcd 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -39,7 +39,7 @@ allow dom0_t dom0_t:domain {
 };
 allow dom0_t dom0_t:domain2 {
 	set_cpuid gettsc settsc setscheduler set_vnumainfo
-	get_vnumainfo psr_cmt_op psr_alloc set_gnttab_limits
+	get_vnumainfo psr_cmt_op psr_alloc
 };
 allow dom0_t dom0_t:resource { add remove };
 
diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index 7dc25be..5af984c 100644
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -52,7 +52,7 @@ define(`create_domain_common', `
 			settime setdomainhandle getvcpucontext set_misc_info };
 	allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim
 			set_vnumainfo get_vnumainfo cacheflush
-			psr_cmt_op psr_alloc soft_reset set_gnttab_limits };
+			psr_cmt_op psr_alloc soft_reset };
 	allow $1 $2:security check_context;
 	allow $1 $2:shadow enable;
 	allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op updatemp };
diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 89c329c..4771750 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -67,6 +67,14 @@ static int build(xc_interface *xch)
         .ssidref = SECINITSID_DOMU,
         .flags = XEN_DOMCTL_CDF_xs_domain,
         .max_evtchn_port = -1, /* No limit. */
+
+        /*
+         * 1 grant frame is enough: we don't need many grants.
+         * Mini-OS doesn't like less than 4, though, so use 4.
+         * 128 maptrack frames: 256 entries per frame, enough for 32768 domains.
+         */
+        .max_grant_frames = 4,
+        .max_maptrack_frames = 128,
     };
 
     xs_fd = open("/dev/xen/xenbus_backend", O_RDWR);
@@ -104,17 +112,6 @@ static int build(xc_interface *xch)
         fprintf(stderr, "xc_domain_setmaxmem failed\n");
         goto err;
     }
-    /*
-     * 1 grant frame is enough: we don't need many grants.
-     * Mini-OS doesn't like less than 4, though, so use 4.
-     * 128 maptrack frames: 256 entries per frame, enough for 32768 domains.
-     */
-    rv = xc_domain_set_gnttab_limits(xch, domid, 4, 128);
-    if ( rv )
-    {
-        fprintf(stderr, "xc_domain_set_gnttab_limits failed\n");
-        goto err;
-    }
     rv = xc_domain_set_memmap_limit(xch, domid, limit_kb);
     if ( rv )
     {
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 88a175f..0c7c07c 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1071,19 +1071,6 @@ int xc_domain_set_access_required(xc_interface *xch,
  */
 int xc_domain_set_virq_handler(xc_interface *xch, uint32_t domid, int virq);
 
-/**
- * Set the maximum number of grant frames and maptrack frames a domain
- * can have. Must be used at domain setup time and only then.
- *
- * @param xch a handle to an open hypervisor interface
- * @param domid the domain id
- * @param grant_frames max. number of grant frames
- * @param maptrack_frames max. number of maptrack frames
- */
-int xc_domain_set_gnttab_limits(xc_interface *xch, uint32_t domid,
-                                uint32_t grant_frames,
-                                uint32_t maptrack_frames);
-
 /*
  * CPUPOOL MANAGEMENT FUNCTIONS
  */
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 2bc695c..e8d0734 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2256,19 +2256,6 @@ int xc_domain_set_virq_handler(xc_interface *xch, uint32_t domid, int virq)
     return do_domctl(xch, &domctl);
 }
 
-int xc_domain_set_gnttab_limits(xc_interface *xch, uint32_t domid,
-                                uint32_t grant_frames,
-                                uint32_t maptrack_frames)
-{
-    DECLARE_DOMCTL;
-
-    domctl.cmd = XEN_DOMCTL_set_gnttab_limits;
-    domctl.domain = domid;
-    domctl.u.set_gnttab_limits.grant_frames = grant_frames;
-    domctl.u.set_gnttab_limits.maptrack_frames = maptrack_frames;
-    return do_domctl(xch, &domctl);
-}
-
 /* Plumbing Xen with vNUMA topology */
 int xc_domain_setvnuma(xc_interface *xch,
                        uint32_t domid,
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 2cb3460..c8eb59c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -566,6 +566,8 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
         struct xen_domctl_createdomain create = {
             .ssidref = info->ssidref,
             .max_evtchn_port = b_info->event_channels,
+            .max_grant_frames = b_info->max_grant_frames,
+            .max_maptrack_frames = b_info->max_maptrack_frames,
         };
 
         if (info->type != LIBXL_DOMAIN_TYPE_PV) {
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 227e6cf..9194109 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -358,12 +358,6 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
         return ERROR_FAIL;
     }
 
-    if (xc_domain_set_gnttab_limits(ctx->xch, domid, info->max_grant_frames,
-                                    info->max_maptrack_frames) != 0) {
-        LOG(ERROR, "Couldn't set grant table limits");
-        return ERROR_FAIL;
-    }
-
     /*
      * Check if the domain has any CPU or node affinity already. If not, try
      * to build up the latter via automatic NUMA placement. In fact, in case
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 6dc0dd7..7c8d6ab 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -64,6 +64,8 @@ type domctl_create_config =
 	handle: string;
 	flags: domain_create_flag list;
 	max_evtchn_port: int32;
+	max_grant_frames: int32;
+	max_maptrack_frames: int32;
 	arch: arch_domainconfig;
 }
 
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index b0fec24..f150a5d 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -56,6 +56,8 @@ type domctl_create_config = {
   handle: string;
   flags: domain_create_flag list;
   max_evtchn_port: int32;
+  max_grant_frames: int32;
+  max_maptrack_frames: int32;
   arch: arch_domainconfig;
 }
 
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index c022de9..882828f 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -128,7 +128,9 @@ CAMLprim value stub_xc_domain_create(value xch, value config)
 #define VAL_HANDLE              Field(config, 1)
 #define VAL_FLAGS               Field(config, 2)
 #define VAL_MAX_EVTCHN_PORT     Field(config, 3)
-#define VAL_ARCH                Field(config, 4)
+#define VAL_MAX_GRANT_FRAMES    Field(config, 4)
+#define VAL_MAX_MAPTRACK_FRAMES Field(config, 5)
+#define VAL_ARCH                Field(config, 6)
 
 	uint32_t domid = 0;
 	int result;
@@ -136,6 +138,8 @@ CAMLprim value stub_xc_domain_create(value xch, value config)
 	struct xen_domctl_createdomain cfg = {
 		.ssidref = Int32_val(VAL_SSIDREF),
 		.max_evtchn_port = Int32_val(VAL_MAX_EVTCHN_PORT),
+		.max_grant_frames = Int32_val(VAL_MAX_GRANT_FRAMES),
+		.max_maptrack_frames = Int32_val(VAL_MAX_MAPTRACK_FRAMES),
 	};
 
 	domain_handle_of_uuid_string(cfg.handle, String_val(VAL_HANDLE));
@@ -164,6 +168,8 @@ CAMLprim value stub_xc_domain_create(value xch, value config)
 	}
 
 #undef VAL_ARCH
+#undef VAL_MAX_MAPTRACK_FRAMES
+#undef VAL_MAX_GRANT_FRAMES
 #undef VAL_MAX_EVTCHN_PORT
 #undef VAL_FLAGS
 #undef VAL_HANDLE
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index a307de7..db99a52 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -126,6 +126,8 @@ static PyObject *pyxc_domain_create(XcObject *self,
             0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef,
         },
         .max_evtchn_port = -1, /* No limit. */
+        .max_grant_frames = 32,
+        .max_maptrack_frames = 1024,
     };
 
     static char *kwd_list[] = { "domid", "ssidref", "handle", "flags", "target", NULL };
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 14dab56..d8ba461 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -539,14 +539,37 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             break;
         }
 
+        /* Stash the new domid for the toolstack. */
+        op->domain = d->domain_id;
+        copyback = true;
+
         d->max_evtchn_port =
             min_t(unsigned int, op->u.createdomain.max_evtchn_port, INT_MAX);
 
-        ret = 0;
-        op->domain = d->domain_id;
-        copyback = 1;
+        ret = grant_table_set_limits(d, op->u.createdomain.max_grant_frames,
+                                     op->u.createdomain.max_maptrack_frames);
+        if ( !ret )
+            goto createdomain_fail_late;
+
         d = NULL;
         break;
+
+    createdomain_fail_late:
+        /*
+         * We've hit an error after putting the domain into the domain list,
+         * meaning that other entities in the system can refer to it.
+         *
+         * Unwinding is substantially more complicated, and without
+         * returning success, the toolstack wont know to clean up.
+         *
+         * Reuse the continuation logic to turn this hypercall into a
+         * destroydomain on behalf of the toolstack.
+         */
+        op->cmd = XEN_DOMCTL_destroydomain;
+        d = NULL;
+
+        ret = hypercall_create_continuation(__HYPERVISOR_domctl, "h", u_domctl);
+        break;
     }
 
     case XEN_DOMCTL_max_vcpus:
@@ -1114,11 +1137,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             copyback = 1;
         break;
 
-    case XEN_DOMCTL_set_gnttab_limits:
-        ret = grant_table_set_limits(d, op->u.set_gnttab_limits.grant_frames,
-                                     op->u.set_gnttab_limits.maptrack_frames);
-        break;
-
     default:
         ret = arch_do_domctl(op, d, u_domctl);
         break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 515671e..424f0a8 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -71,6 +71,8 @@ struct xen_domctl_createdomain {
      * mapping space, xenheap, etc) a guest may consume.
      */
     uint32_t max_evtchn_port;
+    uint32_t max_grant_frames;
+    uint32_t max_maptrack_frames;
 
     struct xen_arch_domainconfig arch;
 };
@@ -1065,11 +1067,6 @@ struct xen_domctl_psr_alloc {
     uint64_t data;      /* IN/OUT */
 };
 
-struct xen_domctl_set_gnttab_limits {
-    uint32_t grant_frames;     /* IN */
-    uint32_t maptrack_frames;  /* IN */
-};
-
 /* XEN_DOMCTL_vuart_op */
 struct xen_domctl_vuart_op {
 #define XEN_DOMCTL_VUART_OP_INIT  0
@@ -1168,7 +1165,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_monitor_op                    77
 #define XEN_DOMCTL_psr_alloc                     78
 #define XEN_DOMCTL_soft_reset                    79
-#define XEN_DOMCTL_set_gnttab_limits             80
+/* #define XEN_DOMCTL_set_gnttab_limits          80 - Moved into XEN_DOMCTL_createdomain */
 #define XEN_DOMCTL_vuart_op                      81
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
@@ -1229,7 +1226,6 @@ struct xen_domctl {
         struct xen_domctl_psr_cmt_op        psr_cmt_op;
         struct xen_domctl_monitor_op        monitor_op;
         struct xen_domctl_psr_alloc         psr_alloc;
-        struct xen_domctl_set_gnttab_limits set_gnttab_limits;
         struct xen_domctl_vuart_op          vuart_op;
         uint8_t                             pad[128];
     } u;
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 1978703..cccd1c7 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -740,9 +740,6 @@ static int flask_domctl(struct domain *d, int cmd)
     case XEN_DOMCTL_soft_reset:
         return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SOFT_RESET);
 
-    case XEN_DOMCTL_set_gnttab_limits:
-        return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SET_GNTTAB_LIMITS);
-
     default:
         return avc_unknown_permission("domctl", cmd);
     }
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index deecfa2..6f6e969 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -246,8 +246,6 @@ class domain2
     mem_sharing
 # XEN_DOMCTL_psr_alloc
     psr_alloc
-# XEN_DOMCTL_set_gnttab_limits
-    set_gnttab_limits
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM domains
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel