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

[Mark H Weaver]

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