RE: [PATCH] kselftest: Minimise dependency of get_size on C library interfaces

[Tim.Bird]

> -----Original Message-----
> From: Masami Hiramatsu <masami.hiramatsu@xxxxxxxxxx>
> Sent: Tuesday, January 14, 2020 8:14 PM
> To: Siddhesh Poyarekar <siddhesh@xxxxxxxxxx>
> Cc: linux-kselftest@xxxxxxxxxxxxxxx; Shuah Khan <shuah@xxxxxxxxxx>; Linux kernel mailing list <linux-kernel@xxxxxxxxxxxxxxx>; Bird, Tim
> <Tim.Bird@xxxxxxxx>
> Subject: Re: [PATCH] kselftest: Minimise dependency of get_size on C library interfaces
> 
> Hi,
> 
> [Cc: Tim Bird]
> 
> 2020å1æ14æ(ç) 1:43 Siddhesh Poyarekar <siddhesh@xxxxxxxxxx>:
> 
> >
> > It was observed[1] on arm64 that __builtin_strlen led to an infinite
> > loop in the get_size selftest.  This is because __builtin_strlen (and
> > other builtins) may sometimes result in a call to the C library
> > function.  The C library implementation of strlen uses an IFUNC
> > resolver to load the most efficient strlen implementation for the
> > underlying machine and hence has a PLT indirection even for static
> > binaries.  Because this binary avoids the C library startup routines,
> > the PLT initialization never happens and hence the program gets stuck
> > in an infinite loop.
> >
> > On x86_64 the __builtin_strlen just happens to expand inline and avoid
> > the call but that is not always guaranteed.
> >
> > Further, while testing on x86_64 (Fedora 31), it was observed that the
> > test also failed with a segfault inside write() because the generated
> > code for the write function in glibc seems to access TLS before the
> > syscall (probably due to the cancellation point check) and fails
> > because TLS is not initialised.
> >
> > To mitigate these problems, this patch reduces the interface with the
> > C library to just the syscall function.  The syscall function still
> > sets errno on failure, which is undesirable but for now it only
> > affects cases where syscalls fail.
> >
> > [1] https://bugs.linaro.org/show_bug.cgi?id=5479
> >
> 
> Thank you for the fix! I confirmed this fixes the issue.
> 
> ----
> root@devnote2:/opt/kselftest/size# ./get_size
> TAP version 13
> # Testing system size.
> ok 1 get runtime memory use
> # System runtime memory report (units in Kilobytes):
>  ---
>  Total:  16085116
>  Free:   2042880
>  Buffer: 814052
>  In use: 13228184
>  ...
> 1..1
> ----
> 
> Tested-by: Masami Hiramatsu <masami.hiramatsu@xxxxxxxxxx>
> 
> 
> > Signed-off-by: Siddhesh Poyarekar <siddhesh@xxxxxxxxxx>
> > Reported-by: Masami Hiramatsu <masami.hiramatsu@xxxxxxxxxx>
> > ---
> >  tools/testing/selftests/size/get_size.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/size/get_size.c b/tools/testing/selftests/size/get_size.c
> > index d4b59ab979a0..f55943b6d1e2 100644
> > --- a/tools/testing/selftests/size/get_size.c
> > +++ b/tools/testing/selftests/size/get_size.c
> > @@ -12,23 +12,35 @@
> >   * own execution.  It also attempts to have as few dependencies
> >   * on kernel features as possible.
> >   *
> > - * It should be statically linked, with startup libs avoided.
> > - * It uses no library calls, and only the following 3 syscalls:
> > + * It should be statically linked, with startup libs avoided.  It uses
> > + * no library calls except the syscall() function for the following 3
> > + * syscalls:
> >   *   sysinfo(), write(), and _exit()
> >   *
> >   * For output, it avoids printf (which in some C libraries
> >   * has large external dependencies) by  implementing it's own
> >   * number output and print routines, and using __builtin_strlen()

Since the code no longer uses __builtin_strlen(), this comment should
change also, to say something like "and string length function.

> > + *
> > + * The test may crash if any of the above syscalls fails because in some
> > + * libc implementations (e.g. the GNU C Library) errno is saved in
> > + * thread-local storage, which does not get initialized due to avoiding
> > + * startup libs.
> >   */
> >
> >  #include <sys/sysinfo.h>
> >  #include <unistd.h>
> > +#include <sys/syscall.h>
> >
> >  #define STDOUT_FILENO 1
> >
> >  static int print(const char *s)
> >  {
> > -       return write(STDOUT_FILENO, s, __builtin_strlen(s));
> > +       size_t len = 0;
> > +
> > +       while (s[len] != '\0')
> > +               len++;
> > +
> > +       return syscall(SYS_write, STDOUT_FILENO, s, len);
> >  }
> >
> >  static inline char *num_to_str(unsigned long num, char *buf, int len)
> > @@ -80,12 +92,12 @@ void _start(void)
> >         print("TAP version 13\n");
> >         print("# Testing system size.\n");
> >
> > -       ccode = sysinfo(&info);
> > +       ccode = syscall(SYS_sysinfo, &info);
> >         if (ccode < 0) {
> >                 print("not ok 1");
> >                 print(test_name);
> >                 print(" ---\n reason: \"could not get sysinfo\"\n ...\n");
> > -               _exit(ccode);
> > +               syscall(SYS_exit, ccode);
> >         }
> >         print("ok 1");
> >         print(test_name);
> > @@ -101,5 +113,5 @@ void _start(void)
> >         print(" ...\n");
> >         print("1..1\n");
> >
> > -       _exit(0);
> > +       syscall(SYS_exit, 0);
> >  }
> > --
> > 2.24.1

Thanks very much for this bug report and fix!!

Reviewed-by: Tim Bird <tim.bird@xxxxxxxx>

 -- Tim