Hi Benjamin, Thanks for your patch!
On 2025-07-01 14:29:10+0200, Benjamin Berg wrote: > From: Benjamin Berg <[email protected]> Please send the next revisions to the nolibc maintainers from the MAINTAINERS file. Willy and my @weissschuh.net address. > In preparation to add tests that use it. Here "tests" is not clear about its meaning. > Note that some architectures do not have a usable linux/signal.h include > file. However, in those cases we can use asm-generic/signal.h instead. This should explain what "unusable" means. > Signed-off-by: Benjamin Berg <[email protected]> > > --- > > Another attempt at signal handling for nolibc which should actually be > working. Some trickery is needed to get the right definition, but I feel > it is sufficiently clean this way. > > Submitting this as RFC mostly because I do not yet have a proper patch > to add a test that uses the feature. We do pick up new features in nolibc without them having in-kernel users. So if you want to get this in already you can drop the RFC state. > Benjamin > --- > tools/include/nolibc/arch-aarch64.h | 3 + In nolibc/for-next, arch-aarch64.h is now called arch-arm64.h. https://git.kernel.org/pub/scm/linux/kernel/git/nolibc/linux-nolibc.git/log/?h=for-next > tools/include/nolibc/arch-arm.h | 7 ++ > tools/include/nolibc/arch-i386.h | 13 +++ ... and arch-i386.h was renamed to arch-x86.h > tools/include/nolibc/arch-loongarch.h | 3 + > tools/include/nolibc/arch-m68k.h | 10 ++ > tools/include/nolibc/arch-mips.h | 3 + > tools/include/nolibc/arch-powerpc.h | 8 ++ > tools/include/nolibc/arch-riscv.h | 3 + > tools/include/nolibc/arch-s390.h | 8 +- > tools/include/nolibc/arch-sparc.h | 43 +++++++++ > tools/include/nolibc/arch-x86_64.h | 10 ++ same for arch-x86_64.h, Unlucky timing. > tools/include/nolibc/signal.h | 97 ++++++++++++++++++++ > tools/include/nolibc/sys.h | 2 +- > tools/include/nolibc/time.h | 3 +- > tools/testing/selftests/nolibc/nolibc-test.c | 52 +++++++++++ > 15 files changed, 261 insertions(+), 4 deletions(-) <snip> > --- a/tools/include/nolibc/arch-i386.h > +++ b/tools/include/nolibc/arch-i386.h > @@ -10,6 +10,19 @@ > #include "compiler.h" > #include "crt.h" > > +/* Needed to get the correct struct sigaction definition */ > +#define SA_RESTORER 0x04000000 > + > +/* Restorer must be set on i386 */ > +#define _NOLIBC_ARCH_NEEDS_SA_RESTORER > + > +/* Otherwise we would need to use sigreturn instead of rt_sigreturn */ > +#define _NOLIBC_ARCH_FORCE_SIG_FLAGS SA_SIGINFO > + > +/* Avoid linux/signal.h, it has an incorrect _NSIG and sigset_t */ > +#include <asm-generic/signal.h> > +#include <asm-generic/siginfo.h> This doesn't work if the user already has <linux/signal.h> included for some other reason. The symbol names will conflict. Can we do something like this: #include <linux/signal.h> /* lifted from asm-generic/signal.h */ struct __nolibc_sigaction { __sighandler_t sa_handler; unsigned long sa_flags; #ifdef SA_RESTORER __sigrestore_t sa_restorer; #endif __nolibc_sigset_t sa_mask; }; int sys_rt_sigaction(int signum, const struct sigaction *act, struct sigaction *oldact) { struct __nolibc_sigaction nolibc_act, nolibc_oldact; int ret; /* Convert whatever gunk we got from linux/signal.h to what the * kernel actually expects. If it is the same structure, * hopefully the compiler manages to optimize it away. * See __nolibc_user_timespec_to_kernel() et al. * (Comment not meant to be copied verbatim) */ __nolibc_sigaction_user_to_kernel(act, &nolibc_act); #if defined(_NOLIBC_ARCH_NEEDS_SA_RESTORER) ... #endif ret = my_syscall4(__NR_rt_sigaction, signum, &nolibc_act, &nolibc_oldact, sizeof(nolibc_act->sa_mask)); __nolibc_sigaction_kernel_to_user(&nolibc_oldact, oldact); return ret; } Or am I missing something? > + > /* Syscalls for i386 : > * - mostly similar to x86_64 > * - registers are 32-bit <snip> > --- a/tools/include/nolibc/arch-sparc.h > +++ b/tools/include/nolibc/arch-sparc.h > @@ -12,6 +12,19 @@ > #include "compiler.h" > #include "crt.h" > > +/* Otherwise we would need to use sigreturn instead of rt_sigreturn */ > +#define _NOLIBC_ARCH_FORCE_SIG_FLAGS SA_SIGINFO > + > +/* The includes are sane, if one sets __WANT_POSIX1B_SIGNALS__ */ > +#define __WANT_POSIX1B_SIGNALS__ > +#include <linux/signal.h> This also assumes the user did not already include <linux/signal.h> before including nolibc. > + > +/* > + * sparc has ODD_RT_SIGACTION, we always pass our restorer as an argument > + * to rt_sigaction. The restorer is implemented in this file. > + */ > +#define _NOLIBC_RT_SIGACTION_PASSES_RESTORER > + > /* > * Syscalls for SPARC: > * - registers are native word size > @@ -188,4 +201,34 @@ pid_t sys_fork(void) > } > #define sys_fork sys_fork > > +#define __nolibc_stringify_1(x...) #x > +#define __nolibc_stringify(x...) __stringify_1(x) If we need this, IMO it belongs into compiler.h. > + > +/* The compiler insists on adding a SAVE call to the start of every function > */ > +#define __nolibc_sa_restorer __nolibc_sa_restorer > +void __nolibc_sa_restorer (void); > +#ifdef __arch64__ > +__asm__( \ We are avoiding bare toplevel asm calls. You could use the same trick as my SuperH _start() function and use asm() inside a function. https://lore.kernel.org/lkml/[email protected]/ > + ".section .text\n" \ > + ".align 4 \n" \ > + "__nolibc_sa_restorer:\n" \ > + "nop\n" \ > + "nop\n" \ > + "mov " __stringify(__NR_rt_sigreturn) ", %g1 \n" \ > + "t 0x6d \n"); > +#else > +__asm__( \ > + ".section .text\n" \ > + ".align 4 \n" \ > + "__nolibc_sa_restorer:\n" \ > + "nop\n" \ > + "nop\n" \ > + "mov " __stringify(__NR_rt_sigreturn) ", %g1 \n" \ > + "t 0x10 \n" \ Only one line differs. I'd prefer the #ifdef around that. > + ); > +#endif > + > +#undef __nolibc_stringify_1(x...) > +#undef __nolibc_stringify And there is no need to undef it again. > + > #endif /* _NOLIBC_ARCH_SPARC_H */ > diff --git a/tools/include/nolibc/arch-x86_64.h > b/tools/include/nolibc/arch-x86_64.h > index 67305e24dbef..9f13a2205876 100644 <snip> > --- a/tools/include/nolibc/signal.h > +++ b/tools/include/nolibc/signal.h > @@ -14,6 +14,8 @@ > #include "arch.h" > #include "types.h" > #include "sys.h" > +#include "string.h" > +/* signal definitions are included by arch.h */ > > /* This one is not marked static as it's needed by libgcc for divide by zero > */ > int raise(int signal); > @@ -23,4 +25,99 @@ int raise(int signal) > return sys_kill(sys_getpid(), signal); > } > > +/* > + * sigaction(int signum, const struct sigaction *act, struct sigaction > *oldact) > + */ > +#if defined(_NOLIBC_ARCH_NEEDS_SA_RESTORER) && !defined(__nolibc_sa_restorer) > +static __attribute__((noreturn)) __nolibc_entrypoint __no_stack_protector __attribute__((noreturn)) is not always available and should be behind __nolibc_has_attribute(). I'm a bit unhappy about reusing the entrypoint machinery, but I guess it's necessary. > +void __nolibc_sa_restorer(void) > +{ > + my_syscall0(__NR_rt_sigreturn); > + __nolibc_entrypoint_epilogue(); > +} > +#endif > + > +static __attribute__((unused)) > +int sys_rt_sigaction(int signum, const struct sigaction *act, struct > sigaction *oldact) > +{ > + struct sigaction real_act = *act; > +#if defined(SA_RESTORER) && defined(_NOLIBC_ARCH_NEEDS_SA_RESTORER) If _NOLIBC_ARCH_NEEDS_SA_RESTORER is set then SA_RESTORER should also be present. SA_RESTORER doesn't need to be checked also. Are there cases where SA_RESTORER is defined but not needed? > + if (!(real_act.sa_flags & SA_RESTORER)) { > + real_act.sa_flags |= SA_RESTORER; > + real_act.sa_restorer = __nolibc_sa_restorer; > + } > +#endif > +#ifdef _NOLIBC_ARCH_FORCE_SIG_FLAGS > + real_act.sa_flags |= _NOLIBC_ARCH_FORCE_SIG_FLAGS; > +#endif > + > +#ifndef _NOLIBC_RT_SIGACTION_PASSES_RESTORER > + return my_syscall4(__NR_rt_sigaction, signum, &real_act, oldact, > + sizeof(act->sa_mask)); > +#else > + return my_syscall5(__NR_rt_sigaction, signum, &real_act, oldact, > + __nolibc_sa_restorer, sizeof(act->sa_mask)); This calling convention is specific to SPARC, so I prefer it to be in arch-sparc.h. Alpha also uses 5 arguments for the syscall, but of course in a different order... (I do have nearly done patches for alpha support). Also if a user specified their custom sa_restorer in 'act', that one should be used, no? > +#endif > +} > + > +static __attribute__((unused)) > +int sigaction(int signum, const struct sigaction *act, struct sigaction > *oldact) > +{ > + return __sysret(sys_rt_sigaction(signum, act, oldact)); > +} > + > +/* > + * int sigemptyset(sigset_t *set) > + */ > +static __attribute__((unused)) > +int sigemptyset(sigset_t *set) > +{ > + memset(set, 0, sizeof(*set)); > + return 0; > +} > + > +/* > + * int sigfillset(sigset_t *set) > + */ > +static __attribute__((unused)) > +int sigfillset(sigset_t *set) > +{ > + memset(set, 0xff, sizeof(*set)); > + return 0; > +} > + > +/* > + * int sigaddset(sigset_t *set, int signum) > + */ > +static __attribute__((unused)) > +int sigaddset(sigset_t *set, int signum) > +{ > + set->sig[(signum - 1) / (8 * sizeof(set->sig[0]))] |= > + 1UL << ((signum - 1) % (8 * sizeof(set->sig[0]))); > + return 0; This is documented to return EINVAL for an invalid signum. > +} > + > +/* > + * int sigdelset(sigset_t *set, int signum) > + */ > +static __attribute__((unused)) > +int sigdelset(sigset_t *set, int signum) > +{ > + set->sig[(signum - 1) / (8 * sizeof(set->sig[0]))] &= > + ~(1UL << ((signum - 1) % (8 * sizeof(set->sig[0])))); > + return 0; > +} > + > +/* > + * int sigismember(sigset_t *set, int signum) > + */ > +static __attribute__((unused)) > +int sigismember(sigset_t *set, int signum) > +{ > + unsigned long res = > + set->sig[(signum - 1) / (8 * sizeof(set->sig[0]))] & > + (1UL << ((signum - 1) % (8 * sizeof(set->sig[0])))); > + return !!res; > +} These are similar to FD_CLR()/FD_SET() etc, no? Moving both sets of functions to common inline helpers would be nice. > + > #endif /* _NOLIBC_SIGNAL_H */ <snip> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c > b/tools/testing/selftests/nolibc/nolibc-test.c > index dbe13000fb1a..af66b739ea18 100644 > --- a/tools/testing/selftests/nolibc/nolibc-test.c > +++ b/tools/testing/selftests/nolibc/nolibc-test.c > @@ -1750,6 +1750,57 @@ static int run_protection(int min > __attribute__((unused)), > } > } > > +volatile int signal_check; Technically this should be sig_atomic_t. Which can be a typedef to int. > +void test_sighandler(int signum) > +{ > + if (signum == SIGUSR1) { > + kill(getpid(), SIGUSR2); > + signal_check = 1; > + } else { > + signal_check++; > + } > +} > + > +int run_signal(int min, int max) > +{ > + struct sigaction sa = { > + .sa_flags = 0, > + .sa_handler = test_sighandler, > + }; > + int llen; /* line length */ > + int ret = 0; > + int res; > + > + (void)min; > + (void)max; > + > + signal_check = 0; > + > + sigemptyset(&sa.sa_mask); > + sigaddset(&sa.sa_mask, SIGUSR2); It would be good to do some assertions after sigemptyset() and sigaddset() to validate the bitfiddling. > + > + res = sigaction(SIGUSR1, &sa, NULL); > + llen = printf("register SIGUSR1: %d", res); > + EXPECT_EQ(1, 0, res); > + res = sigaction(SIGUSR2, &sa, NULL); > + llen = printf("register SIGUSR2: %d", res); > + EXPECT_EQ(1, 0, res); Here it would be nice to validate the old action. > + > + /* Trigger the first signal. */ > + kill(getpid(), SIGUSR1); > + > + /* If signal_check is 1 or higher, then signal emission worked */ > + llen = printf("signal emission: 1 <= signal_check"); > + EXPECT_GE(1, signal_check, 1); > + > + /* If it is 2, then signal masking worked */ > + llen = printf("signal masking: 2 == signal_check"); > + EXPECT_EQ(1, signal_check, 2); > + > + return ret; > +} > + > /* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */ > int prepare(void) > { > @@ -1815,6 +1866,7 @@ static const struct test test_names[] = { > { .name = "stdlib", .func = run_stdlib }, > { .name = "printf", .func = run_printf }, > { .name = "protection", .func = run_protection }, > + { .name = "signal", .func = run_signal }, These 'struct test's are really more test suites. This testcase can be part of the syscall suite. > { 0 } > }; > > -- > 2.50.0 >
