mbox

[RFC,v1,0/8] nolibc signal handling support

Message ID 20221222035134.3467659-1-ammar.faizi@intel.com (mailing list archive)
State New
Headers show

Pull-request

https://github.com/ammarfaizi2/linux-block testing/rfc.v1.2022-12-22.nolibc

Message

Ammar Faizi Dec. 22, 2022, 3:51 a.m. UTC
From: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Hi,

This series adds signal handling support to the nolibc subsystem.

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

    Currently, this implementation is only available on the x86-64 arch.

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

    On Linux x86-64, the "signal trampoline" function has to be written in
    inline Assembly to prevent the compiler from controlling the %rsp
    (e.g., with -fno-omit-frame-pointer, every function has a pushq
    %rbp that makes the %rsp no longer point to struct rt_sigframe).

    The "signal trampoline" function is called __arch_restore_rt in this
    implementation.

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(). This implementation is
    currently only available on the x86-64 arch. When the sigaction()
    function support is expanded to other architectures, this function
    will automatically support those architectures. It's basically just
    a sigaction() wrapper.

3)  Extra nolibc updates.

    Apart from the signal handling support. This series also contains
    nolibc updates, they are:

      - getpagesize() support.
      - CFLAGS update.
      - fork(2) selftest.
      - sigaction(2) selftest.
      - signal(2) selftest.
      - getpagesize(2) selftest.

There 8 patches in this series. It has been tested on Linux x86-64 arch
and all tests OK.

    $ sudo ./nolibc-test
    Running test 'syscall'
    ...
    ...
    66 wait_child = -1 ECHILD                [OK]
    67 waitpid_min = -1 ESRCH                [OK]
    68 waitpid_child = -1 ECHILD             [OK]
    69 write_badf = -1 EBADF                 [OK]
    70 write_zero = 0                        [OK]
    Errors during this test: 0


    Running test 'stdlib'
    ...
    ...
    14 memcmp_60_20 = 64                     [OK]
    15 memcmp_20_e0 = -192                   [OK]
    16 memcmp_e0_20 = 192                    [OK]
    17 memcmp_80_e0 = -96                    [OK]
    18 memcmp_e0_80 = 96                     [OK]
    Errors during this test: 0

    Total number of errors: 0
    Exiting with status 0

    $ make run -j8
    Kernel: arch/x86/boot/bzImage is ready  (#3)
    ...
    82 test(s) passed.

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

It's also available in the Git repository.

The following changes since commit caf5c36025ec9395c8d7c78957b016a284812d23:

  srcu: Update comment after the index flip (2022-12-21 09:01:53 -0800)

are available in the Git repository at:

  https://github.com/ammarfaizi2/linux-block testing/rfc.v1.2022-12-22.nolibc

for you to fetch changes up to ac79aca684125907bfbefadfd6c6be0ccdfe8b33:

  selftests/nolibc: Add `getpagesize(2)` selftest (2022-12-22 09:57:31 +0700)

----------------------------------------------------------------
Ammar Faizi (8):
    nolibc/sys: Implement `sigaction(2)` function
    nolibc/sys: Implement `signal(2)` function
    nolibc/sys: Implement `getpagesize(2)` function
    selftests/nolibc: Add `-Wall` and `-Wno-unsed-function` to the CFLAGS
    selftests/nolibc: Add `fork(2)` selftest
    selftests/nolibc: Add `sigaction(2)` selftest
    selftests/nolibc: Add `signal(2)` selftest
    selftests/nolibc: Add `getpagesize(2)` selftest

 tools/include/nolibc/arch-x86_64.h           |  12 +
 tools/include/nolibc/sys.h                   | 224 +++++++++++++++++++
 tools/testing/selftests/nolibc/Makefile      |   2 +-
 tools/testing/selftests/nolibc/nolibc-test.c | 219 +++++++++++++++++-
 4 files changed, 454 insertions(+), 3 deletions(-)


base-commit: caf5c36025ec9395c8d7c78957b016a284812d23

Comments

Willy Tarreau Dec. 22, 2022, 4:34 a.m. UTC | #1
Hi Ammar,

On Thu, Dec 22, 2022 at 10:51:26AM +0700, Ammar Faizi wrote:
> From: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> 
> Hi,
> 
> This series adds signal handling support to the nolibc subsystem.
(...)

Thank you! I'll have a look at this this week-end.
I noticed one thing that we'll need to discuss further:

> 3)  Extra nolibc updates.
> 
>     Apart from the signal handling support. This series also contains
>     nolibc updates, they are:
> 
>       - getpagesize() support.

