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

[Ludovic Courtès]

Mark H Weaver <address@hidden> skribis:

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

Arrgh, good catch; my bad!  I wonder why this did not cause more
problems on other architectures.

Ludo’.