rcutorture initrd/nolibc build on ARMv8?

rcutorture initrd/nolibc build on ARMv8?

[Paul E. McKenney]

Hello, Willy,

Some people are having trouble running rcutorture on ARMv8.  They
get things like this from the nolibc build of initrd:

https://paste.debian.net/1181762/

The nolibc.h file says this:

/* Some archs (at least aarch64) don't expose the regular syscalls anymore by
 * default, either because they have an "_at" replacement, or because there are
 * more modern alternatives. For now we'd rather still use them.
 */

Are these build failures expected behavior on ARMv8?

							Thanx, Paul

Re: rcutorture initrd/nolibc build on ARMv8?

[Willy Tarreau]

Hi Paul,

On Tue, Jan 19, 2021 at 07:31:47AM -0800, Paul E. McKenney wrote:
> Hello, Willy,
> 
> Some people are having trouble running rcutorture on ARMv8.  They
> get things like this from the nolibc build of initrd:
> 
> https://paste.debian.net/1181762/
> 
> The nolibc.h file says this:
> 
> /* Some archs (at least aarch64) don't expose the regular syscalls anymore by
>  * default, either because they have an "_at" replacement, or because there are
>  * more modern alternatives. For now we'd rather still use them.
>  */
> 
> Are these build failures expected behavior on ARMv8?

No, I don't think so. I'm regularly building my own init using this,
and it works on various platforms including aarch64, while it makes
use of a number of such syscalls.

From what I'm seeing, this seems to happen each time in such a construct:

    #if defined(__NR_new_name)
         use __NR_new_name
    #else
         use __NR_old_name
    #endif

with the error appearing on old_name. So I guess that we're rather facing a
case where a number of such __NR_* entries are not defined because presumably
one include file might be missing (probably from a recent change of headers
dependency).

I can't spot from the report above the original C file that was attempted
to be built, it makes me think we tried to compile directly the .h file.

Having it run through sh -x would help me try to locate the root cause or
possibly even attempt to reproduce it.

Thanks,
Willy

Re: rcutorture initrd/nolibc build on ARMv8?

[Mark Rutland]

Hi Willy,

On Tue, Jan 19, 2021 at 05:19:01PM +0100, Willy Tarreau wrote:
> On Tue, Jan 19, 2021 at 07:31:47AM -0800, Paul E. McKenney wrote:
> > Some people are having trouble running rcutorture on ARMv8.  They
> > get things like this from the nolibc build of initrd:
> > 
> > https://paste.debian.net/1181762/
> > 
> > The nolibc.h file says this:
> > 
> > /* Some archs (at least aarch64) don't expose the regular syscalls anymore by
> >  * default, either because they have an "_at" replacement, or because there are
> >  * more modern alternatives. For now we'd rather still use them.
> >  */
> > 
> > Are these build failures expected behavior on ARMv8?
> 
> No, I don't think so. I'm regularly building my own init using this,
> and it works on various platforms including aarch64, while it makes
> use of a number of such syscalls.
> 
> From what I'm seeing, this seems to happen each time in such a construct:
> 
>     #if defined(__NR_new_name)
>          use __NR_new_name
>     #else
>          use __NR_old_name
>     #endif
> 
> with the error appearing on old_name. So I guess that we're rather facing a
> case where a number of such __NR_* entries are not defined because presumably
> one include file might be missing (probably from a recent change of headers
> dependency).
> 
> I can't spot from the report above the original C file that was attempted
> to be built, it makes me think we tried to compile directly the .h file.

That was the inline snippet in
tools/testing/selftests/rcutorture/bin/mkinitrd.sh:

| #ifndef NOLIBC
| #include <unistd.h>
| #include <sys/time.h>
| #endif
| 
| volatile unsigned long delaycount;
| 
| int main(int argc, int argv[])
| {
|         int i;
|         struct timeval tv;
|         struct timeval tvb;
| 
|         for (;;) {
|                 sleep(1);
|                 /* Need some userspace time. */
|                 if (gettimeofday(&tvb, NULL))
|                         continue;
|                 do {
|                         for (i = 0; i < 1000 * 100; i++)
|                                 delaycount = i * i;
|                         if (gettimeofday(&tv, NULL))
|                                 break;
|                         tv.tv_sec -= tvb.tv_sec;
|                         if (tv.tv_sec > 1)
|                                 break;
|                         tv.tv_usec += tv.tv_sec * 1000 * 1000;
|                         tv.tv_usec -= tvb.tv_usec;
|                 } while (tv.tv_usec < 1000);
|         }
|         return 0;
| }

... which gets written to a file called init.c, and then built with:

| ${CROSS_COMPILE}gcc -fno-asynchronous-unwind-tables -fno-ident \
|         -nostdlib -include ../../../../include/nolibc/nolibc.h \
|         -lgcc -s -static -Os -o init init.c

I was building natively on an arm64 box:

| ./tools/testing/selftests/rcutorture/bin/kvm.sh \
|         --cpus 250 --trust-make --configs "TREE03" \
|         --kmake-arg "CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm64"