This one relies on /proc/self/auxv, but we'll quickly run into a
chicken-and-egg situation given that nolibc is used by init programs
that mount /proc. Instead I think that we should modify the _start
code to retrieve the auxv at startup and store it somewhere. This
"somewhere" is not yet defined, but I'm thinking that it could
deserve reserving some room in the stack to store some nolibc-defined
information (possibly even a copy of a pointer to environ and/or errno)
and figure a reliable and simple way to access this. Note that one way
could also be to know that it's after the NULL that follows envp, and
to start from environ. In fact there are plenty of ways and we just
need to decide what's the least ugly. But once we have access to the
auxv from the process, then we could implement a getauxval() function
to retrieve the information you need for getpagesize().

More on this later.

Thanks!
Willy
Ammar Faizi Dec. 22, 2022, 1:46 p.m. UTC | #2
On Thu, 22 Dec 2022 05:34:52 +0100, Willy Tarreau wrote:
> This one relies on /proc/self/auxv, but we'll quickly run into a
> chicken-and-egg situation given that nolibc is used by init programs
> that mount /proc. Instead I think that we should modify the _start
> code to retrieve the auxv at startup and store it somewhere. This
> "somewhere" is not yet defined, but I'm thinking that it could
> deserve reserving some room in the stack to store some nolibc-defined
> information (possibly even a copy of a pointer to environ and/or errno)
> and figure a reliable and simple way to access this. Note that one way
> could also be to know that it's after the NULL that follows envp, and
> to start from environ. In fact there are plenty of ways and we just
> need to decide what's the least ugly. But once we have access to the
> auxv from the process, then we could implement a getauxval() function
> to retrieve the information you need for getpagesize().

Thanks for the great feedback!

I agree with following the @envp pointer to get the auxv. I was
trying to wire up a new function '__start' (with double underscores)
written in C that accepts @argc, @argv and @envp. Then it calls 'main'.
Then we call '__start' instead of 'main' from '_start'. This way, we
can arrange nolibc-defined data without touching Assembly much in
'__start' (before main).

But then I noticed that it wouldn't work because we may have users
who define the 'main' function differently, e.g.:

    int main(void);
    int main(int argc, char **argv);
    int main(int argc, char **argv, char **envp);

So '__start' can't call main. We still need to call the main from the
inline Assembly (from '_start').

Just a quick dirty patch to get getauxval() works on x86-64 below.
This needs more work, but at least something like this for starting:

  $ ./nolibc-test
  Running test 'syscall'
  AT_SYSINFO_EHDR = 140737354125312
  AT_HWCAP = 3219913727
  AT_PAGESZ = 4096
  AT_CLKTCK = 100
  AT_PHDR = 4194368
  AT_PHENT = 56
  AT_PHNUM = 9
  AT_BASE = 0
  AT_FLAGS = 0
  AT_ENTRY = 4199128
  AT_UID = 0
  AT_EUID = 0
  AT_GID = 0
  AT_EGID = 0
  AT_SECURE = 0
  AT_RANDOM = 140737488349065
  AT_EXECFN = 140737488351210
  AT_PLATFORM = 140737488349081

I'll probably only spare more intensive time on this after the holiday
season ends. I still check in email, but the activity will be sparse.

Signed-off-by: [Do not commit]
---

