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

[PATCH 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            |  8 +++++++-
 tools/libs/call/freebsd.c         |  5 +++++
 tools/libs/call/include/xencall.h |  7 +++++++
 tools/libs/call/libxencall.map    |  5 +++++
 tools/libs/call/linux.c           | 34 ++++++++++++++++++++++++++++++++--
 tools/libs/call/minios.c          |  5 +++++
 tools/libs/call/netbsd.c          |  5 +++++
 tools/libs/call/private.h         |  1 +
 tools/libs/call/solaris.c         |  5 +++++
 tools/libxc/xc_private.c          |  2 +-
 tools/libxc/xc_private.h          | 24 +++++++++++++++++++++---
 12 files changed, 95 insertions(+), 8 deletions(-)

-- 
2.13.7


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

[PATCH 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>
---
 tools/libs/call/core.c    |  8 +++++++-
 tools/libs/call/linux.c   | 29 +++++++++++++++++++++++++++--
 tools/libs/call/private.h |  1 +
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
index f3a34009da..c85ee1936d 100644
--- a/tools/libs/call/core.c
+++ b/tools/libs/call/core.c
@@ -19,7 +19,12 @@
 
 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 )
+        rc = xentoolcore__restrict_by_dup2_null(xcall->fd);
+    return rc;
 }
 
 xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
@@ -30,6 +35,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..fc1125972d 100644
--- a/tools/libs/call/linux.c
+++ b/tools/libs/call/linux.c
@@ -56,12 +56,27 @@ int osdep_xencall_open(xencall_handle *xcall)
     }
 
     xcall->fd = fd;
+
+    /*
+     * Try the same for the hypercall buffer device.
+     */
+    fd = open("/dev/xen/privcmd-buf", O_RDWR|O_CLOEXEC);
+    if ( fd == -1 && ( errno == ENOENT || errno == ENXIO || errno == ENODEV ) )
+    {
+        /* Fallback to /proc/xen/privcmd-buf */
+        fd = open("/proc/xen/privcmd-buf", O_RDWR|O_CLOEXEC);
+    }
+    xcall->buf_fd = fd;
+
     return 0;
 }
 
 int osdep_xencall_close(xencall_handle *xcall)
 {
     int fd = xcall->fd;
+
+    if ( xcall->buf_fd >= 0 )
+        close(xcall->buf_fd);
     if (fd == -1)
         return 0;
     return close(fd);
@@ -78,6 +93,14 @@ void *osdep_alloc_pages(xencall_handle *xcall, size_t npages)
     void *p;
     int rc, i, saved_errno;
 
+    if ( xcall->buf_fd >= 0 )
+    {
+        p = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, xcall->buf_fd, 0);
+        if ( p == MAP_FAILED )
+            PERROR("alloc_pages: mmap failed");
+        return p;
+    }
+
     /* Address returned by mmap is page aligned. */
     p = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_LOCKED, -1, 0);
     if ( p == MAP_FAILED )
@@ -119,8 +142,10 @@ out:
 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..06d159cfb8 100644
--- a/tools/libs/call/private.h
+++ b/tools/libs/call/private.h
@@ -21,6 +21,7 @@ struct xencall_handle {
     xentoollog_logger *logger, *logger_tofree;
     unsigned flags;
     int fd;
+    int buf_fd;
     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 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>
---
 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 fc1125972d..809fd19d2e 100644
--- a/tools/libs/call/linux.c
+++ b/tools/libs/call/linux.c
@@ -152,6 +152,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 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.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/xc_private.c |  2 +-
 tools/libxc/xc_private.h | 24 +++++++++++++++++++++---
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index fcda981744..90f870eaf1 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(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..18add80232 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -254,9 +254,12 @@ out1:
     return ret;
 }
 
-static inline int do_domctl(xc_interface *xch, struct xen_domctl *domctl)
+static inline int do_domctl_maybe_retry(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 +270,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 +287,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(xch, domctl, 0);
+}
+
+static inline int do_domctl_retry(xc_interface *xch, struct xen_domctl *domctl)
+{
+    unsigned int retries = xencall_buffers_never_fault(xch->xcall) ? 0 : 2;
+
+    return do_domctl_maybe_retry(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 2/3] tools/libxencalls: add new function to query hypercall buffer safety

[Jan Beulich]

>>> On 15.06.18 at 15:26, <jgross@suse.com> wrote:
> 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>
> ---
>  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(-)

Did you verify all of them to indeed be safe? If not, wouldn't it be better to
default to unsafe?

For Linux, is there perhaps a way to figure out whether the problem
features don't exist (or are disabled) in the kernel, so that (especially
on older kernels, where the privcmd driver surely is an old one) you
don't needlessly do the retries?

Jan



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

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

[Juergen Gross]

On 15/06/18 16:06, Jan Beulich wrote:
>>>> On 15.06.18 at 15:26, <jgross@suse.com> wrote:
>> 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>
>> ---
>>  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(-)
> 
> Did you verify all of them to indeed be safe? If not, wouldn't it be better to
> default to unsafe?

Minios is safe, Roger told me that BSDs are so, too. I don't think
Solaris is critical, as I can't see how libxencall would even build
there (osdep_hypercall() is misnamed).

> For Linux, is there perhaps a way to figure out whether the problem
> features don't exist (or are disabled) in the kernel, so that (especially
> on older kernels, where the privcmd driver surely is an old one) you
> don't needlessly do the retries?

