Re: [musl] [PATCH v2] MT fork

[Rich Felker <dalias@libc.org>]

On Sun, Nov 08, 2020 at 05:12:15PM +0100, Szabolcs Nagy wrote:
> * Rich Felker <dalias@libc.org> [2020-11-05 22:36:17 -0500]:
> > On Fri, Oct 30, 2020 at 11:31:17PM -0400, Rich Felker wrote:
> > > One thing I know is potentially problematic is interaction with malloc
> > > replacement -- locking of any of the subsystems locked at fork time
> > > necessarily takes place after application atfork handlers, so if the
> > > malloc replacement registers atfork handlers (as many do), it could
> > > deadlock. I'm exploring whether malloc use in these systems can be
> > > eliminated. A few are almost-surely better just using direct mmap
> > > anyway, but for some it's borderline. I'll have a better idea sometime
> > > in the next few days.
> > 
> > OK, here's a summary of the affected locks (where there's a lock order
> > conflict between them and application-replaced malloc):
> 
> if malloc replacements take internal locks in atfork
> handlers then it's not just libc internal locks that
> can cause problems but locks taken by other atfork
> handlers that were registered before the malloc one.

No other locks taken there could impede forward process in libc. The
only reason malloc is special here is because we allowed the
application to redefine a family of functions used by libc. For
MT-fork with malloc inside libc, we do the malloc_atfork code last so
that the lock isn't held while other libc components' locks are being
taken. But we can't start taking libc-internal locks until the
application atfork handlers run, or the application could see a
deadlocked libc state (e.g. if something in the atfork handlers used
time functions, maybe to log time of fork, or gettext functions, maybe
to print a localized message, etc.).

> i don't think there is a clean solution to this. i
> think using mmap is ugly (uless there is some reason
> to prefer that other than the atfork issue).

When the size is exactly a 4k page, it's preferable in terms of memory
usage to use mmap instead of malloc, except on wacky archs with large
pages (which I don't think it makes sense to memory-usage-optimize
for; they're inherently and unfixably memory-inefficient).

But for the most part, yes, it's ugly and I don't want to make such a
change.

> > - atexit: uses calloc to allocate more handler slots of the builtin 32
> >   are exhausted. Could reasonably be changed to just mmap a whole page
> >   of slots in this case.

Not sure on this. Since it's only used in the extremely rare case
where a huge number of atexit handlers are registered, it's probably
nicer to use mmap anyway -- it avoids linking a malloc that will
almost surely never be used by atfork.

> > - dlerror: the lock is just for a queue of buffers to be freed on
> >   future calls, since they can't be freed at thread exit time because
> >   the calling context (thread that's "already exited") is not valid to
> >   call application code, and malloc might be replaced. one plausible
> >   solution here is getting rid of the free queue hack (and thus the
> >   lock) entirely and instead calling libc's malloc/free via dlsym
> >   rather than using the potentially-replaced symbol. but this would
> >   not work for static linking (same dlerror is used even though dlopen
> >   always fails; in future it may work) so it's probably not a good
> >   approach. mmap is really not a good option here because it's
> >   excessive mem usage. It's probably possible to just repeatedly
> >   unlock/relock around performing each free so that only one lock is
> >   held at once.

I think the repeated unlock/relock works fine here, but it would also
work to just null out the list in the child (i.e. never free any
buffers that were queued to be freed in the parent before fork). Fork
of MT process is inherently leaky anyway (you can never free thread
data from the other parent threads). I'm not sure which approach is
nicer. The repeated unlock/relock is less logically invasive I think.

> > - gettext: bindtextdomain calls calloc while holding the lock on list
> >   of bindings. It could drop the lock, allocate, retake it, recheck
> >   for an existing binding, and free in that case, but this is
> >   undesirable because it introduces a dependency on free in
> >   static-linked programs. Otherwise all memory gettext allocates is
> >   permanent. Because of this we could just mmap an area and bump
> >   allocate it, but that's wasteful because most programs will only use
> >   one tiny binding. We could also just leak on the rare possibility of
> >   concurrent binding allocations; the number of such leaks is bounded
> >   by nthreads*ndomains, and we could make it just nthreads by keeping
> >   and reusing abandoned ones.

Thoughts here?

> > - sem_open: a one-time calloc of global semtab takes place with the
> >   lock held. On 32-bit archs this table is exactly 4k; on 64-bit it's
> >   6k. So it seems very reasonable to just mmap instead of calloc.

This should almost surely just be changed to mmap. With mallocng, an
early malloc(4k) will take over 5k, and this table is permanent so the
excess will never be released. mmap avoids that entirely.

> > - timezone: The tz core allocates memory to remember the last-seen
> >   value of TZ env var to react if it changes. Normally it's small, so
> >   perhaps we could just use a small (e.g. 32 byte) static buffer and
> >   replace it with a whole mmapped page if a value too large for that
> >   is seen.
> > 
> > Also, somehow I failed to find one of the important locks MT-fork
> > needs to be taking: locale_map.c has a lock for the records of mapped
> > locales. Allocation also takes place with it held, and for the same
> > reason as gettext it really shouldn't be changed to allocate
> > differently. It could possibly do the allocation without the lock held
> > though and leak it (or save it for reuse later if needed) when another
> > thread races to load the locale.
> 
> yeah this sounds problematic.
> 
> if malloc interposers want to do something around fork
> then libc may need to expose some better api than atfork.

Most of them use existing pthread_atfork if they're intended to be
usable with MT-fork. I don't think inventing a new musl-specific API
is a solution here. Their pthread_atfork approach already fully works
with glibc because glibc *doesn't* make anything but malloc work in
the MT-forked child. All the other subsystems mentioned here will
deadlock or blow up in the child with glibc, but only with 0.01%
probability since it's rare for them to be under hammering at fork
time.

One solution you might actually like: getting rid of
application-provided-malloc use inside libc. This could be achieved by
making malloc a thin wrapper for __libc_malloc or whatever, which
could be called by everything in libc that doesn't actually have a
contract to return "as-if-by-malloc" memory. Only a few functions like
getdelim would be left still calling malloc.

The other pros of such an approach are stuff like making it so
application code doesn't get called as a callback from messy contexts
inside libc, e.g. with dynamic linker in inconsistent state. The major
con I see is that it precludes omitting the libc malloc entirely when
static linking, assuming you link any part of libc that uses malloc
internally. However, almost all such places only call malloc, not
free, so you'd just get the trivial bump allocator gratuitously
linked, rather than full mallocng or oldmalloc, except for dlerror
which shouldn't come up in static linked programs anyway.

Rich