Aravindh Puthiyaparambil
2012-Apr-20 18:38 UTC
[PATCH] [v2] libxc: Replace alloca() with mmap() for pfn array sizes greater than a page in linux_privcmd_map_foreign_bulk()
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.
Signed-off-by: Aravindh Puthiyaparambil <aravindh@virtuata.com>
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
diff -r 7c777cb8f705 -r e62ab14d44af tools/libxc/xc_linux_osdep.c
--- a/tools/libxc/xc_linux_osdep.c Wed Apr 18 16:49:55 2012 +0100
+++ b/tools/libxc/xc_linux_osdep.c Fri Apr 20 11:36:02 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 )
Ian Campbell
2012-Apr-21 08:31 UTC
Re: [PATCH] [v2] libxc: Replace alloca() with mmap() for pfn array sizes greater than a page in linux_privcmd_map_foreign_bulk()
On Fri, 2012-04-20 at 19:38 +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. > > Signed-off-by: Aravindh Puthiyaparambil <aravindh@virtuata.com>Thanks. Did you rule out this bug at the usage of alloca() in linux_gnttab_grant_map? If not then I think that should be changed too.> Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> > > diff -r 7c777cb8f705 -r e62ab14d44af tools/libxc/xc_linux_osdep.c > --- a/tools/libxc/xc_linux_osdep.c Wed Apr 18 16:49:55 2012 +0100 > +++ b/tools/libxc/xc_linux_osdep.c Fri Apr 20 11:36:02 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 )
Aravindh Puthiyaparambil
2012-Apr-23 19:18 UTC
Re: [PATCH] [v2] libxc: Replace alloca() with mmap() for pfn array sizes greater than a page in linux_privcmd_map_foreign_bulk()
On Sat, Apr 21, 2012 at 1:31 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Fri, 2012-04-20 at 19:38 +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. >> >> Signed-off-by: Aravindh Puthiyaparambil <aravindh@virtuata.com> > > Thanks. Did you rule out this bug at the usage of alloca() in > linux_gnttab_grant_map? If not then I think that should be changed too.Yes, you right. It could happen with linux_gnttab_grant_map() also. I will fix that and resubmit. Thanks, Aravindh>> Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> >> >> diff -r 7c777cb8f705 -r e62ab14d44af tools/libxc/xc_linux_osdep.c >> --- a/tools/libxc/xc_linux_osdep.c Wed Apr 18 16:49:55 2012 +0100 >> +++ b/tools/libxc/xc_linux_osdep.c Fri Apr 20 11:36:02 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 ) > >