base-commit: caf5c36025ec9395c8d7c78957b016a284812d23 ("srcu: Update comment after the index flip")
diff --git a/tools/include/nolibc/arch-x86_64.h b/tools/include/nolibc/arch-x86_64.h
index 0e1e9eb8545d..5ee945370ff5 100644
--- a/tools/include/nolibc/arch-x86_64.h
+++ b/tools/include/nolibc/arch-x86_64.h
@@ -199,17 +199,49 @@ struct sys_stat_struct {
  */
 __asm__ (".section .text\n"
     ".weak _start\n"
     "_start:\n"
     "pop %rdi\n"                // argc   (first arg, %rdi)
     "mov %rsp, %rsi\n"          // argv[] (second arg, %rsi)
     "lea 8(%rsi,%rdi,8),%rdx\n" // then a NULL then envp (third arg, %rdx)
     "xor %ebp, %ebp\n"          // zero the stack frame
     "and $-16, %rsp\n"          // x86 ABI : esp must be 16-byte aligned before call
+    "push %rdi\n"               // Save argc
+    "push %rsi\n"               // Save arg
+    "push %rdx\n"               // Save envp.
+    "push %rcx\n"               // Keep the 16-byte alignment
+    "call __start\n"            // Save environ and auxv
+    "pop  %rcx\n"               // Restore alignment
+    "pop  %rdx\n"               // Restore envp
+    "pop  %rsi\n"               // Restore argv
+    "pop  %rdi\n"               // Restore argc
     "call main\n"               // main() returns the status code, we'll exit with it.
     "mov %eax, %edi\n"          // retrieve exit code (32 bit)
     "mov $60, %eax\n"           // NR_exit == 60
     "syscall\n"                 // really exit
     "hlt\n"                     // ensure it does not return
     "");
 
+struct __nolibc_internal {
+	char		**envp;
+	unsigned long	*auxv;
+};
+
+static struct __nolibc_internal __nolibc_internal;
+
+/*
+ * Mark this __used__ to avoid being optimized away.
+ * Reason: Called from inline Assembly.
+ */
+static __attribute__((__used__))
+void __start(int argc, char **argv, char **envp)
+{
+	char **p = envp;
+
+	while (*p)
+		p++;
+
+	__nolibc_internal.auxv = (unsigned long *)++p;
+	__nolibc_internal.envp = envp;
+}
+
 #endif // _NOLIBC_ARCH_X86_64_H
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 3db1dd8c74ee..04bff724e056 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -12,18 +12,19 @@
 
 /* system includes */
 #include <asm/unistd.h>
 #include <asm/signal.h>  // for SIGCHLD
 #include <asm/ioctls.h>
 #include <asm/mman.h>
 #include <linux/fs.h>
 #include <linux/loop.h>
 #include <linux/time.h>
+#include <linux/auxvec.h>
 
 #include "arch.h"
 #include "errno.h"
 #include "types.h"
 
 
 /* Functions in this file only describe syscalls. They're declared static so
  * that the compiler usually decides to inline them while still being allowed
  * to pass a pointer to one of their instances. Each syscall exists in two
@@ -379,18 +380,49 @@ int fsync(int fd)
 
 	if (ret < 0) {
 		SET_ERRNO(-ret);
 		ret = -1;
 	}
 	return ret;
 }
 
 
+/*
+ * On success, getauxval() returns the value corresponding to type.
+ * If type is not found, 0 is returned.
+ *
+ * unsigned long getauxval(unsigned long type);
+ */
+
+static __attribute__((unused))
+unsigned long getauxval(unsigned long type)
+{
+	unsigned long *auxv = __nolibc_internal.auxv;
+
+	if (__builtin_expect(!auxv, 0))
+		return 0;
+
+	while (1) {
+		if (!auxv[0] && !auxv[1])
+			/*
+			 * We've reached the end of auxv.
+			 */
+			return 0;
+
+		if (auxv[0] == type)
+			return auxv[1];
+
+		auxv += 2;
+	}
+	__builtin_unreachable();
+}
+
+
 /*
  * int getdents64(int fd, struct linux_dirent64 *dirp, int count);
  */
 
 static __attribute__((unused))
 int sys_getdents64(int fd, struct linux_dirent64 *dirp, int count)
 {
 	return my_syscall3(__NR_getdents64, fd, dirp, count);
 }
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index f14f5076fb6d..bca29a952c69 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -436,30 +436,60 @@ int test_getdents64(const char *dir)
 
 	ret = getdents64(fd, (void *)buffer, sizeof(buffer));
 	err = errno;
 	close(fd);
 
 	errno = err;
 	return ret;
 }
 
