[PATCH v2 0/3] fix hypercall buffer locking in memory

[PATCH v2 0/3] fix hypercall buffer locking in memory

[Juergen Gross]

On Linux systems hypercall buffers in user memory are allocated with
MAP_LOCKED attribute. Unfortunately that doesn't mean that the buffer
will always be accessible by the hypervisor, as the kernel might set
the PTE for the buffer to invalid or read only for short periods of
time, e.g. due to page migration or compaction.

This results in highly sporadic -EFAULT for hypercalls issued by Xen
tools.

Fix this problem by using a new device node of the Linux privcmd driver
for allocating hypercall buffers.

Add a fallback in case the Linux kernel doesn't support that new device
node by repeating the getpageframeinfo3 hypercall which until now has
been the only one to be observed suffering from that problem.

This series is meant to be included in 4.11, so it can right away have
my:

Release-acked-by: Juergen Gross <jgross@suse.com>

Juergen Gross (3):
  tools/libxencall: use hypercall buffer device if available
  tools/libxencalls: add new function to query hypercall buffer safety
  tools/libxc: retry hypercall in case of EFAULT

 tools/libs/call/Makefile          |  2 +-
 tools/libs/call/core.c            | 12 +++++++-
 tools/libs/call/freebsd.c         |  5 ++++
 tools/libs/call/include/xencall.h |  7 +++++
 tools/libs/call/libxencall.map    |  5 ++++
 tools/libs/call/linux.c           | 63 ++++++++++++++++++++++++++++++++++-----
 tools/libs/call/minios.c          |  5 ++++
 tools/libs/call/netbsd.c          |  5 ++++
 tools/libs/call/private.h         |  7 ++++-
 tools/libs/call/solaris.c         |  5 ++++
 tools/libxc/xc_private.c          |  2 +-
 tools/libxc/xc_private.h          | 25 ++++++++++++++--
 12 files changed, 129 insertions(+), 14 deletions(-)

-- 
2.13.7


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 1/3] tools/libxencall: use hypercall buffer device if available

[Juergen Gross]

Instead of using anonymous memory for hypercall buffers which is then
locked into memory, use the hypercall buffer device of the Linux
privcmd driver if available.

This has the advantage of needing just a single mmap() for allocating
the buffer and page migration or compaction can't make the buffer
unaccessible for the hypervisor.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- add some comments to tools/libs/call/private.h (Ian Jackson)
- some control flow modifications (Ian Jackson)
- rename /dev/xen/provcmd-buf to /dev/xen/hypercall (Andrew Cooper)
- omit fallback to xenfs (Ian Jackson)
- treat errors other than ENOENT as fatal (Ian Jackson)
---
 tools/libs/call/core.c    | 12 +++++++++-
 tools/libs/call/linux.c   | 58 +++++++++++++++++++++++++++++++++++++++++------
 tools/libs/call/private.h |  7 +++++-
 3 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
index f3a34009da..46ca61529e 100644
--- a/tools/libs/call/core.c
+++ b/tools/libs/call/core.c
@@ -19,7 +19,16 @@
 
 static int all_restrict_cb(Xentoolcore__Active_Handle *ah, domid_t domid) {
     xencall_handle *xcall = CONTAINER_OF(ah, *xcall, tc_ah);
-    return xentoolcore__restrict_by_dup2_null(xcall->fd);
+    int rc;
+
+    rc = xentoolcore__restrict_by_dup2_null(xcall->buf_fd);
+    if ( rc )
+        goto out;
+
+    rc = xentoolcore__restrict_by_dup2_null(xcall->fd);
+
+out:
+    return rc;
 }
 
 xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
@@ -30,6 +39,7 @@ xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
     if (!xcall) return NULL;
 
     xcall->fd = -1;
+    xcall->buf_fd = -1;
     xcall->tc_ah.restrict_callback = all_restrict_cb;
     xentoolcore__register_active_handle(&xcall->tc_ah);
 
diff --git a/tools/libs/call/linux.c b/tools/libs/call/linux.c
index 3f1b691fe7..90c2691486 100644
--- a/tools/libs/call/linux.c
+++ b/tools/libs/call/linux.c
@@ -56,15 +56,28 @@ int osdep_xencall_open(xencall_handle *xcall)
     }
 
     xcall->fd = fd;
+
+    /*
+     * Try the same for the hypercall buffer device.
+     */
+    fd = open("/dev/xen/hypercall", O_RDWR|O_CLOEXEC);
+    if ( fd == -1 && errno != ENOENT )
+    {
+        PERROR("Error on trying to open hypercall buffer device");
+        return -1;
+    }
+    xcall->buf_fd = fd;
+
     return 0;
 }
 
 int osdep_xencall_close(xencall_handle *xcall)
 {
-    int fd = xcall->fd;
-    if (fd == -1)
-        return 0;
-    return close(fd);
+    if ( xcall->buf_fd >= 0 )
+        close(xcall->buf_fd);
+    if ( xcall->fd >= 0 )
+        close(xcall->fd);
+    return 0;
 }
 
 int osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
@@ -72,7 +85,22 @@ int osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
     return ioctl(xcall->fd, IOCTL_PRIVCMD_HYPERCALL, hypercall);
 }
 
-void *osdep_alloc_pages(xencall_handle *xcall, size_t npages)
+static void *alloc_pages_bufdev(xencall_handle *xcall, size_t npages)
+{
+    void *p;
+
+    p = mmap(NULL, npages * PAGE_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED,
+             xcall->buf_fd, 0);
+    if ( p == MAP_FAILED )
+    {
+        PERROR("alloc_pages: mmap failed");
+        p = NULL;
+    }
+
+    return p;
+}
+
+static void *alloc_pages_nobufdev(xencall_handle *xcall, size_t npages)
 {
     size_t size = npages * PAGE_SIZE;
     void *p;
@@ -116,11 +144,27 @@ out:
     return NULL;
 }
 
+void *osdep_alloc_pages(xencall_handle *xcall, size_t npages)
+{
+    void *p;
+
+    if ( xcall->buf_fd >= 0 )
+        p = alloc_pages_bufdev(xcall, npages);
+    else
+        p = alloc_pages_nobufdev(xcall, npages);
+
+    return p;
+}
+
 void osdep_free_pages(xencall_handle *xcall, void *ptr, size_t npages)
 {
     int saved_errno = errno;
-    /* Recover the VMA flags. Maybe it's not necessary */
-    madvise(ptr, npages * PAGE_SIZE, MADV_DOFORK);
+
+    if ( xcall->buf_fd < 0 )
+    {
+        /* Recover the VMA flags. Maybe it's not necessary */
+        madvise(ptr, npages * PAGE_SIZE, MADV_DOFORK);
+    }
 
     munmap(ptr, npages * PAGE_SIZE);
     /* We MUST propagate the hypercall errno, not unmap call's. */
diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
index 533f0c4a8b..21f992b37e 100644
--- a/tools/libs/call/private.h
+++ b/tools/libs/call/private.h
@@ -20,7 +20,12 @@
 struct xencall_handle {
     xentoollog_logger *logger, *logger_tofree;
     unsigned flags;
-    int fd;
+
+                     /* partially     with /dev/     no /dev/      */
+                     /* initialised   xen/hypercall  xen/hypercall */
+    int fd;          /*    any           >= 0           >= 0       */
+    int buf_fd;      /*    any           >= 0           -1         */
+
     Xentoolcore__Active_Handle tc_ah;
 
     /*
-- 
2.13.7


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/3] tools/libxencalls: add new function to query hypercall buffer safety

[Juergen Gross]

Add a new function to query whether hypercall buffers are always safe
to access by the hypervisor or might result in EFAULT.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libs/call/Makefile          | 2 +-
 tools/libs/call/freebsd.c         | 5 +++++
 tools/libs/call/include/xencall.h | 7 +++++++
 tools/libs/call/libxencall.map    | 5 +++++
 tools/libs/call/linux.c           | 5 +++++
 tools/libs/call/minios.c          | 5 +++++
 tools/libs/call/netbsd.c          | 5 +++++
 tools/libs/call/solaris.c         | 5 +++++
 8 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/tools/libs/call/Makefile b/tools/libs/call/Makefile
index 39dd207428..252d3973fc 100644
--- a/tools/libs/call/Makefile
+++ b/tools/libs/call/Makefile
@@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
 include $(XEN_ROOT)/tools/Rules.mk
 
 MAJOR    = 1
-MINOR    = 0
+MINOR    = 1
 SHLIB_LDFLAGS += -Wl,--version-script=libxencall.map
 
 CFLAGS   += -Werror -Wmissing-prototypes
diff --git a/tools/libs/call/freebsd.c b/tools/libs/call/freebsd.c
index cadd313c04..28bfd852c1 100644
--- a/tools/libs/call/freebsd.c
+++ b/tools/libs/call/freebsd.c
@@ -107,6 +107,11 @@ void osdep_free_pages(xencall_handle *xcall, void *ptr, size_t npages)
     errno = saved_errno;
 }
 
+int xencall_buffers_never_fault(xencall_handle *xcall)
+{
+    return 1;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/call/include/xencall.h b/tools/libs/call/include/xencall.h
index bafacdd2ba..0d09bc8eae 100644
--- a/tools/libs/call/include/xencall.h
+++ b/tools/libs/call/include/xencall.h
@@ -115,6 +115,13 @@ void xencall_free_buffer_pages(xencall_handle *xcall, void *p, size_t nr_pages);
 void *xencall_alloc_buffer(xencall_handle *xcall, size_t size);
 void xencall_free_buffer(xencall_handle *xcall, void *p);
 
+/*
+ * Are allocated hypercall buffers safe to be accessed by the hypervisor all
+ * the time?
+ * Returns 0 if EFAULT might be possible.
+ */
+int xencall_buffers_never_fault(xencall_handle *xcall);
+
 #endif
 
 /*
diff --git a/tools/libs/call/libxencall.map b/tools/libs/call/libxencall.map
index 2f96144f40..c482195b95 100644
--- a/tools/libs/call/libxencall.map
+++ b/tools/libs/call/libxencall.map
@@ -17,3 +17,8 @@ VERS_1.0 {
 		xencall_free_buffer_pages;
 	local: *; /* Do not expose anything by default */
 };
+
+VERS_1.1 {
+	global:
+		xencall_buffers_never_fault;
+} VERS_1.0;
diff --git a/tools/libs/call/linux.c b/tools/libs/call/linux.c
index 90c2691486..d8a6306e04 100644
--- a/tools/libs/call/linux.c
+++ b/tools/libs/call/linux.c
@@ -171,6 +171,11 @@ void osdep_free_pages(xencall_handle *xcall, void *ptr, size_t npages)
     errno = saved_errno;
 }
 
