Re: [PATCH v4 0/2] Add test to distinguish between thread's signal mask and ucontext_t

[Dev Jain <dev.jain@arm.com>]

On 6/27/24 09:22, Dev Jain wrote:
> 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
> &current->blocked, is passed to setup_rt_frame() -> setup_sigframe() ->
> __copy_to_user(). Hence, &current->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 &current->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.
>
> v3->v4:
>   - Allocate sigsets as automatic variables to avoid malloc()
>
> v2->v3:
>   - ucontext describes current state -> ucontext describes interrupted context
>   - Add a comment for blockage of USR2 even after return from handler
>   - Describe blockage of signals in a better way
>
> v1->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
>
> v1:
>   - https://lore.kernel.org/all/20240607122319.768640-1-dev.jain@arm.com/
>
> 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      | 186 ++++++++++++++++++
>   .../sas.c => signal/sigaltstack.c}            |   0
>   6 files changed, 191 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%)

If everything is fine, can this patchset be pulled? Thanks.

>