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

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

On 22-09-2014 11:01, Joseph S. Myers wrote:
> On Mon, 22 Sep 2014, Adhemerval Zanella wrote:
>
>>>>  #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.
> The main changes - the ones that actually change how the library behaves, 
> and need nontrivial architecture-specific changes, and are the trickiest 
> to review - should be kept as small as possible.  That means that anything 
> that can reasonably be submitted on its own as a preliminary cleanup 
> should be.

Fair enough, I will split the function definitions change.

>>>> 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.
> The aim is to fix all architectures.  For an essentially mechanical change 
> - converting to a new set of macros - that should be feasible.  It is also 
> possible to do it bit by bit - because adding the macros doesn't break any 
> files using the old macros, it isn't necessary to convery all files in the 
> same commit, if you'd like architecture maintainers to test the 
> conversions for their architectures.

It is not just a mechanical change unfortunately: I get rid of *_[enable/disable]_asynccancel
symbol, rewrite the SIGCANCEL handler and some NPTL functions.  A preliminary 
patch to define SYSCALL_CANCEL to expand to current behavior would mean I would have
to conditionally compile the old code while providing the new one and adjust the
new behavior to work with old pieces (which is not the patch intended and I see
it as usefulness work, since the idea will to get rid of it eventually).

I am evaluating it using i386 as base and this generates more trouble than solutions.
I do know this is large patch and requires extensively changes over important
GLIBC parts. However, as bug described, I see this is an important issue the way
pthread cancellation is done now I don't see a way to smooth transition from one
way to another.  It does require a full rewrite, unfortunately.

>
>> 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(-).
> Again, the review is easier if as much as possible goes in preliminary 
> patches that pave the way for the main changes.
>