Re: [PATCH] RFC: Provide a function to reset IFUNC PLTs

[Jan Kratochvil <jkratochvil@azul.com>]

On Thu, 09 Mar 2023 16:47:49 +0100, Adhemerval Zanella Netto wrote:
> I am not sure how the kernel would enumerate new tasks that are created
> while iterating over /proc/self/task.

I have updated the code as you have found a race there. Now this is no longer
relevant as all known tasks are already verified as stopped. So there is no
more running task to create another task. While iterating /proc/self/task:
(1) either there must be already an unstopped task when opendir() was called,
    in such case the iteration of /proc/self/task will be retried anyway.
(2) or all tasks are stopped and therefore no task can create any new task.


> On closefrom Linux fallback we have
> a similar problem, where the code iterates over /proc/self/fd, and everytime
> it closes a file descriptor it lseeks back to beginning.  It works because
> eventually there will be no more entries on /proc/self/fd, so either you
> will need to certify that kernel adds new tasks at the end of the getdents
> call (used by readdir, or lseek and keep track of all tasks already signaled.

That is not needed, see above.


> While it might work on the JVM where you can not fully control who change 
> SIGUSR1  disposition (and I am not sure JVM would prevent a JNI call to do so),
> so you can't really make it generic without explicit reserve a signal to do so, 
> similar to what glibc does for SIGCANCEL and SIGSETXID (used to synchronize 
> setuid functions over threads).  Meaning that callers of sigaction can't 
> not explicit set such reserved signal.
> 
> This is similar to what we do for SIGSETXID, so I think a proper way to
> do it would to do always install a new signal handler to this on pthread_create,
> on signal handle synchronize with proper async-signal-safe interface
> (pthread_mutex_lock is not, you might accomplish with sem_post but most likely
> you will need a atomic+futex way similar to a barrier), iterate over all 
> dl_stack_used (so the interface can work without access to procfs), issue the 
> signal handler or each thread, operate on the maps, then synchronize to resume
> threads.  We can't really make it generic without accessing the internal
> glibc thread states.

Good to know, if the patch gets a serious consideration for upstreaming
I understand the signal number needs to be handled better.


> And you will also need to reallocate not only glibc, but potentially *all*
> libraries (since ifunc can be used by any function).

This is what the patch already does by _dl_relocate_object().


> > So the only remaining option is that all the programs will be doing
> > setenv("GLIBC_TUNABLES=glibc.cpu.hwcaps=...") and re-exec(). That is
> > a peformance kill and definitely not nice compared to any method of an IFUNC
> > reset.
> 
> Assuming you don't reset env variable on process spawning, you can set it as 
> default for the session.

The /usr/bin/java program needs to setenv("GLIBC_TUNABLES=glibc.cpu.hwcaps=...")
and then it can either system("itself") or exec("itself"). This is what you
mean by the session?


> Another option would to deploy a glibc built with 
> --disable-multi-arch; it will disable ifunc generation.

That is not an option. OpenJDK must be compatible with normal existing Linux
OSes.


> And IMHO this is way nicer because this IFUNC reset as-is, without a proper 
> stop-the-word support, is not safe and adds another corner case for the already
> over-complicated ifunc interface.

stop-the-world is already implemented modulo possible bugfixes.
	https://github.com/openjdk/crac/pull/41/files#diff-aeec57d804d56002f26a85359fc4ac8b48cfc249d57c656a30a63fc6bf3457adR6029


> > In Java world the other libraries (in general, there are some JNI exceptions)
> > do not matter as they are a Java code JIT-compiled by JVM.
> 
> And this won't be a Java specific interface, but rather a GNU extension for C
> library.  So we must make it as concise as possible, without adding any other
> security or undefined behavior.  

I agree. Handling IFUNC for other libraries is also possible but it has to be
a next step. It does not make sense to handle IFUNC in other libraries when
glibc still crashes first.


On Thu, 09 Mar 2023 18:43:08 +0100, Adhemerval Zanella Netto wrote:
> And the 'handler' signal handler has some potential shortcomings as well: 
> 
>   * backtrace is not async-signal-safe: glibc implementation on first call
>     issues dlopen, which calls malloc; and libgcc_eh.so *might* also calls malloc.

I do not see it in practice:
	Temporary breakpoint 1, main () at backtrace.c:5
	5	  backtrace(buf, sizeof(buf)/sizeof(*buf));
	(gdb) b dlopen
	Breakpoint 2 at 0x7ffff7c88f20: file dlopen.c, line 77.
	(gdb) c
	Continuing.
	[Inferior 1 (process 825695) exited normally]

And neither in the sources:
glibc$ grep dlopen $(find -iname "*backtrace*")


>   * pthread calls are not async-signal-safe either.

There are no pthread_* calls, everything is based on kernel tasks.


>   * it only handles libc.so, other libraries that uses ifunc for function
>     selection also fails.

You are right, I have mostly implemented this hard-coded "libc.so.6" to make
it general (for any libraries containing at least one STT_GNU_IFUNC) although
I haven't finished this implementation due to the last paragraph below.


>   * the syscall heuristics do not handle partial results (for instance if
>     write syscall returns do EINTR).

I do not think EINTR would matter. The syscall heuristics is there expecting
that any library function which contains syscalls is not an IFUNC function.


> So this code has the potential of deadlock, specially if you have another
> thread issuing malloc.

I may have missed something but I do not see it so according to the answers
above.


According to the other reactions here I doubt this functionality would get
accepted to glibc so we have decided to give up on its upstreaming and use the
setenv("GLIBC_TUNABLES=glibc.cpu.hwcaps=...") + re-exec workaround instead.
That would need to be coded for compatibility with existing/old glibcs anyway.


Thanks,
Jan