+static void test_getauxval(void)
+{
+	#define PRINT_AUXVAL(KEY)				\
+	do {							\
+		printf(#KEY " = %lu\n", getauxval(KEY));	\
+	} while (0)
+
+	PRINT_AUXVAL(AT_SYSINFO_EHDR);
+	PRINT_AUXVAL(AT_HWCAP);
+	PRINT_AUXVAL(AT_PAGESZ);
+	PRINT_AUXVAL(AT_CLKTCK);
+	PRINT_AUXVAL(AT_PHDR);
+	PRINT_AUXVAL(AT_PHENT);
+	PRINT_AUXVAL(AT_PHNUM);
+	PRINT_AUXVAL(AT_BASE);
+	PRINT_AUXVAL(AT_FLAGS);
+	PRINT_AUXVAL(AT_ENTRY);
+	PRINT_AUXVAL(AT_UID);
+	PRINT_AUXVAL(AT_EUID);
+	PRINT_AUXVAL(AT_GID);
+	PRINT_AUXVAL(AT_EGID);
+	PRINT_AUXVAL(AT_SECURE);
+	PRINT_AUXVAL(AT_RANDOM);
+	PRINT_AUXVAL(AT_EXECFN);
+	PRINT_AUXVAL(AT_PLATFORM);
+	exit(0);
+}
+
 /* Run syscall tests between IDs <min> and <max>.
  * Return 0 on success, non-zero on failure.
  */
 int run_syscall(int min, int max)
 {
 	struct stat stat_buf;
 	int proc;
 	int test;
 	int tmp;
 	int ret = 0;
 	void *p1, *p2;
 
+	test_getauxval();
+
 	/* <proc> indicates whether or not /proc is mounted */
 	proc = stat("/proc", &stat_buf) == 0;
 
 	for (test = min; test >= 0 && test <= max; test++) {
 		int llen = 0; // line length
 
 		/* avoid leaving empty lines below, this will insert holes into
 		 * test numbers.
 		 */
Willy Tarreau Dec. 27, 2022, 6:26 a.m. UTC | #3
Hi Ammar,

On Thu, Dec 22, 2022 at 08:46:15PM +0700, Ammar Faizi wrote:
> I agree with following the @envp pointer to get the auxv. I was
> trying to wire up a new function '__start' (with double underscores)
> written in C that accepts @argc, @argv and @envp. Then it calls 'main'.
> Then we call '__start' instead of 'main' from '_start'. This way, we
> can arrange nolibc-defined data without touching Assembly much in
> '__start' (before main).
> 
> But then I noticed that it wouldn't work because we may have users
> who define the 'main' function differently, e.g.:
> 
>     int main(void);
>     int main(int argc, char **argv);
>     int main(int argc, char **argv, char **envp);
> 
> So '__start' can't call main. We still need to call the main from the
> inline Assembly (from '_start').

Yes, and quite frankly I prefer to make that the least complicated.
Doing just a simple loop in the _start code is trivial. The main
concern was to store the data. Till now we had an optional .bss
section, we didn't save environ and errno was optional. But let's
be honest, while it does allow for writing the smallest programs,
most programs will have at least one global variable and will get
this section anyway, so we don't save anything in practice. This
concern used to be valid when I was making tiny executables when
running on floppies where each byte mattered, but now that's pointless.

Thus what I'm proposing is to switch to weak symbol definitions for
errno, environ, and auxv. I did a quick test to make sure that the same
symbol was properly used when accessed from two units and that's OK, I'm
seeing the same instance for all of them (which is better than the current
situation where errno is static, hence per-unit).

My quick-and-dirty test looks like this:

diff --git a/arch-x86_64.h b/arch-x86_64.h
index e780fdf..73f7b5f 100644
--- a/arch-x86_64.h
+++ b/arch-x86_64.h
@@ -209,6 +209,9 @@ struct sys_stat_struct {
        _ret;                                                                 \
 })
 
+char **environ __attribute__((weak,unused));
+long *auxv __attribute__((weak,unused));
+
 /* startup code */
 /*
  * x86-64 System V ABI mandates:
@@ -218,11 +221,17 @@ struct sys_stat_struct {
  */
 asm(".section .text\n"
     ".weak _start\n"
     "_start:\n"
     "pop %rdi\n"                // argc   (first arg, %rdi)
     "mov %rsp, %rsi\n"          // argv[] (second arg, %rsi)
     "lea 8(%rsi,%rdi,8),%rdx\n" // then a NULL then envp (third arg, %rdx)
+    "mov %rdx, environ\n"       // save environ
     "xor %ebp, %ebp\n"          // zero the stack frame
+    "mov %rdx, %rax\n"          // search for auxv (follows NULL after last en>
+    "0: add $8, %rax\n"
+    "   cmp -8(%rax), %rbp\n"
+    "   jnz 0b\n"
+    "mov %rax, auxv\n"          // save auxv
     "and $-16, %rsp\n"          // x86 ABI : esp must be 16-byte aligned befor>
     "call main\n"               // main() returns the status code, we'll exit >
     "mov %eax, %edi\n"          // retrieve exit code (32 bit)

diff --git a/errno.h b/errno.h
index df0e473..9781077 100644
--- a/errno.h
+++ b/errno.h
@@ -29,7 +29,8 @@
 #include <asm/errno.h>
 
 /* this way it will be removed if unused */
-static int errno;
+//static int errno;
+int errno __attribute__((weak));
 
 #ifndef NOLIBC_IGNORE_ERRNO
 #define SET_ERRNO(v) do { errno = (v); } while (0)

$ cat a.c
#include "nolibc.h"

extern void b(void);

int main(int argc, char **argv, char **envp)
{
        //environ = envp;
        errno = 1234;
        printf("main(): errno=%d env(TERM)=%s auxv=%p auxv[0].t=0x%lx auxv[0].v=0x%lx\n",
               errno, getenv("TERM"), auxv, auxv?auxv[0]:0, auxv?auxv[1]:0);
        b();
        return 0;
}

$ cat b.c
#include "nolibc.h"

void b(void)
{
        long *v = auxv;

        printf("b(): errno=%d env(TERM)=%s auxv=%p auxv[0].t=0x%lx auxv[0].v=0x%lx\n",
               errno, getenv("TERM"), auxv, auxv?auxv[0]:0, auxv?auxv[1]:0);

        printf("auxv:\n");
        while (v && v[0]) {
                printf("  0x%lx: 0x%lx\n", v[0], v[1]);
                v += 2;
        }
}

$ gcc -Os -fno-asynchronous-unwind-tables -include /g/public/nolibc/nolibc.h -Wall -nostdlib -static  -o ab a.c b.c

$ nm --size ab
0000000000000004 V errno
0000000000000008 V auxv
0000000000000008 V environ
0000000000000014 W memset
0000000000000018 W memcpy
0000000000000018 W raise
000000000000001b W abort
0000000000000030 W memmove
0000000000000053 t u64toa_r
0000000000000053 t u64toa_r
0000000000000082 T main
00000000000000a4 T b
0000000000000289 t printf
000000000000028c t printf.constprop.0

$ ./ab
main(): errno=1234 env(TERM)=xterm auxv=0x7ffdd0c31df8 auxv[0].t=0x21 auxv[0].v=0x7ffdd0d56000
b(): errno=1234 env(TERM)=xterm auxv=0x7ffdd0c31df8 auxv[0].t=0x21 auxv[0].v=0x7ffdd0d56000
auxv:
  0x21: 0x7ffdd0d56000
  0x10: 0xbfebfbff
  0x6: 0x1000
  0x11: 0x64
  0x3: 0x400040
  0x4: 0x38
  0x5: 0x7
  0x7: 0x0
  0x8: 0x0
  0x9: 0x401082
  0xb: 0x1fd
  0xc: 0x1fd
  0xd: 0x64
  0xe: 0x64
  0x17: 0x0
  0x19: 0x7ffdd0c31f39
  0x1a: 0x2
  0x1f: 0x7ffdd0c33ff3
  0xf: 0x7ffdd0c31f49

Note that I could verify that some of the entries above are valid
(e.g. "x86_64" in 0xf = AT_PLATFORM).

Thus now my focus will be on storing these variables where relevant
for all archs, so that your getauxval() implementation works on top
of it. It will be much cleaner and will also improve programs' ease
of implementation and reliability.

Cheers,
Willy

PS: maybe we should trim the Cc list for future exchanges.
Ammar Faizi Dec. 27, 2022, 1:32 p.m. UTC | #4
On 12/27/22 1:26 PM, Willy Tarreau wrote:
> Yes, and quite frankly I prefer to make that the least complicated.
> Doing just a simple loop in the _start code is trivial. The main
> concern was to store the data. Till now we had an optional .bss
> section, we didn't save environ and errno was optional. But let's
> be honest, while it does allow for writing the smallest programs,
> most programs will have at least one global variable and will get
> this section anyway, so we don't save anything in practice. This
> concern used to be valid when I was making tiny executables when
> running on floppies where each byte mattered, but now that's pointless.
> 
> Thus what I'm proposing is to switch to weak symbol definitions for
> errno, environ, and auxv. I did a quick test to make sure that the same
> symbol was properly used when accessed from two units and that's OK, I'm
> seeing the same instance for all of them (which is better than the current
> situation where errno is static, hence per-unit).

Looks good to me.

> Thus now my focus will be on storing these variables where relevant
> for all archs, so that your getauxval() implementation works on top
> of it. It will be much cleaner and will also improve programs' ease
> of implementation and reliability.

Are you going to wire up a patchset for it?

If so, I'll wait for it. When it's already committed, I'll base this
series on top it.

Or I take your series locally then submit your patches and mine in a
single series.

What do you prefer?
Ammar Faizi Dec. 27, 2022, 1:36 p.m. UTC | #5
On 12/27/22 8:32 PM, Ammar Faizi wrote:
>> Thus now my focus will be on storing these variables where relevant
>> for all archs, so that your getauxval() implementation works on top
>> of it. It will be much cleaner and will also improve programs' ease
>> of implementation and reliability.
> 
> Are you going to wire up a patchset for it?
> 
> If so, I'll wait for it. When it's already committed, I'll base this
> series on top it.
> 
> Or I take your series locally then submit your patches and mine in a
> single series.
> 
> What do you prefer?

Side note:
I only know x86 Assembly. So unfortunately, I can't get them working
on all arch(s).
Willy Tarreau Dec. 27, 2022, 6:49 p.m. UTC | #6
On Tue, Dec 27, 2022 at 08:32:57PM +0700, Ammar Faizi wrote:
> > Thus now my focus will be on storing these variables where relevant
> > for all archs, so that your getauxval() implementation works on top
> > of it. It will be much cleaner and will also improve programs' ease
> > of implementation and reliability.
> 
> Are you going to wire up a patchset for it?
>
> If so, I'll wait for it. When it's already committed, I'll base this
> series on top it.
> 
> Or I take your series locally then submit your patches and mine in a
> single series.
> 
> What do you prefer?

I'll try to do it but do not want to make you wait too long in case it
gets delayed. In the worst case we should only postpone the getauxval()
patch and not the other ones.

BTW, do you think your arch-specific changes for sigaction() will be
easily portable to other architectures ? I feel a bit wary of starting
to have different features per architecture given the purpose of the
lib, so the more uniform the coverage the better.

I'll get back to you soon.

Thanks,
Willy
Willy Tarreau Dec. 27, 2022, 6:58 p.m. UTC | #7
On Tue, Dec 27, 2022 at 08:36:41PM +0700, Ammar Faizi wrote:
> On 12/27/22 8:32 PM, Ammar Faizi wrote:
> > > Thus now my focus will be on storing these variables where relevant
> > > for all archs, so that your getauxval() implementation works on top
> > > of it. It will be much cleaner and will also improve programs' ease
> > > of implementation and reliability.
> > 
> > Are you going to wire up a patchset for it?
> > 
> > If so, I'll wait for it. When it's already committed, I'll base this
> > series on top it.
> > 
> > Or I take your series locally then submit your patches and mine in a
> > single series.
> > 
> > What do you prefer?
> 
> Side note:
> I only know x86 Assembly. So unfortunately, I can't get them working
> on all arch(s).

The nice thing about assembly is that once you know one, others are
easy to learn to permit you to write code that you can test. You can
have a look at MIPS for delayed slots, SPARC for register banks, ARM
for instructions that do multiple operations at once and you'll have
seen most of the basics that you'll ever need. Also all of these are
RISC based and cannot load a full-length register in a single instruction,
that's possibly the most confusing thing when you come from x86. And
it's also very interesting to see differences in constraints ;-)

Willy
Ammar Faizi Dec. 28, 2022, 12:01 p.m. UTC | #8
On 12/28/22 1:49 AM, Willy Tarreau wrote:
> I'll try to do it but do not want to make you wait too long in case it
> gets delayed. In the worst case we should only postpone the getauxval()
> patch and not the other ones.

I will split it into 2 patchset then.

> BTW, do you think your arch-specific changes for sigaction() will be
> easily portable to other architectures ? I feel a bit wary of starting
> to have different features per architecture given the purpose of the
> lib, so the more uniform the coverage the better.

The 'rt_sigaction()' itself doesn't seem to be an arch specific, but
the way it resumes the execution needs to call 'rt_sigreturn()' which
is arch specific. I took a look at the kernel source code, most
architectures read 'struct rt_sigframe' from the stack pointer.

https://github.com/torvalds/linux/blob/631aa744423173bf921191ba695bbc7c1aabd9e0/arch/x86/kernel/signal_32.c#L145
https://github.com/torvalds/linux/blob/631aa744423173bf921191ba695bbc7c1aabd9e0/arch/x86/kernel/signal_64.c#L243-L271
https://github.com/torvalds/linux/blob/a6b450573b912316ad36262bfc70e7c3870c56d1/arch/arm64/kernel/signal.c#L668-L699
https://github.com/torvalds/linux/blob/a6b450573b912316ad36262bfc70e7c3870c56d1/arch/arm64/kernel/signal32.c#L259
https://github.com/torvalds/linux/blob/eb67d239f3aa1711afb0a42eab50459d9f3d672e/arch/riscv/kernel/signal.c#L101

On the x86-64 arch, the implementation is just like this:

    __arch_restore_rt:
        #
        # ((%rsp - sizeof(long)) must point to 'struct rt_sigframe')
        #
        # 'struct rt_sigframe' is automatically constructed by
        # the kernel when a signal is caught.
        #
        movl       $0xf, %eax // __NR_rt_sigreturn == 0xf
        syscall

I believe aarch64 and RISCV don't behave differently, but different
registers.

Not sure what PowerPC does here, it seems a bit different:
https://github.com/torvalds/linux/blob/1612c382ffbdf1f673caec76502b1c00e6d35363/arch/powerpc/kernel/signal_64.c#L744

I haven't taken a look at other archs.

What do you think? Is it affordable for nolibc to implement all of
these?
Ammar Faizi Dec. 28, 2022, 12:23 p.m. UTC | #9
On 12/28/22 1:58 AM, Willy Tarreau wrote:
> The nice thing about assembly is that once you know one, others are
> easy to learn to permit you to write code that you can test. You can
> have a look at MIPS for delayed slots, SPARC for register banks, ARM
> for instructions that do multiple operations at once and you'll have
> seen most of the basics that you'll ever need. Also all of these are
> RISC based and cannot load a full-length register in a single instruction,
> that's possibly the most confusing thing when you come from x86. And
> it's also very interesting to see differences in constraints ;-)

Sounds fun. I'll try to get involved with other arch(s). But before
that, I have to prepare the environment. At least virtualization
that emulates those arch(s).
Willy Tarreau Dec. 28, 2022, 1:35 p.m. UTC | #10
On Wed, Dec 28, 2022 at 07:01:36PM +0700, Ammar Faizi wrote:
> On 12/28/22 1:49 AM, Willy Tarreau wrote:
> > I'll try to do it but do not want to make you wait too long in case it
> > gets delayed. In the worst case we should only postpone the getauxval()
> > patch and not the other ones.
> 
> I will split it into 2 patchset then.

OK thanks!

I've pushed for you an update which starts to do what I proposed. Errno
and environ are now marked weak for all archs, and _auxv is set for i386,
x86_64, arm64 and arm for now:

   https://git.kernel.org/pub/scm/linux/kernel/git/wtarreau/nolibc.git/log/?h=20221227-nolibc-weak-2

You can already use it to implement getauxval(), it will normally work
for these archs.

> > BTW, do you think your arch-specific changes for sigaction() will be
> > easily portable to other architectures ? I feel a bit wary of starting
> > to have different features per architecture given the purpose of the
> > lib, so the more uniform the coverage the better.
> 
> The 'rt_sigaction()' itself doesn't seem to be an arch specific, but
> the way it resumes the execution needs to call 'rt_sigreturn()' which
> is arch specific. I took a look at the kernel source code, most
> architectures read 'struct rt_sigframe' from the stack pointer.
> 
> https://github.com/torvalds/linux/blob/631aa744423173bf921191ba695bbc7c1aabd9e0/arch/x86/kernel/signal_32.c#L145
> https://github.com/torvalds/linux/blob/631aa744423173bf921191ba695bbc7c1aabd9e0/arch/x86/kernel/signal_64.c#L243-L271
> https://github.com/torvalds/linux/blob/a6b450573b912316ad36262bfc70e7c3870c56d1/arch/arm64/kernel/signal.c#L668-L699
> https://github.com/torvalds/linux/blob/a6b450573b912316ad36262bfc70e7c3870c56d1/arch/arm64/kernel/signal32.c#L259
> https://github.com/torvalds/linux/blob/eb67d239f3aa1711afb0a42eab50459d9f3d672e/arch/riscv/kernel/signal.c#L101
> 
> On the x86-64 arch, the implementation is just like this:
> 
>    __arch_restore_rt:
>        #
>        # ((%rsp - sizeof(long)) must point to 'struct rt_sigframe')
>        #
>        # 'struct rt_sigframe' is automatically constructed by
>        # the kernel when a signal is caught.
>        #
>        movl       $0xf, %eax // __NR_rt_sigreturn == 0xf
>        syscall

I think we could avoid the asm specific stuff is we get rid of the frame
pointer. Please look below:

  __attribute__((weak,unused,noreturn,optimize("omit-frame-pointer"),section(".text.nolibc_rt_sigreturn")))
  void sys_rt_sigreturn()
  {
        my_syscall0(__NR_rt_sigreturn);
        __builtin_unreachable();
  }

It gives me the correct code for x86_64 and i586. I don't know if other
architectures will want to add a prologue. I tried with "naked" but it's
ignored by the compiler since the function is not purely asm. Not very
important but given that we already have everything to perform our calls
it would make sense to stay on this. By the way, for the sake of
consistency with other syscalls, I do think the function (or label if
we can't do otherwise) should be called "sys_rt_sigreturn" as it just
performs a syscall.

> I believe aarch64 and RISCV don't behave differently, but different
> registers.
> 
> Not sure what PowerPC does here, it seems a bit different:
> https://github.com/torvalds/linux/blob/1612c382ffbdf1f673caec76502b1c00e6d35363/arch/powerpc/kernel/signal_64.c#L744

It looks similar to me, it's just that the kernel side differs but I
think it's the same.

> I haven't taken a look at other archs.
> 
> What do you think? Is it affordable for nolibc to implement all of
> these?

Yes I think so. I suspect that we might need to have a few arch-specific
implementations, but we've already had this case a few times and we could
easily use a pair of #define/#ifdef to skip the generic version.

Best regards,
Willy
Ammar Faizi Dec. 29, 2022, 11:41 a.m. UTC | #11
On 12/28/22 8:35 PM, Willy Tarreau wrote:
> OK thanks!
> 
> I've pushed for you an update which starts to do what I proposed. Errno
> and environ are now marked weak for all archs, and _auxv is set for i386,
> x86_64, arm64 and arm for now:
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/wtarreau/nolibc.git/log/?h=20221227-nolibc-weak-2
> 
> You can already use it to implement getauxval(), it will normally work
> for these archs.

Will do and be back with two patch series.

> I think we could avoid the asm specific stuff is we get rid of the frame
> pointer. Please look below:
> 
>    __attribute__((weak,unused,noreturn,optimize("omit-frame-pointer"),section(".text.nolibc_rt_sigreturn")))
>    void sys_rt_sigreturn()
>    {
>          my_syscall0(__NR_rt_sigreturn);
>          __builtin_unreachable();
>    }

Wow! You just taught me that we can force optimize a function with
optimize("omit-frame-pointer") attribute. Nice to know this one!

I compile-tested it and it indeed gives the correct code on x86-64.
Hopefully this approach works for all archs.

> It gives me the correct code for x86_64 and i586. I don't know if other
> architectures will want to add a prologue. I tried with "naked" but it's
> ignored by the compiler since the function is not purely asm. Not very
> important but given that we already have everything to perform our calls
> it would make sense to stay on this. By the way, for the sake of
> consistency with other syscalls, I do think the function (or label if
> we can't do otherwise) should be called "sys_rt_sigreturn" as it just
> performs a syscall.

Will call that 'sys_rt_sigreturn' in the next series.

Thanks!
Alviro Iskandar Setiawan Jan. 3, 2023, 3:51 a.m. UTC | #12
On Thu, Dec 29, 2022 at 6:42 PM Ammar Faizi wrote:
> On 12/28/22 8:35 PM, Willy Tarreau wrote:
> > It gives me the correct code for x86_64 and i586. I don't know if other
> > architectures will want to add a prologue. I tried with "naked" but it's
> > ignored by the compiler since the function is not purely asm. Not very
> > important but given that we already have everything to perform our calls
> > it would make sense to stay on this. By the way, for the sake of
> > consistency with other syscalls, I do think the function (or label if
> > we can't do otherwise) should be called "sys_rt_sigreturn" as it just
> > performs a syscall.
>
> Will call that 'sys_rt_sigreturn' in the next series.

From glibc source code says:
GDB needs some intimate knowledge about it to recognize them as signal
trampolines, and make backtraces through signal handlers work right.
Important are both the names (__restore_rt) and the exact instruction
sequence.

link: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/x86_64/sigaction.c;h=4e6d9cc32e1e18746726fa430d092de9a19ba6c6;hb=b4a5d26d8835d972995f0a0a2f805a8845bafa0b#l34

glibc does this:

   "    .type __" #name ",@function\n" \
   "__" #name ":\n"                    \
   "    movq $" #syscall ", %rax\n"    \
   "    syscall\n"                     \

where

   #name = "restore_rt"
   #syscall = __NR_rt_sigreturn

I think it should be called "__restore_rt" instead of "sys_rt_sigreturn"?
glibc also has unwind information, but we probably don't need to care
with that much
Willy Tarreau Jan. 3, 2023, 3:54 a.m. UTC | #13
On Tue, Jan 03, 2023 at 10:51:35AM +0700, Alviro Iskandar Setiawan wrote:
> On Thu, Dec 29, 2022 at 6:42 PM Ammar Faizi wrote:
> > On 12/28/22 8:35 PM, Willy Tarreau wrote:
> > > It gives me the correct code for x86_64 and i586. I don't know if other
> > > architectures will want to add a prologue. I tried with "naked" but it's
> > > ignored by the compiler since the function is not purely asm. Not very
> > > important but given that we already have everything to perform our calls
> > > it would make sense to stay on this. By the way, for the sake of
> > > consistency with other syscalls, I do think the function (or label if
> > > we can't do otherwise) should be called "sys_rt_sigreturn" as it just
> > > performs a syscall.
> >
> > Will call that 'sys_rt_sigreturn' in the next series.
> 
> >From glibc source code says:
> GDB needs some intimate knowledge about it to recognize them as signal
> trampolines, and make backtraces through signal handlers work right.
> Important are both the names (__restore_rt) and the exact instruction
> sequence.
> 
> link: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/x86_64/sigaction.c;h=4e6d9cc32e1e18746726fa430d092de9a19ba6c6;hb=b4a5d26d8835d972995f0a0a2f805a8845bafa0b#l34
> 
> glibc does this:
> 
>    "    .type __" #name ",@function\n" \
>    "__" #name ":\n"                    \
>    "    movq $" #syscall ", %rax\n"    \
>    "    syscall\n"                     \
> 
> where
> 
>    #name = "restore_rt"
>    #syscall = __NR_rt_sigreturn
> 
> I think it should be called "__restore_rt" instead of "sys_rt_sigreturn"?
> glibc also has unwind information, but we probably don't need to care
> with that much

OK, I wasn't aware of this. Of course, if there are some strict rules
for this, let's follow them!

Thanks,
Willy
Ammar Faizi Jan. 3, 2023, 3:59 a.m. UTC | #14
On 1/3/23 10:54 AM, Willy Tarreau wrote:
> On Tue, Jan 03, 2023 at 10:51:35AM +0700, Alviro Iskandar Setiawan wrote:
>> GDB needs some intimate knowledge about it to recognize them as signal
>> trampolines, and make backtraces through signal handlers work right.
>> Important are both the names (__restore_rt) and the exact instruction
>> sequence.

Will follow it, thanks!