Re: [PATCH 2/4] nptl: Handle EPIPE on tst-cancel2

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 26/08/2019 07:06, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> In the new implementation, if there is a signal handler and a signal for
>>> it is delivered before SIGCANCEL (even if the signal was sent *after*
>>> pthread_cancel, from the same thread), I do not think there is any
>>> chance whatsoever that we can hide the behavior difference.  After all,
>>> SIGCANCEL may not trigger an asynchronous cancellation from the signal
>>> handler.
>>
>> Yes, although SIGCANCEL would be set as SA_RESTART, it is explicit disabled
>> by the signal handle by removing it from ucontext_t signal mask.
> 
> Does this mean a thread would lose its ability to be canceled in
> blocking system calls if it setjmps out of a signal handler?  (Where as
> sigsetjmp would keep cancellation working.) 
The setjmp does not really matter here, once the async-cancellation fails
to act due side-effects SIGCANCEL is removed from thread signal mask. The
idea of making it sticky is always allow the thread to act on such cases
regardless whether it calls another cancellation entrypoint.

The thread will still be cancelled in the next cancellation entrypoint
due it is marked as CANCELED_BIT.  The __syscall_cancel_arch will check
for the bit and call __do_cancel before issuing the syscall.

> 
>>> System calls that are cancellation points appear to fall into these
>>> categories:
>>>
>>> (A) Those that do not perform any resource allocation (write, read).
>>>
>>> (B) Those that perform resource allocation, but the allocation can be
>>>     easily reverted (openat, accept4).
>>>
>>> (C) Those that perform resource allocation, but the allocation is
>>>     difficult to undo (recvmsg with descriptor passing).
>>>
>>> (D) close.
> 
>>> For (B), maybe we should undo the resource allocation and then proceed
>>> to act on the cancellation.
>>
>> Besides this requires a lot of more complexity by mapping what kind of
>> resources each syscall would require to free and when to free based on
>> returned codes, it would hide it from the application. My view we just
>> need to return that the syscall has visible side-effects that may result
>> in system resources leaks and let the application deal with.
> 
> My concern is that there might be no further cancellation point, and if
> we do not cancel in the case of (B), we might fail to act on the
> cancellation even though the canceled thread was blocked at a
> cancellation point when pthread_cancel was called.

I shared your concern, but my understanding of the whole BZ#12683 issue is
we can't really cancel the thread for the (B) case.

> 
>>> For (C), the complexity may not be worth it.
>>>
>>> For (A), (B), (C), we can act on the cancellation in the error case,
>>> after we observe the cancellation flag in the signal handler trampoline.
>>> (I think dropping the EINTR restriction from there achieves that.)  If
>>> we do that, we do not need to change the test case.
>>
>> I don't see a real net gain in adding such complexity.  It is not on the 
>> error  case, but rather when visible side-effects has happened and we should
>> alert the program to act upon that. The tst-cancel2 case is an error case
>> because of the EPIPE semantic, but the new tst-cancel28 (based on the leak
>> example from BZ#12683) is a case where the open call returns a valid file
>> descriptor that should be freed.
> 
> I totally agree that the descriptor must not leak.  However,
> tst-cancel28 is written in such a way that it does not expect an error
> from open:
> 
> static void *
> leaker (void *arg)
> {
>   int fd = open (arg, O_RDONLY);
>   pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, 0);
>   close (fd);
>   return NULL;
> }
> 
> It would be clearer if it actually used error checking, but we call
> close (-1) on error, which is invalid.

We can hardness the test to check for open failures (due file descriptor
exhaustion or other arcane issue), but the point of the test is fd is
always for the *close* call.  If the open syscall is interrupted before the
file descriptor is actually opened, the thread will be cancelled before
close call.  Otherwise, the kernel will have to return a valid value
and indicate on the PC from the ucontext_t that the syscall has side-effect.

