[PATCH] Fix tst-setcontext9 for optimized small stacks.

[PATCH] Fix tst-setcontext9 for optimized small stacks.

[Carlos O'Donell]

[-- Attachment #1: Type: text/plain, Size: 1121 bytes --]

HJ,

Can I get a Reviewed-by from you for this? It looks like I'm keeping
the relative structure of the original test, but I'm not sure what
your intent was here with respect to CET testing. I'm seeing crashes
in this test on i686 testing as described in the commit message.

~~~
If the compiler reduces the stack usage in function f1 before calling
into function f2, then when we swapcontext back to f1 and continue
execution we may overwrite registers that were spilled to the stack
while f2 was executing.  Later when we return to f2 the corrupt
registers will be reloaded from the stack and the test will crash.  This
was most commonly observed on i686 with __x86.get_pc_thunk.dx and
needing to save and restore $edx.  Overall i686 has few registers and
the spilling to the stack is bound to happen, therefore the solution to
making this test robust is to split function f1 into two parts f1a and
f1b, and allocate f1b it's own stack such that subsequent execution does
not overwrite the stack in use by function f2.

Tested on i686 and x86_64.

Signed-off-by: Carlos O'Donell <carlos@redhat.com>
-- 
Cheers,
Carlos.

[-- Attachment #2: 0001-Fix-tst-setcontext9-for-optimized-small-stacks.patch --]
[-- Type: text/x-patch, Size: 3649 bytes --]

From 52d90e0548bee31f3f1cb62d63f40f31de5b1d97 Mon Sep 17 00:00:00 2001
From: Carlos O'Donell <carlos@redhat.com>
Date: Wed, 5 Sep 2018 01:16:42 -0400
Subject: [PATCH] Fix tst-setcontext9 for optimized small stacks.

If the compiler reduces the stack usage in function f1 before calling
into function f2, then when we swapcontext back to f1 and continue
execution we may overwrite registers that were spilled to the stack
while f2 was executing.  Later when we return to f2 the corrupt
registers will be reloaded from the stack and the test will crash.  This
was most commonly observed on i686 with __x86.get_pc_thunk.dx and
needing to save and restore $edx.  Overall i686 has few registers and
the spilling to the stack is bound to happen, therefore the solution to
making this test robust is to split function f1 into two parts f1a and
f1b, and allocate f1b it's own stack such that subsequent execution does
not overwrite the stack in use by function f2.

Tested on i686 and x86_64.

Signed-off-by: Carlos O'Donell <carlos@redhat.com>
---
 ChangeLog                |  6 +++++
 stdlib/tst-setcontext9.c | 47 ++++++++++++++++++++++++++++++++--------
 2 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5f588784cb..7e03407a50 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2018-09-18  Carlos O'Donell  <carlos@redhat.com>
+
+	* stdlib/tst-setcontext9.c (f1): Rename to...
+	(f1a): ... this.
+	(f1b): New function implementing lower half of f1 in alternate stack.
+
 2018-09-18  Joseph Myers  <joseph@codesourcery.com>
 
 	* sysdeps/ieee754/ldbl-128ibm/s_ceill.c (ceil): Redirect to
diff --git a/stdlib/tst-setcontext9.c b/stdlib/tst-setcontext9.c
index 4636ce9030..db8355766c 100644
--- a/stdlib/tst-setcontext9.c
+++ b/stdlib/tst-setcontext9.c
@@ -41,26 +41,55 @@ f2 (void)
 }
 
 static void
-f1 (void)
+f1b (void)
 {
-  puts ("start f1");
-  if (getcontext (&ctx[2]) != 0)
-    {
-      printf ("%s: getcontext: %m\n", __FUNCTION__);
-      exit (EXIT_FAILURE);
-    }
   if (done)
     {
-      puts ("set context in f1");
+      puts ("set context in f1b");
       if (setcontext (&ctx[3]) != 0)
 	{
 	  printf ("%s: setcontext: %m\n", __FUNCTION__);
 	  exit (EXIT_FAILURE);
 	}
     }
+  exit (EXIT_FAILURE);
+}
+
+static void
+f1a (void)
+{
+  char st2[32768];
+  puts ("start f1a");
+  if (getcontext (&ctx[2]) != 0)
+    {
+      printf ("%s: getcontext: %m\n", __FUNCTION__);
+      exit (EXIT_FAILURE);
+    }
+  ctx[2].uc_stack.ss_sp = st2;
+  ctx[2].uc_stack.ss_size = sizeof st2;
+  ctx[2].uc_link = &ctx[0];
+  makecontext (&ctx[2], (void (*) (void)) f1b, 0);
   f2 ();
 }
 
+/* The execution path through the test looks like this:
+   do_test (call)
+   -> "making contexts"
+   -> "swap contexts"
+   f1a (via swapcontext to ctx[1], with alternate stack)
+   -> "start f1a"
+   f2 (call)
+   -> "swap contexts in f2"
+   f1b (via swapcontext to ctx[2], with alternate stack)
+   -> "set context in f1b"
+   do_test (via setcontext to ctx[3], main stack)
+   -> "setcontext"
+   f2 (via setcontext to ctx[4], with alternate stack)
+   -> "end f2"
+
+   We must use an alternate stack for f1b, because if we don't then the
+   result of executing an earlier caller may overwrite registers
+   spilled to the stack in f2.  */
 static int
 do_test (void)
 {
@@ -79,7 +108,7 @@ do_test (void)
   ctx[1].uc_stack.ss_sp = st1;
   ctx[1].uc_stack.ss_size = sizeof st1;
   ctx[1].uc_link = &ctx[0];
-  makecontext (&ctx[1], (void (*) (void)) f1, 0);
+  makecontext (&ctx[1], (void (*) (void)) f1a, 0);
   puts ("swap contexts");
   if (swapcontext (&ctx[3], &ctx[1]) != 0)
     {
-- 
2.17.1

Re: [PATCH] Fix tst-setcontext9 for optimized small stacks.

[H.J. Lu]

On Tue, Sep 18, 2018 at 7:30 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> HJ,
>
> Can I get a Reviewed-by from you for this? It looks like I'm keeping
> the relative structure of the original test, but I'm not sure what
> your intent was here with respect to CET testing. I'm seeing crashes
> in this test on i686 testing as described in the commit message.
>
> ~~~
> If the compiler reduces the stack usage in function f1 before calling
> into function f2, then when we swapcontext back to f1 and continue
> execution we may overwrite registers that were spilled to the stack
> while f2 was executing.  Later when we return to f2 the corrupt
> registers will be reloaded from the stack and the test will crash.  This
> was most commonly observed on i686 with __x86.get_pc_thunk.dx and
> needing to save and restore $edx.  Overall i686 has few registers and
> the spilling to the stack is bound to happen, therefore the solution to
> making this test robust is to split function f1 into two parts f1a and
> f1b, and allocate f1b it's own stack such that subsequent execution does
> not overwrite the stack in use by function f2.
>
> Tested on i686 and x86_64.
>
> Signed-off-by: Carlos O'Donell <carlos@redhat.com>
>

OK.

Reviewed-by: H.J. Lu <hjl.tools@gmail.com>

Thanks.

-- 
H.J.

Re: [PATCH] Fix tst-setcontext9 for optimized small stacks.

[Florian Weimer]

* Carlos O'Donell:

> From 52d90e0548bee31f3f1cb62d63f40f31de5b1d97 Mon Sep 17 00:00:00 2001
> From: Carlos O'Donell <carlos@redhat.com>
> Date: Wed, 5 Sep 2018 01:16:42 -0400
> Subject: [PATCH] Fix tst-setcontext9 for optimized small stacks.

Even with this commit, the test still fails on POWER.  I filed:

  <https://sourceware.org/bugzilla/show_bug.cgi?id=23717>

Thanks,
Florian

Re: [PATCH] Fix tst-setcontext9 for optimized small stacks.

[Carlos O'Donell]

On 9/26/18 6:26 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> From 52d90e0548bee31f3f1cb62d63f40f31de5b1d97 Mon Sep 17 00:00:00 2001
>> From: Carlos O'Donell <carlos@redhat.com>
>> Date: Wed, 5 Sep 2018 01:16:42 -0400
>> Subject: [PATCH] Fix tst-setcontext9 for optimized small stacks.
> 
> Even with this commit, the test still fails on POWER.  I filed:
> 
>   <https://sourceware.org/bugzilla/show_bug.cgi?id=23717>
> 

Does it run out of stack?

-- 
Cheers,
Carlos.

Re: [PATCH] Fix tst-setcontext9 for optimized small stacks.

[Florian Weimer]

* Carlos O'Donell:

> On 9/26/18 6:26 PM, Florian Weimer wrote:
>> * Carlos O'Donell:
>> 
>>> From 52d90e0548bee31f3f1cb62d63f40f31de5b1d97 Mon Sep 17 00:00:00 2001
>>> From: Carlos O'Donell <carlos@redhat.com>
>>> Date: Wed, 5 Sep 2018 01:16:42 -0400
>>> Subject: [PATCH] Fix tst-setcontext9 for optimized small stacks.
>> 
>> Even with this commit, the test still fails on POWER.  I filed:
>> 
>>   <https://sourceware.org/bugzilla/show_bug.cgi?id=23717>
>> 
>
> Does it run out of stack?

I don't know.  How can I tell?

Thanks,
Florian

Re: [PATCH] Fix tst-setcontext9 for optimized small stacks.

[Andreas Schwab]

On Sep 26 2018, Carlos O'Donell <carlos@redhat.com> wrote:

> Does it run out of stack?

Definitely.

Andreas.

	[BZ #23717]
	* stdlib/tst-setcontext9.c (f1a): Make st2 static.
	(do_test): Make st1 static.
---
 stdlib/tst-setcontext9.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/stdlib/tst-setcontext9.c b/stdlib/tst-setcontext9.c
index db8355766c..009928235d 100644
--- a/stdlib/tst-setcontext9.c
+++ b/stdlib/tst-setcontext9.c
@@ -58,7 +58,7 @@ f1b (void)
 static void
 f1a (void)
 {
-  char st2[32768];
+  static char st2[32768];
   puts ("start f1a");
   if (getcontext (&ctx[2]) != 0)
     {
@@ -93,7 +93,7 @@ f1a (void)
 static int
 do_test (void)
 {
-  char st1[32768];
+  static char st1[32768];
   puts ("making contexts");
   if (getcontext (&ctx[0]) != 0)
     {
-- 
2.19.0


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix tst-setcontext9 for optimized small stacks.

[Florian Weimer]

* Andreas Schwab:

> On Sep 26 2018, Carlos O'Donell <carlos@redhat.com> wrote:
>
>> Does it run out of stack?
>
> Definitely.
>
> Andreas.
>
> 	[BZ #23717]
> 	* stdlib/tst-setcontext9.c (f1a): Make st2 static.
> 	(do_test): Make st1 static.

I'm happy to report that this fixes powerpc64le for me.  The change
looks reasonable.  Maybe you coulkd mention somewhere (in the ChangeLog
entry or the commit message) that this is to reduce stack usage and
avoid stack overflows.

Thanks,
Florian

Re: [PATCH] Fix tst-setcontext9 for optimized small stacks.

[Carlos O'Donell]

On 9/27/18 5:16 AM, Andreas Schwab wrote:
> On Sep 26 2018, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> Does it run out of stack?
> 
> Definitely.

Thanks for fixing this. I was a dummy.

> Andreas.
> 
> 	[BZ #23717]
> 	* stdlib/tst-setcontext9.c (f1a): Make st2 static.
> 	(do_test): Make st1 static.
> ---
>  stdlib/tst-setcontext9.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/stdlib/tst-setcontext9.c b/stdlib/tst-setcontext9.c
> index db8355766c..009928235d 100644
> --- a/stdlib/tst-setcontext9.c
> +++ b/stdlib/tst-setcontext9.c
> @@ -58,7 +58,7 @@ f1b (void)
>  static void
>  f1a (void)
>  {
> -  char st2[32768];
> +  static char st2[32768];
>    puts ("start f1a");
>    if (getcontext (&ctx[2]) != 0)
>      {
> @@ -93,7 +93,7 @@ f1a (void)
>  static int
>  do_test (void)
>  {
> -  char st1[32768];
> +  static char st1[32768];
>    puts ("making contexts");
>    if (getcontext (&ctx[0]) != 0)
>      {
> 


-- 
Cheers,
Carlos.