[PATCH v4] i386: Use pthread_barrier for synchronization on tst-bz21269

[PATCH v4] i386: Use pthread_barrier for synchronization on tst-bz21269

[Adhemerval Zanella]

To improve the false negative in patchwork buildbot.

Checked on i686-linux-gnu and I also checked by reverting the original
bz21269 fix to pass a bogus sa_restore value (and thus triggering the
test failure).
---
 sysdeps/unix/sysv/linux/i386/tst-bz21269.c | 62 +++++++---------------
 1 file changed, 19 insertions(+), 43 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/i386/tst-bz21269.c b/sysdeps/unix/sysv/linux/i386/tst-bz21269.c
index 1850240c34..deaf9e5ef8 100644
--- a/sysdeps/unix/sysv/linux/i386/tst-bz21269.c
+++ b/sysdeps/unix/sysv/linux/i386/tst-bz21269.c
@@ -20,16 +20,13 @@
    more specifically in do_multicpu_tests function.  The main changes
    are:
 
-   - C11 atomics instead of plain access.
+   - Use pthread_barrier instead of atomic and futexes.
    - Remove x86_64 support which simplifies the syscall handling
      and fallbacks.
    - Replicate only the test required to trigger the issue for the
      BZ#21269.  */
 
-#include <stdatomic.h>
-
 #include <asm/ldt.h>
-#include <linux/futex.h>
 
 #include <setjmp.h>
 #include <signal.h>
@@ -41,6 +38,8 @@
 #include <support/check.h>
 #include <support/xthread.h>
 
+#define NITER 5
+
 static int
 xset_thread_area (struct user_desc *u_info)
 {
@@ -55,13 +54,6 @@ xmodify_ldt (int func, const void *ptr, unsigned long bytecount)
   TEST_VERIFY_EXIT (syscall (SYS_modify_ldt, 1, ptr, bytecount) == 0);
 }
 
-static int
-futex (int *uaddr, int futex_op, int val, void *timeout, int *uaddr2,
-	int val3)
-{
-  return syscall (SYS_futex, uaddr, futex_op, val, timeout, uaddr2, val3);
-}
-
 static void
 xsethandler (int sig, void (*handler)(int, siginfo_t *, void *), int flags)
 {
@@ -123,33 +115,24 @@ setup_low_user_desc (void)
   low_user_desc_clear->seg_not_present = 1;
 }
 
-/* Possible values of futex:
-   0: thread is idle.
-   1: thread armed.
-   2: thread should clear LDT entry 0.
-   3: thread should exit.  */
-static atomic_uint ftx;
+static pthread_barrier_t barrier;
 
 static void *
 threadproc (void *ctx)
 {
-  while (1)
+  for (int i = 0; i < NITER; i++)
     {
-      futex ((int *) &ftx, FUTEX_WAIT, 1, NULL, NULL, 0);
-      while (atomic_load (&ftx) != 2)
-	{
-	  if (atomic_load (&ftx) >= 3)
-	    return NULL;
-	}
+      xpthread_barrier_wait (&barrier);
 
       /* clear LDT entry 0.  */
       const struct user_desc desc = { 0 };
       xmodify_ldt (1, &desc, sizeof (desc));
 
-      /* If ftx == 2, set it to zero,  If ftx == 100, quit.  */
-      if (atomic_fetch_add (&ftx, -2) != 2)
-	return NULL;
+      /* Wait for 'ss' set in main thread.  */
+      xpthread_barrier_wait (&barrier);
     }
+
+  return NULL;
 }
 
 
@@ -180,20 +163,21 @@ do_test (void)
   /* Some kernels send SIGBUS instead.  */
   xsethandler (SIGBUS, sigsegv_handler, 0);
 
+  xpthread_barrier_init (&barrier, NULL, 2);
+
   thread = xpthread_create (0, threadproc, 0);
 
   asm volatile ("mov %%ss, %0" : "=rm" (orig_ss));
 
