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

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

On 26-09-2014 17:40, Joseph S. Myers wrote:
> 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?

Yes, although any comments of 5-7 would be appreciate as well.

>
>> 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.)

I understand now you idea and I will adjust the patchset to do so.  I though
the approach of fixing each arch plus non-arch specific files more feasible,
but I agree about your suggestion.