This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 0/7] nptl: Fix Race conditions in pthread cancellation (BZ#12683)
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- Cc: "GNU C. Library" <libc-alpha at sourceware dot org>
- Date: Fri, 26 Sep 2014 20:40:54 +0000
- Subject: Re: [PATCH 0/7] nptl: Fix Race conditions in pthread cancellation (BZ#12683)
- Authentication-results: sourceware.org; auth=none
- References: <5425C31B dot 2060300 at linux dot vnet dot ibm dot com>
On Fri, 26 Sep 2014, Adhemerval Zanella wrote:
> Hi all,
>
> This is an update of my initial patch set to fix BZ#12683 [1].
I take it you are only seeking reviews of patches 1-4 (the ones that are
intended not to break things for any architecture) not 5-7 at this point?
> However, different from Joseph suggest I do not see changing the
> async enable/delete current behavior to a SYSCALL_CANCEL semantic an
> useful change: it will require to keep some cancellation implementation
> that are meant to be removed.
And I still think that the aim should be to minimise the size of the
patches that need reviewing as an atomic unit, which means as much as
possible should be pulled out of that set into preliminary patches.
A further advantage is that it may be possible for the intermediate patch
that changes code to use SYSCALL_CANCEL not to change the disassembly of
installed shared libraries at all - and if not, the assembly changes
should be reasonably reviewable. That would help provide confidence
against typos in a large patch.
My belief is that such an approach should involve writing maybe a total of
ten lines of code that get thrown away in the end - everything else in
such an intermediate patch would be needed at some point anyway. Along
the lines of:
#define SYSCALL_CANCEL(name, ...) \
({ \
long int sc_cancel_ret; \
if (SINGLE_THREAD_P) \
sc_cancel_ret = INLINE_SYSCALL (name, __SYSCALL_NARGS (__VA_ARGS__), \
__VA_ARGS__); \
else \
{ \
int sc_cancel_oldtype = LIBC_CANCEL_ASYNC (); \
sc_cancel_ret = INLINE_SYSCALL (name, __SYSCALL_NARGS (__VA_ARGS__), \
__VA_ARGS__); \
LIBC_CANCEL_RESET (sc_cancel_oldtype); \
} \
sc_ret; \
})
(untested). The patches that actually change how cancellation is
implemented would then replace this definition with the new one currently
in the series.
In any case, I do not consider it a good idea that patch 6 (x86_64)
contains changes to LIBC_CANCEL_ASYNC code in architecture-independent
files (sysdeps/unix/sysv/linux/fcntl.c) as does patch 7 (ppc32) (several
files). To the extent that architecture-independent changes are separate
from architecture-specific ones (which I think is for review only, given
that bisectability indicates committing those pieces together - only
preliminary patches that don't break things should be committed
separately), they should be properly separate, i.e. fix all
architecture-independent files in a patch that comes before the
architecture-specific ones in the series. (I happen to believe that
should be more than one patch - the ones that can go in now with a
temporary SYSCALL_CANCEL definition separate from the more complicated
ones such as fcntl.c. But in any case, don't leave architecture
maintainers with an unknown set of architecture-independent files to fix.)
--
Joseph S. Myers
joseph@codesourcery.com