[PATCH v7 09/23] aarch64: Try to free the GCS of makecontext

[Adhemerval Zanella Netto]

On 09/01/25 11:39, Yury Khrustalev wrote:
> On Tue, Jan 07, 2025 at 01:36:39PM -0300, Adhemerval Zanella Netto wrote:
>>
>>
>>> +/* From libc.so, arch specific.  */
>>> +extern void ARCH_THREAD_FREERES (void) attribute_hidden;
>>>  
>>
>> This seems strange if ARCH_THREAD_FREERES is not defined, although I am
>> not sure if this is a problem.
> 
> I've been waiting for feedback from Carlos on this change. There is probably
> a better way to do this without #ifdef but it's not obvious to me.
> 
>>> +/* Define empty function if no arch-specific clean-up
>>> +   function has been defined.  */
>>> +#ifndef ARCH_THREAD_FREERES
>>> +void __always_inline
>>> +__libc_arch_thread_freeres (void) {}
>>> +#define ARCH_THREAD_FREERES __libc_arch_thread_freeres
>>> +#endif
>>> +
>>
>> Do we need the __always_inline to force compiler to optimize it away?
> 
> Yes
> 
>>> @@ -58,6 +101,9 @@ alloc_makecontext_gcs (size_t stack_size)
>>>    if (base == (void *) -1)
>>>      /* ENOSYS, bad size or OOM.  */
>>>      abort ();
>>> +
>>> +  record_gcs (base, size);
>>> +
>>
>> I think this will make makecontext non async-signal-safe when GCS is used,
>> when we explict document it as AS-Safe and AC-Safe.  I think using
>> internal_signal_block_all/internal_signal_restore_set would be suffice,
>> but it is also a performance regression.
> 
> Do you refer to the call to malloc() in record_gcs()?
> 
> We need to keep track of shadow stacks allocated via map_shadow_stack()
> when a new context is created with makecontext() so that we can munmap
> it when it is no longer required.
> 
> I think this is not on a performance critical path, so we can add the
> internal_signal_block_all / internal_signal_restore_set pair. Could you
> recommend where it would be best to use it?

Yes, glibc malloc is not async-signal-safe and thus calling on makecontext
make it async-signal-unsafe (similar for setcontext, which ends up calling
free). 

I am not sure if you can really use malloc here, since a makecontext call 
potentially interrupt malloc itself. We had the same issue on the getrandom 
vDSO call; where we ended up using mmap directly instead. 

You will still need to block/unblock signal to avoid reentrant signal
handlers; and there is the consideration of extra the mmap overhead per thread.

Another possibility, which I think would be simpler, is to add a TCB buffer
of N entries and use instead of a linked-list.  It would add an small
memory overhead on each thread, and it would limit the number of in-flight 
makecontext a thread can make; but at least makecontext can to return 
ENOMEM if it can not create a new context, and it way simpler than adding
the mmap-allocator.