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

[Yury Khrustalev]

Hi Adhemerval,

On Fri, Jan 10, 2025 at 10:09:24AM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 10/01/25 10:01, Yury Khrustalev wrote:
> > On Thu, Jan 09, 2025 at 03:53:14PM -0300, Adhemerval Zanella Netto wrote:
> >>
> >>>> 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.
> > 
> > makecontext() does not return a value and doesn't set errno, so we probably
> > cannot "return" ENOMEM. In existing code, when makecontext() failes, it calls
> > abort(). Would the same be appropriate when we reach max number of context
> > instances per thread?
> 
> Sigh, you are right (I confused it with setcontext for some reason).  I think
> it would be the best option and x86_64 already does this if 
> __allocate_shadow_stack fails.
> 
> > 
> > Another option is to silently stop freeing shadow stacks after reaching max
> > number. This will allow code that needs more makecontext's to continue with
> > memory leak being downside (potentially leading to ENOMEM).
> 
> I don't think a silent leak is the best option, specially one that user can
> not take some course of action without a lot of hackery.
> 

Looking deeper into this, I realise that a proper solution might take some time
to implement and test and it will likley be quite intrusive to the existing code
and may potentially affect common code used by other targets.

As an option, would you think it might be acceptable not to deallocate shadow
stacks allocated by makecontext? We can address this after the upcoming Glibc
release with a proper solution.

Thanks,
Yury