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

["Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>]

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

I think you should, and you should include my
Acked-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>

Thanks,
Andres
>
> 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