mbox series

[v3,0/5] nolibc signal handling support

Message ID 20230108135904.851762-1-ammar.faizi@intel.com
Headers show
Series nolibc signal handling support | expand

Message

Ammar Faizi Jan. 8, 2023, 1:58 p.m. UTC
From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Hi Willy,

On top of the series titled "nolibc auxiliary vector retrieval support".
The prerequisite patches of this series are in that series.

This is v2 of nolibc signal handling support. It adds signal handling
support to the nolibc subsystem:

1)  Initial implementation of nolibc sigaction(2) function.

    `sigaction()` needs an architecture-dependent "signal trampoline"
    function that invokes __rt_sigreturn syscall to resume the process
    after a signal gets handled.

    The "signal trampoline" function is called `__restore_rt` in this
    implementation. The naming `__restore_rt` is important for GDB. It
    also has to be given a special optimization attribute
    "omit-frame-pointer" to prevent the compiler from creating a stack
    frame that makes the `%rsp` value no longer points to the `struct
    rt_sigframe` that the kernel constructed.


2)  signal(2) function.

    signal() function is the simpler version of sigaction(). Unlike
    sigaction(), which fully controls the struct sigaction, the caller
    only cares about the sa_handler when calling the signal() function.
    signal() internally calls sigaction().


3)  More selftests.

    This series also adds selftests for:
      - fork(2)
      - sigaction(2)
      - signal(2)


Side note for __restore_rt:
This has been tested on x86-64 arch and `__restore_rt` generates the
correct code. The `__restore_rt` codegen correctness on other
architectures need to be evaluated as well. If it can't generate the
correct code, it has to be written in inline Assembly.

The current codegen for __restore_rt looks like this (gcc (Ubuntu 11.3.0-1ubuntu1~22.04) 11.3.0):

  00000000004038e3 <__restore_rt>:
    4038e3:  endbr64 
    4038e7:  mov    $0xf,%eax
    4038ec:  syscall

## Changes since v2:
  - Fix unintentionally squashed patch. The signal() selftest patch
    was squashed into the sigaction selftest patch.

## Changes since RFC v1:
  - Separate getpagesize() series.
  - Write __restore_rt function in C instead of in inline Assembly.


Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (5):
  nolibc/sys: Implement `sigaction(2)` function
  nolibc/sys: Implement `signal(2)` function
  selftests/nolibc: Add `fork(2)` selftest
  selftests/nolibc: Add `sigaction(2)` selftest
  selftests/nolibc: Add `signal(2)` selftest

 tools/include/nolibc/sys.h                   |  97 +++++++++++
 tools/testing/selftests/nolibc/nolibc-test.c | 172 +++++++++++++++++++
 2 files changed, 269 insertions(+)


base-commit: b6887ec8b0b0c78db414b78e329bf2ce234dedd5
prerequisite-patch-id: 8dd0ca8ecee1732d8f5c0b233f8231dda6ab0d22
prerequisite-patch-id: ff4c08615ebbdc1a04ce39f39f99387ee46b2b31
prerequisite-patch-id: af837a829263849331eb6d73701afd7903146055

Comments

Ammar Faizi Jan. 15, 2023, 4:01 p.m. UTC | #1
On Sun, Jan 08, 2023 at 07:49:30PM +0100, Willy Tarreau wrote:
> On Mon, Jan 09, 2023 at 01:31:17AM +0700, Ammar Faizi wrote:
> > I'll be pondering this code this week (to follow what actually the
> > rt_sigaction wants on i386 and arm):
> > 
> >   https://github.com/torvalds/linux/blob/v6.2-rc3/kernel/signal.c#L4404-L4434
> 
> Seems like it could simply be a matter of sigsetsize, which is the
> first one returning -EINVAL.
> 
> > Hopefully, I can get it sorted before the weekend.
> 
> OK!

I couldn't dedicate much time to this, but I looked into it, and here's
my report on the progress. I didn't manage to find a proper solution to
this. But yes, you're right. It's a matter of 'sizeof(sigset_t)'.

