Re: [PATCH] posix: Optimize Linux posix_spawn

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

> On 07/05/2019 12:38, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> On 07/05/2019 05:33, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>>
>>>>> The current internal posix_spawn symbol for Linux (__spawni) requires
>>>>> to allocate a dynamic stack based on input arguments to handle the
>>>>> SPAWN_XFLAGS_USE_PATH internal flag, which re-issue the input binary
>>>>> as a shell script if execve call return ENOEXEC (to execute shell
>>>>> scripts with an initial shebang).
>>>>>
>>>>> This is done only for compatibility mode and the generic case does not
>>>>> require the extra calculation plus the potential large mmap/munmap
>>>>> call.  For default case, a pre-defined buffer is sufficed to use on the
>>>>> clone call instead.
>>>>>
>>>>> This patch optimizes Linux spawni by allocating a dynamic stack only
>>>>> for compatibility symbol (SPAWN_XFLAGS_USE_PATH).  For generic case,
>>>>> a stack allocated buffer is used instead.
>>>>
>>>> I'm still not convinced that stack switching is required here.
>>>
>>> - Performance-wise it would be just a stack increment (which tends to
>>>   better than a mmap/unmap), 
>>>
>>> - It also should work with any stack hardening (--enable-stack-protector=all
>>>   shows no issue) and the stack uses uses the calling process stack as well
>>>   (which should trigger overflow due guard pages).
>> 
>> No, since there is no guard page as part of the on-stack array, we do
>> not detect any overflow into other parts of the stack frame if it
>> happens to be stored below the frame.  So this still disables some of
>> the hardening checks.
>
> Using a stack allocated array is essentially the same as using the same
> stack of the caller: stack overflow in created helper thread will trigger
> a fault based on the guard page of the calling thread (since stack is
> allocated object will be based on caller thread). I give you this is sketchy
> standard-wise.

The difference is this.  f1 is the outer function (that prepares the new
stack, f2 is the function which runs with a switch stacked.

With a stack reuse in an assembler function, it looks like this:

   +-----------------------------
   | Activation record for f1
   |
   |
   +-----------------------------
   | Activation record for f2
   |
   :
   +=== Guard ===================

So it just looks like an ordinary function call.  Stack overflow in f2
will hit the guard (if built with -fstack-clash-protection).

With an on-stack array for the stack, it looks like this:

   +-----------------------------
   | Activation record for f1
   |
   | [ Array
   | [ +-----------------------------
   | [ | Activation record for f2
   | [ |
   | [ |
   |
   | (*)
   |
   :
   +=== Guard ===================


The problem is that technically, the activation record of f1 continues
after the array, indicated by (*), and we won't know what the compiler
will store there.  This means that even if f2 is built with
-fstack-clash-protection, stack overflow detection is unreliable because
the overflow into f1's activation record does not hit a guard page.

Sorry if this is totally obvious.  If it is and you still maintain that
both cases are equivalent, then perhaps I'm confused.

Thanks,
Florian