Hi,
On Wed, 2025-07-02 at 00:04 +0200, Thomas Weißschuh wrote:
> [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.
I was thinking this is fine. Such a conflict already exists between the
normal glibc <signal.h> and <linux/signal.h>. So there would only be a
problem if the user is explicitly not including <signal.h> to then use
<linux/signal.h>. I doubt that makes sense.
Benjamin
>
> 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
> >
>
