Re: [LTP] ltp_clone alignment issues.

[Carmelo AMOROSO <carmelo.amoroso@st.com>]

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

Regards,
Carmelo


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkyp0OsACgkQoRq/3BrK1s+TPwCgnoZlDNW0yMjfLZAu6gKzywP7
Bp4AoLpYpxtqjtd+tL4+ZpxNhnQv7fpF
=VGJo
-----END PGP SIGNATURE-----

------------------------------------------------------------------------------
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
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list