Re: [LTP] ltp_clone alignment issues.

Garrett Cooper
2010-10-04T13:28:18Z

[-- Attachment #1: Type: text/plain, Size: 5377 bytes --]

On Mon, Oct 4, 2010 at 6:04 AM, Carmelo AMOROSO <carmelo.amoroso@st.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 10/1/2010 6:06 AM, Hannu Heikkinen wrote:
>> On 30/09/10 12:42 +0200, Carmelo AMOROSO wrote:
>> On 9/30/2010 3:31 PM, Hannu Heikkinen wrote:
>>> On 29/09/10 15:34 +0200, ext Carmelo AMOROSO wrote:
>>> > -----BEGIN PGP SIGNED MESSAGE-----
>>> > Hash: SHA1
>>> >
>>> > On 9/27/2010 3:34 PM, Carmelo AMOROSO wrote:
>>> > > Folks,
>>> > > I want to reopen an old discussion on ltp_clone and stack alignment
>>> > > issue (see
>>> > >
>>
>>> http://sourceforge.net/mailarchive/message.php?msg_name=4B421480.1040400%40petalogix.com).
>>> > > Indeed recently a commit has partially fixed a problem on ARM
>>> > > (0056e395170eb8fc3ffbb22d7bd364fe47c2013e), but I think this should be
>>> > > extended to all archs that have stack that grows downwards.
>>> > >
>>> > > Indeed, as Jiri replied in that old thread, the value passed to clone as
>>> > > child stack should be never accessed, because it is the topmost address
>>> > > of the memory allocated for the child process (it's the previous stack
>>> > > pointer).
>>> > >
>>> > > So, in archs that do not like unaligned stack, using (stack - size - 1 )
>>> > > will cause the process to be killed by a SIGBUS, on other archs, we are
>>> > > just wasting one byte of the malloc-ed stack.
>>> > >
>>> > > On my SH4 arch, the stack must be 4byte aligned (as in ARM).
>>> > >
>>> > > Please, find attached a patch against master branch
>>> > >
>>> > > Best regards,
>>> > > Carmelo
>>> >
>>> > Hi,
>>> > any feedback on this ?
>>> >
>>> > Cheers,
>>> > Carmelo
>>
>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>> Signed-off-by: Carmelo Amoroso <carmelo.amoroso@st.com>
>>> ---
>>> lib/cloner.c | 9 +++------
>>> 1 files changed, 3 insertions(+), 6 deletions(-)
>>
>>> diff --git a/lib/cloner.c b/lib/cloner.c
>>> index 6ad4a00..e53c9cf 100644
>>> --- a/lib/cloner.c
>>> +++ b/lib/cloner.c
>>> @@ -59,16 +59,13 @@ ltp_clone(unsigned long clone_flags, int (*fn)(void
>>> *arg), void *arg,
>>> ret = clone(fn, stack, clone_flags, arg);
>>> #elif defined(__ia64__)
>>> ret = clone2(fn, stack, stack_size, clone_flags, arg, NULL, NULL,
>>> NULL);
>>> -#elif defined(__arm__)
>>> +#else
>>> /*
>>> - * Stack size should be a multiple of 32 bit words
>>> - * & stack limit must be aligned to a 32 bit boundary
>>> + * For archs where stack grows downwards, stack points to the topmost
>>> address
>>> + * of the memory space set up for the child stack.
>>> */
>>> ret = clone(fn, (stack ? stack + stack_size : NULL),
>>> clone_flags, arg);
>>> -#else
>>> - ret = clone(fn, (stack ? stack + stack_size - 1 : NULL),
>>> - clone_flags, arg);
>>> #endif
>>
>>> return ret;
>>> --
>>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>>
>>> Hi,
>>
>>> Acked-by Hannu Heikkinen <ext-hannu.m.heikkinen@nokia.com>
>>
>>> Br,
>>> Hannu
>>
>>> --
>>> Hannu Heikkinen
>>> http://www.nixu.com
>>
>>
>> Thanks, Hannu
>
>> Hi,
>
>> I was thinking your patch once again, and I think it would be wiser if
>> we would have something like:
>
>> --- ../tmp2/ltp-full-20100630/lib/cloner.c 2010-07-03 21:29:04.000000000
>> +0300
>> +++ lib/cloner.c 2010-09-03 14:48:20.000000000 +0300
>> @@ -43,6 +43,16 @@
>> pid_t *parent_tid, void *tls, pid_t *child_tid);
>> #endif
>
>> +/*
>> + * Most arch really want to have stack aligned to some sane boundary.
>> + */
>> +#ifdef __arm__
>> +#define ALIGN_STACK(stack) \
>> + ((void *)(((unsigned long)stack) & ~((sizeof(void *)) - 1)))
>> +#else
>> +#define ALIGN_STACK(stack) (stack)
>> +#endif
>> +
>> /***********************************************************************
>> * ltp_clone: wrapper for clone to hide the architecture dependencies.
>> * 1. hppa takes bottom of stack and no stacksize (stack grows up)
>> @@ -60,7 +70,7 @@
>> #elif defined(__ia64__)
>> ret = clone2(fn, stack, stack_size, clone_flags, arg, NULL, NULL,
>> NULL);
>> #else
>> - ret = clone(fn, (stack ? stack + stack_size - 1 : NULL),
>> + ret = clone(fn, (stack ? ALIGN_STACK(stack + stack_size - 1) : NULL),
>> clone_flags, arg);
>> #endif
>
>
>> The above patch iis in use in our ARM based LTP test suite. Note that -1 is
>> needed for not clobbering eg possibly malloced area after child stack.
>> Could u please change that prev patch a bit, for ensuring that child stack
>> is in proper size etc. Above patch is not sufficient, because ALIGN_STACK
>> in that is only used for arm archs.
>
>> br,
>> Hannu
>
>
> Hannu,
> I don't think that it is a good to force the alignment; we could need to
> write a test case that calls LTP_clone with an unaligned stack just to
> test the behavior of the clone implementation.
>
> Likely, it should be useful to add a check inside the C lib clone
> implementation (arch specific) to protect against unaligned stack, but
> this is a different matter.
>
> I would just leave the LTP_clone passing the argument to the clone as
> they came from the caller.

I believe this is already handled to some degree with the clone
testcases. If not that should be added and I'll review patches which
add this testing to LTP.
On a semi-unrelated note could you guys please test out the
attached patch to make sure that there isn't a functional difference
if you have access to ia64, s390, etc?
Thanks,
-Garrett

[-- Attachment #2: remove-clone-platform.diff --]
[-- Type: application/octet-stream, Size: 2660 bytes --]

diff --git a/testcases/kernel/syscalls/clone/clone01.c b/testcases/kernel/syscalls/clone/clone01.c
index 89992a6..def8007 100644
--- a/testcases/kernel/syscalls/clone/clone01.c
+++ b/testcases/kernel/syscalls/clone/clone01.c
@@ -79,7 +79,6 @@
#include <sys/wait.h>
#include "test.h"
#include "usctest.h"
-#include "clone_platform.h"

static void setup();
static void cleanup();
diff --git a/testcases/kernel/syscalls/clone/clone02.c b/testcases/kernel/syscalls/clone/clone02.c
index e65702e..718ab47 100644
--- a/testcases/kernel/syscalls/clone/clone02.c
+++ b/testcases/kernel/syscalls/clone/clone02.c
@@ -113,8 +113,6 @@
#define TRUE 1
#define FALSE 0

-#include "clone_platform.h"
-
static void setup();
static int test_setup();
static void cleanup();
diff --git a/testcases/kernel/syscalls/clone/clone03.c b/testcases/kernel/syscalls/clone/clone03.c
index 2d60ceb..0a49662 100644
--- a/testcases/kernel/syscalls/clone/clone03.c
+++ b/testcases/kernel/syscalls/clone/clone03.c
@@ -77,8 +77,6 @@
#include "test.h"
#include "usctest.h"

-#include "clone_platform.h"
-
static void setup();
static void cleanup();
static int child_fn();
diff --git a/testcases/kernel/syscalls/clone/clone04.c b/testcases/kernel/syscalls/clone/clone04.c
index f5016f7..c14fd81 100644
--- a/testcases/kernel/syscalls/clone/clone04.c
+++ b/testcases/kernel/syscalls/clone/clone04.c
@@ -76,7 +76,6 @@
#include <sys/wait.h>
#include "test.h"
#include "usctest.h"
-#include "clone_platform.h"

static void cleanup(void);
static void setup(void);
diff --git a/testcases/kernel/syscalls/clone/clone05.c b/testcases/kernel/syscalls/clone/clone05.c
index 73ca2e6..2018581 100644
--- a/testcases/kernel/syscalls/clone/clone05.c
+++ b/testcases/kernel/syscalls/clone/clone05.c
@@ -75,7 +75,6 @@
#include <sys/wait.h>
#include "test.h"
#include "usctest.h"
-#include "clone_platform.h"

#define FLAG CLONE_VM | CLONE_VFORK

diff --git a/testcases/kernel/syscalls/clone/clone06.c b/testcases/kernel/syscalls/clone/clone06.c
index a4d6f07..74c4c8a 100644
--- a/testcases/kernel/syscalls/clone/clone06.c
+++ b/testcases/kernel/syscalls/clone/clone06.c
@@ -81,7 +81,6 @@
#include <fcntl.h>
#include "test.h"
#include "usctest.h"
-#include "clone_platform.h"

#define MAX_LINE_LENGTH 256

diff --git a/testcases/kernel/syscalls/clone/clone07.c b/testcases/kernel/syscalls/clone/clone07.c
index f68099a..cd927a5 100644
--- a/testcases/kernel/syscalls/clone/clone07.c
+++ b/testcases/kernel/syscalls/clone/clone07.c
@@ -78,7 +78,6 @@
#include <sys/wait.h>
#include "test.h"
#include "usctest.h"
-#include "clone_platform.h"

#define TRUE 1
#define FALSE 0

[-- Attachment #3: Type: text/plain, Size: 368 bytes --]

------------------------------------------------------------------------------
Virtualization is moving to the mainstream and overtaking non-virtualized
environment for deploying applications. Does it make network security
easier or more difficult to achieve? Read this whitepaper to separate the
two and get a better understanding.
http://p.sf.net/sfu/hp-phase2-d2d

[-- Attachment #4: Type: text/plain, Size: 155 bytes --]