> -----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
