[PATCH] tools/libxengnttab: correct size of allocated memory

Juergen Gross posted 1 patch 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200520083501.31704-1-jgross@suse.com
Maintainers: Wei Liu <wl@xen.org>, Ian Jackson <ian.jackson@eu.citrix.com>
tools/libs/gnttab/freebsd.c | 2 +-
tools/libs/gnttab/linux.c   | 8 +++-----
2 files changed, 4 insertions(+), 6 deletions(-)

[PATCH] tools/libxengnttab: correct size of allocated memory

Posted by Juergen Gross 1 week ago
The size of the memory allocated for the IOCTL_GNTDEV_MAP_GRANT_REF
ioctl() parameters is calculated wrong, which results in too much
memory allocated.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libs/gnttab/freebsd.c | 2 +-
 tools/libs/gnttab/linux.c   | 8 +++-----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index 886b588303..0588501d0f 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -74,7 +74,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     void *addr = NULL;
     int domids_stride;
     unsigned int refs_size = ROUNDUP(count *
-                                     sizeof(struct ioctl_gntdev_map_grant_ref),
+                                     sizeof(struct ioctl_gntdev_grant_ref),
                                      PAGE_SHIFT);
 
     domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index a01bb6c698..74331a4c7b 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -91,9 +91,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 {
     int fd = xgt->fd;
     struct ioctl_gntdev_map_grant_ref *map;
-    unsigned int map_size = ROUNDUP((sizeof(*map) + (count - 1) *
-                                    sizeof(struct ioctl_gntdev_map_grant_ref)),
-                                    PAGE_SHIFT);
+    unsigned int map_size = sizeof(*map) + (count - 1) * sizeof(map->refs[0]);
     void *addr = NULL;
     int domids_stride = 1;
     int i;
@@ -102,10 +100,10 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
         domids_stride = 0;
 
     if ( map_size <= PAGE_SIZE )
-        map = alloca(sizeof(*map) +
-                     (count - 1) * sizeof(struct ioctl_gntdev_map_grant_ref));
+        map = alloca(map_size);
     else
     {
+        map_size = ROUNDUP(map_size, PAGE_SHIFT);
         map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
                    MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
         if ( map == MAP_FAILED )
-- 
2.26.1


Re: [PATCH] tools/libxengnttab: correct size of allocated memory

Posted by Ian Jackson 1 week ago
Juergen Gross writes ("[PATCH] tools/libxengnttab: correct size of allocated memory"):
> The size of the memory allocated for the IOCTL_GNTDEV_MAP_GRANT_REF
> ioctl() parameters is calculated wrong, which results in too much
> memory allocated.

Added Roger to CC.

Firstly,

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

Thank you.


But, looking at this code, why on earth what the ?

The FreeBSD code checks to see if it's less than a page and if so uses
malloc and otherwise uses mmap !  Why not unconditionally use malloc ?

Likewise, the Linux code has its own mmap-based memory-obtainer.  ISTM
that malloc is probably going to be better.  Often it will be able to
give out even a substantial amount without making a syscall.

Essentially, we have two (similar but not identical) tiny custom
memory allocators here.  Also, the Linux and FreeBSD code are
remarkably similar which bothers me.

Anyway, these observations are no criticism of Juergen's patch.

Regards,
Ian.

Re: [PATCH] tools/libxengnttab: correct size of allocated memory

Posted by Roger Pau Monné 1 week ago
On Wed, May 20, 2020 at 03:49:59PM +0100, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH] tools/libxengnttab: correct size of allocated memory"):
> > The size of the memory allocated for the IOCTL_GNTDEV_MAP_GRANT_REF
> > ioctl() parameters is calculated wrong, which results in too much
> > memory allocated.
> 
> Added Roger to CC.
> 
> Firstly,
> 
> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

For the FreeBSD bits:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> 
> Thank you.
> 
> 
> But, looking at this code, why on earth what the ?
> 
> The FreeBSD code checks to see if it's less than a page and if so uses
> malloc and otherwise uses mmap !  Why not unconditionally use malloc ?
> 
> Likewise, the Linux code has its own mmap-based memory-obtainer.  ISTM
> that malloc is probably going to be better.  Often it will be able to
> give out even a substantial amount without making a syscall.
> 
> Essentially, we have two (similar but not identical) tiny custom
> memory allocators here.  Also, the Linux and FreeBSD code are
> remarkably similar which bothers me.

Right. This is due to the FreeBSD file being mostly a clone of the
Linux one. I agree the duplication could be abstracted away.

I really have no idea why malloc or mmap is used, maybe at some point
requesting regions > PAGE_SIZE was considered faster using mmap
directly?

Thanks, Roger.