[PATCH v4] tools/foreignmem: Support querying the size of a resource

[PATCH v4] tools/foreignmem: Support querying the size of a resource

[Andrew Cooper]

With the Xen side of this interface (soon to be) fixed to return real sizes,
userspace needs to be able to make the query.

Introduce xenforeignmemory_resource_size() for the purpose, bumping the
library minor version.

Update both all osdep_xenforeignmemory_map_resource() implementations to
understand size requests, skip the mmap() operation, and copy back the
nr_frames field.

For NetBSD, also fix up the ioctl() error path to issue an unmap(), which was
overlooked by c/s 4a64e2bb39 "libs/foreignmemory: Implement on NetBSD".

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: Wei Liu <wl@xen.org>
CC: Paul Durrant <paul@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: Ian Jackson <iwj@xenproject.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>
CC: Manuel Bouyer <bouyer@netbsd.org>

This depends on a bugfix to the Linux IOCTL to understand size requests and
pass them on to Xen.

  https://lore.kernel.org/xen-devel/20210112115358.23346-1-roger.pau@citrix.com/T/#u

v4:
 * Rebase over NetBSD support being added.

v3:
 * Rewrite from scratch, to avoid breaking restricted domid situations.  In
   particular, we cannot open a xencall interface and issue blind hypercalls.
---
 tools/include/xenforeignmemory.h                 | 15 +++++++++++++++
 tools/libs/foreignmemory/Makefile                |  2 +-
 tools/libs/foreignmemory/core.c                  | 18 ++++++++++++++++++
 tools/libs/foreignmemory/freebsd.c               | 18 +++++++++++++++---
 tools/libs/foreignmemory/libxenforeignmemory.map |  4 ++++
 tools/libs/foreignmemory/linux.c                 | 18 +++++++++++++++---
 tools/libs/foreignmemory/netbsd.c                | 21 ++++++++++++++++++++-
 7 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/tools/include/xenforeignmemory.h b/tools/include/xenforeignmemory.h
index d594be8df0..0ab1dd19d3 100644
--- a/tools/include/xenforeignmemory.h
+++ b/tools/include/xenforeignmemory.h
@@ -179,6 +179,21 @@ xenforeignmemory_resource_handle *xenforeignmemory_map_resource(
 int xenforeignmemory_unmap_resource(
     xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
 
+/**
+ * Determine the maximum size of a specific resource.
+ *
+ * @parm fmem handle to the open foreignmemory interface
+ * @parm domid the domain id
+ * @parm type the resource type
+ * @parm id the type-specific resource identifier
+ *
+ * Return 0 on success and fills in *size, with a value in bytes.  Sets errno
+ * and return -1 on error.
+ */
+int xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, size_t *size);
+
 #endif
 
 /*
diff --git a/tools/libs/foreignmemory/Makefile b/tools/libs/foreignmemory/Makefile
index f191cdbed0..0eb9a3a712 100644
--- a/tools/libs/foreignmemory/Makefile
+++ b/tools/libs/foreignmemory/Makefile
@@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
 include $(XEN_ROOT)/tools/Rules.mk
 
 MAJOR    = 1
-MINOR    = 3
+MINOR    = 4
 
 SRCS-y                 += core.c
 SRCS-$(CONFIG_Linux)   += linux.c
diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 63f12e2450..28ec311af1 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -188,6 +188,24 @@ int xenforeignmemory_unmap_resource(
     return rc;
 }
 
+int xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, size_t *size)
+{
+    xenforeignmemory_resource_handle fres = {
+        .domid = domid,
+        .type  = type,
+        .id    = id,
+    };
+    int rc = osdep_xenforeignmemory_map_resource(fmem, &fres);
+
+    if ( rc )
+        return rc;
+
+    *size = fres.nr_frames << PAGE_SHIFT;
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/foreignmemory/freebsd.c b/tools/libs/foreignmemory/freebsd.c
index 3d403a7cd0..9a2796f0b7 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -119,6 +119,10 @@ int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
     };
     int rc;
 
+    if ( !fres->addr && !fres->nr_frames )
+        /* Request for resource size.  Skip mmap(). */
+        goto skip_mmap;
+
     fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
                       fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
     if ( fres->addr == MAP_FAILED )
@@ -126,6 +130,7 @@ int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
 
     mr.addr = (uintptr_t)fres->addr;
 
+ skip_mmap:
     rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
     if ( rc )
     {
@@ -136,13 +141,20 @@ int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
         else
             errno = EOPNOTSUPP;
 
-        saved_errno = errno;
-        osdep_xenforeignmemory_unmap_resource(fmem, fres);
-        errno = saved_errno;
+        if ( fres->addr )
+        {
+            saved_errno = errno;
+            osdep_xenforeignmemory_unmap_resource(fmem, fres);
+            errno = saved_errno;
+        }
 
         return -1;
     }
 
+    /* If requesting size, copy back. */
+    if ( !fres->addr )
+        fres->nr_frames = mr.num;
+
     return 0;
 }
 
diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map b/tools/libs/foreignmemory/libxenforeignmemory.map
index d5323c87d9..8aca341b99 100644
--- a/tools/libs/foreignmemory/libxenforeignmemory.map
+++ b/tools/libs/foreignmemory/libxenforeignmemory.map
@@ -19,3 +19,7 @@ VERS_1.3 {
 		xenforeignmemory_map_resource;
 		xenforeignmemory_unmap_resource;
 } VERS_1.2;
+VERS_1.4 {
+	global:
+		xenforeignmemory_resource_size;
+} VERS_1.3;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index fe73d5ab72..d0eead1196 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -312,6 +312,10 @@ int osdep_xenforeignmemory_map_resource(
     };
     int rc;
 
+    if ( !fres->addr && !fres->nr_frames )
+        /* Request for resource size.  Skip mmap(). */
+        goto skip_mmap;
+
     fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
                       fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
     if ( fres->addr == MAP_FAILED )
@@ -319,6 +323,7 @@ int osdep_xenforeignmemory_map_resource(
 
     mr.addr = (uintptr_t)fres->addr;
 
+ skip_mmap:
     rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
     if ( rc )
     {
@@ -329,13 +334,20 @@ int osdep_xenforeignmemory_map_resource(
         else
             errno = EOPNOTSUPP;
 
-        saved_errno = errno;
-        (void)osdep_xenforeignmemory_unmap_resource(fmem, fres);
-        errno = saved_errno;
+        if ( fres->addr )
+        {
+            saved_errno = errno;
+            osdep_xenforeignmemory_unmap_resource(fmem, fres);
+            errno = saved_errno;
+        }
 
         return -1;
     }
 
+    /* If requesting size, copy back. */
+    if ( !fres->addr )
+        fres->nr_frames = mr.num;
+
     return 0;
 }
 
diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
index d26566f601..4ae60aafdd 100644
--- a/tools/libs/foreignmemory/netbsd.c
+++ b/tools/libs/foreignmemory/netbsd.c
@@ -132,6 +132,10 @@ int osdep_xenforeignmemory_map_resource(
     };
     int rc;
 
+    if ( !fres->addr && !fres->nr_frames )
+        /* Request for resource size.  Skip mmap(). */
+        goto skip_mmap;
+
     fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
                       fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);
     if ( fres->addr == MAP_FAILED )
