| Message ID | 20230108135904.851762-1-ammar.faizi@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | nolibc signal handling support | expand |
Hi Ammar, On Sun, Jan 08, 2023 at 08:58:59PM +0700, Ammar Faizi wrote: > From: Ammar Faizi <ammarfaizi2@gnuweeb.org> > > 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 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.
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...
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.