So here is my observation. Currently, nolibc's sys.h includes this:

    #include <asm/signal.h>

The definition of 'sigset_t' in that header is: 

    typedef unsigned long sigset_t;

On i386, 'sizeof(unsigned long)' is 4, but on x86-64 it's 8.

That is not the 'sigset_t' that the kernel wants. The kernel wants the
'sigset_t' that is in <asm-generic/signal.h>:

    #define _NSIG       64
    #define _NSIG_BPW   __BITS_PER_LONG      // this 64 on x86-64, but 32 on i386.
    #define _NSIG_WORDS (_NSIG / _NSIG_BPW)

    typedef struct {
        unsigned long sig[_NSIG_WORDS];
    } sigset_t;

The above struct is always 8 bytes in size. In other words:

    _NSIG_WORDS == 2 on i386
    _NSIG_WORDS == 1 on x86-64
    sizeof(unsigned long) == 4 on i386
    sizeof(unsigned long) == 8 on x86-64

Therefore, sizeof(unsigned long [_NSIG_WORDS]) is always 8 on both
architectures. That's the correct size.

I tried to #include <asm-generic/signal.h> but it conflicts with the
other 'sigset_t' definition. So I can't do that.

Why are there two different definitions of 'sigset_t'? I don't know.

I probably should read the story behind this syscall to get it
implemented right. Let me ponder this again on Monday. But at least I
tell what I have found so people can give some comments on it...
Alviro Iskandar Setiawan Jan. 15, 2023, 5:06 p.m. UTC | #2
On Sun, Jan 15, 2023 at 11:01 PM Ammar Faizi wrote:
> That is not the 'sigset_t' that the kernel wants. The kernel wants the
> 'sigset_t' that is in <asm-generic/signal.h>:
>
>     #define _NSIG       64
>     #define _NSIG_BPW   __BITS_PER_LONG      // this 64 on x86-64, but 32 on i386.
>     #define _NSIG_WORDS (_NSIG / _NSIG_BPW)
>
>     typedef struct {
>         unsigned long sig[_NSIG_WORDS];
>     } sigset_t;
>
> The above struct is always 8 bytes in size. In other words:
>
>     _NSIG_WORDS == 2 on i386
>     _NSIG_WORDS == 1 on x86-64
>     sizeof(unsigned long) == 4 on i386
>     sizeof(unsigned long) == 8 on x86-64
>
> Therefore, sizeof(unsigned long [_NSIG_WORDS]) is always 8 on both
> architectures. That's the correct size.
>
> I tried to #include <asm-generic/signal.h> but it conflicts with the
> other 'sigset_t' definition. So I can't do that.

Read the glibc sigaction implementation, it has different struct
sigaction definitions for user and kernel too.

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/libc_sigaction.c;h=3cbf241a5fa28296c910fa40a41b09d2b6113b05;hb=7e31d166510ac4adbf53d5e8144c709a37dd8c7a

Since nolibc compiles everything in a single translation unit, you
can't have a different sigset_t definition. You need to copy the
definition to nolibc header and change the type name to something else
for internal use only.

> Why are there two different definitions of 'sigset_t'? I don't know.
>
> I probably should read the story behind this syscall to get it
> implemented right. Let me ponder this again on Monday. But at least I
> tell what I have found so people can give some comments on it...

`man 2 rt_sigaction` tells the story. Here is the bedtime story, have
a nice dream :-)

The original Linux system call was named sigaction(). However, with
the addition of real-time signals in Linux 2.2, the fixed-size, 32-bit
sigset_t type supported by that system call was no longer fit for
purpose.
Consequently, a new system call, rt_sigaction(), was added to support
an enlarged sigset_t type. The new system call takes a fourth
argument, size_t sigsetsize, which specifies the size in bytes of the
signal sets
in act.sa_mask and oldact.sa_mask. This argument is currently required
to have the value sizeof(sigset_t) (or the error EINVAL results). The
glibc sigaction() wrapper function hides these details from us,
transparā€
ently calling rt_sigaction() when the kernel provides it.