genodelabs / genode

Genode OS Framework
https://genode.org/
Other
1.05k stars 248 forks source link

libc: support to run LibC signals on separate stack #5305

Closed alex-ab closed 1 week ago

alex-ab commented 1 month ago

The basic support to run on a defined stack, beside kernel stack, is required for limited sigaltstack support and also to get rid of the show stoppers in #4984 and #5020.

alex-ab commented 1 month ago

@chelmuth: i pushed two branches, the one eager stack allocation and the second with lazy (on first use) allocation with your comments incorporated

chelmuth commented 1 month ago

I merged the lazy variant to staging.

One last point that I see not addresses is the following remark from https://pubs.opengroup.org/onlinepubs/9699919799/functions/sigaltstack.html.

Signals that have been explicitly declared to execute on the alternate stack shall be delivered on the alternate stack.

In combination with https://pubs.opengroup.org/onlinepubs/9699919799/functions/sigaction.html this means.

SA_ONSTACK If set and an alternate signal stack has been declared with sigaltstack(), the signal shall be delivered to the calling process on that stack. Otherwise, the signal shall be delivered on the current stack.

SO, per default signals are still delivered on the user stack.

alex-ab commented 1 month ago

@chelmuth: thanks for the info, I added a fixup which runs the signal only on the alternative stack if the S_ONSTACK is set actually

chelmuth commented 1 month ago

@alex-ab [init -> depot_autopilot] [init -> test-libc] bails out on all platforms.

[2024-07-25 06:15:28] [init -> depot_autopilot] 3.300 [init -> test-libc] test_sigalt         stack=0x403fe990
[2024-07-25 06:15:28] [init -> depot_autopilot] 3.308 [init -> test-libc] test_signal_handler stack=0x404feea8
[2024-07-25 06:15:28] [init -> depot_autopilot] 3.317 [init -> test-libc] Warning: attempt to nested execution of signal handlers
[2024-07-25 06:15:28] [init -> depot_autopilot] 3.328 [init -> test-libc] test_signal_handler done
[2024-07-25 06:15:28] [init -> depot_autopilot] 3.335 [init -> test-libc] Warning: attempt to nested execution of signal handler
[2024-07-25 06:15:28] [init -> depot_autopilot] 3.346 [init -> test-libc] Error: leaking secondary stack memory
[2024-07-25 06:15:28] [init -> depot_autopilot]
[2024-07-25 06:15:28] [init -> depot_autopilot]  test-libc                       failed     3.346  log
chelmuth commented 3 weeks ago

Hmm, just had a look at the logs of failing run/gdb on nova.

[2024-08-14 04:16:54] [init -> gdb] Warning: sigaltstack using self chosen stack is not supported - stack ptr is ignored !!!
...
[2024-08-14 04:16:54] gdb: warning: Couldn't determine a path for the index cache directory.
[2024-08-14 04:16:54] [init -> gdb] Warning: sigaltstack using self chosen stack is not supported - stack ptr is ignored !!!
[2024-08-14 04:16:54] [init -> gdb] Warning: sigaltstack using self chosen stack is not supported - stack ptr is ignored !!!
[2024-08-14 04:16:54] [init -> gdb] Warning: sigaltstack using self chosen stack is not supported - stack ptr is ignored !!!
...
[2024-08-14 04:16:54] [init -> gdb] Warning: sigaltstack using self chosen stack is not supported - stack ptr is ignored !!!
[2024-08-14 04:16:54] Reading symbols from debug/ld.lib.so...
[2024-08-14 04:16:54] [init -> gdb] Warning: sigaltstack using self chosen stack is not supported - stack ptr is ignored !!!
[2024-08-14 04:16:54] [init -> gdb] Warning: sigaltstack using self chosen stack is not supported - stack ptr is ignored !!!
[2024-08-14 04:16:54] [init -> gdb] Warning: sigaltstack using self chosen stack is not supported - stack ptr is ignored !!!
[2024-08-14 04:16:54] [init -> gdb] Warning: sigaltstack using self chosen stack is not supported - stack ptr is ignored !!!

@cproc @alex-ab any ideas? Is this critical?

alex-ab commented 3 weeks ago

The actual reason is the ram_quota request:

[2024-08-14 04:16:56] [init] child "gdb" requests resources: ram_quota=2101248
[2024-08-14 04:17:16] Error: Test execution timed out

With the sigaltstack commit, alternative stacks are indeed allocated (not done before) and so more memory is required. With increasing the memory a bit, the test succeeds.

The warnings is, as agreed with you @chelmuth in person, to warn about the fact that the actual stack pointer provided by the caller (gdb) can't be used, because of the internal nature of our stacks on Genode. Your argument was, in such cases, the contrib code should be adjusted. I will look into the gdb code, whether this is possible.

alex-ab commented 3 weeks ago

@chelmuth: please have a look, I added two commits. Even so I avoided the double allocation, the increase of the ram quota on the nightly test machine is necessary.

cproc commented 3 weeks ago

@alex-ab: the gdb contrib code is also used for the host gdb (tool/tool_chain script). In case the change is not compatible with glibc, something like #ifdef __GENODE__ might be needed. gdbserver_genode.patch still contains such checks for the removed gdbserver component, but it looks like the __GENODE__ macro is currently not defined for gdb on Genode.

chelmuth commented 3 weeks ago

The additional RAM demand seems not related to the sigaltstack commit as the test failed before the commit was merged

[2024-07-24 04:47:44] #5  Genode::Signal_receiver::block_for_signal (
[2024-07-24 04:47:44]     this=this@entry=0x139f90 <Genode::bootstrap_component(Genode::Platform&)::startup+3536>) at /data/genode/repos/base/src/lib/base/signal.cc:280
[2024-07-24 04:47:44] [init] child "gdb" requests resources: ram_quota=2101248

and with the commit in the same way.

[2024-07-25 06:58:56] #5  Genode::Signal_receiver::block_for_signal (
[2024-07-25 06:58:56]     this=this@entry=0x139f90 <Genode::bootstrap_component(Genode::Platform&)::startup+3536>) at /data/genode/repos/base/src/lib/base/signal.cc:280
[2024-07-25 06:58:56] [init] child "gdb" requests resources: ram_quota=2101248
alex-ab commented 3 weeks ago

@cproc: thanks, did not know about this. I tried to add in repos/ports/src/noux-pkg/gdb/target.inc -DGENODE , but did not succeed. Do you have a idea, where to add this differentiation best ?

cproc commented 3 weeks ago

@cproc: thanks, did not know about this. I tried to add in repos/ports/src/noux-pkg/gdb/target.inc -DGENODE , but did not succeed. Do you have a idea, where to add this differentiation best ?

It is the right place, but the definition caused problems with the changes from gdbserver_genode.patch. Commit 6fcc9df removes gdbserver_genode.patch and commit d54a3b9 adds the Genode check to your fix.

alex-ab commented 3 weeks ago

@cproc: muchas gracias, thank you :+1: