Re: [PATCH v3] Fix clone (CLONE_VM) pid/tid reset (BZ#19957)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 25/04/2016 15:18, Carlos O'Donell wrote:
> On 04/22/2016 02:54 PM, Adhemerval Zanella wrote:
>> Changes from previous version:
>>
>>   * 'Fixed' the remaning ports: alpha, microblaze, sh, ia64, tile,
>>     hppa, m68k, and nio2.  I did actually tested the fix for these
>>     architecture in specific, so any review would be appreciated.
> 
> Looks good to me.
> 
> Thanks for the new test!
> 
> Two required changes:
> * hppa changes are wrong, fixed version provided below.
> * ppc32 comment needs fixup.
> 
> Suggestions:
> * Mention bug number in test.
> * Suggest changes to test comments.

Thanks for the review.  As we discussed in IRC I think I will just commit this
change and let the machine maintainers check if tst-clone2 is really passing
on the cited architectures.

I will prob commit by the end of the week.

> 
>> diff --git a/sysdeps/unix/sysv/linux/hppa/clone.S b/sysdeps/unix/sysv/linux/hppa/clone.S
>> index 0a18137..3716acd 100644
>> --- a/sysdeps/unix/sysv/linux/hppa/clone.S
>> +++ b/sysdeps/unix/sysv/linux/hppa/clone.S

> 
> Should be:
> 
> diff --git a/sysdeps/unix/sysv/linux/hppa/clone.S b/sysdeps/unix/sysv/linux/hppa/clone.S
> index 0a18137..43f5a78 100644
> --- a/sysdeps/unix/sysv/linux/hppa/clone.S
> +++ b/sysdeps/unix/sysv/linux/hppa/clone.S
> @@ -135,17 +135,12 @@ ENTRY(__clone)
>  # define CLONE_VM_BIT          23      /* 0x00000100  */
>  # define CLONE_THREAD_BIT      15      /* 0x00010000  */
>         /* Load original clone flags.
> -          If CLONE_THREAD was passed, don't reset the PID/TID.
> -          If CLONE_VM was passed, we need to store -1 to PID/TID.
> -          If CLONE_VM and CLONE_THREAD were not set store the result
> -          of getpid to PID/TID.  */
> +          If CLONE_VM was passed, don't modify PID/TID.
> +          Otherwise store the result of getpid to PID/TID.  */
>         ldw     -56(%sp), %r26
> -       bb,<,n  %r26, CLONE_THREAD_BIT, 1f
> -       bb,<    %r26, CLONE_VM_BIT, 2f
> -       ldi     -1, %ret0
> +       bb,<,n  %r26, CLONE_VM_BIT, 1f
>         ble     0x100(%sr2, %r0)
>         ldi     __NR_getpid, %r20
> -2:
>         mfctl   %cr27, %r26
>         stw     %ret0, PID_THREAD_OFFSET(%r26)
>         stw     %ret0, TID_THREAD_OFFSET(%r26)
> ---
> 
> Your original v3 patch removes the 'ble' required for the syscall.

Ack, I will change it.

> 
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S b/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S
>> index eb973db..3761ded 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S
>> +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S
>> @@ -76,13 +76,11 @@ ENTRY (__clone)
>>  	crandc	cr1*4+eq,cr1*4+eq,cr0*4+so
>>  	bne-	cr1,L(parent)		/* The '-' is to minimise the race.  */
>>  
>> -	andis.	r0,r28,CLONE_THREAD>>16
>> -	bne+	r0,L(oldpid)
>> -	andi.	r0,r28,CLONE_VM
>> -	li	r3,-1
>> -	bne-	r0,L(nomoregetpid)
>> +	/* If CLONE_VM is set does not update the pid/tid field.  */
> 
> s/does not/do not/g

Likewise.

>> diff --git a/sysdeps/unix/sysv/linux/tst-clone2.c b/sysdeps/unix/sysv/linux/tst-clone2.c
>> new file mode 100644
>> index 0000000..7452a47
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/tst-clone2.c
>> @@ -0,0 +1,178 @@
>> +/* Test if CLONE_VM does not change pthread pid/tid field.
> 
> Mention bug #.

Likewise.

>> +int
>> +do_test (void)
>> +{
>> +  /* It first checks the clone implementation without any flag, which will
>> +     make the child new pid to be cached on its pthread structure.  */
> 
> Suggest:
> 
> /* First, check that the clone implementation, without any flag, updates
>    the struct pthread to contain the new PID and TID.  */

Likewise.

> 
>> +  int ret = clone_test (0);
>> +  /* Then check if clone with CLONE_VM avoid caching it since memory is
>> +     shared between parent and it might overwrite parent's pthread
>> +     structure.  */
> 
> Suggest:
> 
> /* Second, check that with CLONE_VM the struct pthread PID and TID fields
>    remain unmodified after the clone.  Any modifications would cause problem
>    for the parent as described in bug 19957.  */

Likewise.