@@ -139,13 +143,28 @@ int osdep_xenforeignmemory_map_resource(
 
     mr.addr = (uintptr_t)fres->addr;
 
+ skip_mmap:
     rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
     if ( rc )
     {
         PERROR("ioctl failed");
+
+        if ( fres->addr )
+        {
+            int saved_errno = errno;
+
+            osdep_xenforeignmemory_unmap_resource(fmem, fres);
+            errno = saved_errno;
+        }
+
+        return -1;
     }
 
-    return rc;
+    /* If requesting size, copy back. */
+    if ( !fres->addr )
+        fres->nr_frames = mr.num;
+
+    return 0;
 }
 
 /*
-- 
2.11.0

RE: [PATCH v4] tools/foreignmem: Support querying the size of a resource

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 28 January 2021 12:02
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>;
> Roger Pau Monné <roger.pau@citrix.com>; Juergen Gross <jgross@suse.com>; Ian Jackson
> <iwj@xenproject.org>; Michał Leszczyński <michal.leszczynski@cert.pl>; Hubert Jasudowicz
> <hubert.jasudowicz@cert.pl>; Tamas K Lengyel <tamas@tklengyel.com>; Manuel Bouyer <bouyer@netbsd.org>
> Subject: [PATCH v4] tools/foreignmem: Support querying the size of a resource
> 
> With the Xen side of this interface (soon to be) fixed to return real sizes,
> userspace needs to be able to make the query.
> 
> Introduce xenforeignmemory_resource_size() for the purpose, bumping the
> library minor version.
> 
> Update both all osdep_xenforeignmemory_map_resource() implementations to
> understand size requests, skip the mmap() operation, and copy back the
> nr_frames field.
> 
> For NetBSD, also fix up the ioctl() error path to issue an unmap(), which was
> overlooked by c/s 4a64e2bb39 "libs/foreignmemory: Implement on NetBSD".
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH v4] tools/foreignmem: Support querying the size of a resource

[Wei Liu]

On Thu, Jan 28, 2021 at 12:01:52PM +0000, Andrew Cooper wrote:
> With the Xen side of this interface (soon to be) fixed to return real sizes,
> userspace needs to be able to make the query.
> 
> Introduce xenforeignmemory_resource_size() for the purpose, bumping the
> library minor version.
> 
> Update both all osdep_xenforeignmemory_map_resource() implementations to
> understand size requests, skip the mmap() operation, and copy back the
> nr_frames field.
> 
> For NetBSD, also fix up the ioctl() error path to issue an unmap(), which was
> overlooked by c/s 4a64e2bb39 "libs/foreignmemory: Implement on NetBSD".
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wl@xen.org>

Re: [PATCH v4] tools/foreignmem: Support querying the size of a resource

[Manuel Bouyer]

On Thu, Jan 28, 2021 at 12:01:52PM +0000, Andrew Cooper wrote:
> With the Xen side of this interface (soon to be) fixed to return real sizes,
> userspace needs to be able to make the query.
> 
> Introduce xenforeignmemory_resource_size() for the purpose, bumping the
> library minor version.
> 
> Update both all osdep_xenforeignmemory_map_resource() implementations to
> understand size requests, skip the mmap() operation, and copy back the
> nr_frames field.
> 
> For NetBSD, also fix up the ioctl() error path to issue an unmap(), which was
> overlooked by c/s 4a64e2bb39 "libs/foreignmemory: Implement on NetBSD".
> [....]
> diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
> index d26566f601..4ae60aafdd 100644
> --- a/tools/libs/foreignmemory/netbsd.c
> +++ b/tools/libs/foreignmemory/netbsd.c
> @@ -132,6 +132,10 @@ int osdep_xenforeignmemory_map_resource(
>      };
>      int rc;
>  
> +    if ( !fres->addr && !fres->nr_frames )
> +        /* Request for resource size.  Skip mmap(). */
> +        goto skip_mmap;
> +
>      fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
>                        fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);

What happens if fres->addr is not NULL and nr_frames is 0 ?
Is it supposed to happen ? Should we assert that fres->addr is NULL when
nr_frames is 0 ? Or force fres->addr to NULL when nr_frames is 0 ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH v4] tools/foreignmem: Support querying the size of a resource

[Roger Pau Monné]

