[PATCH] [Xend] Move some backend configuration

[PATCH] [Xend] Move some backend configuration

[Pascal Bouchareine]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

This patch moves some dom0 variables and backend device
configuration from frontend directories to 
/local/domain/<backdomid>/backend or /vm.

Also,
- the first console is created as a xend device and moves
  from console/0 to device/console/0
- /vm_path/<domid> is introduced, referencing the /vm path
- /vm_path/device/backend holds the backend device location,
  rather than storing it in the frontend directory
- a few helpers are added to xs.c to deal with this

Signed-off-by: Pascal Bouchareine <pascal@gandi.net>

-- 
\o/   Pascal Bouchareine - Gandi 
 g    0170393757           15, place de la Nation - 75011 Paris      

[-- Attachment #2: 00_devices_in_backend.patch --]
[-- Type: text/plain, Size: 35167 bytes --]

diff -r 6a8faf6d98b2 tools/console/client/main.c
--- a/tools/console/client/main.c	Wed Sep 17 15:46:15 2008 +0100
+++ b/tools/console/client/main.c	Tue Sep 30 13:17:19 2008 +0200
@@ -221,7 +221,7 @@ int main(int argc, char **argv)
 		{ 0 },
 
 	};
-	char *path;
+	char *path, *s;
 	int spty, xsfd;
 	struct xs_handle *xs;
 	char *end;
@@ -257,13 +257,18 @@ int main(int argc, char **argv)
 
 	signal(SIGTERM, sighandler);
 
-	path = xs_get_domain_path(xs, domid);
-	if (path == NULL)
-		err(errno, "xs_get_domain_path()");
-	path = realloc(path, strlen(path) + strlen("/console/tty") + 1);
-	if (path == NULL)
+	path = xs_get_backend_dev(xs, domid, "console", 0);
+	if (path == NULL) {
+		err(errno, "Could not get backend path to tty");
+	}
+
+	s = realloc(path, strlen(path) + 5);
+	if (s == NULL) {
+		free(path);
 		err(ENOMEM, "realloc");
-	strcat(path, "/console/tty");
+	}
+	path = s;
+	strcat(path, "/tty");
 
 	/* FIXME consoled currently does not assume domain-0 doesn't have a
 	   console which is good when we break domain-0 up.  To keep us
diff -r 6a8faf6d98b2 tools/console/daemon/io.c
--- a/tools/console/daemon/io.c	Wed Sep 17 15:46:15 2008 +0100
+++ b/tools/console/daemon/io.c	Tue Sep 30 12:15:51 2008 +0200
@@ -87,8 +87,10 @@ struct domain {
 	bool is_dead;
 	struct buffer buffer;
 	struct domain *next;
-	char *conspath;
-	char *serialpath;
+	char *consfront;
+	char *consback;
+	char *serialfront;
+	char *serialback;
 	int use_consolepath;
 	int ring_ref;
 	evtchn_port_or_error_t local_port;
@@ -415,7 +417,7 @@ static int domain_create_tty(struct doma
 	}
 
 	if (dom->use_consolepath) {
-		success = asprintf(&path, "%s/limit", dom->conspath) !=
+		success = asprintf(&path, "%s/limit", dom->consback) !=
 			-1;
 		if (!success)
 			goto out;
@@ -427,7 +429,7 @@ static int domain_create_tty(struct doma
 		free(path);
 	}
 
-	success = asprintf(&path, "%s/limit", dom->serialpath) != -1;
+	success = asprintf(&path, "%s/limit", dom->serialback) != -1;
 	if (!success)
 		goto out;
 	data = xs_read(xs, XBT_NULL, path, &len);
@@ -437,7 +439,7 @@ static int domain_create_tty(struct doma
 	}
 	free(path);
 
-	success = asprintf(&path, "%s/tty", dom->serialpath) != -1;
+	success = asprintf(&path, "%s/tty", dom->serialback) != -1;
 	if (!success)
 		goto out;
 	success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
@@ -446,7 +448,7 @@ static int domain_create_tty(struct doma
 		goto out;
 
 	if (dom->use_consolepath) {
-		success = (asprintf(&path, "%s/tty", dom->conspath) != -1);
+		success = (asprintf(&path, "%s/tty", dom->consback) != -1);
 		if (!success)
 			goto out;
 		success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
@@ -504,12 +506,12 @@ static int domain_create_ring(struct dom
 	int err, remote_port, ring_ref, rc;
 	char *type, path[PATH_MAX];
 
-	err = xs_gather(xs, dom->serialpath,
+	err = xs_gather(xs, dom->serialfront,
 			"ring-ref", "%u", &ring_ref,
 			"port", "%i", &remote_port,
 			NULL);
 	if (err) {
-		err = xs_gather(xs, dom->conspath,
+		err = xs_gather(xs, dom->consfront,
 				"ring-ref", "%u", &ring_ref,
 				"port", "%i", &remote_port,
 				NULL);
@@ -519,7 +521,7 @@ static int domain_create_ring(struct dom
 	} else
 		dom->use_consolepath = 0;
 
-	sprintf(path, "%s/type", dom->use_consolepath ? dom->conspath: dom->serialpath);
+	sprintf(path, "%s/type", dom->use_consolepath ? dom->consfront: dom->serialfront);
 	type = xs_read(xs, XBT_NULL, path, NULL);
 	if (type && strcmp(type, "xenconsoled") != 0) {
 		free(type);
@@ -594,17 +596,17 @@ static bool watch_domain(struct domain *
 
 	snprintf(domid_str, sizeof(domid_str), "dom%u", dom->domid);
 	if (watch) {
-		success = xs_watch(xs, dom->serialpath, domid_str);
+		success = xs_watch(xs, dom->serialfront, domid_str);
 		if (success) {
-			success = xs_watch(xs, dom->conspath, domid_str);
+			success = xs_watch(xs, dom->consfront, domid_str);
 			if (success)
 				domain_create_ring(dom);
 			else
-				xs_unwatch(xs, dom->serialpath, domid_str);
+				xs_unwatch(xs, dom->serialfront, domid_str);
 		}
 	} else {
-		success = xs_unwatch(xs, dom->serialpath, domid_str);
-		success = xs_unwatch(xs, dom->conspath, domid_str);
+		success = xs_unwatch(xs, dom->serialfront, domid_str);
+		success = xs_unwatch(xs, dom->consfront, domid_str);
 	}
 
 	return success;
@@ -614,7 +616,6 @@ static struct domain *create_domain(int 
 static struct domain *create_domain(int domid)
 {
 	struct domain *dom;
-	char *s;
 	struct timeval tv;
 
 	if (gettimeofday(&tv, NULL) < 0) {
@@ -632,21 +633,20 @@ static struct domain *create_domain(int 
 
 	dom->domid = domid;
 
-	dom->serialpath = xs_get_domain_path(xs, dom->domid);
-	s = realloc(dom->serialpath, strlen(dom->serialpath) +
-		    strlen("/serial/0") + 1);
-	if (s == NULL)
+	if (asprintf(&dom->serialback, "backend/serial/%d/0", domid) == -1) {
+		dolog(LOG_ERR, "Out of memory %s:%s():L%d",
+		      __FILE__, __FUNCTION__, __LINE__);
 		goto out;
-	dom->serialpath = s;
-	strcat(dom->serialpath, "/serial/0");
-
-	dom->conspath = xs_get_domain_path(xs, dom->domid);
-	s = realloc(dom->conspath, strlen(dom->conspath) +
-		    strlen("/console") + 1);
-	if (s == NULL)
+	}
+
+	if (asprintf(&dom->consback, "backend/console/%d/0", domid) == -1) {
+		dolog(LOG_ERR, "Out of memory %s:%s():L%d",
+		      __FILE__, __FUNCTION__, __LINE__);
 		goto out;
-	dom->conspath = s;
-	strcat(dom->conspath, "/console");
+	}
+		
+	dom->serialfront = xs_get_frontend_dev(xs, domid, "serial", 0);
+	dom->consfront = xs_get_frontend_dev(xs, domid, "console", 0);
 
 	dom->master_fd = -1;
 	dom->slave_fd = -1;
@@ -678,8 +678,10 @@ static struct domain *create_domain(int 
 
 	return dom;
  out:
-	free(dom->serialpath);
-	free(dom->conspath);
+ 	free(dom->serialback);
+	free(dom->consback);
+	free(dom->serialfront);
+	free(dom->consfront);
 	free(dom);
 	return NULL;
 }
@@ -716,11 +718,17 @@ static void cleanup_domain(struct domain
 	free(d->buffer.data);
 	d->buffer.data = NULL;
 
-	free(d->serialpath);
-	d->serialpath = NULL;
-
-	free(d->conspath);
-	d->conspath = NULL;
+	free(d->serialback);
+	d->serialback = NULL;
+
+	free(d->consback);
+	d->consback = NULL;
+
+	free(d->serialfront);
+	d->serialfront = NULL;
+
+	free(d->consfront);
+	d->consfront = NULL;
 
 	remove_domain(d);
 }
diff -r 6a8faf6d98b2 tools/console/daemon/utils.c
--- a/tools/console/daemon/utils.c	Wed Sep 17 15:46:15 2008 +0100
+++ b/tools/console/daemon/utils.c	Tue Sep 30 12:01:37 2008 +0200
@@ -109,7 +109,7 @@ bool xen_setup(void)
 bool xen_setup(void)
 {
 	
-	xs = xs_daemon_open();
+	xs = xs_domain_open();
 	if (xs == NULL) {
 		dolog(LOG_ERR,
 		      "Failed to contact xenstore (%m).  Is it running?");
@@ -136,7 +136,7 @@ bool xen_setup(void)
 
  out:
 	if (xs)
-		xs_daemon_close(xs);
+		xs_domain_close(xs);
 	if (xc != -1)
 		xc_interface_close(xc);
 	return false;
diff -r 6a8faf6d98b2 tools/python/xen/xend/XendBootloader.py
--- a/tools/python/xen/xend/XendBootloader.py	Wed Sep 17 15:46:15 2008 +0100
+++ b/tools/python/xen/xend/XendBootloader.py	Mon Sep 29 16:21:31 2008 +0200
@@ -72,7 +72,9 @@ def bootloader(blexec, disk, dom, quiet 
     fcntl.fcntl(m1, fcntl.F_SETFL, os.O_NDELAY);
     os.close(s1)
     slavename = ptsname.ptsname(m1)
-    dom.storeDom("console/tty", slavename)
+
+    dev = dom.getDeviceController('console')
+    dev.writeBackend(0, 'tty', slavename)
 
     # Release the domain lock here, because we definitely don't want 
     # a stuck bootloader to deny service to other xend clients.
diff -r 6a8faf6d98b2 tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Wed Sep 17 15:46:15 2008 +0100
+++ b/tools/python/xen/xend/XendDomainInfo.py	Mon Sep 29 16:21:31 2008 +0200
@@ -113,6 +113,25 @@ def create_from_dict(config_dict):
         raise
     return vm
 
+def _tryVmPathRecover(uuid):
+    id = 0
+    vm_path = None
+    while True:
+        path = '/vm/%s' % (uuid,)
+        if id:
+            path = '%s-%d' % (path, id,)
+        try:
+           uuid = xstransact.Read('%s/uuid' % path)
+           if uuid:
+               vm_path = path
+           else:
+               return vm_path
+        except:
+           # return the last good vm_path, or none
+           # if we never found one
+           return vm_path
+        id += 1
+
 def recreate(info, priv):
     """Create the VM object for an existing domain.  The domain must not
     be dying, as the paths in the store should already have been removed,
@@ -157,26 +176,17 @@ def recreate(info, priv):
     #       entry disappears (eg. xenstore-rm /)
     #
     try:
-        vmpath = xstransact.Read(dompath, "vm")
+        # Do not trust /local/domain/%d/vm as the domU might touch
+        # No need to recheck to vm uuid then.
+        vmpath = xstransact.Read("/vm_path", str(domid))
         if not vmpath:
             if not priv:
-                log.warn('/local/domain/%d/vm is missing. recreate is '
+                log.warn('/vm_path/%d is missing. recreate is '
                          'confused, trying our best to recover' % domid)
-            needs_reinitialising = True
-            raise XendError('reinit')
-        
-        uuid2_str = xstransact.Read(vmpath, "uuid")
-        if not uuid2_str:
-            log.warn('%s/uuid/ is missing. recreate is confused, '
-                     'trying our best to recover' % vmpath)
-            needs_reinitialising = True
-            raise XendError('reinit')
-        
-        uuid2 = uuid.fromString(uuid2_str)
-        if uuid1 != uuid2:
-            log.warn('UUID in /vm does not match the UUID in /dom/%d.'
-                     'Trying out best to recover' % domid)
-            needs_reinitialising = True
+            vmpath = _tryVmPathRecover(uuid1)
+            if not vmpath:
+                needs_reinitialising = True
+                raise XendError('reinit')
     except XendError:
         pass # our best shot at 'goto' in python :)
 
@@ -365,7 +375,7 @@ class XendDomainInfo:
             if i != 0:
                 self.vmpath = self.vmpath + '-' + str(i)
             try:
-                if self._readVm("uuid"):
+                if self.readVm("uuid"):
                     self.vmpath = None
                     i = i + 1
             except:
@@ -450,8 +460,8 @@ class XendDomainInfo:
             try:
                 self._constructDomain()
                 self._storeVmDetails()
+                self._createChannels()
                 self._createDevices()
-                self._createChannels()
                 self._storeDomDetails()
                 self._endRestore()
             except:
@@ -809,7 +819,7 @@ class XendDomainInfo:
             self.info['vcpu_avail'] = (1 << xeninfo['online_vcpus']) - 1
 
         # read image value
-        image_sxp = self._readVm('image')
+        image_sxp = self.readVm('image')
         if image_sxp:
             self.info.update_with_image_sxp(sxp.from_string(image_sxp))
 
@@ -830,27 +840,15 @@ class XendDomainInfo:
         if self.domid == None or self.domid == 0:
             return
 
-        # Update VT100 port if it exists
-        if transaction is None:
-            self.console_port = self.readDom('console/port')
-        else:
-            self.console_port = self.readDomTxn(transaction, 'console/port')
-        if self.console_port is not None:
-            serial_consoles = self.info.console_get_all('vt100')
-            if not serial_consoles:
-                cfg = self.info.console_add('vt100', self.console_port)
-                self._createDevice('console', cfg)
-            else:
-                console_uuid = serial_consoles[0].get('uuid')
-                self.info.console_update(console_uuid, 'location',
-                                         self.console_port)
-                
-
         # Update VNC port if it exists and write to xenstore
         if transaction is None:
+            # XXX todo maybe ?
+            # vnc_port = self.readDom('device/console/0/vnc-port')
             vnc_port = self.readDom('console/vnc-port')
         else:
+            # vnc_port = self.readDomTxn(transaction, 'device/console/0/vnc-port')
             vnc_port = self.readDomTxn(transaction, 'console/vnc-port')
+
         if vnc_port is not None:
             for dev_uuid, (dev_type, dev_info) in self.info['devices'].items():
                 if dev_type == 'vfb':
@@ -870,23 +868,23 @@ class XendDomainInfo:
     # Function to update xenstore /vm/*
     #
 
-    def _readVm(self, *args):
+    def readVm(self, *args):
         return xstransact.Read(self.vmpath, *args)
 
     def _writeVm(self, *args):
         return xstransact.Write(self.vmpath, *args)
 
     def _removeVm(self, *args):
+        self._removeVmPath()
         return xstransact.Remove(self.vmpath, *args)
 
-    def _gatherVm(self, *args):
+    def gatherVm(self, *args):
         return xstransact.Gather(self.vmpath, *args)
 
     def storeVm(self, *args):
         return xstransact.Store(self.vmpath, *args)
 
-
-    def _readVmTxn(self, transaction,  *args):
+    def readVmTxn(self, transaction,  *args):
         paths = map(lambda x: self.vmpath + "/" + x, args)
         return transaction.read(*paths)
 
@@ -923,8 +921,8 @@ class XendDomainInfo:
         return xstransact.Remove(self.dompath, *args)
 
     def storeDom(self, *args):
+        """Stores an information that we share with the domU"""
         return xstransact.Store(self.dompath, *args)
-
 
     def readDomTxn(self, transaction, *args):
         paths = map(lambda x: self.dompath + "/" + x, args)
@@ -956,12 +954,20 @@ class XendDomainInfo:
         t.set_permissions({'dom' : self.domid})
         t.write('vm', self.vmpath)
 
+    def get_console_type(self):
+        # Figure out if we need to tell xenconsoled to ignore this guest's
+        # console - device model will handle console if it is running
+        constype = "ioemu"
+        if 'device_model' not in self.info['platform']:
+            constype = "xenconsoled"
+
+        return constype
+
     def _storeDomDetails(self):
         to_store = {
             'domid':              str(self.domid),
             'vm':                 self.vmpath,
             'name':               self.info['name_label'],
-            'console/limit':      str(xoptions.get_console_limit() * 1024),
             'memory/target':      str(self.info['memory_dynamic_max'] / 1024),
             }
 
@@ -972,17 +978,11 @@ class XendDomainInfo:
                 else:
                     to_store[n] = str(v)
 
-        # Figure out if we need to tell xenconsoled to ignore this guest's
-        # console - device model will handle console if it is running
-        constype = "ioemu"
-        if 'device_model' not in self.info['platform']:
-            constype = "xenconsoled"
-
-        f('console/port',     self.console_port)
-        f('console/ring-ref', self.console_mfn)
-        f('console/type',     constype)
         f('store/port',       self.store_port)
         f('store/ring-ref',   self.store_mfn)
+
+        f('device/console/0/port', self.console_port)
+        f('device/console/0/ring-ref', self.console_mfn)
 
         if arch.type == "x86":
             f('control/platform-feature-multiprocessor-suspend', True)
@@ -1071,7 +1071,7 @@ class XendDomainInfo:
                     changed = True
 
         # Check whether image definition has been updated
-        image_sxp = self._readVm('image')
+        image_sxp = self.readVm('image')
         if image_sxp and image_sxp != sxp.to_string(self.info.image_sxpr()):
             self.info.update_with_image_sxp(sxp.from_string(image_sxp))
             changed = True
@@ -1207,7 +1207,7 @@ class XendDomainInfo:
         self.info['vcpus_params']['weight'] = cpu_weight
 
     def getRestartCount(self):
-        return self._readVm('xend/restart_count')
+        return self.readVm('xend/restart_count')
 
     def refreshShutdown(self, xeninfo = None):
         """ Checks the domain for whether a shutdown is required.
@@ -1366,7 +1366,7 @@ class XendDomainInfo:
         """
         from xen.xend import XendDomain
         
-        if self._readVm(RESTART_IN_PROGRESS):
+        if self.readVm(RESTART_IN_PROGRESS):
             log.error('Xend failed during restart of domain %s.  '
                       'Refusing to restart to avoid loops.',
                       str(self.domid))
@@ -1377,7 +1377,7 @@ class XendDomainInfo:
         self._writeVm(RESTART_IN_PROGRESS, 'True')
 
         now = time.time()
-        rst = self._readVm('xend/previous_restart_time')
+        rst = self.readVm('xend/previous_restart_time')
         if rst:
             rst = float(rst)
             timeout = now - rst
@@ -1397,6 +1397,7 @@ class XendDomainInfo:
             else:
                 self._unwatchVm()
                 self.destroyDomain()
+                self._removeVm()
 
             # new_dom's VM will be the same as this domain's VM, except where
             # the rename flag has instructed us to call preserveForRestart.
@@ -1410,7 +1411,7 @@ class XendDomainInfo:
                     self.info)
                 new_dom.waitForDevices()
                 new_dom.unpause()
-                rst_cnt = self._readVm('xend/restart_count')
+                rst_cnt = self.readVm('xend/restart_count')
                 rst_cnt = int(rst_cnt) + 1
                 self._writeVm('xend/restart_count', str(rst_cnt))
                 new_dom._removeVm(RESTART_IN_PROGRESS)
@@ -1502,6 +1503,16 @@ class XendDomainInfo:
         return self.getDeviceController(deviceClass).reconfigureDevice(
             devid, devconfig)
 
+    def _createConsole(self):
+        """Setups the first console for this domain"""
+        log.debug('Creating initial console ring-ref %s' % self.console_mfn)
+        devid = self._createDevice('console', {
+                       'ring-ref': self.console_mfn,
+                       'port': self.console_port,
+                       'type': self.get_console_type()
+            }
+        )
+
     def _createDevices(self):
         """Create the devices for a vm.
 
@@ -1764,6 +1775,7 @@ class XendDomainInfo:
 
             self._introduceDomain()
 
+            self._createConsole()
             self._createDevices()
 
             self.image.cleanupBootloading()
@@ -1898,11 +1910,11 @@ class XendDomainInfo:
 
         paths = self._prepare_phantom_paths()
 
-        self._cleanupVm()
         if self.dompath is not None:
             self.destroyDomain()
 
         self._cleanup_phantom_devs(paths)
+        self._cleanupVm()
 
         if "transient" in self.info["other_config"] \
            and bool(self.info["other_config"]["transient"]):
@@ -2096,7 +2108,7 @@ class XendDomainInfo:
         """Read the specified parameters from the store.
         """
         try:
-            return self._gatherVm(*params)
+            return self.gatherVm(*params)
         except ValueError:
             # One of the int/float entries in params has a corresponding store
             # entry that is invalid.  We recover, because older versions of
@@ -2183,6 +2195,15 @@ class XendDomainInfo:
                 log.info("Dev still active but hit max loop timeout")
                 break
 
+    def _storeVmPath(self):
+        log.info("storeVmPath(%s) => %s", self.domid, self.vmpath)
+        if self.domid is not None:
+            xstransact.Write('/vm_path', str(self.domid), self.vmpath)
+
+    def _removeVmPath(self):
+        if self.domid is not None:
+            xstransact.Remove('/vm_path/%s' % str(self.domid))
+
     def _storeVmDetails(self):
         to_store = {}
 
@@ -2200,14 +2221,14 @@ class XendDomainInfo:
         if image_sxpr:
             to_store['image'] = sxp.to_string(image_sxpr)
 
-        if not self._readVm('xend/restart_count'):
+        if not self.readVm('xend/restart_count'):
             to_store['xend/restart_count'] = str(0)
 
         log.debug("Storing VM details: %s", scrub_password(to_store))
 
         self._writeVm(to_store)
         self._setVmPermissions()
-
+        self._storeVmPath()
 
     def _setVmPermissions(self):
         """Allow the guest domain to read its UUID.  We don't allow it to
@@ -2227,7 +2248,7 @@ class XendDomainInfo:
              log.warn("".join(traceback.format_stack()))
              return self._stateGet()
          else:
-             raise AttributeError()
+             raise AttributeError(name)
 
     def __setattr__(self, name, value):
         if name == "state":
@@ -2338,12 +2359,6 @@ class XendDomainInfo:
                                   ignore_devices = ignore_store,
                                   legacy_only = legacy_only)
 
-        #if not ignore_store and self.dompath:
-        #    vnc_port = self.readDom('console/vnc-port')
-        #    if vnc_port is not None:
-        #        result.append(['device',
-        #                       ['console', ['vnc-port', str(vnc_port)]]])
-
         return result
 
     # Xen API
@@ -2608,7 +2623,7 @@ class XendDomainInfo:
             if not config.has_key('device'):
                 devid = config.get('id')
                 if devid != None:
-                    config['device'] = 'eth%d' % devid
+                    config['device'] = 'eth%s' % devid
                 else:
                     config['device'] = ''
 
diff -r 6a8faf6d98b2 tools/python/xen/xend/image.py
--- a/tools/python/xen/xend/image.py	Wed Sep 17 15:46:15 2008 +0100
+++ b/tools/python/xen/xend/image.py	Mon Sep 29 16:21:31 2008 +0200
@@ -289,7 +289,7 @@ class ImageHandler:
         log.info("spawning device models: %s %s", self.device_model, args)
         # keep track of pid and spawned options to kill it later
         self.pid = os.spawnve(os.P_NOWAIT, self.device_model, args, env)
-        self.vm.storeDom("image/device-model-pid", self.pid)
+        self.vm.storeVm("image/device-model-pid", self.pid)
         log.info("device model pid: %d", self.pid)
 
     def saveDeviceModel(self):
@@ -320,7 +320,7 @@ class ImageHandler:
     def recreate(self):
         if self.device_model is None:
             return
-        self.pid = self.vm.gatherDom(('image/device-model-pid', int))
+        self.pid = self.vm.gatherVm(('image/device-model-pid', int))
 
     def destroyDeviceModel(self):
         if self.device_model is None:
diff -r 6a8faf6d98b2 tools/python/xen/xend/server/ConsoleController.py
--- a/tools/python/xen/xend/server/ConsoleController.py	Wed Sep 17 15:46:15 2008 +0100
+++ b/tools/python/xen/xend/server/ConsoleController.py	Mon Sep 29 16:21:31 2008 +0200
@@ -2,13 +2,16 @@ from xen.xend.XendLogging import log
 from xen.xend.XendLogging import log
 
 from xen.xend.XendError import VmError
+
+from xen.xend import XendOptions
+xoptions = XendOptions.instance()
 
 class ConsoleController(DevController):
     """A dummy controller for us to represent serial and vnc
     console devices with persistent UUIDs.
     """
 
-    valid_cfg = ['location', 'uuid', 'protocol']
+    valid_cfg = ['location', 'uuid', 'protocol', 'limit']
 
     def __init__(self, vm):
         DevController.__init__(self, vm)
@@ -16,8 +19,17 @@ class ConsoleController(DevController):
 
     def getDeviceDetails(self, config):
         back = dict([(k, config[k]) for k in self.valid_cfg if k in config])
-        return (self.allocateDeviceID(), back, {})
 
+	devid = self.allocateDeviceID()
+	front = { }
+	
+	if devid == 0 and self.vm.console_mfn:
+            front.update({
+		'type': '%s' % self.vm.get_console_type()
+            })
+
+        back['limit'] = str(xoptions.get_console_limit() * 1024)
+        return (devid, back, front)
 
     def getDeviceConfiguration(self, devid, transaction = None):
         result = DevController.getDeviceConfiguration(self, devid, transaction)
diff -r 6a8faf6d98b2 tools/python/xen/xend/server/DevController.py
--- a/tools/python/xen/xend/server/DevController.py	Wed Sep 17 15:46:15 2008 +0100
+++ b/tools/python/xen/xend/server/DevController.py	Mon Sep 29 16:21:31 2008 +0200
@@ -122,8 +122,11 @@ class DevController:
                     log.debug(
                       'DevController: still waiting to write device entries.')
 
+                devpath = self.devicePath(devid)
+
                 t.remove(frontpath)
                 t.remove(backpath)
+                t.remove(devpath)
 
                 t.mkdir(backpath)
                 t.set_permissions(backpath,
@@ -137,6 +140,14 @@ class DevController:
 
                 t.write2(frontpath, front)
                 t.write2(backpath,  back)
+
+                t.mkdir(devpath)
+                t.write2(devpath, {
+                    'backend' : backpath,
+                    'backend-id' : "%i" % backdom,
+                    'frontend' : frontpath,
+                    'frontend-id' : "%i" % self.vm.getDomid()
+                })
 
                 if t.commit():
                     return devid
@@ -233,11 +244,12 @@ class DevController:
 
         if force:
             frontpath = self.frontendPath(dev)
-            backpath = xstransact.Read(frontpath, "backend")
+            backpath = self.readVm(devid, "backend")
             if backpath:
                 xstransact.Remove(backpath)
             xstransact.Remove(frontpath)
 
+        # xstransact.Remove(self.devicePath()) ?? Below is the same ?
         self.vm._removeVm("device/%s/%d" % (self.deviceClass, dev))
 
     def configurations(self, transaction = None):
@@ -281,9 +293,10 @@ class DevController:
         @return: dict
         """
         if transaction is None:
-            backdomid = xstransact.Read(self.frontendPath(devid), "backend-id")
-        else:
-            backdomid = transaction.read(self.frontendPath(devid) + "/backend-id")
+            backdomid = xstransact.Read(self.devicePath(devid), "backend-id")
+        else:
+            backdomid = transaction.read(self.devicePath(devid) + "/backend-id")
+
         if backdomid is None:
             raise VmError("Device %s not connected" % devid)
 
@@ -411,18 +424,22 @@ class DevController:
         t.write("nextDeviceID", str(result + 1))
         return result
 
+    def readVm(self, devid, *args):
+        devpath = self.devicePath(devid)
+        if devpath:
+            return xstransact.Read(devpath, *args)
+        else:
+            raise VmError("Device config %s not found" % devid)
 
     def readBackend(self, devid, *args):
-        frontpath = self.frontendPath(devid)
-        backpath = xstransact.Read(frontpath, "backend")
+        backpath = self.readVm(devid, "backend")
         if backpath:
             return xstransact.Read(backpath, *args)
         else:
             raise VmError("Device %s not connected" % devid)
 
     def readBackendTxn(self, transaction, devid, *args):
-        frontpath = self.frontendPath(devid)
-        backpath = transaction.read(frontpath + "/backend")
+        backpath = self.readVm(devid, "backend")
         if backpath:
             paths = map(lambda x: backpath + "/" + x, args)
             return transaction.read(*paths)
@@ -440,7 +457,7 @@ class DevController:
         """@return The IDs of each of the devices currently configured for
         this instance's deviceClass.
         """
-        fe = self.backendRoot()
+        fe = self.deviceRoot()
 
         if transaction:
             return map(lambda x: int(x.split('/')[-1]), transaction.list(fe))
@@ -449,8 +466,7 @@ class DevController:
 
 
     def writeBackend(self, devid, *args):
-        frontpath = self.frontendPath(devid)
-        backpath = xstransact.Read(frontpath, "backend")
+        backpath = self.readVm(devid, "backend")
 
         if backpath:
             xstransact.Write(backpath, *args)
@@ -515,9 +531,8 @@ class DevController:
 
 
     def waitForBackend(self, devid):
-
         frontpath = self.frontendPath(devid)
-        # lookup a phantom 
+        # lookup a phantom
         phantomPath = xstransact.Read(frontpath, 'phantom_vbd')
         if phantomPath is not None:
             log.debug("Waiting for %s's phantom %s.", devid, phantomPath)
@@ -530,7 +545,7 @@ class DevController:
             if result['status'] != 'Connected':
                 return (result['status'], err)
             
-        backpath = xstransact.Read(frontpath, "backend")
+        backpath = self.readVm(devid, "backend")
 
 
         if backpath:
@@ -579,17 +594,20 @@ class DevController:
     def frontendRoot(self):
         return "%s/device/%s" % (self.vm.getDomainPath(), self.deviceClass)
 
-    def backendRoot(self):
-        """Construct backend root path assuming backend is domain 0."""
-        from xen.xend.XendDomain import DOM0_ID
-        from xen.xend.xenstore.xsutil import GetDomainPath
-        return "%s/backend/%s/%s" % (GetDomainPath(DOM0_ID),
-                                     self.deviceClass, self.vm.getDomid())
-
     def frontendMiscPath(self):
         return "%s/device-misc/%s" % (self.vm.getDomainPath(),
                                       self.deviceClass)
 
+    def deviceRoot(self):
+        """Return the /vm/device. Because backendRoot assumes the
+        backend domain is 0"""
+        return "%s/device/%s" % (self.vm.vmpath, self.deviceClass)
+
+    def devicePath(self, devid):
+        """Return the /device entry of the given VM. We use it to store
+        backend/frontend locations"""
+        return "%s/device/%s/%s" % (self.vm.vmpath,
+                                    self.deviceClass, devid)
 
 def hotplugStatusCallback(statusPath, ev, result):
     log.debug("hotplugStatusCallback %s.", statusPath)
diff -r 6a8faf6d98b2 tools/python/xen/xend/server/netif.py
--- a/tools/python/xen/xend/server/netif.py	Wed Sep 17 15:46:15 2008 +0100
+++ b/tools/python/xen/xend/server/netif.py	Mon Sep 29 16:21:31 2008 +0200
@@ -141,10 +141,6 @@ class NetifController(DevController):
         if sec_lab:
             back['security_label'] = sec_lab
 
-        config_path = "device/%s/%d/" % (self.deviceClass, devid)
-        for x in back:
-            self.vm._writeVm(config_path + x, back[x])
-
         back['handle'] = "%i" % devid
         back['script'] = os.path.join(xoptions.network_script_dir, script)
         if rate:
@@ -188,40 +184,14 @@ class NetifController(DevController):
 
         result = DevController.getDeviceConfiguration(self, devid, transaction)
 
-        config_path = "device/%s/%d/" % (self.deviceClass, devid)
-        devinfo = ()
         for x in ( 'script', 'ip', 'bridge', 'mac',
                    'type', 'vifname', 'rate', 'uuid', 'model', 'accel',
                    'security_label'):
             if transaction is None:
-                y = self.vm._readVm(config_path + x)
+                y = self.readBackend(devid, x)
             else:
-                y = self.vm._readVmTxn(transaction, config_path + x)
-            devinfo += (y,)
-        (script, ip, bridge, mac, typ, vifname, rate, uuid,
-         model, accel, security_label) = devinfo
-
-        if script:
-            result['script'] = script
-        if ip:
-            result['ip'] = ip
-        if bridge:
-            result['bridge'] = bridge
-        if mac:
-            result['mac'] = mac
-        if typ:
-            result['type'] = typ
-        if vifname:
-            result['vifname'] = vifname
-        if rate:
-            result['rate'] = rate
-        if uuid:
-            result['uuid'] = uuid
-        if model:
-            result['model'] = model
-        if accel:
-            result['accel'] = accel
-        if security_label:
-            result['security_label'] = security_label
+                y = self.readBackendTxn(transaction, devid, x)
+            if y:
+                result[x] = y
 
         return result
diff -r 6a8faf6d98b2 tools/xenstore/xs.c
--- a/tools/xenstore/xs.c	Wed Sep 17 15:46:15 2008 +0100
+++ b/tools/xenstore/xs.c	Tue Sep 30 13:19:54 2008 +0200
@@ -17,6 +17,7 @@
     Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 */
 
+#define _GNU_SOURCE
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
@@ -232,6 +233,11 @@ void xs_daemon_close(struct xs_handle *h
 	free(h);
 }
 
+void xs_domain_close(struct xs_handle *h)
+{
+	xs_daemon_close(h);
+}
+
 static bool read_all(int fd, void *data, unsigned int len)
 {
 	while (len) {
@@ -739,6 +745,60 @@ char *xs_get_domain_path(struct xs_handl
 	return xs_single(h, XBT_NULL, XS_GET_DOMAIN_PATH, domid_str, NULL);
 }
 
+/* read the vm path from /vm_path/<domid> */
+char *xs_get_vm_path(struct xs_handle *h, unsigned int domid)
+{
+	char vm_path[MAX_STRLEN(domid) + 9];
+	void *ret;
+	unsigned int  len;
+
+	sprintf(vm_path, "/vm_path/%u", domid);
+
+	ret = xs_single(h, XBT_NULL, XS_READ, vm_path, &len);
+	if (len)
+		return ret;
+	return NULL;
+}
+
+char *xs_get_backend_dev(struct xs_handle *h, unsigned int domid,
+        const char *devclass, unsigned int devid)
+{
+        char *vm_path;
+        char *vm_dev_path;
+        char *dev_path = NULL;
+        unsigned int   len;
+
+        vm_path = xs_get_vm_path(h, domid);
+        if (vm_path == NULL)
+                return NULL;
+
+        if (asprintf(&vm_dev_path, "%s/device/%s/%d/backend", vm_path,
+                devclass, devid) == -1) {
+                errno = ENOMEM;
+                goto out;
+        }
+
+        dev_path = xs_single(h, XBT_NULL, XS_READ, vm_dev_path, &len);
+        if (!dev_path) {
+                errno = ENOENT;
+                goto out;
+        }
+
+out:
+        free(vm_path);
+        free(vm_dev_path);
+        return dev_path;
+}
+
+char *xs_get_frontend_dev(struct xs_handle *h, unsigned int domid,
+	const char *devclass, unsigned int devid)
+{
+	char *path;
+	asprintf(&path, "/local/domain/%d/device/%s/%d",
+			domid, devclass, devid);
+	return path;
+}
+
 bool xs_is_domain_introduced(struct xs_handle *h, unsigned int domid)
 {
 	char *domain = single_with_domid(h, XS_IS_DOMAIN_INTRODUCED, domid);
diff -r 6a8faf6d98b2 tools/xenstore/xs.h
--- a/tools/xenstore/xs.h	Wed Sep 17 15:46:15 2008 +0100
+++ b/tools/xenstore/xs.h	Mon Sep 29 16:21:31 2008 +0200
@@ -42,6 +42,7 @@ struct xs_handle *xs_daemon_open_readonl
 
 /* Close the connection to the xs daemon. */
 void xs_daemon_close(struct xs_handle *);
+void xs_domain_close(struct xs_handle *);
 
 /* Get contents of a directory.
  * Returns a malloced array: call free() on it after use.
@@ -146,6 +147,17 @@ bool xs_release_domain(struct xs_handle 
  */
 char *xs_get_domain_path(struct xs_handle *h, unsigned int domid);
 
+/* Query the vm path for a domain. Call free() after use.
+ */
+char *xs_get_vm_path(struct xs_handle *h, unsigned int domid);
+
+/* Get a device backend path from domain, devclass, devid */
+char *xs_get_backend_dev(struct xs_handle *h, unsigned int domid, const char *devclass, unsigned int devid);
+
+/* Get a device frontend path from domain, devclass, devid */
+char *xs_get_frontend_dev(struct xs_handle *h, unsigned int domid, const char *devclass, unsigned int devid);
+
+
 /* Return whether the domain specified has been introduced to xenstored.
  */
 bool xs_is_domain_introduced(struct xs_handle *h, unsigned int domid);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] [Xend] Move some backend configuration

[John Levon]

On Tue, Sep 30, 2008 at 04:24:43PM +0200, Pascal Bouchareine wrote:

> This patch moves some dom0 variables and backend device
> configuration from frontend directories to 
> /local/domain/<backdomid>/backend or /vm.

What is the point of this? These paths, however wrong they might be, are
API, surely.

regards
john

Re: [PATCH] [Xend] Move some backend configuration

[Keir Fraser]

On 30/9/08 15:43, "John Levon" <levon@movementarian.org> wrote:

> On Tue, Sep 30, 2008 at 04:24:43PM +0200, Pascal Bouchareine wrote:
> 
>> This patch moves some dom0 variables and backend device
>> configuration from frontend directories to
>> /local/domain/<backdomid>/backend or /vm.
> 
> What is the point of this? These paths, however wrong they might be, are
> API, surely.

Which guaranteed API would that be? These paths are private to the toolstack
implementation. Perhaps the only exception is the
xenconsoled-to-console-client xenstore path, but that is the one that most
urgently needs to change, since we can't trust domUs not to mess with the
tty path, for example.

 -- Keir

Re: [PATCH] [Xend] Move some backend configuration

[John Levon]

On Tue, Sep 30, 2008 at 03:48:29PM +0100, Keir Fraser wrote:

> On 30/9/08 15:43, "John Levon" <levon@movementarian.org> wrote:
> 
> > On Tue, Sep 30, 2008 at 04:24:43PM +0200, Pascal Bouchareine wrote:
> > 
> >> This patch moves some dom0 variables and backend device
> >> configuration from frontend directories to
> >> /local/domain/<backdomid>/backend or /vm.
> > 
> > What is the point of this? These paths, however wrong they might be, are
> > API, surely.
> 
> Which guaranteed API would that be? These paths are private to the toolstack
> implementation. Perhaps the only exception is the

Precisely the problem, there's absolutely no idea or indication what is
and isn't private. Thus you get libvirt looking in places it maybe
shouldn't, but how are they supposed to know?

I'm pretty sure this patch breaks libvirt again.

> xenconsoled-to-console-client xenstore path, but that is the one that most
> urgently needs to change, since we can't trust domUs not to mess with the
> tty path, for example.

If it's a security fix (and I see the issue), it needs to be much more
public than this patch was, and of course backported to at least 3.2
ASAP.

regards
john

Re: [PATCH] [Xend] Move some backend configuration

[Keir Fraser]

On 30/9/08 15:58, "John Levon" <levon@movementarian.org> wrote:

>> Which guaranteed API would that be? These paths are private to the toolstack
>> implementation. Perhaps the only exception is the
> 
> Precisely the problem, there's absolutely no idea or indication what is
> and isn't private. Thus you get libvirt looking in places it maybe
> shouldn't, but how are they supposed to know?
> 
> I'm pretty sure this patch breaks libvirt again.

I don't really see why it would. Beyond the console-client-to-daemon
interface change (which we can make not depend on the /vm_path stuff) the
rest is changing things which I believe are internal to xend.

If libvirt is really going in at this private state, perhaps we could patch
xend so that it writes to the new locations *in addition to* the old
locations. It then has its own guaranteed-private and -consistent state,
while libvirt can still read from old locations.

 -- Keir

Re: [PATCH] [Xend] Move some backend configuration

[Ian Jackson]

Keir Fraser writes ("Re: [Xen-devel] [PATCH] [Xend] Move some backend configuration"):
> If libvirt is really going in at this private state, perhaps we could patch
> xend so that it writes to the new locations *in addition to* the old
> locations. It then has its own guaranteed-private and -consistent state,
> while libvirt can still read from old locations.

There are other reasons besides libvirt why we might want to write to
the old locations for a while too:  it will reduce the problems of
version skew between various components - for example, tools, and
hypervisor.

Does qemu-dm need to interact with this at all ?

Ian.

Re: [PATCH] [Xend] Move some backend configuration

[Daniel P. Berrange]

On Tue, Sep 30, 2008 at 04:09:49PM +0100, Keir Fraser wrote:
> On 30/9/08 15:58, "John Levon" <levon@movementarian.org> wrote:
> 
> >> Which guaranteed API would that be? These paths are private to the toolstack
> >> implementation. Perhaps the only exception is the
> > 
> > Precisely the problem, there's absolutely no idea or indication what is
> > and isn't private. Thus you get libvirt looking in places it maybe
> > shouldn't, but how are they supposed to know?
> > 
> > I'm pretty sure this patch breaks libvirt again.
> 
> I don't really see why it would. Beyond the console-client-to-daemon
> interface change (which we can make not depend on the /vm_path stuff) the
> rest is changing things which I believe are internal to xend.
> 
> If libvirt is really going in at this private state, perhaps we could patch
> xend so that it writes to the new locations *in addition to* the old
> locations. It then has its own guaranteed-private and -consistent state,
> while libvirt can still read from old locations.

I've done a quick check of what libvirt accesses in xenstore:

General VM metadata

 /local/domain/%d/vm 
 /local/domain/%d/cpu
 /local/domain/%d/name
 /local/domain/%d/running
 /local/domain/%d/memory/target
 /local/domain/%d/cpu_time

Console data

 /local/domain/%d/console/vnc-port
 /local/domain/%d/console/tty

For fetching disk/net statistics

 /local/domain/0/backend/vbd/%d/%d/state
 /local/domain/0/backend/vif/%d
 /local/domain/0/backend/vbd/%d
 /local/domain/0/backend/tap/%d


The first group is not being changed, so that's not a problem. Likewise
the latter is not impacted. The only problem I see is with the console
TTY paths and VNC port.

While I would prefer it not to change, or have backwards compatability
data written in existing fields, we have the ability to check multiple
different data sources. So if the TTY info and VNC port is available
via the 'xm list --long' output we could make libvirt check there if it
fails to find it in xenstore. We only historically used xenstored for
the few occassions when the 'xm list --long' output was incomplete.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [PATCH] [Xend] Move some backend configuration

[Keir Fraser]

On 30/9/08 16:30, "Daniel P. Berrange" <berrange@redhat.com> wrote:

> Console data
> 
>  /local/domain/%d/console/vnc-port
>  /local/domain/%d/console/tty

Duplicating this pair of nodes sounds fine to me, *but* then libvirt is
simply remaining vulnerable to the kind of attack we're are looking to
avoid? Can any good really come from keeping the old locations?

 -- Keir

Re: [PATCH] [Xend] Move some backend configuration

[John Levon]

On Tue, Sep 30, 2008 at 05:09:21PM +0100, Keir Fraser wrote:

> > Console data
> > 
> >  /local/domain/%d/console/vnc-port
> >  /local/domain/%d/console/tty
> 
> Duplicating this pair of nodes sounds fine to me, *but* then libvirt is
> simply remaining vulnerable to the kind of attack we're are looking to
> avoid? Can any good really come from keeping the old locations?

Why isn't xenstored refusing writes/deletes from domid != 0 for these ?
Isn't this a much better fix?

It's ugly, but the mistake was made a long time ago, and it seems like
it should be lived with.

BTW, the ability to change the name or whatever also seems suspect,
though most likely less serious.

regards
john

Re: [PATCH] [Xend] Move some backend configuration

[Pascal Bouchareine]

On Tue, Sep 30, 2008 at 05:35:37PM +0100, John Levon wrote:
> Why isn't xenstored refusing writes/deletes from domid != 0 for these ?
> Isn't this a much better fix?

We have to manage races and such, and prevent deletion up to the
parent nodes, too - Was not sure this was wanted/easy to do, or
clean as you mention

> BTW, the ability to change the name or whatever also seems suspect,
> though most likely less serious.

Untrusted user input coming into dom0 surely leads to bad things. 
"name" is stored into /vm too, I guess this one is used by tools ?

Most sensitive information in xend can be moved replacing calls to
read/storeDom with calls to read/storeVm.

-- 
\o/   Pascal Bouchareine - Gandi 
 g    0170393757           15, place de la Nation - 75011 Paris      

Re: [PATCH] [Xend] Move some backend configuration

[Daniel P. Berrange]

On Tue, Sep 30, 2008 at 05:09:21PM +0100, Keir Fraser wrote:
> On 30/9/08 16:30, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > Console data
> > 
> >  /local/domain/%d/console/vnc-port
> >  /local/domain/%d/console/tty
> 
> Duplicating this pair of nodes sounds fine to me, *but* then libvirt is
> simply remaining vulnerable to the kind of attack we're are looking to
> avoid? Can any good really come from keeping the old locations?

Given that this is security sensitive, I have no objection to updating
libvirt to read from the new locations. The only thing I need to work 
out is a reliable way to choose when to use the new location, vs the 
looking at old location (for compat with existing deployments). 

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [PATCH] [Xend] Move some backend configuration

[John Levon]

On Tue, Sep 30, 2008 at 05:35:52PM +0100, Daniel P. Berrange wrote:

> On Tue, Sep 30, 2008 at 05:09:21PM +0100, Keir Fraser wrote:
> > On 30/9/08 16:30, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > 
> > > Console data
> > > 
> > >  /local/domain/%d/console/vnc-port
> > >  /local/domain/%d/console/tty
> > 
> > Duplicating this pair of nodes sounds fine to me, *but* then libvirt is
> > simply remaining vulnerable to the kind of attack we're are looking to
> > avoid? Can any good really come from keeping the old locations?
> 
> Given that this is security sensitive, I have no objection to updating
> libvirt to read from the new locations. The only thing I need to work 
> out is a reliable way to choose when to use the new location, vs the 
> looking at old location (for compat with existing deployments). 

I think the existence of /vm_path would do that, but we need to move
*all* this stuff, surely. /local/domain/X/ should be effectively
write-only from dom0 since none of it is trustworthy.

regards
john

Re: [PATCH] [Xend] Move some backend configuration

[Keir Fraser]

On 30/9/08 17:35, "Daniel P. Berrange" <berrange@redhat.com> wrote:

>> Duplicating this pair of nodes sounds fine to me, *but* then libvirt is
>> simply remaining vulnerable to the kind of attack we're are looking to
>> avoid? Can any good really come from keeping the old locations?
> 
> Given that this is security sensitive, I have no objection to updating
> libvirt to read from the new locations. The only thing I need to work
> out is a reliable way to choose when to use the new location, vs the
> looking at old location (for compat with existing deployments).

That's an interesting question. Obviously you don't want to race their
creation and go down the unsafe path unnecessarily.

We could add a node to xenstore, or append version/feature info to the pid
file? Do you have a preference?

 -- Keir

Re: [PATCH] [Xend] Move some backend configuration

[Daniel P. Berrange]

On Tue, Sep 30, 2008 at 05:46:04PM +0100, Keir Fraser wrote:
> On 30/9/08 17:35, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> >> Duplicating this pair of nodes sounds fine to me, *but* then libvirt is
> >> simply remaining vulnerable to the kind of attack we're are looking to
> >> avoid? Can any good really come from keeping the old locations?
> > 
> > Given that this is security sensitive, I have no objection to updating
> > libvirt to read from the new locations. The only thing I need to work
> > out is a reliable way to choose when to use the new location, vs the
> > looking at old location (for compat with existing deployments).
> 
> That's an interesting question. Obviously you don't want to race their
> creation and go down the unsafe path unnecessarily.
> 
> We could add a node to xenstore, or append version/feature info to the pid
> file? Do you have a preference?

I think its probably best to have explicit "feature" info written into
somewhere in xenstore to indicate that the new layout is in use - "version" 
info would get too confusing when we inevitably have to backport this stuff.

To avoid a race condition we'd not want it in the per-VM areas. It'd want
to be a global feature flag we can probe once when libvirt connects, 
rather than probing  per guest.

I notice there's a  /tool area that's unused

  # xenstore-ls /tool
  xenstored = ""

Could put a little feature flag node there perhaps ?


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [PATCH] [Xend] Move some backend configuration

[Pascal Bouchareine]

On Tue, Sep 30, 2008 at 05:35:52PM +0100, Daniel P. Berrange wrote:
> libvirt to read from the new locations. The only thing I need to work 
> out is a reliable way to choose when to use the new location, vs the 
> looking at old location (for compat with existing deployments). 

I think /vm_path existence indicates to look through /vm/, as for
the race I'm not sure how to handle this, however /vm and /vm_path
are updated before device creation - this should be equivalent to
looking at /local/domain.

-- 
\o/   Pascal Bouchareine - Gandi 
 g    0170393757           15, place de la Nation - 75011 Paris      

Re: [PATCH] [Xend] Move some backend configuration

[Ian Jackson]

Keir Fraser writes ("Re: [Xen-devel] [PATCH] [Xend] Move some backend configuration"):
> On 30/9/08 16:30, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > Console data
> >  /local/domain/%d/console/vnc-port
> >  /local/domain/%d/console/tty
> 
> Duplicating this pair of nodes sounds fine to me, *but* then libvirt is
> simply remaining vulnerable to the kind of attack we're are looking to
> avoid? Can any good really come from keeping the old locations?

Once again we have this tradeoff: in an old installation which has not
been properly patched or updated, should we
  (a) continue to let the system `work' but be vulnerable
  (b) make the system report an error so that the administrator
      knows that it needs to be fixed.

At least in this case I suppose we can expect old installations to be
patched _eventually_ ...

Ian.

Re: [PATCH] [Xend] Move some backend configuration

[Keir Fraser]

On 2/10/08 10:34, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote:

> Once again we have this tradeoff: in an old installation which has not
> been properly patched or updated, should we
>   (a) continue to let the system `work' but be vulnerable
>   (b) make the system report an error so that the administrator
>       knows that it needs to be fixed.
> 
> At least in this case I suppose we can expect old installations to be
> patched _eventually_ ...

An update on this: I solved this issue by fiddling permissions in xenstore
after all! /local/domain/<domid> is now read-only to the guest, and specific
subdirs only are writable (currently device, error and control).

This fixes the console vulnerability with no annoying movement of entries,
and also gets rid of the new /vm_path entries in xenstore since
/l/d/<domid>/vm can be trusted now.

I've compacted the changesets together and backported to 3.3 for 3.3.1.
It'll also be an obvious candidate for 3.2 branch if that branch gets an
ongoing maintainer.

Of course the one downside is that this slightly changes the guest-visible
interface since it can't scribble at will in /l/d/<domid> any more. I hope
noone was relying on that! If we need to open up some more specific subdirs
for write access, I will consider that.

 -- Keir

Re: [PATCH] [Xend] Move some backend configuration

[Pascal Bouchareine]

On Thu, Oct 02, 2008 at 10:49:34AM +0100, Keir Fraser wrote:
> An update on this: I solved this issue by fiddling permissions in xenstore
> after all! /local/domain/<domid> is now read-only to the guest, and specific
> subdirs only are writable (currently device, error and control).

writing into device allows the guest to rewrite it's backend
location, this should be protected too i guess ?

the patch stored backend paths into /vm for this reason

-- 
\o/   Pascal Bouchareine - Gandi 
 g    0170393757           15, place de la Nation - 75011 Paris      

Re: [PATCH] [Xend] Move some backend configuration

[Keir Fraser]

On 2/10/08 11:16, "Pascal Bouchareine" <pascal@gandi.net> wrote:

> On Thu, Oct 02, 2008 at 10:49:34AM +0100, Keir Fraser wrote:
>> An update on this: I solved this issue by fiddling permissions in xenstore
>> after all! /local/domain/<domid> is now read-only to the guest, and specific
>> subdirs only are writable (currently device, error and control).
> 
> writing into device allows the guest to rewrite it's backend
> location, this should be protected too i guess ?
> 
> the patch stored backend paths into /vm for this reason

I kept that part of your patch as indeed the guest can still modify its
backend reference.

 -- Keir

Re: [PATCH] [Xend] Move some backend configuration

[Ian Jackson]

Pascal Bouchareine writes ("Re: [Xen-devel] [PATCH] [Xend] Move some backend configuration"):
> On Thu, Oct 02, 2008 at 10:49:34AM +0100, Keir Fraser wrote:
> > An update on this: I solved this issue by fiddling permissions in xenstore
> > after all! /local/domain/<domid> is now read-only to the guest, and specific
> > subdirs only are writable (currently device, error and control).
> 
> writing into device allows the guest to rewrite it's backend
> location, this should be protected too i guess ?

We will arrange for the backend location not to be trusted by anything
important.  In fact, it is entirely formulaic: if you know which
domain the backend is supposed to be in, you can simply shuffle the
path components.  And you can double check against the backend's
frontend path.

Ian.

Re: [PATCH] [Xend] Move some backend configuration

[Keir Fraser]

On 2/10/08 11:21, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote:

>> writing into device allows the guest to rewrite it's backend
>> location, this should be protected too i guess ?
> 
> We will arrange for the backend location not to be trusted by anything
> important.  In fact, it is entirely formulaic: if you know which
> domain the backend is supposed to be in, you can simply shuffle the
> path components.  And you can double check against the backend's
> frontend path.

If you know the backend domid this works great. You don't need to check
anything in this case. If you try to validate the frontend's backend
reference then that's hard: strictly speaking you can only trust the
/local/domain/0 path prefix since otherwise two domains could collude to
redirect you to a backend directory under their control (or a domain could
point you at a 'backend directory' under its own path prefix, for example).
So this approach really only works for backends known to be in dom0 (which
of course is true for the vast majority of Xen installations). Hence xend is
storing the backend path under /vm where it's safe. Equally it could store
only the backend-id and construct the backend path from that.

 -- Keir

Re: [PATCH] [Xend] Move some backend configuration

[Ian Jackson]

Keir Fraser writes ("Re: [Xen-devel] [PATCH] [Xend] Move some backend configuration"):
> If you know the backend domid this works great. You don't need to check
> anything in this case.

Right.  qemu (non-stubdom, which is the relevant case) knows its own
domid and that is the relevant one - because qemu is providing a
parallel path to the pv frontend/backend arrangements, the relevant
backend it is supposed to be the one that qemu is running in.

If it isn't then there is something wrong, so it should check it.

Ian.

Re: [PATCH] [Xend] Move some backend configuration

[John Levon]

On Thu, Oct 02, 2008 at 10:49:34AM +0100, Keir Fraser wrote:

> Of course the one downside is that this slightly changes the guest-visible
> interface since it can't scribble at will in /l/d/<domid> any more. I hope
> noone was relying on that! If we need to open up some more specific subdirs
> for write access, I will consider that.

We're writing to the following paths that aren't standard:

device-misc/vif/default-link
hvmpv/{xpv,xdf,xpvd,xnf}/version

Can they be opened up? (Ideally we'd use the guest/ suggestion below,
and we will, but there's already stuff out there assuming it can do
this.)

Also, I'd like to see an explicit /l/d/domid/guest directory. The guest
can write anything there, but there's a clear indicator that it's not to
be trusted (we could even document this stuff by expanding xenstore.txt
?)

regards
john

Re: [PATCH] [Xend] Move some backend configuration

[Keir Fraser]

On 2/10/08 14:45, "John Levon" <levon@movementarian.org> wrote:

> We're writing to the following paths that aren't standard:
> 
> device-misc/vif/default-link
> hvmpv/{xpv,xdf,xpvd,xnf}/version
> 
> Can they be opened up? (Ideally we'd use the guest/ suggestion below,
> and we will, but there's already stuff out there assuming it can do
> this.)

What happens if you try to write them, and fail? What are they actually used
for (e.g., communication with your own xvm tool stack?).

> Also, I'd like to see an explicit /l/d/domid/guest directory. The guest
> can write anything there, but there's a clear indicator that it's not to
> be trusted (we could even document this stuff by expanding xenstore.txt
> ?)

Probably a good idea.

 -- Keir

Re: [PATCH] [Xend] Move some backend configuration

[Keir Fraser]

On 2/10/08 15:04, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>> Also, I'd like to see an explicit /l/d/domid/guest directory. The guest
>> can write anything there, but there's a clear indicator that it's not to
>> be trusted (we could even document this stuff by expanding xenstore.txt
>> ?)
> 
> Probably a good idea.

On the other hand, should we be encouraging guests to squirrel
guest-specific stuff away in xenstore? Really it's for communication with
backends (via device/ subdir) and with toolstacks (which can open up access
to other subdirs if they wish, since presumably they run in dom0). It's not
supposed to be a handy storage area (and indeed it doesn't persist across
save/restore).

 -- Keir

Re: [PATCH] [Xend] Move some backend configuration

[John Levon]

On Thu, Oct 02, 2008 at 03:04:48PM +0100, Keir Fraser wrote:

> > We're writing to the following paths that aren't standard:
> > 
> > device-misc/vif/default-link
> > hvmpv/{xpv,xdf,xpvd,xnf}/version
> > 
> > Can they be opened up? (Ideally we'd use the guest/ suggestion below,
> > and we will, but there's already stuff out there assuming it can do
> > this.)
> 
> What happens if you try to write them, and fail? What are they actually used
> for (e.g., communication with your own xvm tool stack?).

They're both tool stack values, so in the sense that we can permanently
patch xenstored, you don't need the change, you're right. Although, we're already
carrying way way too much customizations.

The former contains the "main" IP address of the domU. The latter
contain driver versions, to allow for the tool stack to determine a need
for updates. Note that neither are general enough to try and merge up as
a standard thing IMO, perhaps you disagree.

Regarding guest/ - xenstore is by far the simplest and easiest way to
communicate with the tool stack. I strongly doubt that we're the only
vendor to have done, or want to do, something like the above. Of course
there are caveats, but compared to the alternative (guest additions -
blech), it's pretty low cost.

I don't see it going away, so in my mind explicitly saying now that you
should only use guest/$(VENDOR) will avoid a lot of trouble down the
line.

regards
john

Re: [PATCH] [Xend] Move some backend configuration

[Keir Fraser]

On 2/10/08 16:09, "John Levon" <levon@movementarian.org> wrote:

>> What happens if you try to write them, and fail? What are they actually used
>> for (e.g., communication with your own xvm tool stack?).
> 
> They're both tool stack values, so in the sense that we can permanently
> patch xenstored, you don't need the change, you're right. Although, we're
> already
> carrying way way too much customizations.

Well, you don't need to patch xenstored. Your customised toolstack -- the
one that consumes these extra nodes -- can also open them up, right? The
only reason to open them in xend is if that is the difference between them
working and not working running against the default open-source toolstack.

I will allow these through if there is a net win for compatibility with the
default open-source toolstack. But I don't particularly want to do so just
to save you a one-line patch to your xend or equivalent.

> I don't see it going away, so in my mind explicitly saying now that you
> should only use guest/$(VENDOR) will avoid a lot of trouble down the
> line.

Mmm... I suppose I can see logic here. Vendor daemons running alongside
open-source xend for example.

 -- Keir

Re: [PATCH] [Xend] Move some backend configuration

[John Levon]

On Thu, Oct 02, 2008 at 04:25:39PM +0100, Keir Fraser wrote:

> > They're both tool stack values, so in the sense that we can permanently
> > patch xenstored, you don't need the change, you're right. Although, we're
> > already
> > carrying way way too much customizations.
> 
> Well, you don't need to patch xenstored. Your customised toolstack -- the
> one that consumes these extra nodes -- can also open them up, right? The
> only reason to open them in xend is if that is the difference between them
> working and not working running against the default open-source toolstack.
> 
> I will allow these through if there is a net win for compatibility with the
> default open-source toolstack. But I don't particularly want to do so just
> to save you a one-line patch to your xend or equivalent.

I should have read your fix. We will probably do that one-liner to xend
too (the toolstack lives at least two levels above xend, so it's not the
best place). 

thanks,
john

Re: [PATCH] [Xend] Move some backend configuration

[Ian Jackson]

John Levon writes ("Re: [Xen-devel] [PATCH] [Xend] Move some backend configuration"):
> On Thu, Oct 02, 2008 at 10:49:34AM +0100, Keir Fraser wrote:
> > Of course the one downside is that this slightly changes the guest-visible
> > interface since it can't scribble at will in /l/d/<domid> any more. I hope
> > noone was relying on that! If we need to open up some more specific subdirs
> > for write access, I will consider that.
> 
> We're writing to the following paths that aren't standard:
> 
> device-misc/vif/default-link
> hvmpv/{xpv,xdf,xpvd,xnf}/version

What are these and how are they used ?

>  (we could even document this stuff by expanding xenstore.txt ?)

xenstore.txt currently has only the xenstored<->domains protocol, and
doesn't specify the structure or meaning of the xenstore tree.

interface.tex is supposed to do the latter but it's hopelessly out of
date and of course no-one really wants to edit TeX when they update
anything.

If some kind soul were to make a stab at extracting the relevant
(true!) parts of interface.tex into a new .txt file that would be very
nice :-).

Ian.

Re: [PATCH] [Xend] Move some backend configuration

[Pascal Bouchareine]

On Tue, Sep 30, 2008 at 03:58:18PM +0100, John Levon wrote:
> Precisely the problem, there's absolutely no idea or indication what is
> and isn't private. Thus you get libvirt looking in places it maybe
> shouldn't, but how are they supposed to know?
> 
> I'm pretty sure this patch breaks libvirt again.

Regarding compatibility:

This might not break too much stuff as we kept the original backend
path(s) at their location. Most obvious fix for other clients is
changing the reading of the backend location from the frontend directory,
but the old behaviour still works.

However, some parts (the console tty, location) have changed and
require attention.

> If it's a security fix (and I see the issue), it needs to be much more
> public than this patch was, and of course backported to at least 3.2
> ASAP.

It might well be a security fix, even if this sounds like a moderate
issue yet.

I agree this requires a quick update, but we need to discuss it here
before publicizing ?

-- 
\o/   Pascal Bouchareine - Gandi 
 g    0170393757           15, place de la Nation - 75011 Paris      

Re: [PATCH] [Xend] Move some backend configuration

[Keir Fraser]

On 30/9/08 15:48, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> On 30/9/08 15:43, "John Levon" <levon@movementarian.org> wrote:
> 
>> On Tue, Sep 30, 2008 at 04:24:43PM +0200, Pascal Bouchareine wrote:
>> 
>>> This patch moves some dom0 variables and backend device
>>> configuration from frontend directories to
>>> /local/domain/<backdomid>/backend or /vm.
>> 
>> What is the point of this? These paths, however wrong they might be, are
>> API, surely.
> 
> Which guaranteed API would that be? These paths are private to the toolstack
> implementation. Perhaps the only exception is the
> xenconsoled-to-console-client xenstore path, but that is the one that most
> urgently needs to change, since we can't trust domUs not to mess with the
> tty path, for example.

Actually I'm wrong: avoiding finding backend paths by looking in
frontend-writable xenstore nodes is even more important. But that bit of the
patch should be uncontentious: it's just xend adding extra private state in
xenstore for its own private use.

The 'helper' functions added to libxenstore perhaps make this look more like
an interface change than it actually is. Noone is forced to use those extra
utility functions, and they're only going to make sense when the /vm_path/
stuff has been set up by xend. Arguably those functions should be
implemented in the xend python wrapper for xenstore, and perhaps duplicated
as necessary in the console client (or arguably the client could safely
assume the console daemon is in dom0 and hardcode the backend path prefix!).

The console client-to-daemon interface change is unfortunate, but I can't
think of an easy alternative fix.

 -- Keir

Re: [PATCH] [Xend] Move some backend configuration

[John Levon]

On Tue, Sep 30, 2008 at 04:02:32PM +0100, Keir Fraser wrote:

> frontend-writable xenstore nodes is even more important. But that bit of the
> patch should be uncontentious: it's just xend adding extra private state in

Yep, no problem with that.

> The console client-to-daemon interface change is unfortunate, but I can't
> think of an easy alternative fix.

No me neither, I think you're indeed forced to break the API and libvirt
will just have to change.

regards
john