[PATCH 0/2] tools/libs: don't use alloca(3)

[PATCH 0/2] tools/libs: don't use alloca(3)

[Wei Liu]

Wei Liu (2):
  libs/gnttab: do not use alloca(3)
  libs/foreignmemory: do not use alloca(3)

 tools/libs/foreignmemory/linux.c | 21 +++++++--------------
 tools/libs/gnttab/linux.c        | 19 ++++++-------------
 2 files changed, 13 insertions(+), 27 deletions(-)

-- 
2.1.4


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

[PATCH 1/2] libs/gnttab: do not use alloca(3)

[Wei Liu]

The semantics of alloca(3) is not very nice. If the stack overflows,
program behaviour is undefined.

Remove the use of alloca(3) and always use mmap.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libs/gnttab/linux.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 7b0fba4..535ce34 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -102,18 +102,12 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     if (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN)
         domids_stride = 0;
 
-    if ( map_size <= PAGE_SIZE )
-        map = alloca(sizeof(*map) +
-                     (count - 1) * sizeof(struct ioctl_gntdev_map_grant_ref));
-    else
+    map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
+               MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
+    if ( map == MAP_FAILED )
     {
-        map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
-                   MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
-        if ( map == MAP_FAILED )
-        {
-            GTERROR(xgt->logger, "mmap of map failed");
-            return NULL;
-        }
+        GTERROR(xgt->logger, "mmap of map failed");
+        return NULL;
     }
 
     for ( i = 0; i < count; i++ )
@@ -187,8 +181,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     }
 
  out:
-    if ( map_size > PAGE_SIZE )
-        munmap(map, map_size);
+    munmap(map, map_size);
 
     return addr;
 }
-- 
2.1.4


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

[PATCH 2/2] libs/foreignmemory: do not use alloca(3)

[Wei Liu]

The semantics of alloca(3) is not very nice. If the stack overflows,
program behaviour is undefined.

Remove the use of alloca(3) and always use mmap.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libs/foreignmemory/linux.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index 423c744..316d988 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -17,7 +17,6 @@
  * Copyright 2006 Sun Microsystems, Inc.  All rights reserved.
  */
 
-#include <alloca.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <unistd.h>
@@ -187,18 +186,13 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
         xen_pfn_t *pfn;
         unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)), PAGE_SHIFT);
 
-        if ( pfn_arr_size <= 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 )
         {
-            pfn = mmap(NULL, pfn_arr_size, PROT_READ | PROT_WRITE,
-                       MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
-            if ( pfn == MAP_FAILED )
-            {
-                PERROR("mmap of pfn array failed");
-                (void)munmap(addr, num << PAGE_SHIFT);
-                return NULL;
-            }
+            PERROR("mmap of pfn array failed");
+            (void)munmap(addr, num << PAGE_SHIFT);
+            return NULL;
         }
 
         memcpy(pfn, arr, num * sizeof(*arr));
@@ -241,8 +235,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
             break;
         }
 
-        if ( pfn_arr_size > PAGE_SIZE )
-            munmap(pfn, pfn_arr_size);
+        munmap(pfn, pfn_arr_size);
 
         if ( rc == -ENOENT && i == num )
             rc = 0;
-- 
2.1.4


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

Re: [PATCH 0/2] tools/libs: don't use alloca(3)

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 752 bytes --]

On Wed, 2016-08-17 at 15:33 +0100, Wei Liu wrote:
> Wei Liu (2):
>   libs/gnttab: do not use alloca(3)
>   libs/foreignmemory: do not use alloca(3)
> 
FWIW, both patches:

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Out of curiosity, I've tried to figure out what the advantage was in
using alloca() instead of mmap(), in case of mappings smaller than
PAGE_SIZE (did also a small bit of archaeology but that didn't help
either).

Oh, well.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 0/2] tools/libs: don't use alloca(3)

[Wei Liu]

On Thu, Aug 18, 2016 at 01:39:08AM +0200, Dario Faggioli wrote:
> On Wed, 2016-08-17 at 15:33 +0100, Wei Liu wrote:
> > Wei Liu (2):
> >   libs/gnttab: do not use alloca(3)
> >   libs/foreignmemory: do not use alloca(3)
> > 
> FWIW, both patches:
> 
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> 

Thanks for reviewing.

Wei.

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

Re: [PATCH 1/2] libs/gnttab: do not use alloca(3)

[David Vrabel]

On 17/08/16 15:33, Wei Liu wrote:
> The semantics of alloca(3) is not very nice. If the stack overflows,
> program behaviour is undefined.
> 
> Remove the use of alloca(3) and always use mmap.

This is only using alloca() if the allocation is < PAGE_SIZE.  I think
assuming there's this much extra stack is fine.

Using alloca() avoids the (expensive) mmap() system call.

David

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

Re: [PATCH 1/2] libs/gnttab: do not use alloca(3)

[Wei Liu]

On Mon, Aug 22, 2016 at 10:46:50AM +0100, David Vrabel wrote:
> On 17/08/16 15:33, Wei Liu wrote:
> > The semantics of alloca(3) is not very nice. If the stack overflows,
> > program behaviour is undefined.
> > 
> > Remove the use of alloca(3) and always use mmap.
> 
> This is only using alloca() if the allocation is < PAGE_SIZE.  I think
> assuming there's this much extra stack is fine.
> 

A library is not in a position assume how deep the stack is IMHO.

Wei.

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

Re: [PATCH 1/2] libs/gnttab: do not use alloca(3)

[David Vrabel]

On 22/08/16 11:10, Wei Liu wrote:
> On Mon, Aug 22, 2016 at 10:46:50AM +0100, David Vrabel wrote:
>> On 17/08/16 15:33, Wei Liu wrote:
>>> The semantics of alloca(3) is not very nice. If the stack overflows,
>>> program behaviour is undefined.
>>>
>>> Remove the use of alloca(3) and always use mmap.
>>
>> This is only using alloca() if the allocation is < PAGE_SIZE.  I think
>> assuming there's this much extra stack is fine.
>>
> 
> A library is not in a position assume how deep the stack is IMHO.

This suggests a library cannot use any stack, which is clearly silly.

But ok, in which case you should consider using malloc() instead of
alloca()/mmap(), then small allocation might come out of some
pre-existing or cached allocations.

David

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

Re: [PATCH 1/2] libs/gnttab: do not use alloca(3)

[Wei Liu]

On Mon, Aug 22, 2016 at 11:24:56AM +0100, David Vrabel wrote:
> On 22/08/16 11:10, Wei Liu wrote:
> > On Mon, Aug 22, 2016 at 10:46:50AM +0100, David Vrabel wrote:
> >> On 17/08/16 15:33, Wei Liu wrote:
> >>> The semantics of alloca(3) is not very nice. If the stack overflows,
> >>> program behaviour is undefined.
> >>>
> >>> Remove the use of alloca(3) and always use mmap.
> >>
> >> This is only using alloca() if the allocation is < PAGE_SIZE.  I think
> >> assuming there's this much extra stack is fine.
> >>
> > 
> > A library is not in a position assume how deep the stack is IMHO.
> 
> This suggests a library cannot use any stack, which is clearly silly.
> 

Of course not. Please don't take my words out of context and further
imply things I never said. This is not how a conversation should work
out. And name calling is toxic. Please just stop.

I care about the undefined behaviour aspect of alloca -- I believe you
dislike that as much as I do.

> But ok, in which case you should consider using malloc() instead of
> alloca()/mmap(), then small allocation might come out of some
> pre-existing or cached allocations.
> 

Ok, that seems sensible.

Wei.

> David

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