Re: [RFC v2] tools/nolibc: add sigaction()

["Thomas Weißschuh" <linux@weissschuh.net>]

Hi Benjamin,

Thanks for your patch!

On 2025-07-01 14:29:10+0200, Benjamin Berg wrote:
> From: Benjamin Berg <benjamin.berg@intel.com>

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 <benjamin.berg@intel.com>
> 
> ---
> 
> 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/20250623-nolibc-sh-v2-3-0f5b4b303025@weissschuh.net/

> +	".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
>