> Having it run through sh -x would help me try to locate the root cause or
> possibly even attempt to reproduce it.

I ran with sh -x, but it didn't log the compiler invocation; hopefully
the above is sufficient?

Thanks,
Mark.

Re: rcutorture initrd/nolibc build on ARMv8?

[Willy Tarreau]

Hi Mark,

On Tue, Jan 19, 2021 at 05:02:38PM +0000, Mark Rutland wrote:
> > I can't spot from the report above the original C file that was attempted
> > to be built, it makes me think we tried to compile directly the .h file.
> 
> That was the inline snippet in
> tools/testing/selftests/rcutorture/bin/mkinitrd.sh:
> 
> | #ifndef NOLIBC
> | #include <unistd.h>
> | #include <sys/time.h>
> | #endif
> | 
> | volatile unsigned long delaycount;
> | 
> | int main(int argc, int argv[])
> | {
> |         int i;
> |         struct timeval tv;
> |         struct timeval tvb;
> | 
> |         for (;;) {
> |                 sleep(1);
> |                 /* Need some userspace time. */
> |                 if (gettimeofday(&tvb, NULL))
> |                         continue;
> |                 do {
> |                         for (i = 0; i < 1000 * 100; i++)
> |                                 delaycount = i * i;
> |                         if (gettimeofday(&tv, NULL))
> |                                 break;
> |                         tv.tv_sec -= tvb.tv_sec;
> |                         if (tv.tv_sec > 1)
> |                                 break;
> |                         tv.tv_usec += tv.tv_sec * 1000 * 1000;
> |                         tv.tv_usec -= tvb.tv_usec;
> |                 } while (tv.tv_usec < 1000);
> |         }
> |         return 0;
> | }
> 
> ... which gets written to a file called init.c, and then built with:
> 
> | ${CROSS_COMPILE}gcc -fno-asynchronous-unwind-tables -fno-ident \
> |         -nostdlib -include ../../../../include/nolibc/nolibc.h \
> |         -lgcc -s -static -Os -o init init.c

OK I'll retry this, thank you!

> I was building natively on an arm64 box:
> 
> | ./tools/testing/selftests/rcutorture/bin/kvm.sh \
> |         --cpus 250 --trust-make --configs "TREE03" \
> |         --kmake-arg "CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm64"
> 
> > Having it run through sh -x would help me try to locate the root cause or
> > possibly even attempt to reproduce it.
> 
> I ran with sh -x, but it didn't log the compiler invocation; hopefully
> the above is sufficient?

I guess so, yes. I'm pretty sure I'll come back with new questions
soon :-)

Thanks,
Willy

Re: rcutorture initrd/nolibc build on ARMv8?

[Willy Tarreau]

On Tue, Jan 19, 2021 at 06:16:37PM +0100, Willy Tarreau wrote:
> > | ${CROSS_COMPILE}gcc -fno-asynchronous-unwind-tables -fno-ident \
> > |         -nostdlib -include ../../../../include/nolibc/nolibc.h \
> > |         -lgcc -s -static -Os -o init init.c
> 
> OK I'll retry this, thank you!

For me on my x86 PC it worked using a cross-compiler:

  $ /f/tc/gcc-linaro-6.4.1-2018.05-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include tools/include/nolibc/nolibc.h -lgcc -s -static -Os -o init-fail init-fail.c 
  $ file init-fail
  init-fail: ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), statically linked, BuildID[sha1]=1088f8fad85b7b8a7f00eef4918d24c4c4fdaadf, stripped

Given that you used a native compiler we can't suspect an issue with a
bare-metal compiler possibly affecting how kernel headers are passed
there. But nevertheless, I'd still not disregard the possibility that
the headers found under "linux/" are picked from the libc which for
whatever reason would be missing a lot of them.

