Re: [PATCH] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 11/04/2022 11:54, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> @@ -56,10 +63,29 @@ __pthread_disable_asynccancel (int oldtype)
>>  {
> 
>> +  /* We cannot return when we are being canceled.  Upon return the
>> +     thread might be things which would have to be undone.  The
>> +     following loop should loop until the cancellation signal is
>> +     delivered.  */
>> +  while (__glibc_unlikely ((newval & (CANCELING_BITMASK | CANCELED_BITMASK))
>> +			   == CANCELING_BITMASK))
>> +    {
>> +      futex_wait_simple ((unsigned int *) &self->cancelhandling, newval,
>> +			 FUTEX_PRIVATE);
>> +      newval = atomic_load_relaxed (&self->cancelhandling);
>> +    }
> 
> Is there no FUTEX_WAKE in the patch because futex_wait_simple already
> fails with EINTR once the signal finally arrives?

My understanding is it tries to handle interrupted syscall with side
effects on thread with async cancellation enabled where pthread_cancel
was issued, but SIGCANCEL has not yet arrived.  In this case the
cancellation handler will handle thread cancellation.

This restores the previous semantic, which I think it not fully correct
(the old BZ#12683).

> 
>> diff --git a/nptl/descr.h b/nptl/descr.h
>> index ea8aca08e6..58e3849a63 100644
>> --- a/nptl/descr.h
>> +++ b/nptl/descr.h
>> @@ -279,6 +279,15 @@ struct pthread
>>  
>>    /* Flags determining processing of cancellation.  */
>>    int cancelhandling;
>> +  /* Bit set if cancellation is disabled.  */
>> +#define CANCELSTATE_BIT		0
>> +#define CANCELSTATE_BITMASK	(0x01 << CANCELSTATE_BIT)
>> +  /* Bit set if asynchronous cancellation mode is selected.  */
>> +#define CANCELTYPE_BIT		1
>> +#define CANCELTYPE_BITMASK	(0x01 << CANCELTYPE_BIT)
>> +  /* Bit set if canceling has been initiated.  */
>> +#define CANCELING_BIT		2
>> +#define CANCELING_BITMASK	(0x01 << CANCELING_BIT)
>>    /* Bit set if canceled.  */
>>  #define CANCELED_BIT		3
>>  #define CANCELED_BITMASK	(0x01 << CANCELED_BIT)
> 
> 0x01 looks like a shifted bit pattern.  Maybe a plain 1 is enough?

Ack.

> 
> Maybe add a reference to the pthread_cancel comment explaining the need
> for this?

Right, I will extend the comment below to add why we need both the
cancellation type and mode to be modified atomically along cancellation
status.

> 
>> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
>> index 7524c7ce4d..5aad1bab78 100644
>> --- a/nptl/pthread_cancel.c
>> +++ b/nptl/pthread_cancel.c
> 
>> @@ -92,29 +103,66 @@ __pthread_cancel (pthread_t th)
> 
>> +  /* Some syscalls are never restarted after being interrupted by a signal
>> +     handler, regardless of the use of SA_RESTART (they always fail with
>> +     EINTR).  So pthread_cancel can not send SIGCANCEL unless the cancellation
> 
> typo: can[]not
> 
>> +     is enabled and set as asynchronous (in this case the cancellation will
>> +     be acted in the cancellation handler instead by the syscall wrapper).
>> +     Otherwise the target thread is set as 'cancelling' (CANCELING_BITMASK)
>> +     by atomically set 'cancelhandling' and the cancelation will be acted
> 
> typo: atomically set[ting]
> 
>> +     uppon on next cancellation entrypoing in the target thread.  */
> 
> typos: up[]on, cancellation [point]

Ack.

> 
> Test looks okay to me.
> 
> Reviewed-by: Florian Weimer <fweimer@redhat.com>
> 
> Thanks,
> Florian
>