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

[AP <apxeng@xxxxxxxxx>]

On Tue, Apr 17, 2012 at 1:59 PM, AP <apxeng@xxxxxxxxx> wrote:
> 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;
>         }
>     }
>

Just wondering what is the consensus on this? Is the mmap() method
acceptable? Should I go ahead and submit this as a patch?

Thanks,
AP

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