This patch series is motivated by the following observation:
Raise a signal, jump to signal handler. The ucontext_t structure dumped by kernel to userspace has a uc_sigmask field having the mask of blocked signals. If you run a fresh minimalistic program doing this, this field is empty, even if you block some signals while registering the handler with sigaction().
Here is what the man-pages have to say:
sigaction(2): "sa_mask specifies a mask of signals which should be blocked (i.e., added to the signal mask of the thread in which the signal handler is invoked) during execution of the signal handler. In addition, the signal which triggered the handler will be blocked, unless the SA_NODEFER flag is used."
signal(7): Under "Execution of signal handlers", (1.3) implies:
"The thread's current signal mask is accessible via the ucontext_t object that is pointed to by the third argument of the signal handler."
But, (1.4) states:
"Any signals specified in act->sa_mask when registering the handler with sigprocmask(2) are added to the thread's signal mask. The signal being delivered is also added to the signal mask, unless SA_NODEFER was specified when registering the handler. These signals are thus blocked while the handler executes."
There clearly is no distinction being made in the man pages between "Thread's signal mask" and ucontext_t; this logically should imply that a signal blocked by populating struct sigaction should be visible in ucontext_t.
Here is what the kernel code does (for Aarch64):
do_signal() -> handle_signal() -> sigmask_to_save(), which returns ¤t->blocked, is passed to setup_rt_frame() -> setup_sigframe() -> __copy_to_user(). Hence, ¤t->blocked is copied to ucontext_t exposed to userspace. Returning back to handle_signal(), signal_setup_done() -> signal_delivered() -> sigorsets() and set_current_blocked() are responsible for using information from struct ksignal ksig, which was populated through the sigaction() system call in kernel/signal.c: copy_from_user(&new_sa.sa, act, sizeof(new_sa.sa)), to update ¤t->blocked; hence, the set of blocked signals for the current thread is updated AFTER the kernel dumps ucontext_t to userspace.
Assuming that the above is indeed the intended behaviour, because it semantically makes sense, since the signals blocked using sigaction() remain blocked only till the execution of the handler, and not in the context present before jumping to the handler (but nothing can be confirmed from the man-pages), the series introduces a test for mangling with uc_sigmask. I will send a separate series to fix the man-pages.
The proposed selftest has been tested out on Aarch32, Aarch64 and x86_64.
Dev Jain (2): selftests: Rename sigaltstack to generic signal selftests: Add a test mangling with uc_sigmask
tools/testing/selftests/Makefile | 2 +- .../{sigaltstack => signal}/.gitignore | 3 +- .../{sigaltstack => signal}/Makefile | 3 +- .../current_stack_pointer.h | 0 .../selftests/signal/mangle_uc_sigmask.c | 141 ++++++++++++++++++ .../sas.c => signal/sigaltstack.c} | 0 6 files changed, 146 insertions(+), 3 deletions(-) rename tools/testing/selftests/{sigaltstack => signal}/.gitignore (57%) rename tools/testing/selftests/{sigaltstack => signal}/Makefile (53%) rename tools/testing/selftests/{sigaltstack => signal}/current_stack_pointer.h (100%) create mode 100644 tools/testing/selftests/signal/mangle_uc_sigmask.c rename tools/testing/selftests/{sigaltstack/sas.c => signal/sigaltstack.c} (100%)
Rename sigaltstack to signal, and rename the existing test to sigaltstack.c.
Signed-off-by: Dev Jain dev.jain@arm.com --- tools/testing/selftests/Makefile | 2 +- tools/testing/selftests/{sigaltstack => signal}/.gitignore | 2 +- tools/testing/selftests/{sigaltstack => signal}/Makefile | 2 +- .../selftests/{sigaltstack => signal}/current_stack_pointer.h | 0 .../selftests/{sigaltstack/sas.c => signal/sigaltstack.c} | 0 5 files changed, 3 insertions(+), 3 deletions(-) rename tools/testing/selftests/{sigaltstack => signal}/.gitignore (76%) rename tools/testing/selftests/{sigaltstack => signal}/Makefile (72%) rename tools/testing/selftests/{sigaltstack => signal}/current_stack_pointer.h (100%) rename tools/testing/selftests/{sigaltstack/sas.c => signal/sigaltstack.c} (100%)
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 9039f3709aff..eee1031dc18f 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -85,7 +85,7 @@ TARGETS += rtc TARGETS += rust TARGETS += seccomp TARGETS += sgx -TARGETS += sigaltstack +TARGETS += signal TARGETS += size TARGETS += sparc64 TARGETS += splice diff --git a/tools/testing/selftests/sigaltstack/.gitignore b/tools/testing/selftests/signal/.gitignore similarity index 76% rename from tools/testing/selftests/sigaltstack/.gitignore rename to tools/testing/selftests/signal/.gitignore index 50a19a8888ce..98a7bbc4f325 100644 --- a/tools/testing/selftests/sigaltstack/.gitignore +++ b/tools/testing/selftests/signal/.gitignore @@ -1,2 +1,2 @@ # SPDX-License-Identifier: GPL-2.0-only -sas +sigaltstack diff --git a/tools/testing/selftests/sigaltstack/Makefile b/tools/testing/selftests/signal/Makefile similarity index 72% rename from tools/testing/selftests/sigaltstack/Makefile rename to tools/testing/selftests/signal/Makefile index 3e96d5d47036..dd6be992fd81 100644 --- a/tools/testing/selftests/sigaltstack/Makefile +++ b/tools/testing/selftests/signal/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only CFLAGS = -Wall -TEST_GEN_PROGS = sas +TEST_GEN_PROGS = sigaltstack
include ../lib.mk
diff --git a/tools/testing/selftests/sigaltstack/current_stack_pointer.h b/tools/testing/selftests/signal/current_stack_pointer.h similarity index 100% rename from tools/testing/selftests/sigaltstack/current_stack_pointer.h rename to tools/testing/selftests/signal/current_stack_pointer.h diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/signal/sigaltstack.c similarity index 100% rename from tools/testing/selftests/sigaltstack/sas.c rename to tools/testing/selftests/signal/sigaltstack.c
On Fri, Jun 07, 2024 at 05:53:18PM +0530, Dev Jain wrote:
Rename sigaltstack to signal, and rename the existing test to sigaltstack.c.
I think this is reasonable if we're going to add more generic signal tests - sigaltstack is a fairly small bit of functionality and having it covered as part of a broader signal suite and the overhead of setting up the suite separately is probably not worth it.
Reviewed-by: Mark Brown broonie@kernel.org
This test asserts the relation between blocked signal, delivered signal, and ucontext. The ucontext is mangled with, by adding a signal mask to it; on return from the handler, the thread must block the corresponding signal.
Signed-off-by: Dev Jain dev.jain@arm.com --- tools/testing/selftests/signal/.gitignore | 1 + tools/testing/selftests/signal/Makefile | 1 + .../selftests/signal/mangle_uc_sigmask.c | 141 ++++++++++++++++++ 3 files changed, 143 insertions(+) create mode 100644 tools/testing/selftests/signal/mangle_uc_sigmask.c
diff --git a/tools/testing/selftests/signal/.gitignore b/tools/testing/selftests/signal/.gitignore index 98a7bbc4f325..ccba56247942 100644 --- a/tools/testing/selftests/signal/.gitignore +++ b/tools/testing/selftests/signal/.gitignore @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0-only sigaltstack +mangle_uc_sigmask diff --git a/tools/testing/selftests/signal/Makefile b/tools/testing/selftests/signal/Makefile index dd6be992fd81..4ebf6ac2e303 100644 --- a/tools/testing/selftests/signal/Makefile +++ b/tools/testing/selftests/signal/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only CFLAGS = -Wall TEST_GEN_PROGS = sigaltstack +TEST_GEN_PROGS += mangle_uc_sigmask
include ../lib.mk
diff --git a/tools/testing/selftests/signal/mangle_uc_sigmask.c b/tools/testing/selftests/signal/mangle_uc_sigmask.c new file mode 100644 index 000000000000..0803aeb248a0 --- /dev/null +++ b/tools/testing/selftests/signal/mangle_uc_sigmask.c @@ -0,0 +1,141 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 ARM Ltd. + * + * Author: Dev Jain dev.jain@arm.com + * + * Test describing a clear distinction between signal states - delivered and + * blocked, and their relation with ucontext. + */ + +#include <signal.h> +#include <unistd.h> +#include <stdlib.h> +#include <stdio.h> +#include <ucontext.h> +#include <assert.h> + +#include "../kselftest.h" + +void handler_verify_ucontext(int signo, siginfo_t *info, void *uc) +{ + int ret; + + /* Kernel dumps ucontext with USR2 blocked */ + ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR2); + ksft_test_result(ret == 1, "USR2 in ucontext\n"); + + raise(SIGUSR2); +} + +void handler_segv(int signo, siginfo_t *info, void *uc) +{ + /* + * Three cases possible: + * 1. Program already terminated due to segmentation fault. + * 2. SEGV was blocked even after returning from handler_usr. + * 3. SEGV was delivered on returning from handler_usr. + * The last option must happen. + */ + ksft_test_result_pass("SEGV delivered\n"); +} + +static int cnt; + +void handler_usr(int signo, siginfo_t *info, void *uc) +{ + int ret; + + /* + * Break out of infinite recursion caused by raise(SIGUSR1) invoked + * from inside the handler + */ + ++cnt; + if (cnt > 1) + return; + + ksft_print_msg("In handler_usr\n"); + + /* SEGV blocked during handler execution, delivered on return */ + raise(SIGPIPE); + ksft_print_msg("SEGV bypassed successfully\n"); + + /* + * Signal responsible for handler invocation is blocked by default; + * delivered on return, leading to an infinite recursion + */ + raise(SIGUSR1); + ksft_test_result(cnt == 1, + "USR1 is blocked, cannot invoke handler again\n"); + + /* SIGPIPE has been blocked in sa_mask, but ucontext is invariant */ + ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGPIPE); + ksft_test_result(ret == 0, "USR1 not in ucontext\n"); + + /* SIGUSR1 has been blocked, but ucontext is invariant */ + ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR1); + ksft_test_result(ret == 0, "SEGV not in ucontext\n"); + + /* + * Mangle ucontext; this will be copied back into ¤t->blocked + * on return from the handler. + */ + if (sigaddset(&((ucontext_t *)uc)->uc_sigmask, SIGUSR2)) + ksft_exit_fail_perror("Cannot add into uc_sigmask"); +} + +int main(int argc, char *argv[]) +{ + struct sigaction act, act2; + sigset_t *set, *oldset; + + ksft_print_header(); + ksft_set_plan(6); + + act.sa_flags = SA_SIGINFO; + act.sa_sigaction = &handler_usr; + + /* add SEGV to blocked mask */ + if (sigemptyset(&act.sa_mask) || sigaddset(&act.sa_mask, SIGPIPE) + || (sigismember(&act.sa_mask, SIGPIPE) != 1)) + ksft_exit_fail_msg("Cannot add SEGV to blocked mask\n"); + + if (sigaction(SIGUSR1, &act, NULL)) + ksft_exit_fail_perror("Cannot install handler"); + + act2.sa_flags = SA_SIGINFO; + act2.sa_sigaction = &handler_segv; + + if (sigaction(SIGPIPE, &act2, NULL)) + ksft_exit_fail_perror("Cannot install handler"); + + /* invoke handler */ + raise(SIGUSR1); + + /* Mangled ucontext implies USR2 is blocked for current thread */ + raise(SIGUSR2); + ksft_print_msg("USR2 bypassed successfully\n"); + + act.sa_sigaction = &handler_verify_ucontext; + if (sigaction(SIGUSR1, &act, NULL)) + ksft_exit_fail_perror("Cannot install handler"); + + raise(SIGUSR1); + + ksft_print_msg("USR2 still blocked on return from handler\n"); + + /* Confirm USR2 blockage by sigprocmask() too */ + set = malloc(sizeof(sigset_t *)); + oldset = malloc(sizeof(sigset_t *)); + + if (sigemptyset(set)) + ksft_exit_fail_perror("Cannot empty set"); + + if (sigprocmask(SIG_BLOCK, set, oldset)) + ksft_exit_fail_perror("sigprocmask()"); + + ksft_test_result(sigismember(oldset, SIGUSR2) == 1, + "USR2 present in ¤t->blocked\n"); + + ksft_finished(); +}
On Fri, Jun 07, 2024 at 05:53:19PM +0530, Dev Jain wrote:
This test asserts the relation between blocked signal, delivered signal, and ucontext. The ucontext is mangled with, by adding a signal mask to it; on return from the handler, the thread must block the corresponding signal.
@@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0-only sigaltstack +mangle_uc_sigmask
Please keep these build files sorted alphabetically, this reduces spurioius conflicts between serieses.
- Author: Dev Jain dev.jain@arm.com
- Test describing a clear distinction between signal states - delivered and
- blocked, and their relation with ucontext.
This would be clearer if it said more positiviely what the relationship between these things is actually expected to be and how they're tested. Right now it's a bit hard to tell what the test is actually verifying.
+void handler_verify_ucontext(int signo, siginfo_t *info, void *uc) +{
- int ret;
- /* Kernel dumps ucontext with USR2 blocked */
- ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR2);
- ksft_test_result(ret == 1, "USR2 in ucontext\n");
"USR2 blocked in ucontext".
- raise(SIGUSR2);
+}
A comment explaining that we're verifying that the signal is blocked might be good (I think that's what this is doing?). We're also not checking the return value of raise() anywhere in the program, this would be a useful diagnostic.
- /* SEGV blocked during handler execution, delivered on return */
- raise(SIGPIPE);
- ksft_print_msg("SEGV bypassed successfully\n");
SIGPIPE or SIGEGV?
- /* SIGPIPE has been blocked in sa_mask, but ucontext is invariant */
- ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGPIPE);
- ksft_test_result(ret == 0, "USR1 not in ucontext\n");
The relationship between the comment and test are not clear here, nor is that between the sigismembber() call and the test name we print?
- /* SIGUSR1 has been blocked, but ucontext is invariant */
- ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR1);
- ksft_test_result(ret == 0, "SEGV not in ucontext\n");
Similarly here.
- /* add SEGV to blocked mask */
- if (sigemptyset(&act.sa_mask) || sigaddset(&act.sa_mask, SIGPIPE)
|| (sigismember(&act.sa_mask, SIGPIPE) != 1))ksft_exit_fail_msg("Cannot add SEGV to blocked mask\n");
SIGPIPE vs SIGSEGV.
On 6/7/24 18:42, Mark Brown wrote:
On Fri, Jun 07, 2024 at 05:53:19PM +0530, Dev Jain wrote:
This test asserts the relation between blocked signal, delivered signal, and ucontext. The ucontext is mangled with, by adding a signal mask to it; on return from the handler, the thread must block the corresponding signal. @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0-only sigaltstack +mangle_uc_sigmask
Please keep these build files sorted alphabetically, this reduces spurioius conflicts between serieses.
Sure.
- Author: Dev Jain dev.jain@arm.com
- Test describing a clear distinction between signal states - delivered and
- blocked, and their relation with ucontext.
This would be clearer if it said more positiviely what the relationship between these things is actually expected to be and how they're tested. Right now it's a bit hard to tell what the test is actually verifying.
I thought I had described that quite well in the code comments.
Anyways, I shall incorporate some detail into the initial test
description too.
+void handler_verify_ucontext(int signo, siginfo_t *info, void *uc) +{
- int ret;
- /* Kernel dumps ucontext with USR2 blocked */
- ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR2);
- ksft_test_result(ret == 1, "USR2 in ucontext\n");
"USR2 blocked in ucontext".
- raise(SIGUSR2);
+}
A comment explaining that we're verifying that the signal is blocked might be good (I think that's what this is doing?). We're also not checking the return value of raise() anywhere in the program, this would be a useful diagnostic.
Sure.
- /* SEGV blocked during handler execution, delivered on return */
- raise(SIGPIPE);
- ksft_print_msg("SEGV bypassed successfully\n");
SIGPIPE or SIGEGV?
- /* SIGPIPE has been blocked in sa_mask, but ucontext is invariant */
- ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGPIPE);
- ksft_test_result(ret == 0, "USR1 not in ucontext\n");
The relationship between the comment and test are not clear here, nor is that between the sigismembber() call and the test name we print?
- /* SIGUSR1 has been blocked, but ucontext is invariant */
- ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR1);
- ksft_test_result(ret == 0, "SEGV not in ucontext\n");
Similarly here.
- /* add SEGV to blocked mask */
- if (sigemptyset(&act.sa_mask) || sigaddset(&act.sa_mask, SIGPIPE)
|| (sigismember(&act.sa_mask, SIGPIPE) != 1))ksft_exit_fail_msg("Cannot add SEGV to blocked mask\n");SIGPIPE vs SIGSEGV.
Ah sorry, I was testing out something else, and then I
did something and it partially changed it back to SEGV.
I shall revert all mentions of PIPE with SEGV. Please read
all mentions of pipe, or PIPE, as segv and SEGV.
On Fri, Jun 07, 2024 at 06:53:27PM +0530, Dev Jain wrote:
On 6/7/24 18:42, Mark Brown wrote:
On Fri, Jun 07, 2024 at 05:53:19PM +0530, Dev Jain wrote:
- Test describing a clear distinction between signal states - delivered and
- blocked, and their relation with ucontext.
This would be clearer if it said more positiviely what the relationship between these things is actually expected to be and how they're tested. Right now it's a bit hard to tell what the test is actually verifying.
I thought I had described that quite well in the code comments.
Anyways, I shall incorporate some detail into the initial test description too.
If the overview is confusing and people have to read the code to figure out what it means then that's an issue...
On 6/7/24 19:12, Mark Brown wrote:
On Fri, Jun 07, 2024 at 06:53:27PM +0530, Dev Jain wrote:
On 6/7/24 18:42, Mark Brown wrote:
On Fri, Jun 07, 2024 at 05:53:19PM +0530, Dev Jain wrote:
- Test describing a clear distinction between signal states - delivered and
- blocked, and their relation with ucontext.
This would be clearer if it said more positiviely what the relationship between these things is actually expected to be and how they're tested. Right now it's a bit hard to tell what the test is actually verifying.
I thought I had described that quite well in the code comments. Anyways, I shall incorporate some detail into the initial test description too.
If the overview is confusing and people have to read the code to figure out what it means then that's an issue...
You are right.
I shall post a v2 rather quickly, perhaps in 1-2 days;
the SIGPIPE vs SIGSEGV mistake basically renders
this patch useless (although the test would still
pass), and makes the code unnecessarily hard
to review.
linux-kselftest-mirror@lists.linaro.org
-
Dev Jain -
Mark Brown