Ian Jackson
2012-Nov-01 17:46 UTC
[PATCH 4.2] properly handle errors from page sharing hypercalls
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@citrix.com>
> 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@citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> Committed-by: Ian Campbell <ian.campbell@citrix.com>
>
> xen-unstable changeset: 25894:95a971c8058f
> Backport-requested-by: Ian Campbell <Ian.Campbell@citrix.com>
> Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
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@eu.citrix.com>
# 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@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
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;