Re: [Xen-devel] alloca() in linux_privcmd_map_foreign_bulk causing segfault

[AP <apxeng@xxxxxxxxx>]

On Tue, Apr 17, 2012 at 8:56 AM, Andres Lagar-Cavilla
<andres@xxxxxxxxxxxxxxxx> wrote:
>>
>> On Tue, 2012-04-17 at 16:19 +0100, Santosh Jodh wrote:
>>> I only cared about linux_gnttab_grant_map for user mode blkback. You
>>> told me to change all others while I was in there.
>>
>> Oh, right, I remember now.
>>
>> Given that reverting it for this case will still leave the same issue
>> for the grant case (which I'd forgotten about) I suppose the appropriate
>> workaround is to touch the alloca'd memory in the appropriate order in
>> all cases (perhaps with an alloca helper macro).
>
> FWIW, I am very skeptical about this whole alloca business. It's very very
> dangerous, no surprise on the bug report. On my code I tend to map
> arbitrarily-sized pfn arrays, and I've been thinking of disabling alloca.
>
> If your only safeguard is to put a loop that touches everything so the
> stack gets allocated .... then what's your gain?
>
> Just mmap('/dev/zero', MAP_PRIVATE|MAP_POPULATE, PROT_WRITE,
> round_to_page_size(what_you_need)). That's likely the fastest way to get
> the array in Linux. It isn't that slow either. And it's safe.

I tried and it worked for me.

diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -34,16 +34,17 @@
 #include <xen/memory.h>
 #include <xen/sys/evtchn.h>
 #include <xen/sys/gntdev.h>
 #include <xen/sys/gntalloc.h>

 #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))

 static xc_osdep_handle linux_privcmd_open(xc_interface *xch)
 {
     int flags, saved_errno;
     int fd = open("/proc/xen/privcmd", O_RDWR);
@@ -281,17 +282,24 @@ static void *linux_privcmd_map_foreign_b
     /* Command was not recognized, use fall back */
     else if ( rc < 0 && errno == EINVAL && (int)num > 0 )
     {
         /*
          * IOCTL_PRIVCMD_MMAPBATCH_V2 is not supported - fall back to
          * IOCTL_PRIVCMD_MMAPBATCH.
          */
         privcmd_mmapbatch_t ioctlx;
-        xen_pfn_t *pfn = alloca(num * sizeof(*pfn));
+        xen_pfn_t *pfn = mmap(NULL, ROUNDUP((num * sizeof(*pfn)),
XC_PAGE_SHIFT),
+                              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));

         ioctlx.num = num;
         ioctlx.dom = dom;
         ioctlx.addr = (unsigned long)addr;
         ioctlx.arr = pfn;

@@ -323,16 +331,18 @@ static void *linux_privcmd_map_foreign_b
                     break;
                 }
                 rc = -ENOENT;
                 continue;
             }
             break;
         }

+        munmap(pfn, ROUNDUP((num * sizeof(*pfn)), XC_PAGE_SHIFT));
+
         if ( rc == -ENOENT && i == num )
             rc = 0;
         else if ( rc )
         {
             errno = -rc;
             rc = -1;
         }
     }


> Andres
>
>>
>> Ian.
>>
>>>
>>> -----Original Message-----
>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>> Sent: Tuesday, April 17, 2012 12:56 AM
>>> To: Ian Campbell; AP
>>> Cc: Santosh Jodh; xen-devel@xxxxxxxxxxxxxxxxxxx
>>> Subject: Re: [Xen-devel] alloca() in linux_privcmd_map_foreign_bulk
>>> causing segfault
>>>
>>> >>> On 17.04.12 at 09:27, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
>>> > On Tue, 2012-04-17 at 05:57 +0100, AP wrote:
>>> >> On xen-unstable 25164:5bbda657a016, when I try to map in large
>>> >> amounts of pages (in the GB range) from a guest in to Dom0
>>> >
>>> > Out of interest -- what are you doing this for?
>>> >
>>> >>  using
>>> >> xc_map_foreign_bulk() I am hitting a segfault.
>>> >>
>>> >> Program received signal SIGSEGV, Segmentation fault.
>>> >> 0x00007ffff7bd38d5 in linux_privcmd_map_foreign_bulk (xch=0x605050,
>>> >>     h=<optimized out>, dom=2, prot=<optimized out>,
>>> arr=0x7ffff6bf5010,
>>> >>     err=0x7ffff67f4010, num=<optimized out>)
>>> >>     at /usr/include/x86_64-linux-gnu/bits/string3.h:52
>>> >> 52          return __builtin___memcpy_chk (__dest, __src, __len, __bos0
>>> (__dest));
>>> >> (gdb) bt
>>> >> #0  0x00007ffff7bd38d5 in linux_privcmd_map_foreign_bulk
>>> (xch=0x605050,
>>> >>     h=<optimized out>, dom=2, prot=<optimized out>,
>>> arr=0x7ffff6bf5010,
>>> >>     err=0x7ffff67f4010, num=<optimized out>)
>>> >>     at /usr/include/x86_64-linux-gnu/bits/string3.h:52
>>> >> #1  0x00007ffff7bd1ffc in xc_map_foreign_bulk (xch=<optimized out>,
>>> >>     dom=<optimized out>, prot=<optimized out>, arr=<optimized out>,
>>> >>     err=<optimized out>, num=<optimized out>) at
>>> >> xc_foreign_memory.c:79
>>> >>
>>> >> This was working for me with Xen 4.1.2. On comparing
>>> >> linux_privcmd_map_foreign_bulk() between 4.1.2 and unstable I see
>>> >> that the pfn array in linux_privcmd_map_foreign_bulk() is being
>>> >> allocated using alloca() in unstable vs malloc() in 4.1.2. So I am
>>> >> blowing the stack with the call.
>>> >
>>> > I bet this is due to Linux's stack guard page. This means that if you
>>> > try and increase the stack by more than ~1 page you skip entirely over
>>> > the "next" stack page and into the guard.
>>> >
>>> > Does it help if after the alloca you add a loop which touches the
>>> > first word of each page of the new buffer? Since the stack grows down
>>> > you might actually need to do it backwards from the end of the array
>>> > in order to start at the end which is nearest the existing stack?
>>> > (it's before coffee o'clock so thinking about stack direction isn't my
>>> > strong point yet...)
>>>
>>> This should really be done by the alloca() implementation itself -
>>> anything else is a bug.
>>>
>>> > The switch to alloca was made recently in order to optimise the
>>> > hotpath for a userspace I/O backend.
>>>
>>> Probably this should be made size-dependent ...
>>>
>>> > BTW, Santosh, it didn't occur to me at the time but what is privcmd
>>> > mmap doing on the hot path for the userspace I/O anyway? Most hotpath
>>> > operations should really be grant table ops, a backend shouldn't be
>>> > relying on the privileges accorded to dom0 -- for one thing it should
>>> > be expected to work in a driver domain.
>>>
>>> ... if this indeed turns out to be a hot path for something at all?
>>>
>>> Jan
>>>
>>> >>  If I replace the alloca() with malloc() the call goes through. What
>>> >> is the way around this? Should I be using
>>> >> xc_map_foreign_batch() instead, which I think is deprecated? Please
>>> >> advice...
>>> >>
>>> >> Thanks,
>>> >> AP
>>> >>
>
>

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