TBH, I think this would be overkill. Any EFAULT is either a bug in the
tools, so a retry would not change anything, or it is due to the problem
the patch is addressing. Adding additional coding to avoid retries in
the case of a bug in the tools seems not to be appropriate.


Juergen

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

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

[Ian Jackson]

Juergen Gross writes ("[PATCH 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.

This code looks reasonable to me (making some assumptions about the
behaviour of /dev/xen/privcmd-buf).  However, I find myself quibbling
with the flow control style.  And I have some other comments:

> diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
> index 533f0c4a8b..06d159cfb8 100644
> --- a/tools/libs/call/private.h
> +++ b/tools/libs/call/private.h
> @@ -21,6 +21,7 @@ struct xencall_handle {
>      xentoollog_logger *logger, *logger_tofree;
>      unsigned flags;
>      int fd;
> +    int buf_fd;

I think this deserves a comment, along the following lines:

                     /* partially     with         no          */
                     /* initialised   privcmd-buf  privcmd-buf */
       int fd;       /*      any        >=0          -1        */
  +    int buf_fd;   /*      any        >=0          >=0       */

or some such.

>  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 )
> +        rc = xentoolcore__restrict_by_dup2_null(xcall->fd);
> +    return rc;
>  }

Would a `goto out' approach not be clearer here ?

>      xcall->fd = fd;
> +
> +    /*
> +     * Try the same for the hypercall buffer device.
> +     */
> +    fd = open("/dev/xen/privcmd-buf", O_RDWR|O_CLOEXEC);
> +    if ( fd == -1 && ( errno == ENOENT || errno == ENXIO || errno == ENODEV ) )
> +    {
> +        /* Fallback to /proc/xen/privcmd-buf */
> +        fd = open("/proc/xen/privcmd-buf", O_RDWR|O_CLOEXEC);

Firstly, is it necessary to try both /proc/xen and /dev/xen ?  Surely
nowadays only /dev/xen is relevant.  Unless we intend to backport this
new driver to 2.6.18-based Classic Xen Linux kernels which are
probably not affected by the bug anyway ?

Secondly, please treat errors other than ENOENT on opening
/dev/xen/privcmd-buf as fatal (ie, make osdep_xencall_open return -1
in those cases).

>  int osdep_xencall_close(xencall_handle *xcall)
>  {
>      int fd = xcall->fd;
> +
> +    if ( xcall->buf_fd >= 0 )
> +        close(xcall->buf_fd);
>      if (fd == -1)
>          return 0;
>      return close(fd);

This now looks quite clumsy.  I would do this:

  -    int fd = xcall->fd;
  -
  -    if (fd == -1)
  -        return 0;

  +    if ( xcall->fd >= 0 )
  +        close(xcall->fd);
> +    if ( xcall->buf_fd >= 0 )
> +        close(xcall->buf_fd);
  +    return 0;

which is equivalent but makes the symmetry and idempotency much
clearer.

> @@ -78,6 +93,14 @@ void *osdep_alloc_pages(xencall_handle *xcall, size_t npages)
>      void *p;
>      int rc, i, saved_errno;
>  
> +    if ( xcall->buf_fd >= 0 )
> +    {
> +        p = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, xcall->buf_fd, 0);
> +        if ( p == MAP_FAILED )
> +            PERROR("alloc_pages: mmap failed");
> +        return p;
> +    }
> +

I find this early exit approach a bit clumsy, but maybe putting all
the rest in an else branch would be worse.

If you do decide to lift the rest into an else branch, I think you
should keep the `out' clause outside it.  (It's a shame we don't have
the libxl-style correct error handling approach here, ie: initialise
p=NULL at the top; always `goto out' rather than `return NULL' on
error; and have the out section check p before calling munmap.

