Xen devel - Apr 2012 - [PATCH] [v3] libxc: Replace alloca() with mmap() for array sizes greater than a page in xc_linux_osdep.c

When mapping in large amounts of pages (in the GB range) from a guest in to Dom0
using xc_map_foreign_bulk(), a segfault occurs in the libxc client application.
This is because the pfn array in linux_privcmd_map_foreign_bulk() is being
allocated using alloca() and the subsequent memcpy causes the stack to blow.
This patch replaces the alloca() with mmap() for pfn array sizes greater than a
page.

Fix an error print with the correct function name.

Do the same for the map array in linux_gnttab_grant_map()

Signed-off-by: Aravindh Puthiyaparambil <aravindh@virtuata.com>
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 274e5accd62d -r 6ef297a3761f tools/libxc/xc_linux_osdep.c
--- a/tools/libxc/xc_linux_osdep.c	Fri Apr 20 09:49:06 2012 +0100
+++ b/tools/libxc/xc_linux_osdep.c	Mon Apr 23 15:16:34 2012 -0700
@@ -39,6 +39,7 @@
 #include "xenctrl.h"
 #include "xenctrlosdep.h"
 
+#define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) &
~((1UL<<(_w))-1))
 #define ERROR(_m, _a...)  xc_osdep_log(xch,XTL_ERROR,XC_INTERNAL_ERROR,_m , ##
_a )
 #define PERROR(_m, _a...) xc_osdep_log(xch,XTL_ERROR,XC_INTERNAL_ERROR,_m \
                   " (%d = %s)", ## _a , errno, xc_strerror(xch,
errno))
@@ -258,7 +259,7 @@ static void *linux_privcmd_map_foreign_b
                 fd, 0);
     if ( addr == MAP_FAILED )
     {
-        PERROR("xc_map_foreign_batch: mmap failed");
+        PERROR("xc_map_foreign_bulk: mmap failed");
         return NULL;
     }
 
@@ -286,7 +287,21 @@ static void *linux_privcmd_map_foreign_b
          * IOCTL_PRIVCMD_MMAPBATCH.
          */
         privcmd_mmapbatch_t ioctlx;