+int xencall_buffers_never_fault(xencall_handle *xcall)
+{
+    return xcall->buf_fd >= 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/call/minios.c b/tools/libs/call/minios.c
index f04688f63c..9f7a96995f 100644
--- a/tools/libs/call/minios.c
+++ b/tools/libs/call/minios.c
@@ -70,6 +70,11 @@ void osdep_free_pages(xencall_handle *xcall, void *ptr, size_t npages)
     free(ptr);
 }
 
+int xencall_buffers_never_fault(xencall_handle *xcall)
+{
+    return 1;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/call/netbsd.c b/tools/libs/call/netbsd.c
index e96fbf16f7..a5502da377 100644
--- a/tools/libs/call/netbsd.c
+++ b/tools/libs/call/netbsd.c
@@ -110,6 +110,11 @@ int do_xen_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
         return hypercall->retval;
 }
 
+int xencall_buffers_never_fault(xencall_handle *xcall)
+{
+    return 1;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/call/solaris.c b/tools/libs/call/solaris.c
index 5aa330e4eb..c63b6a329a 100644
--- a/tools/libs/call/solaris.c
+++ b/tools/libs/call/solaris.c
@@ -86,6 +86,11 @@ int do_xen_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
     return ioctl(fd, IOCTL_PRIVCMD_HYPERCALL, hypercall);
 }
 
+int xencall_buffers_never_fault(xencall_handle *xcall)
+{
+    return 1;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.13.7


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 3/3] tools/libxc: retry hypercall in case of EFAULT

[Juergen Gross]

A hypercall issued via the privcmd driver can very rarely return
-EFAULT even if the hypercall buffers are locked in memory. This
happens for hypercall buffers in user memory when the Linux kernel
is doing memory scans e.g. for page migration or compaction.