On Fri, Jan 29, 2021 at 11:38:43AM +0100, Manuel Bouyer wrote:
> On Thu, Jan 28, 2021 at 12:01:52PM +0000, Andrew Cooper wrote:
> > With the Xen side of this interface (soon to be) fixed to return real sizes,
> > userspace needs to be able to make the query.
> > 
> > Introduce xenforeignmemory_resource_size() for the purpose, bumping the
> > library minor version.
> > 
> > Update both all osdep_xenforeignmemory_map_resource() implementations to
> > understand size requests, skip the mmap() operation, and copy back the
> > nr_frames field.
> > 
> > For NetBSD, also fix up the ioctl() error path to issue an unmap(), which was
> > overlooked by c/s 4a64e2bb39 "libs/foreignmemory: Implement on NetBSD".
> > [....]
> > diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
> > index d26566f601..4ae60aafdd 100644
> > --- a/tools/libs/foreignmemory/netbsd.c
> > +++ b/tools/libs/foreignmemory/netbsd.c
> > @@ -132,6 +132,10 @@ int osdep_xenforeignmemory_map_resource(
> >      };
> >      int rc;
> >  
> > +    if ( !fres->addr && !fres->nr_frames )
> > +        /* Request for resource size.  Skip mmap(). */
> > +        goto skip_mmap;
> > +
> >      fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
> >                        fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);
> 
> What happens if fres->addr is not NULL and nr_frames is 0 ?

mmap would return MAP_FAILED and errno == EINVAL in that case AFAICT
on Linux and FreeBSD. NetBSD mmap man page doesn't seem to mention
what happens in that case, so the comments below apply to Linux and
FreeBSD. Maybe we need to handle this differently for NetBSD?

> Is it supposed to happen ?

I think that's fine. Calling osdep_xenforeignmemory_map_resource with
nr_frames == 0 is pointing to a bug in the caller, so returning error
should be fine.

> Should we assert that fres->addr is NULL when
> nr_frames is 0 ? Or force fres->addr to NULL when nr_frames is 0 ?

Doesn't really matter, mmap will return EINVAL if nr_frames == 0
regardless of the value of addr.

Roger.

Re: [PATCH v4] tools/foreignmem: Support querying the size of a resource

[Andrew Cooper]

On 29/01/2021 14:59, Roger Pau Monné wrote:
> On Fri, Jan 29, 2021 at 11:38:43AM +0100, Manuel Bouyer wrote:
>> On Thu, Jan 28, 2021 at 12:01:52PM +0000, Andrew Cooper wrote:
>>> With the Xen side of this interface (soon to be) fixed to return real sizes,
>>> userspace needs to be able to make the query.
>>>
>>> Introduce xenforeignmemory_resource_size() for the purpose, bumping the
>>> library minor version.
>>>
>>> Update both all osdep_xenforeignmemory_map_resource() implementations to
>>> understand size requests, skip the mmap() operation, and copy back the
>>> nr_frames field.
>>>
>>> For NetBSD, also fix up the ioctl() error path to issue an unmap(), which was
>>> overlooked by c/s 4a64e2bb39 "libs/foreignmemory: Implement on NetBSD".
>>> [....]
>>> diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
>>> index d26566f601..4ae60aafdd 100644
>>> --- a/tools/libs/foreignmemory/netbsd.c
>>> +++ b/tools/libs/foreignmemory/netbsd.c
>>> @@ -132,6 +132,10 @@ int osdep_xenforeignmemory_map_resource(
>>>      };
>>>      int rc;
>>>  
>>> +    if ( !fres->addr && !fres->nr_frames )
>>> +        /* Request for resource size.  Skip mmap(). */
>>> +        goto skip_mmap;
>>> +
>>>      fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
>>>                        fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);
>> What happens if fres->addr is not NULL and nr_frames is 0 ?
> mmap would return MAP_FAILED and errno == EINVAL in that case AFAICT
> on Linux and FreeBSD. NetBSD mmap man page doesn't seem to mention
> what happens in that case, so the comments below apply to Linux and
> FreeBSD. Maybe we need to handle this differently for NetBSD?
>
>> Is it supposed to happen ?
> I think that's fine. Calling osdep_xenforeignmemory_map_resource with
> nr_frames == 0 is pointing to a bug in the caller, so returning error
> should be fine.
>
>> Should we assert that fres->addr is NULL when
>> nr_frames is 0 ? Or force fres->addr to NULL when nr_frames is 0 ?
> Doesn't really matter, mmap will return EINVAL if nr_frames == 0
> regardless of the value of addr.

