On Mon, Oct 4, 2010 at 6:04 AM, Carmelo AMOROSO <[email protected]> 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 <[email protected]> >>> --- >>> 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 <[email protected]> >> >>> 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
remove-clone-platform.diff
Description: Binary data
------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/ltp-list