> @@ -119,8 +142,10 @@ out:
>  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);

This part LGTM but given the multiple lines inside the if, maybe { }
would be warranted.

Regards,
Ian.

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

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

[Ian Jackson]

Juergen Gross writes ("[PATCH 2/3] tools/libxencalls: add new function to query hypercall buffer safety"):
> Add a new function to query whether hypercall buffers are always safe
> to access by the hypervisor or might result in EFAULT.

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 3/3] tools/libxc: retry hypercall in case of EFAULT

[Ian Jackson]

Juergen Gross writes ("[PATCH 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.

It would be worth mentioning in the commit message that this is a
workaround for systems without privcmd-buf, which does not have this
bug.  But nevertheless:

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

Overall, thanks for chasing up this long-standing bug.

Thanks,
Ian.

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

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

[Andrew Cooper]

On 15/06/18 14:26, Juergen Gross wrote:
> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
> index 03bc9a7776..18add80232 100644
> --- a/tools/libxc/xc_private.h
> +++ b/tools/libxc/xc_private.h
> @@ -254,9 +254,12 @@ out1:
>      return ret;
>  }
>  
> -static inline int do_domctl(xc_interface *xch, struct xen_domctl *domctl)
> +static inline int do_domctl_maybe_retry(xc_interface *xch, struct xen_domctl *domctl,
> +                                        unsigned int retries)

I recommend renaming this do_domctl_retry_efault() to make it explicit
which error causes a retry.  We've got other a few other adhoc places
which retry on other errors.

> @@ -281,6 +287,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(xch, domctl, 0);
> +}
> +
> +static inline int do_domctl_retry(xc_interface *xch, struct xen_domctl *domctl)
> +{
> +    unsigned int retries = xencall_buffers_never_fault(xch->xcall) ? 0 : 2;

Probably a very minor issue, but is it worth caching never_fault once
when opening the xc_interface?  Calling into a separate shared object on
every domctl isn't the height of efficiency.

~Andrew

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

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

[Juergen Gross]

On 15/06/18 16:48, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 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.
> 
> This code looks reasonable to me (making some assumptions about the
> behaviour of /dev/xen/privcmd-buf).  However, I find myself quibbling
> with the flow control style.  And I have some other comments:
> 
>> diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
>> index 533f0c4a8b..06d159cfb8 100644
>> --- a/tools/libs/call/private.h
>> +++ b/tools/libs/call/private.h
>> @@ -21,6 +21,7 @@ struct xencall_handle {
>>      xentoollog_logger *logger, *logger_tofree;
>>      unsigned flags;
>>      int fd;
>> +    int buf_fd;
> 
> I think this deserves a comment, along the following lines:
> 
>                      /* partially     with         no          */
>                      /* initialised   privcmd-buf  privcmd-buf */
>        int fd;       /*      any        >=0          -1        */
>   +    int buf_fd;   /*      any        >=0          >=0       */
> 
> or some such.

Okay.

> 
>>  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 )
>> +        rc = xentoolcore__restrict_by_dup2_null(xcall->fd);
>> +    return rc;
>>  }
> 
> Would a `goto out' approach not be clearer here ?

Can do.

> 
>>      xcall->fd = fd;
>> +
>> +    /*
>> +     * Try the same for the hypercall buffer device.
>> +     */
>> +    fd = open("/dev/xen/privcmd-buf", O_RDWR|O_CLOEXEC);
>> +    if ( fd == -1 && ( errno == ENOENT || errno == ENXIO || errno == ENODEV ) )
>> +    {
>> +        /* Fallback to /proc/xen/privcmd-buf */
>> +        fd = open("/proc/xen/privcmd-buf", O_RDWR|O_CLOEXEC);
> 
> Firstly, is it necessary to try both /proc/xen and /dev/xen ?  Surely
> nowadays only /dev/xen is relevant.  Unless we intend to backport this
> new driver to 2.6.18-based Classic Xen Linux kernels which are
> probably not affected by the bug anyway ?

Hmm, yes.

> 
> Secondly, please treat errors other than ENOENT on opening
> /dev/xen/privcmd-buf as fatal (ie, make osdep_xencall_open return -1
> in those cases).

Okay.

> 
>>  int osdep_xencall_close(xencall_handle *xcall)
>>  {
>>      int fd = xcall->fd;
>> +
>> +    if ( xcall->buf_fd >= 0 )
>> +        close(xcall->buf_fd);
>>      if (fd == -1)
>>          return 0;
>>      return close(fd);
> 
> This now looks quite clumsy.  I would do this:
> 
>   -    int fd = xcall->fd;
>   -
>   -    if (fd == -1)
>   -        return 0;
> 
>   +    if ( xcall->fd >= 0 )
>   +        close(xcall->fd);
>> +    if ( xcall->buf_fd >= 0 )
>> +        close(xcall->buf_fd);
>   +    return 0;
> 
> which is equivalent but makes the symmetry and idempotency much
> clearer.

Right.

> 
>> @@ -78,6 +93,14 @@ void *osdep_alloc_pages(xencall_handle *xcall, size_t npages)
>>      void *p;
>>      int rc, i, saved_errno;
>>  
>> +    if ( xcall->buf_fd >= 0 )
>> +    {
>> +        p = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, xcall->buf_fd, 0);
>> +        if ( p == MAP_FAILED )
>> +            PERROR("alloc_pages: mmap failed");
>> +        return p;
>> +    }
>> +
> 
> I find this early exit approach a bit clumsy, but maybe putting all
> the rest in an else branch would be worse.

What about two sub-functions and osdep_alloc_pages() just deciding which
to call?

> 
> If you do decide to lift the rest into an else branch, I think you
> should keep the `out' clause outside it.  (It's a shame we don't have
> the libxl-style correct error handling approach here, ie: initialise
> p=NULL at the top; always `goto out' rather than `return NULL' on
> error; and have the out section check p before calling munmap.
> 
>> @@ -119,8 +142,10 @@ out:
>>  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);
> 
> This part LGTM but given the multiple lines inside the if, maybe { }
> would be warranted.