Retry the getpageframeinfo3 hypercall up to 2 times in case
-EFAULT is returned and the hypervisor might see invalid PTEs for
user hypercall buffers (which should be the case only if the kernel
doesn't offer a /dev/xen/hypercall node).

Signed-off-by: Juergen Gross <jgross@suse.com>
--
V2:
- rename do_domctl_retry() to do_domctl_retry_efault() (Andrew Cooper)
---
 tools/libxc/xc_private.c |  2 +-
 tools/libxc/xc_private.h | 25 ++++++++++++++++++++++---
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index fcda981744..e6e3d9913c 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -224,7 +224,7 @@ int xc_get_pfn_type_batch(xc_interface *xch, uint32_t dom,
     domctl.domain = dom;
     domctl.u.getpageframeinfo3.num = num;
     set_xen_guest_handle(domctl.u.getpageframeinfo3.array, arr);
-    rc = do_domctl(xch, &domctl);
+    rc = do_domctl_retry_efault(xch, &domctl);
     xc_hypercall_bounce_post(xch, arr);
     return rc;
 }
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 03bc9a7776..03bdfca7d4 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -254,9 +254,13 @@ out1:
     return ret;
 }
 
-static inline int do_domctl(xc_interface *xch, struct xen_domctl *domctl)
+static inline int do_domctl_maybe_retry_efault(xc_interface *xch,
+                                               struct xen_domctl *domctl,
+                                               unsigned int retries)
 {
     int ret = -1;
+    unsigned int retry_cnt = 0;
+
     DECLARE_HYPERCALL_BOUNCE(domctl, sizeof(*domctl), XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
 
     domctl->interface_version = XEN_DOMCTL_INTERFACE_VERSION;
@@ -267,8 +271,11 @@ static inline int do_domctl(xc_interface *xch, struct xen_domctl *domctl)
         goto out1;
     }
 
-    ret = xencall1(xch->xcall, __HYPERVISOR_domctl,
-                   HYPERCALL_BUFFER_AS_ARG(domctl));
+    do {
+        ret = xencall1(xch->xcall, __HYPERVISOR_domctl,
+                       HYPERCALL_BUFFER_AS_ARG(domctl));
+    } while ( ret < 0 && errno == EFAULT && retry_cnt++ < retries );
+
     if ( ret < 0 )
     {
         if ( errno == EACCES )
@@ -281,6 +288,18 @@ static inline int do_domctl(xc_interface *xch, struct xen_domctl *domctl)
     return ret;
 }
 
+static inline int do_domctl(xc_interface *xch, struct xen_domctl *domctl)
+{
+    return do_domctl_maybe_retry_efault(xch, domctl, 0);
+}
+
+static inline int do_domctl_retry_efault(xc_interface *xch, struct xen_domctl *domctl)
+{
+    unsigned int retries = xencall_buffers_never_fault(xch->xcall) ? 0 : 2;
+
+    return do_domctl_maybe_retry_efault(xch, domctl, retries);
+}
+
 static inline int do_sysctl(xc_interface *xch, struct xen_sysctl *sysctl)
 {
     int ret = -1;
-- 
2.13.7


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/3] tools/libxencall: use hypercall buffer device if available

[Ian Jackson]

Juergen Gross writes ("[PATCH v2 1/3] tools/libxencall: use hypercall buffer device if available"):
> Instead of using anonymous memory for hypercall buffers which is then
> locked into memory, use the hypercall buffer device of the Linux
> privcmd driver if available.
> 
> This has the advantage of needing just a single mmap() for allocating
> the buffer and page migration or compaction can't make the buffer
> unaccessible for the hypervisor.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/3] tools/libxc: retry hypercall in case of EFAULT

[Ian Jackson]

Juergen Gross writes ("[PATCH v2 3/3] tools/libxc: retry hypercall in case of EFAULT"):
> A hypercall issued via the privcmd driver can very rarely return
> -EFAULT even if the hypercall buffers are locked in memory. This
> happens for hypercall buffers in user memory when the Linux kernel
> is doing memory scans e.g. for page migration or compaction.
> 
> Retry the getpageframeinfo3 hypercall up to 2 times in case
> -EFAULT is returned and the hypervisor might see invalid PTEs for
> user hypercall buffers (which should be the case only if the kernel
> doesn't offer a /dev/xen/hypercall node).
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel