Weekly VMX status report. Xen: #18577 & Xen0: #696

Weekly VMX status report. Xen: #18577 & Xen0: #696

[Li, Haicheng]

Hi,

Our testing was blocke by bug #1367; and totally 2 new issues were found this week,

New Bugs:
=====================================================================
1. [Qcow]Guest cannot boot up with Qcow image
http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1367
2. [vt-d][MSI] some call traces printed to serial port
http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1368

Old Bugs:
=====================================================================
1. TSC not accurate in Windows HVM.
http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1291
2. SMP 32e RHEL5.1 timer would be slow,if under working pressure.
http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1290
3. Guest will hang after Save/Restore.
http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1339

ioemu: Re-enable guest boot with blktap devices (Weekly VMX status report. Xen: #18577 & Xen0: #696)

[Yosuke Iwamatsu]

Li, Haicheng wrote:
> Our testing was blocke by bug #1367; and totally 2 new issues were found this week,
> 
> New Bugs:
> =====================================================================
> 1. [Qcow]Guest cannot boot up with Qcow image
> http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1367

Bug #1367 looks caused by the xenstore path checking code recently added
to ioemu-remote. I saw the same problem and found that the code didn't
consider the case blktap devices were attached to the guest. The patch
below should avoid the problem.

Signed-off-by: Yosuke Iwamatsu <y-iwamatsu@ab.jp.nec.com>

diff --git a/xenstore.c b/xenstore.c
index f5aa8a7..39ff8a6 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -158,6 +158,7 @@ static void xenstore_get_backend_path(char 
**backend, const char *devtype,
      char *frontend_path=0;
      char *backend_dompath=0;
      char *expected_backend=0;
+    char *expected_devtype=0;
      char *frontend_backend_path=0;
      char *backend_frontend_path=0;
      char *frontend_doublecheck=0;
@@ -191,11 +192,18 @@ static void xenstore_get_backend_path(char 
**backend, const char *devtype,
      backend_dompath = xs_get_domain_path(xsh, domid_backend);
      if (!backend_dompath) goto out;

+    if (pasprintf(&expected_devtype, "%s", devtype) == -1) goto out;
+
+ again:
      if (pasprintf(&expected_backend, "%s/backend/%s/%lu/%s",
-                  backend_dompath, devtype, frontend_domid, inst_danger)
+                  backend_dompath, expected_devtype, frontend_domid, 
inst_danger)
          == -1) goto out;

      if (strcmp(bpath, expected_backend)) {
+        if (!strcmp(expected_devtype, "vbd")) {
+            pasprintf(&expected_devtype, "tap");
+            goto again;
+        }
          fprintf(stderr, "frontend `%s' expected backend `%s' got `%s',"
                  " ignoring\n", frontend_path, expected_backend, bpath);
          errno = EINVAL;
@@ -223,6 +231,7 @@ static void xenstore_get_backend_path(char 
**backend, const char *devtype,
      free(frontend_path);
      free(backend_dompath);
      free(expected_backend);
+    free(expected_devtype);
      free(frontend_backend_path);
      free(backend_frontend_path);
      free(frontend_doublecheck);

Re: ioemu: Re-enable guest boot with blktap devices ( Weekly VMX status report. Xen: #18577 & Xen0: #696)

[Keir Fraser]

On 14/10/08 05:04, "Yosuke Iwamatsu" <y-iwamatsu@ab.jp.nec.com> wrote:

> Bug #1367 looks caused by the xenstore path checking code recently added
> to ioemu-remote. I saw the same problem and found that the code didn't
> consider the case blktap devices were attached to the guest. The patch
> below should avoid the problem.

Alternatively could just check a shorter path prefix (just /local/domain/0/
would be sufficient I think). Full path checking is obviously inherently a
bit more fragile.

 -- Keir

Re: ioemu: Re-enable guest boot with blktap devices ( Weekly VMX status report. Xen: #18577 & Xen0: #696)

[Ian Jackson]

Yosuke Iwamatsu writes ("ioemu: Re-enable guest boot with blktap devices ([Xen-devel] Weekly VMX status report. Xen: #18577 & Xen0: #696)"):
> Bug #1367 looks caused by the xenstore path checking code recently added
> to ioemu-remote. I saw the same problem and found that the code didn't
> consider the case blktap devices were attached to the guest. The patch
> below should avoid the problem.

Thanks for the report.  Could you try the attached patch instead and
let me know whether it fixes the problem ?

Keir Fraser writes ("Re: ioemu: Re-enable guest boot with blktap devices ([Xen-devel] Weekly VMX status report. Xen: #18577 & Xen0: #696)"):
> Alternatively could just check a shorter path prefix (just /local/domain/0/
> would be sufficient I think). Full path checking is obviously inherently a
> bit more fragile.

Well, yes, but this is a security check and I think those are really
supposed to be brittle.  So I'd prefer to make us check that the path
is definitely completely of an expected form.

Ian.

commit 629adb3f5244169731ff18b16ae919641d81ad76
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Tue Oct 14 11:46:53 2008 +0100

    Fix blktap device backend patch check
    
    Regarding http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1367,
    it appears that the expected backend path check is too strict for'
    blktap devices.  Therefore if the devtype is `vbd' we allow the
    backend to be `tap'.
    
    Thanks to report and inspiration from Yosuke Iwamatsu.
    
    Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff --git a/xenstore.c b/xenstore.c
index f5aa8a7..5cd5063 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -191,17 +191,36 @@ static void xenstore_get_backend_path(char **backend, const char *devtype,
     backend_dompath = xs_get_domain_path(xsh, domid_backend);
     if (!backend_dompath) goto out;
     
-    if (pasprintf(&expected_backend, "%s/backend/%s/%lu/%s",
-                  backend_dompath, devtype, frontend_domid, inst_danger)
-        == -1) goto out;
+    const char *expected_devtypes[3];
+    const char **expected_devtype = expected_devtypes;
+
+    *expected_devtype++ = devtype;
+    if (!strcmp(devtype, "vbd")) *expected_devtype++ = "tap";
+    *expected_devtype = 0;
+    assert(expected_devtype <
+           expected_devtypes + ARRAY_SIZE(expected_devtypes));
+
+    for (expected_devtype = expected_devtypes;
+         *expected_devtype;
+         expected_devtype++) {
+    
+        if (pasprintf(&expected_backend, "%s/backend/%s/%lu/%s",
+                      backend_dompath, *expected_devtype,
+                      frontend_domid, inst_danger)
+            == -1) goto out;
 
-    if (strcmp(bpath, expected_backend)) {
-        fprintf(stderr, "frontend `%s' expected backend `%s' got `%s',"
-                " ignoring\n", frontend_path, expected_backend, bpath);
-        errno = EINVAL;
-        goto out;
+        if (!strcmp(bpath, expected_backend))
+            goto found;
     }
 
+    fprintf(stderr, "frontend `%s' devtype `%s' expected backend `%s'"
+            " got `%s', ignoring\n",
+            frontend_path, devtype, expected_backend, bpath);
+    errno = EINVAL;
+    goto out;
+
+ found:
+
     if (pasprintf(&backend_frontend_path, "%s/frontend", bpath)
         == -1) goto out;

-- 

Re: ioemu: Re-enable guest boot with blktap devices ( Weekly VMX status report. Xen: #18577 & Xen0: #696)

[Yosuke Iwamatsu]

Ian Jackson wrote:
> Yosuke Iwamatsu writes ("ioemu: Re-enable guest boot with blktap devices ([Xen-devel] Weekly VMX status report. Xen: #18577 & Xen0: #696)"):
>> Bug #1367 looks caused by the xenstore path checking code recently added
>> to ioemu-remote. I saw the same problem and found that the code didn't
>> consider the case blktap devices were attached to the guest. The patch
>> below should avoid the problem.
> 
> Thanks for the report.  Could you try the attached patch instead and
> let me know whether it fixes the problem ?

It looks fine to me.

-- Yosuke

> 
> Keir Fraser writes ("Re: ioemu: Re-enable guest boot with blktap devices ([Xen-devel] Weekly VMX status report. Xen: #18577 & Xen0: #696)"):
>> Alternatively could just check a shorter path prefix (just /local/domain/0/
>> would be sufficient I think). Full path checking is obviously inherently a
>> bit more fragile.
> 
> Well, yes, but this is a security check and I think those are really
> supposed to be brittle.  So I'd prefer to make us check that the path
> is definitely completely of an expected form.
> 
> Ian.
> 
> commit 629adb3f5244169731ff18b16ae919641d81ad76
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Tue Oct 14 11:46:53 2008 +0100
> 
>     Fix blktap device backend patch check
>     
>     Regarding http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1367,
>     it appears that the expected backend path check is too strict for'
>     blktap devices.  Therefore if the devtype is `vbd' we allow the
>     backend to be `tap'.
>     
>     Thanks to report and inspiration from Yosuke Iwamatsu.
>     
>     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> diff --git a/xenstore.c b/xenstore.c
> index f5aa8a7..5cd5063 100644
> --- a/xenstore.c
> +++ b/xenstore.c
> @@ -191,17 +191,36 @@ static void xenstore_get_backend_path(char **backend, const char *devtype,
>      backend_dompath = xs_get_domain_path(xsh, domid_backend);
>      if (!backend_dompath) goto out;
>      
> -    if (pasprintf(&expected_backend, "%s/backend/%s/%lu/%s",
> -                  backend_dompath, devtype, frontend_domid, inst_danger)
> -        == -1) goto out;
> +    const char *expected_devtypes[3];
> +    const char **expected_devtype = expected_devtypes;
> +
> +    *expected_devtype++ = devtype;
> +    if (!strcmp(devtype, "vbd")) *expected_devtype++ = "tap";
> +    *expected_devtype = 0;
> +    assert(expected_devtype <
> +           expected_devtypes + ARRAY_SIZE(expected_devtypes));
> +
> +    for (expected_devtype = expected_devtypes;
> +         *expected_devtype;
> +         expected_devtype++) {
> +    
> +        if (pasprintf(&expected_backend, "%s/backend/%s/%lu/%s",
> +                      backend_dompath, *expected_devtype,
> +                      frontend_domid, inst_danger)
> +            == -1) goto out;
>  
> -    if (strcmp(bpath, expected_backend)) {
> -        fprintf(stderr, "frontend `%s' expected backend `%s' got `%s',"
> -                " ignoring\n", frontend_path, expected_backend, bpath);
> -        errno = EINVAL;
> -        goto out;
> +        if (!strcmp(bpath, expected_backend))
> +            goto found;
>      }
>  
> +    fprintf(stderr, "frontend `%s' devtype `%s' expected backend `%s'"
> +            " got `%s', ignoring\n",
> +            frontend_path, devtype, expected_backend, bpath);
> +    errno = EINVAL;
> +    goto out;
> +
> + found:
> +
>      if (pasprintf(&backend_frontend_path, "%s/frontend", bpath)
>          == -1) goto out;
>