Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] kselftest: Minimise dependency of get_size on C library interfaces
@ 2020-01-13 16:41 Siddhesh Poyarekar
  2020-01-15  6:14 ` Masami Hiramatsu
  0 siblings, 1 reply; 4+ messages in thread
From: Siddhesh Poyarekar @ 2020-01-13 16:41 UTC (permalink / raw)
  To: linux-kselftest
  Cc: Shuah Khan, linux-kernel, Siddhesh Poyarekar, Masami Hiramatsu

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

Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
Reported-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 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()
+ *
+ * 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] kselftest: Minimise dependency of get_size on C library interfaces
  2020-01-13 16:41 [PATCH] kselftest: Minimise dependency of get_size on C library interfaces Siddhesh Poyarekar
@ 2020-01-15  6:14 ` Masami Hiramatsu
  2020-01-16 17:32   ` Tim.Bird
  0 siblings, 1 reply; 4+ messages in thread
From: Masami Hiramatsu @ 2020-01-15  6:14 UTC (permalink / raw)
  To: Siddhesh Poyarekar
  Cc: linux-kselftest, Shuah Khan, Linux kernel mailing list, Tim Bird

Hi,

[Cc: Tim Bird]

2020年1月14日(火) 1:43 Siddhesh Poyarekar <siddhesh@gotplt.org>:

>
> 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@linaro.org>


> Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
> Reported-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
>  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()
> + *
> + * 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
>


--
Masami Hiramatsu

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] kselftest: Minimise dependency of get_size on C library interfaces
  2020-01-15  6:14 ` Masami Hiramatsu
@ 2020-01-16 17:32   ` Tim.Bird
  2020-01-17  5:21     ` Siddhesh Poyarekar
  0 siblings, 1 reply; 4+ messages in thread
From: Tim.Bird @ 2020-01-16 17:32 UTC (permalink / raw)
  To: masami.hiramatsu, siddhesh; +Cc: linux-kselftest, shuah, linux-kernel



> -----Original Message-----
> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> Sent: Tuesday, January 14, 2020 8:14 PM
> To: Siddhesh Poyarekar <siddhesh@gotplt.org>
> Cc: linux-kselftest@vger.kernel.org; Shuah Khan <shuah@kernel.org>; Linux kernel mailing list <linux-kernel@vger.kernel.org>; Bird, Tim
> <Tim.Bird@sony.com>
> 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@gotplt.org>:
> 
> >
> > 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@linaro.org>
> 
> 
> > Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
> > Reported-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > ---
> >  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@sony.com>

 -- Tim


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] kselftest: Minimise dependency of get_size on C library interfaces
  2020-01-16 17:32   ` Tim.Bird
@ 2020-01-17  5:21     ` Siddhesh Poyarekar
  0 siblings, 0 replies; 4+ messages in thread
From: Siddhesh Poyarekar @ 2020-01-17  5:21 UTC (permalink / raw)
  To: Tim.Bird, masami.hiramatsu; +Cc: linux-kselftest, shuah, linux-kernel

On 16/01/20 11:02 pm, Tim.Bird@sony.com wrote:
> 
> Since the code no longer uses __builtin_strlen(), this comment should
> change also, to say something like "and string length function.
> 

Oops, I had made the change (to drop the __builtin_strlen mention since
the length is just computed inline now) and somehow dropped it in some
copy-paste confusion.  Looks like a patchwork-bot applied my patch, so
should I send an updated patch or just a comment fix follow-up?

Thanks,
Siddhesh

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-13 16:41 [PATCH] kselftest: Minimise dependency of get_size on C library interfaces Siddhesh Poyarekar
2020-01-15  6:14 ` Masami Hiramatsu
2020-01-16 17:32   ` Tim.Bird
2020-01-17  5:21     ` Siddhesh Poyarekar

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org
	public-inbox-index linux-kselftest

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git