Re: [PATCH 3/6] daemon: On aarch64, use increments of 16 on the stack.

[Efraim Flashner]

On Sat, Aug 05, 2017 at 02:21:55AM -0400, Mark H Weaver wrote:
> Reviving a very old thread...
> 
> address@hidden (Ludovic Courtès) writes:
> 
> > Efraim Flashner <address@hidden> skribis:
> >
> >> man2 clone: EINVAL: ... on aarch64, child_stack must be a multiple of 16.
> >>
> >> * nix/libstore/build.cc (DerivationGoal::startBuilder): When on aarch64,
> >> when calling clone(), increment the stack by 16.
> >> ---
> >>  nix/libstore/build.cc | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> >> index cebc404d1..362b2d91d 100644
> >> --- a/nix/libstore/build.cc
> >> +++ b/nix/libstore/build.cc
> >> @@ -2008,7 +2008,12 @@ void DerivationGoal::startBuilder()
> >>    char stack[32 * 1024];
> >>    int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | 
> >> SIGCHLD;
> >>    if (!fixedOutput) flags |= CLONE_NEWNET;
> >> -  pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> >> +// if statements are hard, fix this
> >> +//#if __AARCH64__
> >> +  pid = clone(childEntry, stack + sizeof(stack) - 16, flags, this);
> >> +//#else
> >> +//        pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> >> +//#endif
> >
> > I think we can make it unconditional.  Could you test whether the
> > attached patch works for aarch64?
> >
> > Thanks!
> >
> > Ludo’.
> >
> > diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> > index cebc404d1..9b7bb5391 100644
> > --- a/nix/libstore/build.cc
> > +++ b/nix/libstore/build.cc
> > @@ -26,6 +26,7 @@
> >  #include <errno.h>
> >  #include <stdio.h>
> >  #include <cstring>
> > +#include <stdint.h>
> >  
> >  #include <pwd.h>
> >  #include <grp.h>
> > @@ -2008,7 +2009,11 @@ void DerivationGoal::startBuilder()
> >     char stack[32 * 1024];
> >     int flags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | 
> > SIGCHLD;
> >     if (!fixedOutput) flags |= CLONE_NEWNET;
> > -   pid = clone(childEntry, stack + sizeof(stack) - 8, flags, this);
> > +
> > +   /* Ensure proper alignment on the stack.  On aarch64, it has to be 16
> > +      bytes.  */
> > +   pid = clone(childEntry, (char *)(((uintptr_t)stack + 16) & ~0xf),
> > +               flags, this);
> >     if (pid == -1)
> >         throw SysError("cloning builder process");
> >      } else
> 
> This patch, applied in February, contains a serious error.  The stack
> address passed to 'clone' is supposed to be near the end of the memory
> block allocated for the stack, and that's how it was before this patch
> was applied.  Since this patch was applied, it now passes an address
> very close to the *start* of the memory block.
> 
> This broke the daemon on mips64el in a subtle way that was rather
> difficult to debug.  After about six months of being too busy with other
> things to investigate properly, I finally tracked it down to this
> change.
> 
> I reverted this commit.  Let's try again to find a proper fix for this
> issue on aarch64.
> 
>      Thanks,
>        Mark

How about doubling the size of the stack to [32 * 1024  * 2] and
changing the clone location to 'stack + sizeof(stack) - 16', does that
work for mips64el? With this revert the daemon is completely broken on
aarch64.

-- 
Efraim Flashner   <address@hidden>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

signature.asc
Description: PGP signature