Xen bug 647: Creation of Mini-OS domain causes certain xm commands to hang

Xen bug 647: Creation of Mini-OS domain causes certain xm commands to hang

[Melvin Anderson]

Dear Aravindh,

Did you or anyone else on the Xen distribution list make any progress
with bug 647: "Creation of Mini-OS domain causes certain xm commands to
hang"?

I am getting exactly the same fault with changeset 10508:1da8f53ce65b
when trying to run mini-OS.  I have tried to trace exactly what the
Python code in xend is doing, but I am not a Python expert, and I am not
convinced that what I am seeing makes sense.  

After starting mini-OS and running "xm list" it seems as if the function
"domains" in XMLRPCServer.py is entered, but never returns.

Regards,

Melvin.

--

<Melvin.Anderson@hp.com>, HPLabs, Bristol.

RE: Xen bug 647: Creation of Mini-OS domain causes certain xm commandsto hang

[Puthiyaparambil, Aravindh]

Melvin,

> Did you or anyone else on the Xen distribution list make any progress
> with bug 647: "Creation of Mini-OS domain causes certain xm commands
to
> hang"?

Gregor said that he would be looking into this. I am not sure if he has
made any progress.

> After starting mini-OS and running "xm list" it seems as if the
function
> "domains" in XMLRPCServer.py is entered, but never returns.

Yup that is where the hang happens. BTW, if you don't initialize xenbus
this problem does not happen the last time I tried. So I think it is a
locking issue in the mini-os xenbus code.

Cheers,
Aravindh

Re: Xen bug 647: Creation of Mini-OS domain causes certain xm commandsto hang

[Grzegorz Milos]

Steven Smith, who originally wrote XenBus code has now updated it as a 
part of a larger patch. It's possible that the problem will go away 
then. We are currently working on checking that in.

I'll report back when that happens.

Cheers
Gregor

> Melvin,
> 
> 
>>Did you or anyone else on the Xen distribution list make any progress
>>with bug 647: "Creation of Mini-OS domain causes certain xm commands
> 
> to
> 
>>hang"?
> 
> 
> Gregor said that he would be looking into this. I am not sure if he has
> made any progress.
> 
> 
>>After starting mini-OS and running "xm list" it seems as if the
> 
> function
> 
>>"domains" in XMLRPCServer.py is entered, but never returns.
> 
> 
> Yup that is where the hang happens. BTW, if you don't initialize xenbus
> this problem does not happen the last time I tried. So I think it is a
> locking issue in the mini-os xenbus code.
> 
> Cheers,
> Aravindh

Re: Re: Xen bug 647: Creation of Mini-OS domain causes certain xm commandsto hang

[Keir Fraser]

On 29 Jun 2006, at 18:40, Grzegorz Milos wrote:

> Steven Smith, who originally wrote XenBus code has now updated it as a 
> part of a larger patch. It's possible that the problem will go away 
> then. We are currently working on checking that in.
>
> I'll report back when that happens.

It'd be nice if broken guests didn't hang the control server!

  -- Keir

[PATCH] Re: Re: Xen bug 647: Creation of Mini-OS domain causes certain xm commandsto hang

[Grzegorz Milos]

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

Hello all!

Xenbus patch seems to have solved the xend/xm problem. Could please  
apply it Keir?

Patch message:
Implements XenBus transactions in MiniOS.

Signed-of-by: Steven Smith <sos22@cam.ac.uk>



Cheers
Gregor

