diff mbox series

[2/2] selftests: Add a test mangling with uc_sigmask

Message ID 20240607122319.768640-3-dev.jain@arm.com (mailing list archive)
State New
Headers show
Series Add test to distinguish between thread's signal mask and ucontext_t | expand

Commit Message

Dev Jain June 7, 2024, 12:23 p.m. UTC 
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

Comments

Mark Brown June 7, 2024, 1:12 p.m. UTC | #1 
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.
Dev Jain June 7, 2024, 1:23 p.m. UTC | #2 
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.
Mark Brown June 7, 2024, 1:42 p.m. UTC | #3 
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...
Dev Jain June 7, 2024, 2:26 p.m. UTC | #4 
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.
diff mbox series

Patch

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 &current->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 &current->blocked\n");
+
+	ksft_finished();
+}