This is the mail archive of the
mailing list for the glibc project.
Re: [PATCH 1/4] nptl: Fix Race conditions in pthread cancellation, (BZ#12683)
- From: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- To: "Joseph S. Myers" <joseph at codesourcery dot com>
- Cc: "GNU C. Library" <libc-alpha at sourceware dot org>
- Date: Mon, 22 Sep 2014 10:14:58 -0300
- Subject: Re: [PATCH 1/4] nptl: Fix Race conditions in pthread cancellation, (BZ#12683)
- Authentication-results: sourceware.org; auth=none
- References: <541C2901 dot 1050609 at linux dot vnet dot ibm dot com> <541C2DC8 dot 3010505 at linux dot vnet dot ibm dot com> <Pine dot LNX dot 4 dot 64 dot 1409191646050 dot 11496 at digraph dot polyomino dot org dot uk>
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
Thanks for catching it, I though it had cleanup all modification of such type.
>> #include <sysdep.h>
>> -pthread_cancel (th)
>> - pthread_t th;
>> +pthread_cancel (pthread_t th)
> Likewise, send any such changes of style for function definitions
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
>> 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 @@
>> 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,
>> - 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
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
Thanks, I'll split it.