mmap() of 0 is an unconditional failure.  So sayeth POSIX.

For the size request, we don't mmap(), and a pointer of 0 is the signal
to Xen.

For the regular mapping, we support both NULL (let the kernel choose),
and non-NULL (I want my mapping here please), just like regular mmap().

~Andrew

Re: [PATCH v4] tools/foreignmem: Support querying the size of a resource

[Manuel Bouyer]

On Fri, Jan 29, 2021 at 03:09:30PM +0000, Andrew Cooper wrote:
> On 29/01/2021 14:59, Roger Pau Monné wrote:
> > On Fri, Jan 29, 2021 at 11:38:43AM +0100, Manuel Bouyer wrote:
> >> On Thu, Jan 28, 2021 at 12:01:52PM +0000, Andrew Cooper wrote:
> >>> With the Xen side of this interface (soon to be) fixed to return real sizes,
> >>> userspace needs to be able to make the query.
> >>>
> >>> Introduce xenforeignmemory_resource_size() for the purpose, bumping the
> >>> library minor version.
> >>>
> >>> Update both all osdep_xenforeignmemory_map_resource() implementations to
> >>> understand size requests, skip the mmap() operation, and copy back the
> >>> nr_frames field.
> >>>
> >>> For NetBSD, also fix up the ioctl() error path to issue an unmap(), which was
> >>> overlooked by c/s 4a64e2bb39 "libs/foreignmemory: Implement on NetBSD".
> >>> [....]
> >>> diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
> >>> index d26566f601..4ae60aafdd 100644
> >>> --- a/tools/libs/foreignmemory/netbsd.c
> >>> +++ b/tools/libs/foreignmemory/netbsd.c
> >>> @@ -132,6 +132,10 @@ int osdep_xenforeignmemory_map_resource(
> >>>      };
> >>>      int rc;
> >>>  
> >>> +    if ( !fres->addr && !fres->nr_frames )
> >>> +        /* Request for resource size.  Skip mmap(). */
> >>> +        goto skip_mmap;
> >>> +
> >>>      fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
> >>>                        fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);
> >> What happens if fres->addr is not NULL and nr_frames is 0 ?
> > mmap would return MAP_FAILED and errno == EINVAL in that case AFAICT
> > on Linux and FreeBSD. NetBSD mmap man page doesn't seem to mention
> > what happens in that case, so the comments below apply to Linux and
> > FreeBSD. Maybe we need to handle this differently for NetBSD?
> >
> >> Is it supposed to happen ?
> > I think that's fine. Calling osdep_xenforeignmemory_map_resource with
> > nr_frames == 0 is pointing to a bug in the caller, so returning error
> > should be fine.
> >
> >> Should we assert that fres->addr is NULL when
> >> nr_frames is 0 ? Or force fres->addr to NULL when nr_frames is 0 ?
> > Doesn't really matter, mmap will return EINVAL if nr_frames == 0
> > regardless of the value of addr.
> 
> mmap() of 0 is an unconditional failure.  So sayeth POSIX.
> 
> For the size request, we don't mmap(), and a pointer of 0 is the signal
> to Xen.
> 
> For the regular mapping, we support both NULL (let the kernel choose),
> and non-NULL (I want my mapping here please), just like regular mmap().

OK, so that's no a bug. Thanks

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--