-  for (int i = 0; i < 5; i++)
+  for (int i = 0; i < NITER; i++)
     {
       if (sigsetjmp (jmpbuf, 1) != 0)
 	continue;
 
       /* Make sure the thread is ready after the last test. */
-      while (atomic_load (&ftx) != 0)
-	;
+      xpthread_barrier_wait (&barrier);
 
-      struct user_desc desc = {
+      const struct user_desc desc = {
 	.entry_number       = 0,
 	.base_addr          = 0,
 	.limit              = 0xffff,
@@ -207,28 +191,20 @@ do_test (void)
 
       xmodify_ldt (0x11, &desc, sizeof (desc));
 
-      /* Arm the thread.  */
-      ftx = 1;
-      futex ((int*) &ftx, FUTEX_WAKE, 0, NULL, NULL, 0);
+      /* Wait thread clear LDT entry 0.  */
+      xpthread_barrier_wait (&barrier);
 
       asm volatile ("mov %0, %%ss" : : "r" (0x7));
 
-      /* Fire up thread modify_ldt call.  */
-      atomic_store (&ftx, 2);
-
-      while (atomic_load (&ftx) != 0)
-	;
-
       /* On success, modify_ldt will segfault us synchronously and we will
 	 escape via siglongjmp.  */
       support_record_failure ();
     }
 
-  atomic_store (&ftx, 100);
-  futex ((int*) &ftx, FUTEX_WAKE, 0, NULL, NULL, 0);
-
   xpthread_join (thread);
 
+  xpthread_barrier_destroy (&barrier);
+
   return 0;
 }
 
-- 
2.34.1

Re: [PATCH v4] i386: Use pthread_barrier for synchronization on tst-bz21269

[DJ Delorie]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>  static void *
>  threadproc (void *ctx)
>  {
> -  while (1)
> +  for (int i = 0; i < NITER; i++)
>      {
> -      futex ((int *) &ftx, FUTEX_WAIT, 1, NULL, NULL, 0);
> -      while (atomic_load (&ftx) != 2)
> -	{
> -	  if (atomic_load (&ftx) >= 3)
> -	    return NULL;
> -	}
> +      xpthread_barrier_wait (&barrier);
>  
>        /* clear LDT entry 0.  */
>        const struct user_desc desc = { 0 };
>        xmodify_ldt (1, &desc, sizeof (desc));
>  
> -      /* If ftx == 2, set it to zero,  If ftx == 100, quit.  */
> -      if (atomic_fetch_add (&ftx, -2) != 2)
> -	return NULL;
> +      /* Wait for 'ss' set in main thread.  */
> +      xpthread_barrier_wait (&barrier);
>      }
> +
> +  return NULL;
>  }
>  
>  
> @@ -180,20 +163,21 @@ do_test (void)
> -  for (int i = 0; i < 5; i++)
> +  for (int i = 0; i < NITER; i++)
>      {
>        if (sigsetjmp (jmpbuf, 1) != 0)
>  	continue;
>  
>        /* Make sure the thread is ready after the last test. */
> -      while (atomic_load (&ftx) != 0)
> -	;
> +      xpthread_barrier_wait (&barrier);
>  
> -      struct user_desc desc = {
> +      const struct user_desc desc = {
>  	.entry_number       = 0,
>  	.base_addr          = 0,
>  	.limit              = 0xffff,
> @@ -207,28 +191,20 @@ do_test (void)
>  
>        xmodify_ldt (0x11, &desc, sizeof (desc));
>  
> -      /* Arm the thread.  */
> -      ftx = 1;
> -      futex ((int*) &ftx, FUTEX_WAKE, 0, NULL, NULL, 0);
> +      /* Wait thread clear LDT entry 0.  */
> +      xpthread_barrier_wait (&barrier);
>  
>        asm volatile ("mov %0, %%ss" : : "r" (0x7));
>  
> -      /* Fire up thread modify_ldt call.  */
> -      atomic_store (&ftx, 2);
> -
> -      while (atomic_load (&ftx) != 0)
> -	;
> -
>        /* On success, modify_ldt will segfault us synchronously and we will
>  	 escape via siglongjmp.  */
>        support_record_failure ();
>      }

this does...

child		parent
-----		------

<nothing>	setjmp

barrier	-------	barrier

clear LDT	set LDT		<-- these happen at the "same time"

barrier	-------	barrier

<nothing>	set ss
		support_record_failure (which hopefully segfaults before
			recording failure


IIRC what's supposed to happen (based on the original test) is:

<nothing>	setjmp

barrier	-------	barrier

		set LDT
<nothing>	set ss

 -- do some syscall --

clear LDT	-- longjmp


When the test fails, it does this:
		setjmp
		set LDT
		set ss
- task switch -
segfaults

Part of me wonders if the taken longjmp causes one of the barriers to be
skipped, putting the test out of sync.  If so, we won't be able to use
barriers this way.

Ok so I did some testing on the original test case, and it seems that
the first syscall after setting %ss causes the signal, handler, and
longjmp.  Which means two things:

1. The test must have some sort of forced syscall (INT3) or other
   stack-using operation after setting %ss, to trigger the test, and

2. At that point, the parent longjumps to the top of the loop

I'm not sure how we can satisfy both of these with pthread barriers,
since the one on the parent side would fault and not sync with the child
side.

Atomics do no syscalls so do not care if %ss is bogus.

However, I also found a bug in the original code:

      futex ((int*) &ftx, FUTEX_WAKE, 0, NULL, NULL, 0);

Per the manual, this wakes up at most zero threads.  The child thread
avoids this because ftx is never set right and it's FUTEX_WAIT always
returns with EAGAIN - except when it is right, and hangs.

/me is still investigating...

Re: [PATCH v4] i386: Use pthread_barrier for synchronization on tst-bz21269

[Adhemerval Zanella Netto]

On 01/03/23 18:37, DJ Delorie wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>  static void *
>>  threadproc (void *ctx)
>>  {
>> -  while (1)
>> +  for (int i = 0; i < NITER; i++)
>>      {
>> -      futex ((int *) &ftx, FUTEX_WAIT, 1, NULL, NULL, 0);
>> -      while (atomic_load (&ftx) != 2)
>> -	{
>> -	  if (atomic_load (&ftx) >= 3)
>> -	    return NULL;
>> -	}
>> +      xpthread_barrier_wait (&barrier);
>>  
>>        /* clear LDT entry 0.  */
>>        const struct user_desc desc = { 0 };
>>        xmodify_ldt (1, &desc, sizeof (desc));
>>  
>> -      /* If ftx == 2, set it to zero,  If ftx == 100, quit.  */
>> -      if (atomic_fetch_add (&ftx, -2) != 2)
>> -	return NULL;
>> +      /* Wait for 'ss' set in main thread.  */
>> +      xpthread_barrier_wait (&barrier);
>>      }
>> +
>> +  return NULL;
>>  }
>>  
>>  
>> @@ -180,20 +163,21 @@ do_test (void)
>> -  for (int i = 0; i < 5; i++)
>> +  for (int i = 0; i < NITER; i++)
>>      {
>>        if (sigsetjmp (jmpbuf, 1) != 0)
>>  	continue;
>>  
>>        /* Make sure the thread is ready after the last test. */
>> -      while (atomic_load (&ftx) != 0)
>> -	;
>> +      xpthread_barrier_wait (&barrier);
>>  
>> -      struct user_desc desc = {
>> +      const struct user_desc desc = {
>>  	.entry_number       = 0,
>>  	.base_addr          = 0,
>>  	.limit              = 0xffff,
>> @@ -207,28 +191,20 @@ do_test (void)
>>  
>>        xmodify_ldt (0x11, &desc, sizeof (desc));
>>  
>> -      /* Arm the thread.  */
>> -      ftx = 1;
>> -      futex ((int*) &ftx, FUTEX_WAKE, 0, NULL, NULL, 0);
>> +      /* Wait thread clear LDT entry 0.  */
>> +      xpthread_barrier_wait (&barrier);
>>  
>>        asm volatile ("mov %0, %%ss" : : "r" (0x7));
>>  
>> -      /* Fire up thread modify_ldt call.  */
>> -      atomic_store (&ftx, 2);
>> -
>> -      while (atomic_load (&ftx) != 0)
>> -	;
>> -
>>        /* On success, modify_ldt will segfault us synchronously and we will
>>  	 escape via siglongjmp.  */
>>        support_record_failure ();
>>      }
> 
> this does...
> 
> child		parent
> -----		------
> 
> <nothing>	setjmp
> 
> barrier	-------	barrier
> 
> clear LDT	set LDT		<-- these happen at the "same time"

Yeah, ant is is clearly wrong...

> 
> barrier	-------	barrier
> 
> <nothing>	set ss
> 		support_record_failure (which hopefully segfaults before
> 			recording failure
> 
> 
> IIRC what's supposed to happen (based on the original test) is:
> 
> <nothing>	setjmp
> 
> barrier	-------	barrier
> 
> 		set LDT
> <nothing>	set ss
> 
>  -- do some syscall --
> 
> clear LDT	-- longjmp
> 
> 
> When the test fails, it does this:
> 		setjmp
> 		set LDT
> 		set ss
> - task switch -
> segfaults
> 
> Part of me wonders if the taken longjmp causes one of the barriers to be
> skipped, putting the test out of sync.  If so, we won't be able to use
> barriers this way.
> 
> Ok so I did some testing on the original test case, and it seems that
> the first syscall after setting %ss causes the signal, handler, and
> longjmp.  Which means two things:
> 
> 1. The test must have some sort of forced syscall (INT3) or other
>    stack-using operation after setting %ss, to trigger the test, and
> 
> 2. At that point, the parent longjumps to the top of the loop
> 
> I'm not sure how we can satisfy both of these with pthread barriers,
> since the one on the parent side would fault and not sync with the child
> side.
> 
> Atomics do no syscalls so do not care if %ss is bogus.

It makes sense, the problem is indeed the stack access that triggers
the expected failure.

> 
> However, I also found a bug in the original code:
> 
>       futex ((int*) &ftx, FUTEX_WAKE, 0, NULL, NULL, 0);
> 
> Per the manual, this wakes up at most zero threads.  The child thread
> avoids this because ftx is never set right and it's FUTEX_WAIT always
> returns with EAGAIN - except when it is right, and hangs.
> 
> /me is still investigating...
> 

Another possibility I was wondering was to just remove this test altogether,
it is already covered by kernel testing code.