Re: [PATCH 0/7] nptl: Fix Race conditions in pthread cancellation (BZ#12683)

["Joseph S. Myers" <joseph at codesourcery 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