Re: [Xen-devel] [PATCH] [v2] libxc: Replace alloca() with mmap() for pfn array sizes greater than a page in linux_privcmd_map_foreign_bulk()

[Aravindh Puthiyaparambil <aravindh@xxxxxxxxxxxx>]

On Sat, Apr 21, 2012 at 1:31 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxx>
>>
>> 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 )
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel