[Xen-devel] [PATCH 4.2] properly handle errors from page sharing hypercalls

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

xen.org writes ("[xen-4.2-testing bisection] complete test-i386-i386-xl"):
> branch xen-4.2-testing
> xen branch xen-4.2-testing
> job test-i386-i386-xl
> test guest-start
> 
> Tree: linux git://xenbits.xen.org/linux-pvops.git
> Tree: qemu git://xenbits.xen.org/staging/qemu-xen-4.2-testing.git
> Tree: qemuu git://xenbits.xen.org/staging/qemu-upstream-4.2-testing.git
> Tree: xen http://xenbits.xen.org/hg/staging/xen-4.2-testing.hg
> 
> *** Found and reproduced problem changeset ***
> 
>   Bug is in tree:  xen http://xenbits.xen.org/hg/staging/xen-4.2-testing.hg
>   Bug introduced:  051e2a30e3fb
>   Bug not present: c23d938e3e64
> 
> 
>   changeset:   25886:051e2a30e3fb
>   user:        Ian Campbell <Ian.Campbell@xxxxxxxxxx>
>   date:        Fri Oct 26 16:40:18 2012 +0100
> 
>       libxl: handle errors from xc_sharing_* info functions
> 
>       On a 32 bit hypervisor xl info currently reports:
>       sharing_freed_memory   : 72057594037927935
>       sharing_used_memory    : 72057594037927935
> 
>       Eat the ENOSYS and turn it into 0. Log and propagate other errors.
> 
>       I don't have a 32 bit system handy, so tested on x86_64 with a libxc
>       hacked to return -ENOSYS and -EINVAL.
> 
>       Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
>       Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>       Acked-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>       Committed-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
>       xen-unstable changeset: 25894:95a971c8058f
>       Backport-requested-by: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
>       Committed-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

I have been investigating this.

I don't see how it could possibly have ever worked.  AFAICT the
problem is as follows:

 * XENMEM_get_sharing_freed_pages is handled by subarch_memory_op.
   On 32-bit, it is not supported and returs ENOSYS.

 * Eventually, this causes the ioctl IOCTL_PRIVCMD_HYPERCALL to return
   -1 setting errno to ENOSYS.  This return value is propagated by
   libxc, with xc_sharing_freed_pages returning -1.

 * So in libxl_get_physinfo l==-1.  But the code mistakenly assumes
   that xc_sharing_freed_pages would have returned -errno.
   We therefore take the LIBXL__LOG_ERRNOVAL branch.

 * Additionally because we failed to make the errno value nonnegative
   again (if l is -errno, then LIBXL__LOG_ERRNOVAL should take -l for
   the errno value), libxl__logv does not in fact log the supposed
   errno value of 1.  (Which would be EPERM.)

I don't think this patch in this form can ever have worked as
intended.  The only reason we didn't spot that this doesn't work when
this patch was tested in xen-unstable is that xen-unstable doesn't
have 32-bit any more.  The tests described in the commit message must
have involved making the same mistake as in the patch, of course.

I have committed the attached patch to xen-4.2-testing.hg.  Hopefully
this will get us a test push.  After that we should consider what
other backports are needed.

Ian.

# HG changeset patch
# User Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
# Date 1351791764 0
# Node ID 9d2ad9218dcc5a541ca09f47f60d7c3fec1039eb
# Parent  4727c234198d51fdd248c3ba92560faa147b2ac6
libxl: properly handle errors from page sharing hypercalls

25886:051e2a30e3fb (25894:95a971c8058f in xen-unstable) is wrong
because it assumes that xc_sharing_freed_pages etc. return -errnoval
on error.  However, like other libxc calls they return -1 setting
errno.

Correct this, checking for l<0 and then testing errno against ENOSYS,
and also log the correct errno value.

Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Committed-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

diff -r 4727c234198d -r 9d2ad9218dcc tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Fri Oct 26 17:03:13 2012 +0100
+++ b/tools/libxl/libxl.c       Thu Nov 01 17:42:44 2012 +0000
@@ -3653,21 +3653,25 @@ int libxl_get_physinfo(libxl_ctx *ctx, l
     physinfo->free_pages = xcphysinfo.free_pages;
     physinfo->scrub_pages = xcphysinfo.scrub_pages;
     l = xc_sharing_freed_pages(ctx->xch);
-    if (l == -ENOSYS) {
-        l = 0;
-    } else if (l < 0) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, l,
-                            "getting sharing freed pages");
-        return ERROR_FAIL;
+    if (l < 0) {
+        if (errno == ENOSYS) {
+            l = 0;
+        } else {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "getting sharing freed pages");
+            return ERROR_FAIL;
+        }
     }
     physinfo->sharing_freed_pages = l;
     l = xc_sharing_used_frames(ctx->xch);
-    if (l == -ENOSYS) {
-        l = 0;
-    } else if (l < 0) {
-        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, l,
-                            "getting sharing used frames");
-        return ERROR_FAIL;
+    if (l < 0) {
+        if (errno == ENOSYS) {
+            l = 0;
+        } else {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                             "getting sharing used frames");
+            return ERROR_FAIL;
+        }
     }
     physinfo->sharing_used_frames = l;
     physinfo->nr_nodes = xcphysinfo.nr_nodes;

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