> 
> If this test represents how application code is actually written, I
> think we really need to check for cancellation before returning the file
> descriptor.
> 
>> If glibc starts to add extra semantics for cancellation, we will have
>> a standard extensions since now 'open', for instance, would close the
>> file descriptor if the cancellation signal is delivered after the OS
>> allocates the file descriptor resource.  IMHO this is clearly what the
>> program itself should be handling, because it might the case where it
>> is prepared to close itself the file descriptor in a different thread.
> 
> open is a cancellation point, so according to POSIX, the thread is
> canceled if the open call completes after pthread_cancel returns.  (Due
> to integration with the memory model, thread ordering issues are
> difficult to reason about in POSIX, though.)  I do not think we should
> return a descriptor to the application that can only exist because of
> something that happened *after* the pthread_cancel call.

That's not the interpretation I see, the cancellation description for
such cases is afaiu:

"The side-effects of acting upon a cancellation request while suspended
during a call of a function are the same as the side-effects that may be
seen in a single-threaded program when a call to a function is interrupted
by a signal and the given function returns [EINTR]"

And on signal concepts:

"[EINTR]
Interrupted function call. An asynchronous signal was caught by the process 
during the execution of an interruptible function. If the signal handler 
performs a normal return, the interrupted function call may return this 
condition (see the Base Definitions volume of POSIX.1-2017, <signal.h>)."

And my understanding is, since glibc implemented cancellation using signals
and it requires to use SA_RESTART we can actually return to user if the
syscall has side-effects visible to caller.

It follows the same interpretation Rich used in explaining the semantic
required for the a possible kernel helper for cancellation [1]

> 
>> This would lead to possible another incompatibilities regarding libc
>> system implementation, besides adding more complexity.
> 
> I think we would just implement POSIX behavior more closely because open
> is required to be a cancellation point.  It's unfortunate that we do not
> have kernel support for this.  Other systems can likely implement
> cancellation semantics correctly more easily.

I don't think there is an easy solution that kernel can provide to help
cancellation to be standard conformant *and* race free.  On the same
kernel thread [2], Rich discuss some option with kernel developers and
although the conclusion is open the ideas are either no conformant (the
'stick signal') or feasible (a new set of syscalls with an extra sigmak
mask).

> 
>>> (D) is very special.  Ideally, we would specify what happens with the
>>> descriptor if the close call is canceled.  POSIX does not even specify
>>> what the state of file descriptor is after an EINTR error, so it doesn't
>>> say what happens with cancellation, either.  Maybe we have to leave that
>>> undefined.
>>
>> Indeed even after the POSIX close language clarification [1], the specification 
>> is messy (it really does not help that some system does have interruptible
>> close implementation that might return -1).
>>
>> However my understanding is we can assume POSIX_CLOSE_RESTART 0 for Linux
>> since it shouldn't return -1/EINTR in any case (and I don't know if/when
>> there are kernels that actually does it) [2].  So assuming the nptl is Linux 
>> specific, we can assume that, although close is specified as a cancellable
>> entrypoint, it won't happen in practice.
>>
>> [1] http://austingroupbugs.net/view.php?id=529
>> [2] https://lwn.net/Articles/576478/
> 
> I think this is slightly different from the EINTR case.  With
> SA_RESTART, perhaps the signal handler can run *before* the descriptor
> is closed, so if we never return from the handler, the descriptor leaks?

My understanding is the cancellation handler for close won't interrupt the
syscall, regardless SA_RESTART. But indeed it might be the case where the
signal is acted just before the syscall is issue and after the test for
CANCELED_BITMASK is done.  In this case the thread will be cancelled and
the descriptor leaked.  The musl handler SYS_close differently by not using
the syscall bridge, so cancellation can't be acted upon and so close always
succeed. On the tst-cancel28.c, pthread cancellation is explicit disable
before calling close.

[1] https://lkml.org/lkml/2016/3/9/1065
[2] https://lkml.org/lkml/2016/3/10/424