From: Ammar Faizi <[email protected]>
Hi Willy,
On top of the series titled "nolibc auxiliary vector retrieval support".
The prerequisite patches of this series are in that series.
This is v2 of nolibc signal handling support. It adds signal handling
support to the nolibc subsystem:
1) Initial implementation of nolibc sigaction(2) function.
`sigaction()` needs an architecture-dependent "signal trampoline"
function that invokes __rt_sigreturn syscall to resume the process
after a signal gets handled.
The "signal trampoline" function is called `__restore_rt` in this
implementation. The naming `__restore_rt` is important for GDB. It
also has to be given a special optimization attribute
"omit-frame-pointer" to prevent the compiler from creating a stack
frame that makes the `%rsp` value no longer points to the `struct
rt_sigframe` that the kernel constructed.
2) signal(2) function.
signal() function is the simpler version of sigaction(). Unlike
sigaction(), which fully controls the struct sigaction, the caller
only cares about the sa_handler when calling the signal() function.
signal() internally calls sigaction().
3) More selftests.
This series also adds selftests for:
- fork(2)
- sigaction(2)
- signal(2)
Side note for __restore_rt:
This has been tested on x86-64 arch and `__restore_rt` generates the
correct code. The `__restore_rt` codegen correctness on other
architectures need to be evaluated as well. If it can't generate the
correct code, it has to be written in inline Assembly.
The current codegen for __restore_rt looks like this (gcc (Ubuntu 11.3.0-1ubuntu1~22.04) 11.3.0):
00000000004038e3 <__restore_rt>:
4038e3: endbr64
4038e7: mov $0xf,%eax
4038ec: syscall
## Changes since v2:
- Fix unintentionally squashed patch. The signal() selftest patch
was squashed into the sigaction selftest patch.
## Changes since RFC v1:
- Separate getpagesize() series.
- Write __restore_rt function in C instead of in inline Assembly.
Signed-off-by: Ammar Faizi <[email protected]>
---
Ammar Faizi (5):
nolibc/sys: Implement `sigaction(2)` function
nolibc/sys: Implement `signal(2)` function
selftests/nolibc: Add `fork(2)` selftest
selftests/nolibc: Add `sigaction(2)` selftest
selftests/nolibc: Add `signal(2)` selftest
tools/include/nolibc/sys.h | 97 +++++++++++
tools/testing/selftests/nolibc/nolibc-test.c | 172 +++++++++++++++++++
2 files changed, 269 insertions(+)
base-commit: b6887ec8b0b0c78db414b78e329bf2ce234dedd5
prerequisite-patch-id: 8dd0ca8ecee1732d8f5c0b233f8231dda6ab0d22
prerequisite-patch-id: ff4c08615ebbdc1a04ce39f39f99387ee46b2b31
prerequisite-patch-id: af837a829263849331eb6d73701afd7903146055
--
Ammar Faizi
From: Ammar Faizi <[email protected]>
signal() function is the simpler version of sigaction(). Unlike
sigaction(), which fully controls the `struct sigaction`, the caller
only cares about the sa_handler when calling the signal() function.
signal() internally calls sigaction().
Signed-off-by: Ammar Faizi <[email protected]>
---
tools/include/nolibc/sys.h | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 7d594155e77f..54e51f154b1f 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -1119,6 +1119,31 @@ int sigaction(int signum, const struct sigaction *act, struct sigaction *oldact)
return ret;
}
+/*
+ * sighandler_t signal(int signum, sighandler_t handler);
+ */
+
+static __attribute__((unused))
+sighandler_t signal(int signum, sighandler_t handler)
+{
+ const struct sigaction act = {
+ .sa_handler = handler,
+ .sa_flags = SA_RESTORER,
+ .sa_restorer = __restore_rt
+ };
+ sighandler_t old_sah;
+ struct sigaction old;
+ int ret;
+
+ ret = sys_sigaction(signum, &act, &old);
+ if (ret < 0) {
+ SET_ERRNO(-ret);
+ old_sah = SIG_ERR;
+ } else {
+ old_sah = old.sa_handler;
+ }
+ return old_sah;
+}
/*
* int stat(const char *path, struct stat *buf);
--
Ammar Faizi
From: Ammar Faizi <[email protected]>
Just like the sigaction() selftest, but for signal().
Signed-off-by: Ammar Faizi <[email protected]>
---
tools/testing/selftests/nolibc/nolibc-test.c | 54 ++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index c348c92d26f6..946ed0132f93 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -565,6 +565,45 @@ static int test_sigaction_sig(int sig)
return 0;
}
+static int test_signal_sig(int sig)
+{
+ sighandler_t old;
+
+ /*
+ * Set the signal handler.
+ */
+ old = signal(sig, test_signal_handler);
+ if (old == SIG_ERR) {
+ printf("test_signal_sig(%d): Failed to set a signal handler\n", sig);
+ return -1;
+ }
+
+ /*
+ * Test the signal handler.
+ */
+ g_test_sig = 0;
+ kill(getpid(), sig);
+
+ /*
+ * test_signal_handler() must set @g_test_sig to @sig.
+ */
+ if (g_test_sig != sig) {
+ printf("test_signal_sig(%d): Invalid g_test_sig value (%d != %d)\n", sig, g_test_sig, sig);
+ return -1;
+ }
+
+ /*
+ * Restore the original signal handler.
+ */
+ old = signal(sig, old);
+ if (old == SIG_ERR) {
+ printf("test_signal_sig(%d): Failed to restore the signal handler\n", sig);
+ return -1;
+ }
+
+ return 0;
+}
+
static const int g_sig_to_test[] = {
SIGINT,
SIGHUP,
@@ -587,6 +626,20 @@ static int test_sigaction(void)
return 0;
}
+static int test_signal(void)
+{
+ size_t i;
+ int ret;
+
+ for (i = 0; i < (sizeof(g_sig_to_test) / sizeof(g_sig_to_test[0])); i++) {
+ ret = test_signal_sig(g_sig_to_test[i]);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
/* Run syscall tests between IDs <min> and <max>.
* Return 0 on success, non-zero on failure.
*/
@@ -669,6 +722,7 @@ int run_syscall(int min, int max)
CASE_TEST(select_stdout); EXPECT_SYSNE(1, ({ fd_set fds; FD_ZERO(&fds); FD_SET(1, &fds); select(2, NULL, &fds, NULL, NULL); }), -1); break;
CASE_TEST(select_fault); EXPECT_SYSER(1, select(1, (void *)1, NULL, NULL, 0), -1, EFAULT); break;
CASE_TEST(sigaction); EXPECT_SYSZR(1, test_sigaction()); break;
+ CASE_TEST(signal); EXPECT_SYSZR(1, test_signal()); break;
CASE_TEST(stat_blah); EXPECT_SYSER(1, stat("/proc/self/blah", &stat_buf), -1, ENOENT); break;
CASE_TEST(stat_fault); EXPECT_SYSER(1, stat(NULL, &stat_buf), -1, EFAULT); break;
CASE_TEST(symlink_root); EXPECT_SYSER(1, symlink("/", "/"), -1, EEXIST); break;
--
Ammar Faizi
From: Ammar Faizi <[email protected]>
Ensure the fork() function can create a child process. Also, when the
child exits, the parent must be able to get the child's exit code
via waitpid().
Signed-off-by: Ammar Faizi <[email protected]>
---
tools/testing/selftests/nolibc/nolibc-test.c | 45 ++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 3a78399f4624..cb6ec9f71aae 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -471,6 +471,50 @@ static int test_getpagesize(void)
return !c;
}
+/*
+ * Test fork().
+ * Make sure the exit code can be read from the parent process.
+ */
+static int test_fork_and_exit(int expected_code)
+{
+ int status;
+ int code;
+ pid_t ret;
+ pid_t p;
+
+ p = fork();
+ if (p < 0)
+ return p;
+
+ if (!p)
+ exit(expected_code);
+
+ do {
+ ret = waitpid(p, &status, 0);
+ if (ret < 0)
+ return ret;
+ } while (!WIFEXITED(status));
+
+ code = WEXITSTATUS(status);
+ if (code != expected_code) {
+ printf("test_fork_and_exit(): waitpid(): Invalid exit code: %d; expected = %d\n", code, expected_code);
+ return -1;
+ }
+
+ return 0;
+}
+
+static int test_fork(void)
+{
+ int i;
+
+ for (i = 0; i < 255; i++) {
+ if (test_fork_and_exit(i))
+ return -1;
+ }
+ return 0;
+}
+
/* Run syscall tests between IDs <min> and <max>.
* Return 0 on success, non-zero on failure.
*/
@@ -523,6 +567,7 @@ int run_syscall(int min, int max)
CASE_TEST(dup3_0); tmp = dup3(0, 100, 0); EXPECT_SYSNE(1, tmp, -1); close(tmp); break;
CASE_TEST(dup3_m1); tmp = dup3(-1, 100, 0); EXPECT_SYSER(1, tmp, -1, EBADF); if (tmp != -1) close(tmp); break;
CASE_TEST(execve_root); EXPECT_SYSER(1, execve("/", (char*[]){ [0] = "/", [1] = NULL }, NULL), -1, EACCES); break;
+ CASE_TEST(fork); EXPECT_SYSZR(1, test_fork()); break;
CASE_TEST(getdents64_root); EXPECT_SYSNE(1, test_getdents64("/"), -1); break;
CASE_TEST(getdents64_null); EXPECT_SYSER(1, test_getdents64("/dev/null"), -1, ENOTDIR); break;
CASE_TEST(gettimeofday_null); EXPECT_SYSZR(1, gettimeofday(NULL, NULL)); break;
--
Ammar Faizi
From: Ammar Faizi <[email protected]>
Test the sigaction() function implementation. Test steps:
- Set a signal handler.
- Then send a signal to itself using the kill() syscall.
- The program has to survive and store the caught signal number in a
volatile global variable.
- Validate the volatile global variable value.
- Restore the original signal handler.
Signed-off-by: Ammar Faizi <[email protected]>
---
tools/testing/selftests/nolibc/nolibc-test.c | 73 ++++++++++++++++++++
1 file changed, 73 insertions(+)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index cb6ec9f71aae..c348c92d26f6 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -515,6 +515,78 @@ static int test_fork(void)
return 0;
}
+static volatile int g_test_sig;
+
+static void test_signal_handler(int sig)
+{
+ g_test_sig = sig;
+}
+
+static int test_sigaction_sig(int sig)
+{
+ const struct sigaction new = {
+ .sa_handler = test_signal_handler
+ };
+ struct sigaction old;
+ int ret;
+
+ /*
+ * Set the signal handler.
+ */
+ ret = sigaction(sig, &new, &old);
+ if (ret) {
+ printf("test_sigaction_sig(%d): Failed to set a signal handler\n", sig);
+ return ret;
+ }
+
+ /*
+ * Test the signal handler.
+ */
+ g_test_sig = 0;
+ kill(getpid(), sig);
+
+ /*
+ * test_signal_handler() must set @g_test_sig to @sig.
+ */
+ if (g_test_sig != sig) {
+ printf("test_sigaction_sig(%d): Invalid g_test_sig value (%d != %d)\n", sig, g_test_sig, sig);
+ return -1;
+ }
+
+ /*
+ * Restore the original signal handler.
+ */
+ ret = sigaction(sig, &old, NULL);
+ if (ret) {
+ printf("test_sigaction_sig(%d): Failed to restore the signal handler\n", sig);
+ return ret;
+ }
+
+ return 0;
+}
+
+static const int g_sig_to_test[] = {
+ SIGINT,
+ SIGHUP,
+ SIGTERM,
+ SIGQUIT,
+ SIGSEGV
+};
+
+static int test_sigaction(void)
+{
+ size_t i;
+ int ret;
+
+ for (i = 0; i < (sizeof(g_sig_to_test) / sizeof(g_sig_to_test[0])); i++) {
+ ret = test_sigaction_sig(g_sig_to_test[i]);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
/* Run syscall tests between IDs <min> and <max>.
* Return 0 on success, non-zero on failure.
*/
@@ -596,6 +668,7 @@ int run_syscall(int min, int max)
CASE_TEST(select_null); EXPECT_SYSZR(1, ({ struct timeval tv = { 0 }; select(0, NULL, NULL, NULL, &tv); })); break;
CASE_TEST(select_stdout); EXPECT_SYSNE(1, ({ fd_set fds; FD_ZERO(&fds); FD_SET(1, &fds); select(2, NULL, &fds, NULL, NULL); }), -1); break;
CASE_TEST(select_fault); EXPECT_SYSER(1, select(1, (void *)1, NULL, NULL, 0), -1, EFAULT); break;
+ CASE_TEST(sigaction); EXPECT_SYSZR(1, test_sigaction()); break;
CASE_TEST(stat_blah); EXPECT_SYSER(1, stat("/proc/self/blah", &stat_buf), -1, ENOENT); break;
CASE_TEST(stat_fault); EXPECT_SYSER(1, stat(NULL, &stat_buf), -1, EFAULT); break;
CASE_TEST(symlink_root); EXPECT_SYSER(1, symlink("/", "/"), -1, EEXIST); break;
--
Ammar Faizi
On Sun, Jan 08, 2023 at 06:58:42PM +0100, Willy Tarreau wrote:
> I'm currently testing it on various archs. For now:
>
> - x86_64 and arm64 pass the test
Thanks for testing.
> - i386 and arm fail:
> 59 sigactiontest_sigaction_sig(2): Failed to set a signal handler
> = -1 EINVAL [FAIL]
> 60 signaltest_signal_sig(2): Failed to set a signal handler
> = -1 EINVAL [FAIL]
I'll take a look at i386 for now.
> - riscv and mips build are now broken:
> sysroot/riscv/include/sys.h:1110:18: error: 'struct sigaction' has no member named 'sa_restorer'
> 1110 | if (!act2.sa_restorer) {
> | ^
> sysroot/riscv/include/sys.h:1111:34: error: 'SA_RESTORER' undeclared (first use in this function); did you mean 'SA_RESTART'?
> 1111 | act2.sa_flags |= SA_RESTORER;
> | ^~~~~~~~~~~
> | SA_RESTART
Just a speculation:
This is probably because not all architectures have a SA_RESTORER. I'll
need to figure out how Linux handles signal on those architectures.
> - s390 segfaults:
> 58 select_fault = -1 EFAULT [OK]
> 59 sigactionqemu: uncaught target signal 11 (Segmentation fault) - core dumped
> Segmentation fault
>
> It dies in __restore_rt at 1006ba4 while performing the syscall,
> I don't know why, maybe this arch requires an alt stack or whatever :
>
> 0000000001006ba0 <__restore_rt>:
> 1006ba0: a7 19 00 ad lghi %r1,173
> 1006ba4: 0a 00 svc 0
> 1006ba6: 07 07 nopr %r7
Bah, no clue on this. I'll CC s390 people in the next version and ask
them to shed some light.
> At the very least we need to make sure we don't degrade existing tests,
> which means making sure that it builds everywhere and that all those
> which build do work.
Understand.
> It would be nice to figure what's failing on i386. Given that both it
> and arm fail on EINVAL while both x86_64 and arm64 work, I suspect that
> once you figure what breaks i386 it'll fix the problem on arm at the
> same time. I had a quick look but didn't spot anything suspicious.
> Once we've figured this, we could decide to tag archs supporting
> sig_action() and condition the functions definition and the tests to
> these.
I'll be pondering this code this week (to follow what actually the
rt_sigaction wants on i386 and arm):
https://github.com/torvalds/linux/blob/v6.2-rc3/kernel/signal.c#L4404-L4434
Hopefully, I can get it sorted before the weekend.
> The advantage of trying with i386 is that your regular tools and the
> debugger you used for x86_64 will work. I'm proceeding like this with
> the toolchains from https://mirrors.edge.kernel.org/pub/tools/crosstool/ :
>
> $ make nolibc-test LDFLAGS=-g CFLAGS=-g ARCH=i386 CC=/path/to/gcc-11.3.0-nolibc/i386-linux/bin/i386-linux-gcc
> $ gdb ./nolibc-test
> > b sigaction
> > run
> > s
> ...
Nice tip! I'll be playing with that.
> Note that the code looks correct at first glance:
>
> 0804b4a0 <__restore_rt>:
> 804b4a0: b8 ad 00 00 00 mov $0xad,%eax
> 804b4a5: cd 80 int $0x80
>
> I also think that the printf() in test_sigaction_sig() are not welcome
> as they corrupt the output. Maybe one thing you could do to preserve the
> info would be to prepend a space in front of the message and remove the
> LF. For example the simple patch below:
[...]
> Which is way more readable and still grep-friendly.
Yeah, that looks much better. Applied to my local git tree with
attribution.
--
Ammar Faizi
Hi Ammar,
On Sun, Jan 08, 2023 at 08:58:59PM +0700, Ammar Faizi wrote:
> From: Ammar Faizi <[email protected]>
>
> Hi Willy,
>
> On top of the series titled "nolibc auxiliary vector retrieval support".
> The prerequisite patches of this series are in that series.
>
> This is v2 of nolibc signal handling support. It adds signal handling
> support to the nolibc subsystem:
>
> 1) Initial implementation of nolibc sigaction(2) function.
>
> `sigaction()` needs an architecture-dependent "signal trampoline"
> function that invokes __rt_sigreturn syscall to resume the process
> after a signal gets handled.
>
> The "signal trampoline" function is called `__restore_rt` in this
> implementation. The naming `__restore_rt` is important for GDB. It
> also has to be given a special optimization attribute
> "omit-frame-pointer" to prevent the compiler from creating a stack
> frame that makes the `%rsp` value no longer points to the `struct
> rt_sigframe` that the kernel constructed.
>
>
> 2) signal(2) function.
>
> signal() function is the simpler version of sigaction(). Unlike
> sigaction(), which fully controls the struct sigaction, the caller
> only cares about the sa_handler when calling the signal() function.
> signal() internally calls sigaction().
>
>
> 3) More selftests.
>
> This series also adds selftests for:
> - fork(2)
> - sigaction(2)
> - signal(2)
>
>
> Side note for __restore_rt:
> This has been tested on x86-64 arch and `__restore_rt` generates the
> correct code. The `__restore_rt` codegen correctness on other
> architectures need to be evaluated as well. If it can't generate the
> correct code, it has to be written in inline Assembly.
I'm currently testing it on various archs. For now:
- x86_64 and arm64 pass the test
- i386 and arm fail:
59 sigactiontest_sigaction_sig(2): Failed to set a signal handler
= -1 EINVAL [FAIL]
60 signaltest_signal_sig(2): Failed to set a signal handler
= -1 EINVAL [FAIL]
- riscv and mips build are now broken:
sysroot/riscv/include/sys.h:1110:18: error: 'struct sigaction' has no member named 'sa_restorer'
1110 | if (!act2.sa_restorer) {
| ^
sysroot/riscv/include/sys.h:1111:34: error: 'SA_RESTORER' undeclared (first use in this function); did you mean 'SA_RESTART'?
1111 | act2.sa_flags |= SA_RESTORER;
| ^~~~~~~~~~~
| SA_RESTART
- s390 segfaults:
58 select_fault = -1 EFAULT [OK]
59 sigactionqemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault
It dies in __restore_rt at 1006ba4 while performing the syscall,
I don't know why, maybe this arch requires an alt stack or whatever :
0000000001006ba0 <__restore_rt>:
1006ba0: a7 19 00 ad lghi %r1,173
1006ba4: 0a 00 svc 0
1006ba6: 07 07 nopr %r7
At the very least we need to make sure we don't degrade existing tests,
which means making sure that it builds everywhere and that all those
which build do work.
It would be nice to figure what's failing on i386. Given that both it
and arm fail on EINVAL while both x86_64 and arm64 work, I suspect that
once you figure what breaks i386 it'll fix the problem on arm at the
same time. I had a quick look but didn't spot anything suspicious.
Once we've figured this, we could decide to tag archs supporting
sig_action() and condition the functions definition and the tests to
these.
The advantage of trying with i386 is that your regular tools and the
debugger you used for x86_64 will work. I'm proceeding like this with
the toolchains from https://mirrors.edge.kernel.org/pub/tools/crosstool/ :
$ make nolibc-test LDFLAGS=-g CFLAGS=-g ARCH=i386 CC=/path/to/gcc-11.3.0-nolibc/i386-linux/bin/i386-linux-gcc
$ gdb ./nolibc-test
> b sigaction
> run
> s
...
Note that the code looks correct at first glance:
0804b4a0 <__restore_rt>:
804b4a0: b8 ad 00 00 00 mov $0xad,%eax
804b4a5: cd 80 int $0x80
I also think that the printf() in test_sigaction_sig() are not welcome
as they corrupt the output. Maybe one thing you could do to preserve the
info would be to prepend a space in front of the message and remove the
LF. For example the simple patch below:
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index a1883467451a..42f794c646b7 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -535,7 +535,7 @@ static int test_sigaction_sig(int sig)
*/
ret = sigaction(sig, &new, &old);
if (ret) {
- printf("test_sigaction_sig(%d): Failed to set a signal handler\n", sig);
+ printf(" (failed to set handler for signal %d)", sig);
return ret;
}
@@ -549,7 +549,7 @@ static int test_sigaction_sig(int sig)
* test_signal_handler() must set @g_test_sig to @sig.
*/
if (g_test_sig != sig) {
- printf("test_sigaction_sig(%d): Invalid g_test_sig value (%d != %d)\n", sig, g_test_sig, sig);
+ printf(" (invalid g_test_sig value (%d != %d))", sig, g_test_sig);
return -1;
}
@@ -558,7 +558,7 @@ static int test_sigaction_sig(int sig)
*/
ret = sigaction(sig, &old, NULL);
if (ret) {
- printf("test_sigaction_sig(%d): Failed to restore the signal handler\n", sig);
+ printf(" (Failed to restore handler for signal %d)", sig);
return ret;
}
@@ -574,7 +574,7 @@ static int test_signal_sig(int sig)
*/
old = signal(sig, test_signal_handler);
if (old == SIG_ERR) {
- printf("test_signal_sig(%d): Failed to set a signal handler\n", sig);
+ printf(" (failed to set handler for signal %d)", sig);
return -1;
}
@@ -588,7 +588,7 @@ static int test_signal_sig(int sig)
* test_signal_handler() must set @g_test_sig to @sig.
*/
if (g_test_sig != sig) {
- printf("test_signal_sig(%d): Invalid g_test_sig value (%d != %d)\n", sig, g_test_sig, sig);
+ printf(" (invalid g_test_sig value (%d != %d))", sig, g_test_sig);
return -1;
}
@@ -597,7 +597,7 @@ static int test_signal_sig(int sig)
*/
old = signal(sig, old);
if (old == SIG_ERR) {
- printf("test_signal_sig(%d): Failed to restore the signal handler\n", sig);
+ printf(" (Failed to restore handler for signal %d)", sig);
return -1;
}
Gives me this:
...
56 select_null = 0 [OK]
57 select_stdout = 1 [OK]
58 select_fault = -1 EFAULT [OK]
59 sigaction (failed to set handler for signal 2) = -1 EINVAL [FAIL]
60 signal (failed to set handler for signal 2) = -1 EINVAL [FAIL]
61 stat_blah = -1 ENOENT [OK]
62 stat_fault = -1 EFAULT [OK]
63 symlink_root = -1 EEXIST [OK]
...
Which is way more readable and still grep-friendly.
Thanks!
Willy
On Mon, Jan 09, 2023 at 01:31:17AM +0700, Ammar Faizi wrote:
> > - riscv and mips build are now broken:
> > sysroot/riscv/include/sys.h:1110:18: error: 'struct sigaction' has no member named 'sa_restorer'
> > 1110 | if (!act2.sa_restorer) {
> > | ^
> > sysroot/riscv/include/sys.h:1111:34: error: 'SA_RESTORER' undeclared (first use in this function); did you mean 'SA_RESTART'?
> > 1111 | act2.sa_flags |= SA_RESTORER;
> > | ^~~~~~~~~~~
> > | SA_RESTART
>
> Just a speculation:
> This is probably because not all architectures have a SA_RESTORER. I'll
> need to figure out how Linux handles signal on those architectures.
Yes that's the case, there's even some ifdef around it in the generic
code. I have no idea how it works there, at least it seems worth having
a look to make sure we don't miss something easy.
> > - s390 segfaults:
> > 58 select_fault = -1 EFAULT [OK]
> > 59 sigactionqemu: uncaught target signal 11 (Segmentation fault) - core dumped
> > Segmentation fault
> >
> > It dies in __restore_rt at 1006ba4 while performing the syscall,
> > I don't know why, maybe this arch requires an alt stack or whatever :
> >
> > 0000000001006ba0 <__restore_rt>:
> > 1006ba0: a7 19 00 ad lghi %r1,173
> > 1006ba4: 0a 00 svc 0
> > 1006ba6: 07 07 nopr %r7
>
> Bah, no clue on this. I'll CC s390 people in the next version and ask
> them to shed some light.
OK.
> I'll be pondering this code this week (to follow what actually the
> rt_sigaction wants on i386 and arm):
>
> https://github.com/torvalds/linux/blob/v6.2-rc3/kernel/signal.c#L4404-L4434
Seems like it could simply be a matter of sigsetsize, which is the
first one returning -EINVAL.
> Hopefully, I can get it sorted before the weekend.
OK!
> > I also think that the printf() in test_sigaction_sig() are not welcome
> > as they corrupt the output. Maybe one thing you could do to preserve the
> > info would be to prepend a space in front of the message and remove the
> > LF. For example the simple patch below:
> [...]
> > Which is way more readable and still grep-friendly.
>
> Yeah, that looks much better. Applied to my local git tree with
> attribution.
Don't worry with attribution for such patches from me. I'd rather see
the first patch looking good at once than having an extra one change
the output just for the sake of attribution! It was just a suggestion
anyway and whatever solution you find that improves the output will be
fine.
Thank you!
Willy
On Sun, Jan 08, 2023 at 07:49:30PM +0100, Willy Tarreau wrote:
> On Mon, Jan 09, 2023 at 01:31:17AM +0700, Ammar Faizi wrote:
> > I'll be pondering this code this week (to follow what actually the
> > rt_sigaction wants on i386 and arm):
> >
> > https://github.com/torvalds/linux/blob/v6.2-rc3/kernel/signal.c#L4404-L4434
>
> Seems like it could simply be a matter of sigsetsize, which is the
> first one returning -EINVAL.
>
> > Hopefully, I can get it sorted before the weekend.
>
> OK!
I couldn't dedicate much time to this, but I looked into it, and here's
my report on the progress. I didn't manage to find a proper solution to
this. But yes, you're right. It's a matter of 'sizeof(sigset_t)'.
So here is my observation. Currently, nolibc's sys.h includes this:
#include <asm/signal.h>
The definition of 'sigset_t' in that header is:
typedef unsigned long sigset_t;
On i386, 'sizeof(unsigned long)' is 4, but on x86-64 it's 8.
That is not the 'sigset_t' that the kernel wants. The kernel wants the
'sigset_t' that is in <asm-generic/signal.h>:
#define _NSIG 64
#define _NSIG_BPW __BITS_PER_LONG // this 64 on x86-64, but 32 on i386.
#define _NSIG_WORDS (_NSIG / _NSIG_BPW)
typedef struct {
unsigned long sig[_NSIG_WORDS];
} sigset_t;
The above struct is always 8 bytes in size. In other words:
_NSIG_WORDS == 2 on i386
_NSIG_WORDS == 1 on x86-64
sizeof(unsigned long) == 4 on i386
sizeof(unsigned long) == 8 on x86-64
Therefore, sizeof(unsigned long [_NSIG_WORDS]) is always 8 on both
architectures. That's the correct size.
I tried to #include <asm-generic/signal.h> but it conflicts with the
other 'sigset_t' definition. So I can't do that.
Why are there two different definitions of 'sigset_t'? I don't know.
I probably should read the story behind this syscall to get it
implemented right. Let me ponder this again on Monday. But at least I
tell what I have found so people can give some comments on it...
--
Ammar Faizi
On Sun, Jan 15, 2023 at 11:01 PM Ammar Faizi wrote:
> That is not the 'sigset_t' that the kernel wants. The kernel wants the
> 'sigset_t' that is in <asm-generic/signal.h>:
>
> #define _NSIG 64
> #define _NSIG_BPW __BITS_PER_LONG // this 64 on x86-64, but 32 on i386.
> #define _NSIG_WORDS (_NSIG / _NSIG_BPW)
>
> typedef struct {
> unsigned long sig[_NSIG_WORDS];
> } sigset_t;
>
> The above struct is always 8 bytes in size. In other words:
>
> _NSIG_WORDS == 2 on i386
> _NSIG_WORDS == 1 on x86-64
> sizeof(unsigned long) == 4 on i386
> sizeof(unsigned long) == 8 on x86-64
>
> Therefore, sizeof(unsigned long [_NSIG_WORDS]) is always 8 on both
> architectures. That's the correct size.
>
> I tried to #include <asm-generic/signal.h> but it conflicts with the
> other 'sigset_t' definition. So I can't do that.
Read the glibc sigaction implementation, it has different struct
sigaction definitions for user and kernel too.
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/libc_sigaction.c;h=3cbf241a5fa28296c910fa40a41b09d2b6113b05;hb=7e31d166510ac4adbf53d5e8144c709a37dd8c7a
Since nolibc compiles everything in a single translation unit, you
can't have a different sigset_t definition. You need to copy the
definition to nolibc header and change the type name to something else
for internal use only.
> Why are there two different definitions of 'sigset_t'? I don't know.
>
> I probably should read the story behind this syscall to get it
> implemented right. Let me ponder this again on Monday. But at least I
> tell what I have found so people can give some comments on it...
`man 2 rt_sigaction` tells the story. Here is the bedtime story, have
a nice dream :-)
The original Linux system call was named sigaction(). However, with
the addition of real-time signals in Linux 2.2, the fixed-size, 32-bit
sigset_t type supported by that system call was no longer fit for
purpose.
Consequently, a new system call, rt_sigaction(), was added to support
an enlarged sigset_t type. The new system call takes a fourth
argument, size_t sigsetsize, which specifies the size in bytes of the
signal sets
in act.sa_mask and oldact.sa_mask. This argument is currently required
to have the value sizeof(sigset_t) (or the error EINVAL results). The
glibc sigaction() wrapper function hides these details from us,
transpar‐
ently calling rt_sigaction() when the kernel provides it.
On Mon, Jan 16, 2023 at 12:06:44AM +0700, Alviro Iskandar Setiawan wrote:
> Read the glibc sigaction implementation, it has different struct
> sigaction definitions for user and kernel too.
>
> https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/libc_sigaction.c;h=3cbf241a5fa28296c910fa40a41b09d2b6113b05;hb=7e31d166510ac4adbf53d5e8144c709a37dd8c7a
>
> Since nolibc compiles everything in a single translation unit, you
> can't have a different sigset_t definition. You need to copy the
> definition to nolibc header and change the type name to something else
> for internal use only.
I'll give it a try.
--
Ammar Faizi