-        xen_pfn_t *pfn = alloca(num * sizeof(*pfn));
+        xen_pfn_t *pfn;
+        unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)),
XC_PAGE_SHIFT);
+
+        if ( pfn_arr_size <= XC_PAGE_SIZE )
+            pfn = alloca(num * sizeof(*pfn));
+        else
+        {
+            pfn = mmap(NULL, pfn_arr_size, PROT_READ | PROT_WRITE,
+                       MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
+            if ( pfn == MAP_FAILED )
+            {
+                PERROR("xc_map_foreign_bulk: mmap of pfn array
failed");
+                return NULL;
+            }
+        }
 
         memcpy(pfn, arr, num * sizeof(*arr));
 
@@ -328,6 +343,9 @@ static void *linux_privcmd_map_foreign_b
             break;
         }
 
+        if ( pfn_arr_size > XC_PAGE_SIZE )
+            munmap(pfn, pfn_arr_size);
+
         if ( rc == -ENOENT && i == num )
             rc = 0;
         else if ( rc )
@@ -549,6 +567,9 @@ static void *linux_gnttab_grant_map(xc_g
 {
     int fd = (int)h;
     struct ioctl_gntdev_map_grant_ref *map;
+    unsigned int map_size = ROUNDUP((sizeof(*map) + (count - 1) *
+                                    sizeof(struct ioctl_gntdev_map_grant_ref)),
+                                    XC_PAGE_SHIFT);
     void *addr = NULL;
     int domids_stride = 1;
     int i;
@@ -556,8 +577,19 @@ static void *linux_gnttab_grant_map(xc_g
     if (flags & XC_GRANT_MAP_SINGLE_DOMAIN)
         domids_stride = 0;
 
-    map = alloca(sizeof(*map) +
-                 (count - 1) * sizeof(struct ioctl_gntdev_map_grant_ref));
+    if ( map_size <= XC_PAGE_SIZE )
+        map = alloca(sizeof(*map) +
+                     (count - 1) * sizeof(struct ioctl_gntdev_map_grant_ref));
+    else
+    {
+        map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
+                   MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
+        if ( map == MAP_FAILED )
+        {
+            PERROR("linux_gnttab_grant_map: mmap of map failed");
+            return NULL;
+        }
+    }
 
     for ( i = 0; i < count; i++ )
     {
@@ -628,6 +660,8 @@ static void *linux_gnttab_grant_map(xc_g
     }
 
  out:
+    if ( map_size > XC_PAGE_SIZE )
+        munmap(map, map_size);
 
     return addr;
 }

On Mon, 2012-04-23 at 23:21 +0100, Aravindh Puthiyaparambil
wrote:
> When mapping in large amounts of pages (in the GB range) from a guest in to
Dom0 using xc_map_foreign_bulk(), a segfault occurs in the libxc client
application. This is because the pfn array in linux_privcmd_map_foreign_bulk()
is being allocated using alloca() and the subsequent memcpy causes the stack to
blow. This patch replaces the alloca() with mmap() for pfn array sizes greater
than a page.
> 
> Fix an error print with the correct function name.
> 
> Do the same for the map array in linux_gnttab_grant_map()
> 
> Signed-off-by: Aravindh Puthiyaparambil <aravindh@virtuata.com>
> Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> diff -r 274e5accd62d -r 6ef297a3761f tools/libxc/xc_linux_osdep.c
> --- a/tools/libxc/xc_linux_osdep.c	Fri Apr 20 09:49:06 2012 +0100
> +++ b/tools/libxc/xc_linux_osdep.c	Mon Apr 23 15:16:34 2012 -0700
> @@ -39,6 +39,7 @@
>  #include "xenctrl.h"
>  #include "xenctrlosdep.h"
>  
> +#define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) &
~((1UL<<(_w))-1))
>  #define ERROR(_m, _a...)  xc_osdep_log(xch,XTL_ERROR,XC_INTERNAL_ERROR,_m
, ## _a )
>  #define PERROR(_m, _a...) xc_osdep_log(xch,XTL_ERROR,XC_INTERNAL_ERROR,_m
\
>                    " (%d = %s)", ## _a , errno, xc_strerror(xch,
errno))
> @@ -258,7 +259,7 @@ static void *linux_privcmd_map_foreign_b
>                  fd, 0);
>      if ( addr == MAP_FAILED )
>      {
> -        PERROR("xc_map_foreign_batch: mmap failed");
> +        PERROR("xc_map_foreign_bulk: mmap failed");
>          return NULL;
>      }
>  
> @@ -286,7 +287,21 @@ static void *linux_privcmd_map_foreign_b
>           * IOCTL_PRIVCMD_MMAPBATCH.
>           */
>          privcmd_mmapbatch_t ioctlx;
> -        xen_pfn_t *pfn = alloca(num * sizeof(*pfn));
> +        xen_pfn_t *pfn;
> +        unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)),
XC_PAGE_SHIFT);
> +
> +        if ( pfn_arr_size <= XC_PAGE_SIZE )
> +            pfn = alloca(num * sizeof(*pfn));
> +        else
> +        {
> +            pfn = mmap(NULL, pfn_arr_size, PROT_READ | PROT_WRITE,
> +                       MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
> +            if ( pfn == MAP_FAILED )
> +            {
> +                PERROR("xc_map_foreign_bulk: mmap of pfn array
failed");
> +                return NULL;
> +            }
> +        }
>  
>          memcpy(pfn, arr, num * sizeof(*arr));
>  
> @@ -328,6 +343,9 @@ static void *linux_privcmd_map_foreign_b
>              break;
>          }
>  
> +        if ( pfn_arr_size > XC_PAGE_SIZE )
> +            munmap(pfn, pfn_arr_size);
> +
>          if ( rc == -ENOENT && i == num )
>              rc = 0;
>          else if ( rc )
> @@ -549,6 +567,9 @@ static void *linux_gnttab_grant_map(xc_g
>  {
>      int fd = (int)h;
>      struct ioctl_gntdev_map_grant_ref *map;
> +    unsigned int map_size = ROUNDUP((sizeof(*map) + (count - 1) *
> +                                    sizeof(struct
ioctl_gntdev_map_grant_ref)),
> +                                    XC_PAGE_SHIFT);
>      void *addr = NULL;
>      int domids_stride = 1;
>      int i;
> @@ -556,8 +577,19 @@ static void *linux_gnttab_grant_map(xc_g
>      if (flags & XC_GRANT_MAP_SINGLE_DOMAIN)
>          domids_stride = 0;
>  
> -    map = alloca(sizeof(*map) +
> -                 (count - 1) * sizeof(struct ioctl_gntdev_map_grant_ref));
> +    if ( map_size <= XC_PAGE_SIZE )
> +        map = alloca(sizeof(*map) +
> +                     (count - 1) * sizeof(struct
ioctl_gntdev_map_grant_ref));
> +    else
> +    {
> +        map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
> +                   MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
> +        if ( map == MAP_FAILED )
> +        {
> +            PERROR("linux_gnttab_grant_map: mmap of map
failed");
> +            return NULL;
> +        }
> +    }
>  
>      for ( i = 0; i < count; i++ )
>      {
> @@ -628,6 +660,8 @@ static void *linux_gnttab_grant_map(xc_g
>      }
>  
>   out:
> +    if ( map_size > XC_PAGE_SIZE )
> +        munmap(map, map_size);
>  
>      return addr;
>  }

Ian Campbell writes ("Re: [Xen-devel] [PATCH] [v3] libxc: Replace alloca()
with mmap() for array sizes greater than a page in
xc_linux_osdep.c"):
> On Mon, 2012-04-23 at 23:21 +0100, Aravindh Puthiyaparambil wrote:
> > When mapping in large amounts of pages (in the GB range) from a guest
in to Dom0 using xc_map_foreign_bulk(), a segfault occurs in the libxc client
application. This is because the pfn array in linux_privcmd_map_foreign_bulk()
is being allocated using alloca() and the subsequent memcpy causes the stack to
blow. This patch replaces the alloca() with mmap() for pfn array sizes greater
than a page.
> > 
> > Fix an error print with the correct function name.
> > 
> > Do the same for the map array in linux_gnttab_grant_map()
> > 
> > Signed-off-by: Aravindh Puthiyaparambil <aravindh@virtuata.com>
> > Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks,

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