Re: [PATCH 1/4] nptl: Fix Race conditions in pthread cancellation, (BZ#12683)

[Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>]

Thanks for the review, comments below.


On 19-09-2014 13:58, Joseph S. Myers wrote:
> On Fri, 19 Sep 2014, Adhemerval Zanella wrote:
>
>> +long int
>> +__syscall_cancel (long int nr, long int arg1, long int arg2, long int arg3,
>> +		  long int arg4, long int arg5, long int arg6)
>> +{
>> +  INTERNAL_SYSCALL_DECL (err);
>> +  return INTERNAL_SYSCALL_NCS (nr, err, 6, arg1, arg2, arg3, arg4, arg5, arg6);
>> +}
> Avoid hardcoding "long" as syscall argument type.  It ought to be possible 
> to use the same code for cases using "long long" (MIPS n32 at least) if 
> you allow architectures to override the choice of type.

Right, I was not aware of such ABI cases.  I will make this parametrized, defined
in arch-specific header (sysdep-cancel.h).

>
>> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
>> index aeba1ff..810deec 100644
>> --- a/nptl/pthread_cancel.c
>> +++ b/nptl/pthread_cancel.c
>> @@ -18,14 +18,12 @@
>>  
>>  #include <errno.h>
>>  #include <signal.h>
>> -#include "pthreadP.h"
>> -#include "atomic.h"
>> +#include <pthreadP.h>
>> +#include <atomic.h>
> Don't mix this sort of thing with substantive changes; send any patch for 
> such #include style changes separately.  This will reduce the size of your 
> patches....

Thanks for catching it, I though it had cleanup all modification of such type.

>>  #include <sysdep.h>
>>  
>> -
>>  int
>> -pthread_cancel (th)
>> -     pthread_t th;
>> +pthread_cancel (pthread_t th)
>>  {
> Likewise, send any such changes of style for function definitions 
> separately.

I sincerely don't think we need *another* patch for such changes.  They are fairly
mechanical and I think is is safe to use this kinda of extensive patch set to correct
this.

>
>> diff --git a/sysdeps/unix/sysv/linux/accept4.c b/sysdeps/unix/sysv/linux/accept4.c
>> index a017224..a02dc35 100644
>> --- a/sysdeps/unix/sysv/linux/accept4.c
>> +++ b/sysdeps/unix/sysv/linux/accept4.c
>> @@ -37,16 +37,8 @@
>>  int
>>  accept4 (int fd, __SOCKADDR_ARG addr, socklen_t *addr_len, int flags)
>>  {
>> -  if (SINGLE_THREAD_P)
>> -    return INLINE_SYSCALL (accept4, 4, fd, addr.__sockaddr__, addr_len, flags);
>> -
>> -  int oldtype = LIBC_CANCEL_ASYNC ();
>> -
>> -  int result = INLINE_SYSCALL (accept4, 4, fd, addr.__sockaddr__, addr_len,
>> +  int result = SYSCALL_CANCEL (accept4, fd, addr.__sockaddr__, addr_len,
>>  			       flags);
>> -
>> -  LIBC_CANCEL_RESET (oldtype);
>> -
> For cases like this, it seems to me that you should be able to do a 
> preliminary patch that defines SYSCALL_CANCEL in a way that expands to the 
> existing code using LIBC_CANCEL_ASYNC, INLINE_SYSCALL and 
> LIBC_CANCEL_RESET.  If you can define some of the new macros like that, 
> then you can change most or all such code for all architectures to the new 
> idiom (reasonably safely without being able to test on all the 
> architectures), reducing the size of the per-architecture patches needed 
> later and of the patches with the changes that actually fix the main 
> issue.

I did think about it and I wasn't sure the direction required for such extensive patch:
should we correct only a subset of architecture and let other arch maintainers fix them
eventually? Or should we aim to fix all architecture at once? I latter chose on later because
it will create less permutations.

I see no problem on work to push for such patch, but it will require more work, more testing,
and more.  And I *really* would prefer to focus such efforts on enable other arches for 
this new system and on the review itself.  For instance, ppc32-fp took me just some hours 
and 16 files changed, 102 insertions(+), 340 deletions(-).

>
>> diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h
>> index 14f5e8b..a20aa01 100644
>> --- a/sysdeps/unix/sysv/linux/not-cancel.h
>> +++ b/sysdeps/unix/sysv/linux/not-cancel.h
> These also look like changes that don't depend on the main changes.  
> Anything not depending on the main changes should be separated out and go 
> first.

Thanks, I'll split it.