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

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 27/02/23 19:21, DJ Delorie wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>>  static void *
>>  threadproc (void *ctx)
>>  {
>> -  while (1)
>> +  for (int i = 0; i < NITER; i++)
> 
> Ok.
> 
>> -      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);
> 
> - parent up to this point has set up the LDT
> block 1 - thread waits for parent;
> 
>>        /* 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);
> 
> block 2 - child sets ldt, parent does nothing
> 
> - post, parent sets SS.  This still isn't doing what the original test
> case was doing.
> 
> The original code did this:
> 
> 
> Parent changes LDT and sets SS
> - force a task switch, which should segfault
>   - sigaction's handler either works, or fails  <-- important part
> - child resets LDT
> 
> Do you have a version of libc newer than the patch in BZ#21269 that you
> can test the modified test against, to make sure it still detects the
> failing case?

What I did was to actually revert the original fix and check if the test
in fact trigger the issue:

diff --git a/sysdeps/unix/sysv/linux/i386/libc_sigaction.c b/sysdeps/unix/sysv/linux/i386/libc_sigaction.c
index 0665b41bbc..b2454d8d55 100644
--- a/sysdeps/unix/sysv/linux/i386/libc_sigaction.c
+++ b/sysdeps/unix/sysv/linux/i386/libc_sigaction.c
@@ -33,7 +33,7 @@ extern void restore (void) asm ("__restore") attribute_hidden;
                               ? &restore_rt : &restore);       \
        }                                                       \
      else                                                      \
-       (kact)->sa_restorer = NULL;                             \
+       (kact)->sa_restorer = (void*)0xdeadbeef;                        \
   })

 #define RESET_SA_RESTORER(act, kact) \

And it does not, because as you pointed out the barrier are not fully
correct.  So I circle back and I think the v2 is actually correctly,
using a bogus sa_restore does trigger a failure and the test succeeds
with current code.

> 
> Thinking about it, I suspect this change shouldn't be done:
> 
>> -  TEST_VERIFY_EXIT (sigaction (sig, &sa, 0) == 0);
>> +  xsigaction (sig, &sa, 0);
> 
> Although they do the same things, since it's sigaction we're actually
> testing here, hiding it in an xfunction isn't appropriate.
> 
Alright.