Okay.


Juergen


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

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

[Juergen Gross]

On 15/06/18 16:54, Andrew Cooper wrote:
> On 15/06/18 14:26, Juergen Gross wrote:
>> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
>> index 03bc9a7776..18add80232 100644
>> --- a/tools/libxc/xc_private.h
>> +++ b/tools/libxc/xc_private.h
>> @@ -254,9 +254,12 @@ out1:
>>      return ret;
>>  }
>>  
>> -static inline int do_domctl(xc_interface *xch, struct xen_domctl *domctl)
>> +static inline int do_domctl_maybe_retry(xc_interface *xch, struct xen_domctl *domctl,
>> +                                        unsigned int retries)
> 
> I recommend renaming this do_domctl_retry_efault() to make it explicit
> which error causes a retry.  We've got other a few other adhoc places
> which retry on other errors.

Okay.

> 
>> @@ -281,6 +287,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(xch, domctl, 0);
>> +}
>> +
>> +static inline int do_domctl_retry(xc_interface *xch, struct xen_domctl *domctl)
>> +{
>> +    unsigned int retries = xencall_buffers_never_fault(xch->xcall) ? 0 : 2;
> 
> Probably a very minor issue, but is it worth caching never_fault once
> when opening the xc_interface?  Calling into a separate shared object on
> every domctl isn't the height of efficiency.

Via a global variable or by extending struct xc_interface_core? Ian?


Juergen


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

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

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH 1/3] tools/libxencall: use hypercall buffer device if available"):
> On 15/06/18 16:48, Ian Jackson wrote:
> > I find this early exit approach a bit clumsy, but maybe putting all
> > the rest in an else branch would be worse.
> 
> What about two sub-functions and osdep_alloc_pages() just deciding which
> to call?

I would be quite happy with that.

Thanks,
Ian.

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

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

[Ian Jackson]

Juergen Gross writes ("Re: [Xen-devel] [PATCH 3/3] tools/libxc: retry hypercall in case of EFAULT"):
> On 15/06/18 16:54, Andrew Cooper wrote:
> > Probably a very minor issue, but is it worth caching never_fault once
> > when opening the xc_interface?  Calling into a separate shared object on
> > every domctl isn't the height of efficiency.
> 
> Via a global variable or by extending struct xc_interface_core? Ian?

I think this is a very minor point.  Are domctls really a hot path ?
A cross-shared-library call is a very minor overhead.

If this is to be done, it should not be a global variable.

Ian.

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