[-- Attachment #2: minios.xenbus.diff --]
[-- Type: application/octet-stream, Size: 9359 bytes --]

diff -r 5188352a7417 extras/mini-os/include/xenbus.h
--- a/extras/mini-os/include/xenbus.h	Thu Jun 29 08:39:28 2006
+++ b/extras/mini-os/include/xenbus.h	Tue Jul  4 18:25:34 2006
@@ -1,5 +1,8 @@
 #ifndef XENBUS_H__
 #define XENBUS_H__
+
+typedef unsigned long xenbus_transaction_t;
+#define XBT_NIL ((xenbus_transaction_t)0)
 
 /* Initialize the XenBus system. */
 void init_xenbus(void);
@@ -7,28 +10,42 @@
 /* Read the value associated with a path.  Returns a malloc'd error
    string on failure and sets *value to NULL.  On success, *value is
    set to a malloc'd copy of the value. */
-char *xenbus_read(const char *path, char **value);
+char *xenbus_read(xenbus_transaction_t xbt, const char *path, char **value);
 
 /* Associates a value with a path.  Returns a malloc'd error string on
    failure. */
-char *xenbus_write(const char *path, const char *value);
+char *xenbus_write(xenbus_transaction_t xbt, const char *path, const char *value);
 
 /* Removes the value associated with a path.  Returns a malloc'd error
    string on failure. */
-char *xenbus_rm(const char *path);
+char *xenbus_rm(xenbus_transaction_t xbt, const char *path);
 
 /* List the contents of a directory.  Returns a malloc'd error string
    on failure and sets *contents to NULL.  On success, *contents is
    set to a malloc'd array of pointers to malloc'd strings.  The array
    is NULL terminated.  May block. */
-char *xenbus_ls(const char *prefix, char ***contents);
+char *xenbus_ls(xenbus_transaction_t xbt, const char *prefix, char ***contents);
 
 /* Reads permissions associated with a path.  Returns a malloc'd error
    string on failure and sets *value to NULL.  On success, *value is
    set to a malloc'd copy of the value. */
-char *xenbus_get_perms(const char *path, char **value);
+char *xenbus_get_perms(xenbus_transaction_t xbt, const char *path, char **value);
 
 /* Sets the permissions associated with a path.  Returns a malloc'd
    error string on failure. */
-char *xenbus_set_perms(const char *path, domid_t dom, char perm);
+char *xenbus_set_perms(xenbus_transaction_t xbt, const char *path, domid_t dom, char perm);
+
+/* Start a xenbus transaction.  Returns the transaction in xbt on
+   success or a malloc'd error string otherwise. */
+char *xenbus_transaction_start(xenbus_transaction_t *xbt);
+
+/* End a xenbus transaction.  Returns a malloc'd error string if it
+   fails.  abort says whether the transaction should be aborted.
+   Returns 1 in *retry iff the transaction should be retried. */
+char *xenbus_transaction_end(xenbus_transaction_t, int abort,
+			     int *retry);
+
+/* Read path and parse it as an integer.  Returns -1 on error. */
+int xenbus_read_integer(char *path);
+
 #endif /* XENBUS_H__ */
diff -r 5188352a7417 extras/mini-os/xenbus/xenbus.c
--- a/extras/mini-os/xenbus/xenbus.c	Thu Jun 29 08:39:28 2006
+++ b/extras/mini-os/xenbus/xenbus.c	Tue Jul  4 18:25:34 2006
@@ -174,7 +174,7 @@
     create_thread("xenstore", xenbus_thread_func, NULL);
     DEBUG("buf at %p.\n", xenstore_buf);
     err = bind_evtchn(start_info.store_evtchn,
-            xenbus_evtchn_handler);
+		      xenbus_evtchn_handler);
     DEBUG("xenbus on irq %d\n", err);
 }
 
@@ -187,8 +187,8 @@
    by xenbus as if sent atomically.  The header is added
    automatically, using type %type, req_id %req_id, and trans_id
    %trans_id. */