I've now tested natively on a local rpi4b onto which I copied the files
(not enough I/O BW on the SD card to clone a kernel there). Tried:

  $ gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include nolibc.h -lgcc -s -static -Os -o init-fail init-fail.c
  specs.
  COLLECT_GCC=gcc
  COLLECT_LTO_WRAPPER=/usr/lib/gcc/aarch64-linux-gnu/7/lto-wrapper
  Target: aarch64-linux-gnu
  Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro 7.5.0-3ubuntu1~18.04' --with-bugurl=file:///usr/share/doc/gcc-7/README.Bugs --enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++ --prefix=/usr --with-gcc-major-version-only --program-suffix=-7 --program-prefix=aarch64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --enable-bootstrap --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-libquadmath --disable-libquadmath-support --enable-plugin --enable-default-pie --with-system-zlib --enable-multiarch --enable-fix-cortex-a53-843419 --disable-werror --enable-checking=release --build=aarch64-linux-gnu --host=aarch64-linux-gnu --target=aarch64-linux-gnu
  Thread model: posix
  gcc version 7.5.0 (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 

Could you please check the output of this from your intermediary "init.c"
file:

   $ gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include tools/include/nolibc/nolibc.h -lgcc -s -static -E  init.c | grep '^#'

It might give us a clue about where it's finding its includes. And possibly
the list of __NR_ entries reported here as well:

   $ gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include tools/include/nolibc/nolibc.h -lgcc -s -static -E -dM init.c | grep __NR_

We've seen that __NR_fork or __NR_dup2 for example were missing in your
output, on my native machine I can see them, so that could give us a clue
about the root cause of the issue:

  $ gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include nolibc.h -lgcc -s -static -E -dM init-fail.c | egrep '__NR_(fork|dup2)'
  #define __NR_dup2 1041
  #define __NR_syscalls (__NR_fork+1)
  #define __NR_fork 1079

I could easily imagine a missing "linux" link or entry somewhere in the
default includes path, but here since there's no such error it rather looks
like an incorrect file which is a bit more concerning. I purposely avoided
to maintain my own definitions for __NR_* let's hope we won't need to do
that :-/

Thanks,
Willy

Re: rcutorture initrd/nolibc build on ARMv8?

[Mark Rutland]

On Tue, Jan 19, 2021 at 06:43:58PM +0100, Willy Tarreau wrote:
> On Tue, Jan 19, 2021 at 06:16:37PM +0100, Willy Tarreau wrote:
> Given that you used a native compiler we can't suspect an issue with a
> bare-metal compiler possibly affecting how kernel headers are passed
> there. But nevertheless, I'd still not disregard the possibility that
> the headers found under "linux/" are picked from the libc which for
> whatever reason would be missing a lot of them.

I think the actual issue here is a misapprehension in nolibc.h, which
started blowing up due to a refactoring in asm/unistd.h.

In nolibc.h, we do:

| /* Some archs (at least aarch64) don't expose the regular syscalls anymore by
|  * default, either because they have an "_at" replacement, or because there are
|  * more modern alternatives. For now we'd rather still use them.
|  */
| #define __ARCH_WANT_SYSCALL_NO_AT
| #define __ARCH_WANT_SYSCALL_NO_FLAGS
| #define __ARCH_WANT_SYSCALL_DEPRECATED

... but this isn't quite right -- it's not that the syscalls aren't
exposed by default, but rather that these syscall numbers are not valid
for architectures which do not define the corresponding __ARCH_WANT_*
flags. Architectures without those have never implemented the syscalls,
and would have returned -ENOSYS for the unrecognized syscall numbers,
but the numbers could be allocated to (distinct) syscalls in future.

Since commit:

  a0673fdbcd421052 ("asm-generic: clean up asm/unistd.h")

... those definitions got pulled out of <asm-generic/unistd.h>, and
hence it's no longer possible to accidentally get those where a
userspace header defines __ARCH_WANT_* in an architecture where they
don't exist (e.g. arm64).

It seems that the headers on my Debian 10.7 system were generated after
that commit, whereas yours were generated before that.

> We've seen that __NR_fork or __NR_dup2 for example were missing in your
> output, on my native machine I can see them, so that could give us a clue
> about the root cause of the issue:
> 
>   $ gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include nolibc.h -lgcc -s -static -E -dM init-fail.c | egrep '__NR_(fork|dup2)'
>   #define __NR_dup2 1041
>   #define __NR_syscalls (__NR_fork+1)
>   #define __NR_fork 1079

As above, these are bogus for arm64. There is no syscall number for dup2
or fork, and __NR_syscalls is currently only 442.

I think the right thing to do is to have nolibc.h detect which syscalls
are implemented, and to not define __ARCH_WANT_*.

Thanks,
Mark.

Re: rcutorture initrd/nolibc build on ARMv8?

[Willy Tarreau]

Hi Mark,

On Wed, Jan 20, 2021 at 12:07:25PM +0000, Mark Rutland wrote:
> On Tue, Jan 19, 2021 at 06:43:58PM +0100, Willy Tarreau wrote:
> > On Tue, Jan 19, 2021 at 06:16:37PM +0100, Willy Tarreau wrote:
> > Given that you used a native compiler we can't suspect an issue with a
> > bare-metal compiler possibly affecting how kernel headers are passed
> > there. But nevertheless, I'd still not disregard the possibility that
> > the headers found under "linux/" are picked from the libc which for
> > whatever reason would be missing a lot of them.
> 
> I think the actual issue here is a misapprehension in nolibc.h, which
> started blowing up due to a refactoring in asm/unistd.h.
> 
> In nolibc.h, we do:
> 
> | /* Some archs (at least aarch64) don't expose the regular syscalls anymore by
> |  * default, either because they have an "_at" replacement, or because there are
> |  * more modern alternatives. For now we'd rather still use them.
> |  */
> | #define __ARCH_WANT_SYSCALL_NO_AT
> | #define __ARCH_WANT_SYSCALL_NO_FLAGS
> | #define __ARCH_WANT_SYSCALL_DEPRECATED
> 
> ... but this isn't quite right -- it's not that the syscalls aren't
> exposed by default, but rather that these syscall numbers are not valid
> for architectures which do not define the corresponding __ARCH_WANT_*
> flags. Architectures without those have never implemented the syscalls,
> and would have returned -ENOSYS for the unrecognized syscall numbers,
> but the numbers could be allocated to (distinct) syscalls in future.
> 
> Since commit:
> 
>   a0673fdbcd421052 ("asm-generic: clean up asm/unistd.h")
> 
> ... those definitions got pulled out of <asm-generic/unistd.h>, and
> hence it's no longer possible to accidentally get those where a
> userspace header defines __ARCH_WANT_* in an architecture where they
> don't exist (e.g. arm64).

I didn't remember the details behind these definitions, I've found
this in the related commit message:

    Some syscall numbers are not exported
    by default unless a few macros are defined before including unistd.h :
    
      #define __ARCH_WANT_SYSCALL_NO_AT
      #define __ARCH_WANT_SYSCALL_NO_FLAGS
      #define __ARCH_WANT_SYSCALL_DEPRECATED

So it seems I faced difficulties getting the __NR_* entries and only
could get them using such definitions :-/

Did you figure the correct way to get __NR_* defined on your machine or
should I search ?

> It seems that the headers on my Debian 10.7 system were generated after
> that commit, whereas yours were generated before that.

It's very possible indeed.

> > We've seen that __NR_fork or __NR_dup2 for example were missing in your
> > output, on my native machine I can see them, so that could give us a clue
> > about the root cause of the issue:
> > 
> >   $ gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include nolibc.h -lgcc -s -static -E -dM init-fail.c | egrep '__NR_(fork|dup2)'
> >   #define __NR_dup2 1041
> >   #define __NR_syscalls (__NR_fork+1)
> >   #define __NR_fork 1079
> 
> As above, these are bogus for arm64. There is no syscall number for dup2
> or fork, and __NR_syscalls is currently only 442.

Ah that's very interesting indeed because actually these ones should
only be used when __NR_dup3 or __NR_clone are not defined. Thus I wanted
to check the definitions that were reported in your error output but
actually what was needed was to figure whether the correct ones were
present, and they are, here on my machine (and yes I agree that in this
case the dup2/fork above are bofus):

  ubuntu@ubuntu:~$ gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include nolibc.h -lgcc -s -static -E -dM init-fail.c | egrep '__NR_(clone|dup3)'
  #define __NR_clone 220
  #define __NR_dup3 24

Do you have these ones with your more recent includes ? Or are these wrong
again ? Again I'd be surprised given that I didn't invent them and that my
systems boot fine using these.

> I think the right thing to do is to have nolibc.h detect which syscalls
> are implemented, and to not define __ARCH_WANT_*.

Actually that's what was attempted by already focusing on ifdefs but
without *any* __NR_* we can't even hope to call a syscall and check for
ENOSYS for example. So the code really tries to figure which is the right
__NR_ value for a given syscall, and a quick check at my userland code
shows that I'm already using dup2() and fork() as defined from dup3()
and clone().

Thanks!
Willy

Re: rcutorture initrd/nolibc build on ARMv8?

[Mark Rutland]

On Wed, Jan 20, 2021 at 01:43:40PM +0100, Willy Tarreau wrote:
> On Wed, Jan 20, 2021 at 12:07:25PM +0000, Mark Rutland wrote:
> > On Tue, Jan 19, 2021 at 06:43:58PM +0100, Willy Tarreau wrote:
> > > On Tue, Jan 19, 2021 at 06:16:37PM +0100, Willy Tarreau wrote:

> Did you figure the correct way to get __NR_* defined on your machine or
> should I search ?

Please see below. There is no way to get a number for some syscalls, as
these do not have a legitimate number on arm64, but this doesn't matter
as we can avoid using the number entirely when it does not exist.

[...]

> > >   $ gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include nolibc.h -lgcc -s -static -E -dM init-fail.c | egrep '__NR_(fork|dup2)'
> > >   #define __NR_dup2 1041
> > >   #define __NR_syscalls (__NR_fork+1)
> > >   #define __NR_fork 1079
> > 
> > As above, these are bogus for arm64. There is no syscall number for dup2
> > or fork, and __NR_syscalls is currently only 442.
> 
> Ah that's very interesting indeed because actually these ones should
> only be used when __NR_dup3 or __NR_clone are not defined. Thus I wanted
> to check the definitions that were reported in your error output but
> actually what was needed was to figure whether the correct ones were
> present, and they are, here on my machine (and yes I agree that in this
> case the dup2/fork above are bofus):

The issue is that even if a function is unused, the compiler still has
to parse and compile the code, so where __NR_dup2 is not defined, we'll
get a build error for:

static __attribute__((unused))
int sys_dup2(int old, int new)
{
       return my_syscall2(__NR_dup2, old, new);
}

... we can deal with that by always returning -ENOSYS for unimplemented
syscalls, e.g.

static __attribute__((unused))
int sys_dup2(int old, int new)
{
#ifdef __NR_dup2
       return my_syscall2(__NR_dup2, old, new);
#else
       return -ENOSYS;
#endif
}

I can spin a patch fixing up all the relevant syscalls, if you'd like?

[...]

>   ubuntu@ubuntu:~$ gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include nolibc.h -lgcc -s -static -E -dM init-fail.c | egrep '__NR_(clone|dup3)'
>   #define __NR_clone 220
>   #define __NR_dup3 24
> 
> Do you have these ones with your more recent includes ? Or are these wrong
> again ?

Those are correct (and all the syscall numbers in unistd.h should be
correct so long as you don't erroneously set the __ARCH_WANT_* flags):

[mark@gravadlaks:~/src/linux]% gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include tools/include/nolibc/nolibc.h -lgcc -s -static -E -dM init-fail.c | egrep '__NR_(clone|dup3)'
#define __NR_clone 220
#define __NR_dup3 24

> > I think the right thing to do is to have nolibc.h detect which syscalls
> > are implemented, and to not define __ARCH_WANT_*.
> 
> Actually that's what was attempted by already focusing on ifdefs but
> without *any* __NR_* we can't even hope to call a syscall and check for
> ENOSYS for example. So the code really tries to figure which is the right
> __NR_ value for a given syscall, and a quick check at my userland code
> shows that I'm already using dup2() and fork() as defined from dup3()
> and clone().

Please see above for how to get the -ENOSYS behaviour without relying on bogus
syscall numbers.

I have a local patch for this, which I can send if you'd like?

There's still some latent issue when using nolibc (compared to using
glibc) where the init process never seems to exit, but that looks to be
orthogonal to the syscall numbering issue -- I'm currently digging into
that.

Thanks,
Mark.

Re: rcutorture initrd/nolibc build on ARMv8?

[Willy Tarreau]

On Wed, Jan 20, 2021 at 01:45:11PM +0000, Mark Rutland wrote:
> > Ah that's very interesting indeed because actually these ones should
> > only be used when __NR_dup3 or __NR_clone are not defined. Thus I wanted
> > to check the definitions that were reported in your error output but
> > actually what was needed was to figure whether the correct ones were
> > present, and they are, here on my machine (and yes I agree that in this
> > case the dup2/fork above are bofus):
> 
> The issue is that even if a function is unused, the compiler still has
> to parse and compile the code, so where __NR_dup2 is not defined, we'll
> get a build error for:
> 
> static __attribute__((unused))
> int sys_dup2(int old, int new)
> {
>        return my_syscall2(__NR_dup2, old, new);
> }

For sure but this is supposed to be used only when __NR_dup3 is not
defined. Ah now I understand where my mistake is: after it built
successfully for me I inspected the most recent tree which has these
in place. Sorry for being stupid!

In my local tree it's defined like this:

 static __attribute__((unused))
 int sys_dup2(int old, int new)
 {
#ifdef __NR_dup3
       return my_syscall3(__NR_dup3, old, new, 0);
#else
       return my_syscall2(__NR_dup2, old, new);
#endif
 }

I guess I fixed it in a hurry and forgot to upstream it. I hate doing
that :-(

I'm going to push an update then. On a quick glance I'm seeing that I
addressed dup2() using dup3(), fork() using clone(), getpgrp() using
getpgid(), and poll() using ppoll().

> ... we can deal with that by always returning -ENOSYS for unimplemented
> syscalls, e.g.
> 
> static __attribute__((unused))
> int sys_dup2(int old, int new)
> {
> #ifdef __NR_dup2
>        return my_syscall2(__NR_dup2, old, new);
> #else
>        return -ENOSYS;
> #endif
> }

I didn't want to do that because that would break userland which needs
dup2(), hence the mapping to dup3 instead:

  static __attribute__((unused))
  int sys_dup2(int old, int new)
  {
  #ifdef __NR_dup3
          return my_syscall3(__NR_dup3, old, new, 0);
  #else
          return my_syscall2(__NR_dup2, old, new);
  #endif
  }

> I can spin a patch fixing up all the relevant syscalls, if you'd like?

I shouldn't need since these are already fixed in my tree. At first glance
the equivalent of the following commits is missing from the kernel version:

   https://github.com/wtarreau/nolibc/commit/2379f25073f906d0bad22c48566fcffee8fc9cde
   https://github.com/wtarreau/nolibc/commit/fd5272ec2c66e6f5b195d41c9c8978f58eb74668
   https://github.com/wtarreau/nolibc/commit/47cc42a79c92305f4f8bc02fb28628a4fdd63eaa
   https://github.com/wtarreau/nolibc/commit/d2dc42fd614991c741dfdc8b984864fa3cf64c8e
   https://github.com/wtarreau/nolibc/commit/800f75c13ede49097325f82a4cca3515c44a7939

However I'm interested in knowing if the latest version fixes everything
for you or not :

  https://raw.githubusercontent.com/wtarreau/nolibc/master/nolibc.h

> [...]
> 
> >   ubuntu@ubuntu:~$ gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include nolibc.h -lgcc -s -static -E -dM init-fail.c | egrep '__NR_(clone|dup3)'
> >   #define __NR_clone 220
> >   #define __NR_dup3 24
> > 
> > Do you have these ones with your more recent includes ? Or are these wrong
> > again ?
> 
> Those are correct (and all the syscall numbers in unistd.h should be
> correct so long as you don't erroneously set the __ARCH_WANT_* flags):
> 
> [mark@gravadlaks:~/src/linux]% gcc -fno-asynchronous-unwind-tables -fno-ident -nostdlib -include tools/include/nolibc/nolibc.h -lgcc -s -static -E -dM init-fail.c | egrep '__NR_(clone|dup3)'
> #define __NR_clone 220
> #define __NR_dup3 24

OK thanks! I will retry here without setting those. I'm pretty sure I
needed these ones to find the __NR_* values but it's possible that it
was before I had the alternate ones and that these are finally not
nedeed at all (which I would prefer as these are ugly).

> There's still some latent issue when using nolibc (compared to using
> glibc) where the init process never seems to exit, but that looks to be
> orthogonal to the syscall numbering issue -- I'm currently digging into
> that.

OK! Usually for me it does as in my preinit (which uses nolibc), if I
exit I instantly get a kernel panic. In addition if I launch it after
boot, it immediately exits and shows no issue. But maybe you're observing
an artefact related to something else (process session, opened FD or
anything else maybe).

I'll send an update ASAP, likely this evening.

Many thanks for digging into this, and sorry for this mess, as I was
absolutely certain it was up to date :-(

Willy

Re: rcutorture initrd/nolibc build on ARMv8?

[Mark Rutland]

On Wed, Jan 20, 2021 at 03:25:00PM +0100, Willy Tarreau wrote:
> On Wed, Jan 20, 2021 at 01:45:11PM +0000, Mark Rutland wrote:
> > There's still some latent issue when using nolibc (compared to using
> > glibc) where the init process never seems to exit, but that looks to be
> > orthogonal to the syscall numbering issue -- I'm currently digging into
> > that.
> 
> OK! Usually for me it does as in my preinit (which uses nolibc), if I
> exit I instantly get a kernel panic. In addition if I launch it after
> boot, it immediately exits and shows no issue. But maybe you're observing
> an artefact related to something else (process session, opened FD or
> anything else maybe).

Luckily this was nothing to do with nolibc -- the build system wasn't
regenerating the initramfs to use the correctly-compiled init, and
things were blowing up because the kernel couldn't find an init process.
With that regenerated it worked as intended.

I'll reply separately for the rest of the nolibc bits shortly.

Thanks,
Mark.

Re: rcutorture initrd/nolibc build on ARMv8?

[Mark Rutland]

On Wed, Jan 20, 2021 at 03:25:00PM +0100, Willy Tarreau wrote:
> On Wed, Jan 20, 2021 at 01:45:11PM +0000, Mark Rutland wrote:
> > > Ah that's very interesting indeed because actually these ones should
> > > only be used when __NR_dup3 or __NR_clone are not defined. Thus I wanted
> > > to check the definitions that were reported in your error output but
> > > actually what was needed was to figure whether the correct ones were
> > > present, and they are, here on my machine (and yes I agree that in this
> > > case the dup2/fork above are bofus):
> > 
> > The issue is that even if a function is unused, the compiler still has
> > to parse and compile the code, so where __NR_dup2 is not defined, we'll
> > get a build error for:
> > 
> > static __attribute__((unused))
> > int sys_dup2(int old, int new)
> > {
> >        return my_syscall2(__NR_dup2, old, new);
> > }
> 
> For sure but this is supposed to be used only when __NR_dup3 is not
> defined. Ah now I understand where my mistake is: after it built
> successfully for me I inspected the most recent tree which has these
> in place. Sorry for being stupid!
> 
> In my local tree it's defined like this:
> 
>  static __attribute__((unused))
>  int sys_dup2(int old, int new)
>  {
> #ifdef __NR_dup3
>        return my_syscall3(__NR_dup3, old, new, 0);
> #else
>        return my_syscall2(__NR_dup2, old, new);
> #endif
>  }

Ah, much better!

For robustness, I think it would be worth doing:

static __attribute__((unused))
int sys_dup2(int old, int new)
{
#if defined(__NR_dup3)
	return my_syscall3(__NR_dup3, old, new, 0);
#elif defined(__NR_dup2)
	return my_syscall2(__NR_dup2, old, new);
#else
#error Cannot implement dup2
#endif
}

... and getting rid of the ARCH_WANT_* definitions (which are never
legitimate for userspace to set). That way, there's no risk that we
accidentally use a bogus syscall number in future. Where the kernel does
implement a syscall, it will have done whatever is necessary to expose
the corresponding __NR_<syscall> to userspace without userspace needing
to define anything.

> I didn't want to do that because that would break userland which needs
> dup2(), hence the mapping to dup3 instead:
> 
>   static __attribute__((unused))
>   int sys_dup2(int old, int new)
>   {
>   #ifdef __NR_dup3
>           return my_syscall3(__NR_dup3, old, new, 0);
>   #else
>           return my_syscall2(__NR_dup2, old, new);
>   #endif
>   }

Sure, makes sense, though as above it might be worth adding an explicit
check for the fallback syscall number.

> I shouldn't need since these are already fixed in my tree. At first glance
> the equivalent of the following commits is missing from the kernel version:
> 
>    https://github.com/wtarreau/nolibc/commit/2379f25073f906d0bad22c48566fcffee8fc9cde
>    https://github.com/wtarreau/nolibc/commit/fd5272ec2c66e6f5b195d41c9c8978f58eb74668
>    https://github.com/wtarreau/nolibc/commit/47cc42a79c92305f4f8bc02fb28628a4fdd63eaa
>    https://github.com/wtarreau/nolibc/commit/d2dc42fd614991c741dfdc8b984864fa3cf64c8e
>    https://github.com/wtarreau/nolibc/commit/800f75c13ede49097325f82a4cca3515c44a7939
> 
> However I'm interested in knowing if the latest version fixes everything
> for you or not :
> 
>   https://raw.githubusercontent.com/wtarreau/nolibc/master/nolibc.h

I replaced my in-tree copy with that, and it built without issues, and
the tests ran correctly.

> OK thanks! I will retry here without setting those. I'm pretty sure I
> needed these ones to find the __NR_* values but it's possible that it
> was before I had the alternate ones and that these are finally not
> nedeed at all (which I would prefer as these are ugly).

Great! I reckon they're not needed at all so long as usage of each
__NR_* is suitably guarded (such as above).

If you do spot issues when removing ARCH_WANT_*, I'm happy to take a
look, and/or to test patches handling any fallout.

Thanks,
Mark.

Re: rcutorture initrd/nolibc build on ARMv8?

[Willy Tarreau]

On Wed, Jan 20, 2021 at 02:54:47PM +0000, Mark Rutland wrote:
> On Wed, Jan 20, 2021 at 03:25:00PM +0100, Willy Tarreau wrote:
> > On Wed, Jan 20, 2021 at 01:45:11PM +0000, Mark Rutland wrote:
> > > > Ah that's very interesting indeed because actually these ones should
> > > > only be used when __NR_dup3 or __NR_clone are not defined. Thus I wanted
> > > > to check the definitions that were reported in your error output but
> > > > actually what was needed was to figure whether the correct ones were
> > > > present, and they are, here on my machine (and yes I agree that in this
> > > > case the dup2/fork above are bofus):
> > > 
> > > The issue is that even if a function is unused, the compiler still has
> > > to parse and compile the code, so where __NR_dup2 is not defined, we'll
> > > get a build error for:
> > > 
> > > static __attribute__((unused))
> > > int sys_dup2(int old, int new)
> > > {
> > >        return my_syscall2(__NR_dup2, old, new);
> > > }
> > 
> > For sure but this is supposed to be used only when __NR_dup3 is not
> > defined. Ah now I understand where my mistake is: after it built
> > successfully for me I inspected the most recent tree which has these
> > in place. Sorry for being stupid!
> > 
> > In my local tree it's defined like this:
> > 
> >  static __attribute__((unused))
> >  int sys_dup2(int old, int new)
> >  {
> > #ifdef __NR_dup3
> >        return my_syscall3(__NR_dup3, old, new, 0);
> > #else
> >        return my_syscall2(__NR_dup2, old, new);
> > #endif
> >  }
> 
> Ah, much better!
> 
> For robustness, I think it would be worth doing:
> 
> static __attribute__((unused))
> int sys_dup2(int old, int new)
> {
> #if defined(__NR_dup3)
> 	return my_syscall3(__NR_dup3, old, new, 0);
> #elif defined(__NR_dup2)
> 	return my_syscall2(__NR_dup2, old, new);
> #else
> #error Cannot implement dup2
> #endif
> }

Yep I'm fine with this. We'll probably adjust between return -ENOSYS
or #error depending on the "criticity" of the syscalls. E.g. we can
miss poll() for plenty of init stuff band be done with ENOSYS but
missing dup is quickly going to cause trouble.

> ... and getting rid of the ARCH_WANT_* definitions (which are never
> legitimate for userspace to set). That way, there's no risk that we
> accidentally use a bogus syscall number in future. Where the kernel does
> implement a syscall, it will have done whatever is necessary to expose
> the corresponding __NR_<syscall> to userspace without userspace needing
> to define anything.

I'm really willing to get rid of that crap, I hated it, I vaguely
remember having gone through layers of glibc indirections when using
the pre-processed asm/* and found this to be the only way to expose
some of them. The fact that it's not needed for you is pretty much
encouraging. I'll just retry on an older libc I've used a lot (2.18)
to make sure I didn't overlook anything.

> > However I'm interested in knowing if the latest version fixes everything
> > for you or not :
> > 
> >   https://raw.githubusercontent.com/wtarreau/nolibc/master/nolibc.h
> 
> I replaced my in-tree copy with that, and it built without issues, and
> the tests ran correctly.

Perfect, thanks for the quick feedback!

> > OK thanks! I will retry here without setting those. I'm pretty sure I
> > needed these ones to find the __NR_* values but it's possible that it
> > was before I had the alternate ones and that these are finally not
> > nedeed at all (which I would prefer as these are ugly).
> 
> Great! I reckon they're not needed at all so long as usage of each
> __NR_* is suitably guarded (such as above).
> 
> If you do spot issues when removing ARCH_WANT_*, I'm happy to take a
> look, and/or to test patches handling any fallout.

Many thanks. I'm pretty confident that it's OK without them based on
your feedback but if need I'll ask you for some advices.

cheers,
Willy

Re: rcutorture initrd/nolibc build on ARMv8?

[Willy Tarreau]

On Wed, Jan 20, 2021 at 04:02:23PM +0100, Willy Tarreau wrote:
> > ... and getting rid of the ARCH_WANT_* definitions (which are never
> > legitimate for userspace to set). That way, there's no risk that we
> > accidentally use a bogus syscall number in future. Where the kernel does
> > implement a syscall, it will have done whatever is necessary to expose
> > the corresponding __NR_<syscall> to userspace without userspace needing
> > to define anything.
> 
> I'm really willing to get rid of that crap, I hated it, I vaguely
> remember having gone through layers of glibc indirections when using
> the pre-processed asm/* and found this to be the only way to expose
> some of them. The fact that it's not needed for you is pretty much
> encouraging. I'll just retry on an older libc I've used a lot (2.18)
> to make sure I didn't overlook anything.

I've now retested and can confirm that the only reason I included them
by then was to have access to these (wrong) __NR_* definitions. Now with
the fixed syscalls these ones are not needed anymore and I indeed only
see the correct definitions, so I've removed these bogus definitions.

I've completed my cleanup and tests, I'll send an updated patch set
later today after I've carefully rediffed everything to synchronise
the in-kernel version with the out-of-tree one.

Cheers,
Willy

[tip: core/rcu] tools/nolibc: Remove incorrect definitions of __ARCH_WANT_*

[tip-bot2 for Willy Tarreau]

The following commit has been merged into the core/rcu branch of tip:

Commit-ID:     f65d7117785cb8ab04f1af55909807c7eb9ed30b
Gitweb:        https://git.kernel.org/tip/f65d7117785cb8ab04f1af55909807c7eb9ed30b
Author:        Willy Tarreau <w@1wt.eu>
AuthorDate:    Thu, 21 Jan 2021 08:20:29 +01:00
Committer:     Paul E. McKenney <paulmck@kernel.org>
CommitterDate: Thu, 21 Jan 2021 10:06:45 -08:00

tools/nolibc: Remove incorrect definitions of __ARCH_WANT_*

The __ARCH_WANT_* definitions were added in order to support aarch64
when it was missing some syscall definitions (including __NR_dup2,
__NR_fork, and __NR_getpgrp), but these __ARCH_WANT_* definitions were
actually wrong because these syscalls do not exist on this platform.
Defining these resulted in exposing invalid definitions, resulting in
failures on aarch64.

The missing syscalls were since implemented based on the newer ones
(__NR_dup3,  __NR_clone, __NR_getpgid) so these incorrect __ARCH_WANT_*
definitions are no longer needed.

Thanks to Mark Rutland for spotting this incorrect analysis and
explaining why it was wrong.

This is a port of nolibc's upstream commit 00b1b0d9b2a4 to the Linux
kernel.

Reported-by: Mark Rutland <mark.rutland@arm.com>
Link: https://lore.kernel.org/lkml/20210119153147.GA5083@paulmck-ThinkPad-P72
Tested-by: Valentin Schneider <valentin.schneider@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com> [arm64]
Signed-off-by: Willy Tarreau <w@1wt.eu>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 tools/include/nolibc/nolibc.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/tools/include/nolibc/nolibc.h b/tools/include/nolibc/nolibc.h
index 611d9d1..475d956 100644
--- a/tools/include/nolibc/nolibc.h
+++ b/tools/include/nolibc/nolibc.h
@@ -81,14 +81,6 @@
  *
  */
 
-/* Some archs (at least aarch64) don't expose the regular syscalls anymore by
- * default, either because they have an "_at" replacement, or because there are
- * more modern alternatives. For now we'd rather still use them.
- */
-#define __ARCH_WANT_SYSCALL_NO_AT
-#define __ARCH_WANT_SYSCALL_NO_FLAGS
-#define __ARCH_WANT_SYSCALL_DEPRECATED
-
 #include <asm/unistd.h>
 #include <asm/ioctls.h>
 #include <asm/errno.h>