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.
Changes in v2:
- Replace all occurrences of SIGPIPE with SIGSEGV
- Fixed a mismatch between code comment and ksft log
- Add a testcase: Raise the same signal again; it must not be queued
- Remove unneeded <assert.h>, <unistd.h>
- Give a detailed test description in the comments; also describe the
exact meaning of delivered and blocked
- Handle errors for all libc functions/syscalls
- Mention tests in Makefile and .gitignore in alphabetical order
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 | 194 ++++++++++++++++++
.../sas.c => signal/sigaltstack.c} | 0
6 files changed, 199 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%)
--
2.34.1
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.
In the test description, I have also described what it exactly means for
a signal to be delivered or blocked, for ease of clarity.
Signed-off-by: Dev Jain <[email protected]>
---
tools/testing/selftests/signal/.gitignore | 1 +
tools/testing/selftests/signal/Makefile | 3 +-
.../selftests/signal/mangle_uc_sigmask.c | 194 ++++++++++++++++++
3 files changed, 197 insertions(+), 1 deletion(-)
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..397fef11c89f 100644
--- a/tools/testing/selftests/signal/.gitignore
+++ b/tools/testing/selftests/signal/.gitignore
@@ -1,2 +1,3 @@
# SPDX-License-Identifier: GPL-2.0-only
+mangle_uc_sigmask
sigaltstack
diff --git a/tools/testing/selftests/signal/Makefile b/tools/testing/selftests/signal/Makefile
index dd6be992fd81..735387a53114 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
+TEST_GEN_PROGS += sigaltstack
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..9d4644106465
--- /dev/null
+++ b/tools/testing/selftests/signal/mangle_uc_sigmask.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 ARM Ltd.
+ *
+ * Author: Dev Jain <[email protected]>
+ *
+ * Test describing a clear distinction between signal states - delivered and
+ * blocked, and their relation with ucontext.
+ *
+ * A signal is said to be delivered, when the program takes action on the
+ * signal: such action may involve termination of the process, ignoring the
+ * signal, terminating with core dump, stopping the process, or continuing the
+ * process if it was currently stopped. A signal is said to be blocked when the
+ * program refuses to take any of the above actions; note that, this is not the
+ * same as ignoring the signal. At a later time, the program may unblock the
+ * signal and then it will have to take one of the five actions
+ * described above.
+ *
+ * We test the following functionalities of the kernel:
+ *
+ * ucontext_t describes the current state of the thread; this implies that, in
+ * case of registering a handler and catching the corresponding signal, that
+ * state is before what was jumping into the handler.
+ *
+ * The thread's mask of blocked signals can be permanently changed, i.e, not
+ * just during the execution of the handler, by mangling with uc_sigmask
+ * from inside the handler.
+ *
+ * Assume that we block the set of signals, S1, by sigaction(), and say, the
+ * signal for which the handler was installed, is S2. When S2 is sent to the
+ * program, it will be considered "delivered", since we will act on the
+ * signal and jump to the handler. Any instances of S1 or S2 raised, while the
+ * program is executing inside the handler, will be blocked; they will be
+ * delivered immediately upon termination of the handler.
+ *
+ * For standard signals (also see real-time signals in the man page), multiple
+ * blocked instances of the same signal are not queued; such a signal will
+ * be delivered just once.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <ucontext.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 blocked in ucontext\n");
+
+ /*
+ * USR2 is blocked; can be delivered neither here, nor after
+ * exit from handler
+ */
+ if (raise(SIGUSR2))
+ ksft_exit_fail_perror("raise");
+}
+
+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 */
+ if (raise(SIGSEGV))
+ ksft_exit_fail_perror("raise");
+
+ ksft_print_msg("SEGV bypassed successfully\n");
+
+ /*
+ * Signal responsible for handler invocation is blocked by default;
+ * delivered on return, leading to recursion
+ */
+ if (raise(SIGUSR1))
+ ksft_exit_fail_perror("raise");
+
+ ksft_test_result(cnt == 1,
+ "USR1 is blocked, cannot invoke handler right now\n");
+
+ /* Raise USR1 again; only one instance must be delivered upon exit */
+ if (raise(SIGUSR1))
+ ksft_exit_fail_perror("raise");
+
+ /* SEGV has been blocked in sa_mask, but ucontext is invariant */
+ ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGSEGV);
+ ksft_test_result(ret == 0, "SEGV not blocked in ucontext\n");
+
+ /* USR1 has been blocked, but ucontext is invariant */
+ ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR1);
+ ksft_test_result(ret == 0, "USR1 not blocked 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("sigaddset");
+}
+
+int main(int argc, char *argv[])
+{
+ struct sigaction act, act2;
+ sigset_t *set, *oldset;
+
+ ksft_print_header();
+ ksft_set_plan(7);
+
+ act.sa_flags = SA_SIGINFO;
+ act.sa_sigaction = &handler_usr;
+
+ /* Add SEGV to blocked mask */
+ if (sigemptyset(&act.sa_mask) || sigaddset(&act.sa_mask, SIGSEGV)
+ || (sigismember(&act.sa_mask, SIGSEGV) != 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(SIGSEGV, &act2, NULL))
+ ksft_exit_fail_perror("Cannot install handler");
+
+ /* Invoke handler */
+ if (raise(SIGUSR1))
+ ksft_exit_fail_perror("raise");
+
+ /* USR1 must not be queued */
+ ksft_test_result(cnt == 2, "handler invoked only twice\n");
+
+ /* Mangled ucontext implies USR2 is blocked for current thread */
+ if (raise(SIGUSR2))
+ ksft_exit_fail_perror("raise");
+
+ 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");
+
+ if (raise(SIGUSR1))
+ ksft_exit_fail_perror("raise");
+
+ ksft_print_msg("USR2 still blocked on return from handler\n");
+
+ /* Confirm USR2 blockage by sigprocmask() too */
+ set = malloc(sizeof(sigset_t *));
+ if (!set)
+ ksft_exit_fail_perror("malloc");
+
+ oldset = malloc(sizeof(sigset_t *));
+ if (!oldset)
+ ksft_exit_fail_perror("malloc");
+
+ if (sigemptyset(set))
+ ksft_exit_fail_perror("sigemptyset");
+
+ 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();
+}
--
2.34.1
Rename sigaltstack to signal, and rename the existing test to
sigaltstack.c.
Signed-off-by: Dev Jain <[email protected]>
---
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
--
2.34.1
On Tue, Jun 11, 2024 at 01:26:49PM +0530, Dev Jain wrote:
> Rename sigaltstack to signal, and rename the existing test to
> sigaltstack.c.
>
> Signed-off-by: Dev Jain <[email protected]>
> ---
If people review patches and give a tag for them you should carry the
tag forward to avoid having to duplicate the review. From v1:
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 <[email protected]>
On 6/11/24 16:24, Mark Brown wrote:
> On Tue, Jun 11, 2024 at 01:26:49PM +0530, Dev Jain wrote:
>> Rename sigaltstack to signal, and rename the existing test to
>> sigaltstack.c.
>>
>> Signed-off-by: Dev Jain <[email protected]>
>> ---
> If people review patches and give a tag for them you should carry the
> tag forward to avoid having to duplicate the review. From v1:
>
> 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 <[email protected]>
Ah yes, I did know it, I forgot. Shall take care.
On Tue, Jun 11, 2024 at 01:26:50PM +0530, Dev Jain wrote:
> + * A signal is said to be delivered, when the program takes action on the
> + * signal: such action may involve termination of the process, ignoring the
> + * signal, terminating with core dump, stopping the process, or continuing the
> + * process if it was currently stopped. A signal is said to be blocked when the
> + * program refuses to take any of the above actions; note that, this is not the
> + * same as ignoring the signal. At a later time, the program may unblock the
> + * signal and then it will have to take one of the five actions
> + * described above.
I'm not sure that's what my understanding of a blocked signal is, I
would interpret "blocked" as a signal being masked (this usage can be
seen in for example sigaction(2)). I'd also interpret delivery of the
signal as happening when the signal handler is invoked rather than
something that the handler has control over (the comment later on says
that so I think it's just an issue here). Perhaps I'm confused about
terminology though, this is just usage I've picked up and ICBW.
> + * For standard signals (also see real-time signals in the man page), multiple
> + * blocked instances of the same signal are not queued; such a signal will
> + * be delivered just once.
See also SA_NODEFER.
> + /* SEGV has been blocked in sa_mask, but ucontext is invariant */
> + ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGSEGV);
> + ksft_test_result(ret == 0, "SEGV not blocked in ucontext\n");
> +
> + /* USR1 has been blocked, but ucontext is invariant */
> + ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR1);
> + ksft_test_result(ret == 0, "USR1 not blocked in ucontext\n");
We're not manipulating the masks outside of main() so it's a bit unclear
what the mention of ucontext being invariant is all about here?
> + /* Mangled ucontext implies USR2 is blocked for current thread */
> + if (raise(SIGUSR2))
> + ksft_exit_fail_perror("raise");
> +
> + 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");
> +
> + if (raise(SIGUSR1))
> + ksft_exit_fail_perror("raise");
> +
> + ksft_print_msg("USR2 still blocked on return from handler\n");
But we just raised SIGUSR1 rather than SIGUSR2? If nothing else this
bit is a little unclear.
On 6/11/24 16:55, Mark Brown wrote:
> On Tue, Jun 11, 2024 at 01:26:50PM +0530, Dev Jain wrote:
>
>> + * A signal is said to be delivered, when the program takes action on the
>> + * signal: such action may involve termination of the process, ignoring the
>> + * signal, terminating with core dump, stopping the process, or continuing the
>> + * process if it was currently stopped. A signal is said to be blocked when the
>> + * program refuses to take any of the above actions; note that, this is not the
>> + * same as ignoring the signal. At a later time, the program may unblock the
>> + * signal and then it will have to take one of the five actions
>> + * described above.
> I'm not sure that's what my understanding of a blocked signal is, I
> would interpret "blocked" as a signal being masked (this usage can be
> seen in for example sigaction(2)). I'd also interpret delivery of the
> signal as happening when the signal handler is invoked rather than
> something that the handler has control over (the comment later on says
> that so I think it's just an issue here). Perhaps I'm confused about
> terminology though, this is just usage I've picked up and ICBW.
Isn't "signal being masked" equivalent to what I wrote...
man signal(7): Under "Signal mask and pending signals":-
"A signal may be blocked, which means that it will not be delivered
until it is later unblocked."
Under "Signal dispositions":-
"Each signal has a current disposition, which determines how the
process behaves when it is delivered the signal."
The above must imply that, the delivery of a signal implies a signal
disposition coming into picture; so in case of blocked signal, the
following should happen:
Set disposition (default, ignore, or jump to handler) -> block SIG_x using,
say, sigprocmask() -> raise(SIG_x) -> nothing happens, do normal work ->
unblock SIG_x by sigprocmask() -> immediately act on disposition, since the
signal will be delivered.
When I wrote "such action may involve termination of the process..." I should
have also included "or jump to a signal handler".
"The comment later on says that", which comment and what does it say,
sorry didn't get you.
>
>> + * For standard signals (also see real-time signals in the man page), multiple
>> + * blocked instances of the same signal are not queued; such a signal will
>> + * be delivered just once.
> See also SA_NODEFER.
Yes, thanks for the note, but do need to include it in the
comments? This is a specific setting...
>
>> + /* SEGV has been blocked in sa_mask, but ucontext is invariant */
>> + ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGSEGV);
>> + ksft_test_result(ret == 0, "SEGV not blocked in ucontext\n");
>> +
>> + /* USR1 has been blocked, but ucontext is invariant */
>> + ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGUSR1);
>> + ksft_test_result(ret == 0, "USR1 not blocked in ucontext\n");
> We're not manipulating the masks outside of main() so it's a bit unclear
> what the mention of ucontext being invariant is all about here?
This is the point I raised in the cover letter and in this program: the mask
stores the set of blocked signals. What should happen when I block signals
using sigaction()? According to the man pages, one could easily come to
an erroneous conclusion that these signals will also be present as blocked
in ucontext. I am making a point that, SEGV and USR1 have been blocked,
but they have not been added into ucontext, i.e ucontext is invariant w.r.t
to before and in the handler.
>
>> + /* Mangled ucontext implies USR2 is blocked for current thread */
>> + if (raise(SIGUSR2))
>> + ksft_exit_fail_perror("raise");
>> +
>> + 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");
>> +
>> + if (raise(SIGUSR1))
>> + ksft_exit_fail_perror("raise");
>> +
>> + ksft_print_msg("USR2 still blocked on return from handler\n");
> But we just raised SIGUSR1 rather than SIGUSR2? If nothing else this
> bit is a little unclear.
Before raise(SIGUSR1), we register a handler for it: handler_verify_ucontext.
So, we jump there, we verify that USR2 is present in ucontext (since we mangled
with ucontext before), then we raise(SIGUSR2): the program must not terminate
since USR2 is blocked in ¤t->blocked. This is described by ksft_print_msg().
On Wed, Jun 12, 2024 at 10:14:01AM +0530, Dev Jain wrote:
> On 6/11/24 16:55, Mark Brown wrote:
> > On Tue, Jun 11, 2024 at 01:26:50PM +0530, Dev Jain wrote:
> > > + * A signal is said to be delivered, when the program takes action on the
> > > + * signal: such action may involve termination of the process, ignoring the
> > > + * signal, terminating with core dump, stopping the process, or continuing the
> > > + * process if it was currently stopped. A signal is said to be blocked when the
> > > + * program refuses to take any of the above actions; note that, this is not the
> > > + * same as ignoring the signal. At a later time, the program may unblock the
> > > + * signal and then it will have to take one of the five actions
> > > + * described above.
> > I'm not sure that's what my understanding of a blocked signal is, I
> > would interpret "blocked" as a signal being masked (this usage can be
> > seen in for example sigaction(2)). I'd also interpret delivery of the
> > signal as happening when the signal handler is invoked rather than
> > something that the handler has control over (the comment later on says
> > that so I think it's just an issue here). Perhaps I'm confused about
> > terminology though, this is just usage I've picked up and ICBW.
> Isn't "signal being masked" equivalent to what I wrote...
> man signal(7): Under "Signal mask and pending signals":-
> "A signal may be blocked, which means that it will not be delivered
> until it is later unblocked."
> Under "Signal dispositions":-
> "Each signal has a current disposition, which determines how the
> process behaves when it is delivered the signal."
The point is that the delivery and blocking are done prior to the
process getting involved in the handling of the signal, the delivery
happens when the signal handler is invoked. The program requests
delivery or blocking but it doesn't actually do the delivery or blocking
itself.
> "The comment later on says that", which comment and what does it say,
> sorry didn't get you.
That signals are blocked before the process sees them.
> > > + * For standard signals (also see real-time signals in the man page), multiple
> > > + * blocked instances of the same signal are not queued; such a signal will
> > > + * be delivered just once.
> > See also SA_NODEFER.
> Yes, thanks for the note, but do need to include it in the
> comments? This is a specific setting...
TBH I'm not sure what you mean there by real time signals, I can't see
a reference to real time in the copies of signal(2), signal(7) or
sigaction(2) on my system. I suspect SA_NODEFER is the actual thing
here.
> > > + /* SEGV has been blocked in sa_mask, but ucontext is invariant */
> > > + ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGSEGV);
> > > + ksft_test_result(ret == 0, "SEGV not blocked in ucontext\n");
> > We're not manipulating the masks outside of main() so it's a bit unclear
> > what the mention of ucontext being invariant is all about here?
> This is the point I raised in the cover letter and in this program: the mask
> stores the set of blocked signals. What should happen when I block signals
> using sigaction()? According to the man pages, one could easily come to
> an erroneous conclusion that these signals will also be present as blocked
> in ucontext. I am making a point that, SEGV and USR1 have been blocked,
> but they have not been added into ucontext, i.e ucontext is invariant w.r.t
> to before and in the handler.
I still don't follow what the above means. When you say "invariant" you
don't specify with respect to what, and it's not clear to me why the
saved context in ucontext would have changed without the handler writing
to it. For clarity I think this needs to say what the ucontext is
expected to be the same as/different to.
The general flow with signals is that the context at the time the signal
is delivered is saved to the context structure, then the signal handler
context is set up and the signal handler invoked. There are a number of
ways in which the signal handler context may differ from the context
that was interrupted, additional signals being masked is one of those.
On return from the signal handler the context is then restored from
memory and we restart from that context, potentially including
modifications made during handling. This means that the state in the
signal handler may be different to the state in the context that was
preempted by it.
> > > + act.sa_sigaction = &handler_verify_ucontext;
> > > + if (sigaction(SIGUSR1, &act, NULL))
> > > + ksft_exit_fail_perror("Cannot install handler");
> > > +
> > > + if (raise(SIGUSR1))
> > > + ksft_exit_fail_perror("raise");
> > > +
> > > + ksft_print_msg("USR2 still blocked on return from handler\n");
> > But we just raised SIGUSR1 rather than SIGUSR2? If nothing else this
> > bit is a little unclear.
> Before raise(SIGUSR1), we register a handler for it: handler_verify_ucontext.
> So, we jump there, we verify that USR2 is present in ucontext (since we mangled
> with ucontext before), then we raise(SIGUSR2): the program must not terminate
> since USR2 is blocked in ¤t->blocked. This is described by ksft_print_msg().
Like I say I think this needs a comment, it's not obvious from the
immediate code what the USR1 handler is doing and we're not doing
anything in this context to verify anything about USR2 so it looks like
a missed search/replace.
On 6/12/24 18:46, Mark Brown wrote:
> On Wed, Jun 12, 2024 at 10:14:01AM +0530, Dev Jain wrote:
>> On 6/11/24 16:55, Mark Brown wrote:
>>> On Tue, Jun 11, 2024 at 01:26:50PM +0530, Dev Jain wrote:
>>>> + * A signal is said to be delivered, when the program takes action on the
>>>> + * signal: such action may involve termination of the process, ignoring the
>>>> + * signal, terminating with core dump, stopping the process, or continuing the
>>>> + * process if it was currently stopped. A signal is said to be blocked when the
>>>> + * program refuses to take any of the above actions; note that, this is not the
>>>> + * same as ignoring the signal. At a later time, the program may unblock the
>>>> + * signal and then it will have to take one of the five actions
>>>> + * described above.
>>> I'm not sure that's what my understanding of a blocked signal is, I
>>> would interpret "blocked" as a signal being masked (this usage can be
>>> seen in for example sigaction(2)). I'd also interpret delivery of the
>>> signal as happening when the signal handler is invoked rather than
>>> something that the handler has control over (the comment later on says
>>> that so I think it's just an issue here). Perhaps I'm confused about
>>> terminology though, this is just usage I've picked up and ICBW.
>> Isn't "signal being masked" equivalent to what I wrote...
>> man signal(7): Under "Signal mask and pending signals":-
>> "A signal may be blocked, which means that it will not be delivered
>> until it is later unblocked."
>> Under "Signal dispositions":-
>> "Each signal has a current disposition, which determines how the
>> process behaves when it is delivered the signal."
> The point is that the delivery and blocking are done prior to the
> process getting involved in the handling of the signal, the delivery
> happens when the signal handler is invoked. The program requests
> delivery or blocking but it doesn't actually do the delivery or blocking
> itself.
I guess we agree on the same thing; so, how about I rephrase the delivery
and blocking code comments this way:
"A process can request blocking of a signal by masking it into its set of
blocked signals; such a signal, when sent to the process by the kernel, will
get blocked by the process and it may later unblock it and take an action.
At that point, the signal will be "delivered".
A signal sent by the kernel to the process, is said to be delivered to the
process when the process takes an action upon receipt of the signal: such
action may include termination, or jumping to a signal handler."
>
>> "The comment later on says that", which comment and what does it say,
>> sorry didn't get you.
> That signals are blocked before the process sees them.
>
>>>> + * For standard signals (also see real-time signals in the man page), multiple
>>>> + * blocked instances of the same signal are not queued; such a signal will
>>>> + * be delivered just once.
>>> See also SA_NODEFER.
>> Yes, thanks for the note, but do need to include it in the
>> comments? This is a specific setting...
> TBH I'm not sure what you mean there by real time signals, I can't see
> a reference to real time in the copies of signal(2), signal(7) or
> sigaction(2) on my system. I suspect SA_NODEFER is the actual thing
> here.
Real-time signals get a mention on signal(7), under the heading
"Real-time signals":
"Multiple instances of real-time signals can be queued. By
contrast, if multiple instances of a standard signal are
delivered while that signal is currently blocked, then only
one instance is queued."
I guess SA_NODEFER has no relation with queuing; it is used to not
block the signal for which the handler was installed.
>
>>>> + /* SEGV has been blocked in sa_mask, but ucontext is invariant */
>>>> + ret = sigismember(&(((ucontext_t *)uc)->uc_sigmask), SIGSEGV);
>>>> + ksft_test_result(ret == 0, "SEGV not blocked in ucontext\n");
>>> We're not manipulating the masks outside of main() so it's a bit unclear
>>> what the mention of ucontext being invariant is all about here?
>> This is the point I raised in the cover letter and in this program: the mask
>> stores the set of blocked signals. What should happen when I block signals
>> using sigaction()? According to the man pages, one could easily come to
>> an erroneous conclusion that these signals will also be present as blocked
>> in ucontext. I am making a point that, SEGV and USR1 have been blocked,
>> but they have not been added into ucontext, i.e ucontext is invariant w.r.t
>> to before and in the handler.
> I still don't follow what the above means. When you say "invariant" you
> don't specify with respect to what, and it's not clear to me why the
> saved context in ucontext would have changed without the handler writing
> to it. For clarity I think this needs to say what the ucontext is
> expected to be the same as/different to.
The ucontext at this stage is supposed to be empty, I guess I'll replace
the word "invariant" then.
"it's not clear to me why the saved context in ucontext would have changed
without the handler writing to it" - by invariant I meant, the set of blocked
signals before invocation of handler is exactly the set of signals blocked in
ucontext, which, in this case, is the empty set. I'll just write that ucontext
is empty.
>
> The general flow with signals is that the context at the time the signal
> is delivered is saved to the context structure, then the signal handler
> context is set up and the signal handler invoked. There are a number of
> ways in which the signal handler context may differ from the context
> that was interrupted, additional signals being masked is one of those.
> On return from the signal handler the context is then restored from
> memory and we restart from that context, potentially including
> modifications made during handling. This means that the state in the
> signal handler may be different to the state in the context that was
> preempted by it.
>
>>>> + act.sa_sigaction = &handler_verify_ucontext;
>>>> + if (sigaction(SIGUSR1, &act, NULL))
>>>> + ksft_exit_fail_perror("Cannot install handler");
>>>> +
>>>> + if (raise(SIGUSR1))
>>>> + ksft_exit_fail_perror("raise");
>>>> +
>>>> + ksft_print_msg("USR2 still blocked on return from handler\n");
>>> But we just raised SIGUSR1 rather than SIGUSR2? If nothing else this
>>> bit is a little unclear.
>> Before raise(SIGUSR1), we register a handler for it: handler_verify_ucontext.
>> So, we jump there, we verify that USR2 is present in ucontext (since we mangled
>> with ucontext before), then we raise(SIGUSR2): the program must not terminate
>> since USR2 is blocked in ¤t->blocked. This is described by ksft_print_msg().
> Like I say I think this needs a comment, it's not obvious from the
> immediate code what the USR1 handler is doing and we're not doing
> anything in this context to verify anything about USR2 so it looks like
> a missed search/replace.
Okay, I'll add a comment.
On Thu, Jun 13, 2024 at 10:21:39AM +0530, Dev Jain wrote:
> I guess we agree on the same thing; so, how about I rephrase the delivery
> and blocking code comments this way:
> "A process can request blocking of a signal by masking it into its set of
> blocked signals; such a signal, when sent to the process by the kernel, will
> get blocked by the process and it may later unblock it and take an action.
> At that point, the signal will be "delivered".
Yes.
> A signal sent by the kernel to the process, is said to be delivered to the
> process when the process takes an action upon receipt of the signal: such
> action may include termination, or jumping to a signal handler."
I'd just drop this last paragraph.
> > TBH I'm not sure what you mean there by real time signals, I can't see
> > a reference to real time in the copies of signal(2), signal(7) or
> > sigaction(2) on my system. I suspect SA_NODEFER is the actual thing
> > here.
> Real-time signals get a mention on signal(7), under the heading
> "Real-time signals":
Ah, it's got a - in there so it doesn't show up in searches.
> > I still don't follow what the above means. When you say "invariant" you
> > don't specify with respect to what, and it's not clear to me why the
> > saved context in ucontext would have changed without the handler writing
> > to it. For clarity I think this needs to say what the ucontext is
> > expected to be the same as/different to.
> The ucontext at this stage is supposed to be empty, I guess I'll replace
> the word "invariant" then.
> "it's not clear to me why the saved context in ucontext would have changed
> without the handler writing to it" - by invariant I meant, the set of blocked
> signals before invocation of handler is exactly the set of signals blocked in
> ucontext, which, in this case, is the empty set. I'll just write that ucontext
> is empty.
Yes, or like I say in general it's the the interrupted context (there's
other parts of the signal frame which are changed).