-static void xb_write(int type, int req_id, int trans_id,
-        const struct write_req *req, int nr_reqs)
+static void xb_write(int type, int req_id, xenbus_transaction_t trans_id,
+		     const struct write_req *req, int nr_reqs)
 {
     XENSTORE_RING_IDX prod;
     int r;
@@ -266,9 +266,9 @@
    freed by the caller. */
 static struct xsd_sockmsg *
 xenbus_msg_reply(int type,
-        int trans,
-        struct write_req *io,
-        int nr_reqs)
+		 xenbus_transaction_t trans,
+		 struct write_req *io,
+		 int nr_reqs)
 {
     int id;
     DEFINE_WAIT(w);
@@ -322,14 +322,14 @@
 /* List the contents of a directory.  Returns a malloc()ed array of
    pointers to malloc()ed strings.  The array is NULL terminated.  May
    block. */
-char *xenbus_ls(const char *pre, char ***contents)
+char *xenbus_ls(xenbus_transaction_t xbt, const char *pre, char ***contents)
 {
     struct xsd_sockmsg *reply, *repmsg;
     struct write_req req[] = { { pre, strlen(pre)+1 } };
     int nr_elems, x, i;
     char **res;
 
-    repmsg = xenbus_msg_reply(XS_DIRECTORY, 0, req, ARRAY_SIZE(req));
+    repmsg = xenbus_msg_reply(XS_DIRECTORY, xbt, req, ARRAY_SIZE(req));
     char *msg = errmsg(repmsg);
     if (msg) {
 	*contents = NULL;
@@ -351,12 +351,12 @@
     return NULL;
 }
 
-char *xenbus_read(const char *path, char **value)
+char *xenbus_read(xenbus_transaction_t xbt, const char *path, char **value)
 {
     struct write_req req[] = { {path, strlen(path) + 1} };
     struct xsd_sockmsg *rep;
     char *res;
-    rep = xenbus_msg_reply(XS_READ, 0, req, ARRAY_SIZE(req));
+    rep = xenbus_msg_reply(XS_READ, xbt, req, ARRAY_SIZE(req));
     char *msg = errmsg(rep);
     if (msg) {
 	*value = NULL;
@@ -370,14 +370,14 @@
     return NULL;
 }
 
-char *xenbus_write(const char *path, const char *value)
+char *xenbus_write(xenbus_transaction_t xbt, const char *path, const char *value)
 {
     struct write_req req[] = { 
 	{path, strlen(path) + 1},
 	{value, strlen(value) + 1},
     };
     struct xsd_sockmsg *rep;
-    rep = xenbus_msg_reply(XS_WRITE, 0, req, ARRAY_SIZE(req));
+    rep = xenbus_msg_reply(XS_WRITE, xbt, req, ARRAY_SIZE(req));
     char *msg = errmsg(rep);
     if (msg)
 	return msg;
@@ -385,11 +385,11 @@
     return NULL;
 }
 
-char *xenbus_rm(const char *path)
+char *xenbus_rm(xenbus_transaction_t xbt, const char *path)
 {
     struct write_req req[] = { {path, strlen(path) + 1} };
     struct xsd_sockmsg *rep;
-    rep = xenbus_msg_reply(XS_RM, 0, req, ARRAY_SIZE(req));
+    rep = xenbus_msg_reply(XS_RM, xbt, req, ARRAY_SIZE(req));
     char *msg = errmsg(rep);
     if (msg)
 	return msg;
@@ -397,12 +397,12 @@
     return NULL;
 }
 
-char *xenbus_get_perms(const char *path, char **value)
+char *xenbus_get_perms(xenbus_transaction_t xbt, const char *path, char **value)
 {
     struct write_req req[] = { {path, strlen(path) + 1} };
     struct xsd_sockmsg *rep;
     char *res;
-    rep = xenbus_msg_reply(XS_GET_PERMS, 0, req, ARRAY_SIZE(req));
+    rep = xenbus_msg_reply(XS_GET_PERMS, xbt, req, ARRAY_SIZE(req));
     char *msg = errmsg(rep);
     if (msg) {
 	*value = NULL;
@@ -417,7 +417,7 @@
 }
 
 #define PERM_MAX_SIZE 32
-char *xenbus_set_perms(const char *path, domid_t dom, char perm)
+char *xenbus_set_perms(xenbus_transaction_t xbt, const char *path, domid_t dom, char perm)
 {
     char value[PERM_MAX_SIZE];
     snprintf(value, PERM_MAX_SIZE, "%c%hu", perm, dom);
@@ -426,7 +426,7 @@
 	{value, strlen(value) + 1},
     };
     struct xsd_sockmsg *rep;
-    rep = xenbus_msg_reply(XS_SET_PERMS, 0, req, ARRAY_SIZE(req));
+    rep = xenbus_msg_reply(XS_SET_PERMS, xbt, req, ARRAY_SIZE(req));
     char *msg = errmsg(rep);
     if (msg)
 	return msg;
@@ -434,13 +434,72 @@
     return NULL;
 }
 
+char *xenbus_transaction_start(xenbus_transaction_t *xbt)
+{
+    /* xenstored becomes angry if you send a length 0 message, so just
+       shove a nul terminator on the end */
+    struct write_req req = { "", 1};
+    struct xsd_sockmsg *rep;
+    char *err;
+
+    rep = xenbus_msg_reply(XS_TRANSACTION_START, 0, &req, 1);
+    err = errmsg(rep);
+    if (err)
+	return err;
+    sscanf((char *)(rep + 1), "%u", xbt);
+    free(rep);
+    return NULL;
+}
+
+char *
+xenbus_transaction_end(xenbus_transaction_t t, int abort, int *retry)
+{
+    struct xsd_sockmsg *rep;
+    struct write_req req;
+    char *err;
+
+    *retry = 0;
+
+    req.data = abort ? "F" : "T";
+    req.len = 2;
+    rep = xenbus_msg_reply(XS_TRANSACTION_END, t, &req, 1);
+    err = errmsg(rep);
+    if (err) {
+	if (!strcmp(err, "EAGAIN")) {
+	    *retry = 1;
+	    free(err);
+	    return NULL;
+	} else {
+	    return err;
+	}
+    }
+    free(rep);
+    return NULL;
+}
+
+int xenbus_read_integer(char *path)
+{
+    char *res, *buf;
+    int t;
+
+    res = xenbus_read(XBT_NIL, path, &buf);
+    if (res) {
+	printk("Failed to read %s.\n", path);
+	free(res);
+	return -1;
+    }
+    sscanf(buf, "%d", &t);
+    free(buf);
+    return t;
+}
+
 static void do_ls_test(const char *pre)
 {
     char **dirs;
     int x;
 
     DEBUG("ls %s...\n", pre);
-    char *msg = xenbus_ls(pre, &dirs);
+    char *msg = xenbus_ls(XBT_NIL, pre, &dirs);
     if (msg) {
 	DEBUG("Error in xenbus ls: %s\n", msg);
 	free(msg);
@@ -458,7 +517,7 @@
 {
     char *res;
     DEBUG("Read %s...\n", path);
-    char *msg = xenbus_read(path, &res);
+    char *msg = xenbus_read(XBT_NIL, path, &res);
     if (msg) {
 	DEBUG("Error in xenbus read: %s\n", msg);
 	free(msg);
@@ -471,7 +530,7 @@
 static void do_write_test(const char *path, const char *val)
 {
     DEBUG("Write %s to %s...\n", val, path);
-    char *msg = xenbus_write(path, val);
+    char *msg = xenbus_write(XBT_NIL, path, val);
     if (msg) {
 	DEBUG("Result %s\n", msg);
 	free(msg);
@@ -483,7 +542,7 @@
 static void do_rm_test(const char *path)
 {
     DEBUG("rm %s...\n", path);
-    char *msg = xenbus_rm(path);
+    char *msg = xenbus_rm(XBT_NIL, path);
     if (msg) {
 	DEBUG("Result %s\n", msg);
 	free(msg);

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

>> Steven Smith, who originally wrote XenBus code has now updated it  
>> as a part of a larger patch. It's possible that the problem will  
>> go away then. We are currently working on checking that in.
>>
>> I'll report back when that happens.
>
> It'd be nice if broken guests didn't hang the control server!
>
>  -- Keir


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

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

Re: [PATCH] Re: Re: Xen bug 647: Creation of Mini-OS domain causes certain xm commandsto hang

[Muli Ben-Yehuda]

On Tue, Jul 04, 2006 at 06:35:18PM +0100, Grzegorz Milos wrote:
> Hello all!
> 
> Xenbus patch seems to have solved the xend/xm problem. Could please  
> apply it Keir?
> 
> Patch message:
> Implements XenBus transactions in MiniOS.

It looks like this patch only updates MiniOS - does this mean that
broken (or malicious...) guests can still hang the control server, or
has that been fixed independently?

Cheers,
Muli

Re: [PATCH] Re: Re: Xen bug 647: Creation of Mini-OS domain causes certain xm commandsto hang

[Grzegorz Milos]

>> Hello all!
>>
>> Xenbus patch seems to have solved the xend/xm problem. Could please
>> apply it Keir?
>>
>> Patch message:
>> Implements XenBus transactions in MiniOS.
>
> It looks like this patch only updates MiniOS - does this mean that
> broken (or malicious...) guests can still hang the control server, or
> has that been fixed independently?
>

Further tests revealed that the problem has not gone away completely  
(although it seems to be happening more sporadically).

If Mini-OS does not initialise XenBus (if you remove xenbus_init()  
call from kernel.c) the problem will not appear at all. So far I have  
not been able to pinpoint the root cause of the problem though. Maybe  
somebody more familiar with XenStore will be able to help.

The patch certainly does not break anything though (this is mostly  
for Keir's attention).

I'll get back to you if I find anything more.

Cheers
Gregor

Re: [PATCH] Re: Re: Xen bug 647: Creation of Mini-OS domain causes certain xm commandsto hang

[Grzegorz Milos]

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

>> It looks like this patch only updates MiniOS - does this mean that
>> broken (or malicious...) guests can still hang the control server, or
>> has that been fixed independently?
>>
>
> Further tests revealed that the problem has not gone away  
> completely (although it seems to be happening more sporadically).
>
> If Mini-OS does not initialise XenBus (if you remove xenbus_init()  
> call from kernel.c) the problem will not appear at all. So far I  
> have not been able to pinpoint the root cause of the problem  
> though. Maybe somebody more familiar with XenStore will be able to  
> help.
>
> The patch certainly does not break anything though (this is mostly  
> for Keir's attention).
>
> I'll get back to you if I find anything more.
>

Ok, found the cause:
MiniOS does test_xenbus soon after booting up. This in turn tries to  
write to some file under device/vif/0/. XenStrore wil create device/ 
vif/0 directory to make the write possible even if no network  
interfaces are defined. This confuses Xend as it expects to find a  
backend corresponding to the bogus frontend. Clearly MiniOS induces  
the incorrect behaviour but it's more of Xend bug.

To answer your question Muli - yes, it's possible for broken/ 
malicious guest to hang the control server.

I'm attaching a simple patch for MiniOS that disables the execution  
of test_xenbus (but removing the writes would do as well).

Keir could you apply please?

Cheers
Gregor

[-- Attachment #2: minios.xenbus.patch --]
[-- Type: application/octet-stream, Size: 697 bytes --]

# HG changeset patch
# User gm281@brent.cl.cam.ac.uk
# Node ID c3d6610fc0f0510a09bd3d6c63a4e8a3f78312cc
# Parent  e1e27f110dab0e5c09bd709c93a0d8bfa4a043b1
XenBus tests disabled in MiniOS. Xend bug induced by write to device/vif/0. 

Signed-off-by: Grzegorz Milos <gm281@cam.ac.uk>

diff -r e1e27f110dab -r c3d6610fc0f0 extras/mini-os/kernel.c
--- a/extras/mini-os/kernel.c	Tue Jul  4 17:41:59 2006
+++ b/extras/mini-os/kernel.c	Wed Jul  5 10:06:14 2006
@@ -104,7 +104,8 @@
 
 void xenbus_tester(void *p)
 {
-    test_xenbus();
+    printk("Xenbus tests disabled, because of a Xend bug.\n");
+    /* test_xenbus(); */
 }
 
 /* This should be overridden by the application we are linked against. */

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

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