diff mbox series

[3/6] nptl: Make pthread_kill async-signal-safe

Message ID 20201204180944.3774769-3-adhemerval.zanella@linaro.org
State Superseded
Headers show
Series [1/6] nptl: Move Linux pthread_kill to nptl | expand

Commit Message

Adhemerval Zanella Dec. 4, 2020, 6:09 p.m. UTC
Simiar to raise (BZ #15368), pthread_kill is also defined as
async-signal-safe (POSIX.1-2008 TC1).  However, similar to raise
it has the same issues relating to signal handling.

Different than raise, all the signal are blocked so it would be
possible to implement pthread_cancel (which also should be
async-cancel safe).

Checked on x86_64-linux-gnu.
---
 nptl/pthread_kill.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Florian Weimer Dec. 4, 2020, 6:20 p.m. UTC | #1
* Adhemerval Zanella via Libc-alpha:

> Simiar to raise (BZ #15368), pthread_kill is also defined as
> async-signal-safe (POSIX.1-2008 TC1).  However, similar to raise
> it has the same issues relating to signal handling.
>
> Different than raise, all the signal are blocked so it would be
> possible to implement pthread_cancel (which also should be
> async-cancel safe).

What do you mean by implementing pthread_cancel?

Patch looks okay as far as I can see, but we'll eventually need a
different approach to deal with the TID reuse race.
Adhemerval Zanella Dec. 4, 2020, 6:26 p.m. UTC | #2
On 04/12/2020 15:20, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> Simiar to raise (BZ #15368), pthread_kill is also defined as
>> async-signal-safe (POSIX.1-2008 TC1).  However, similar to raise
>> it has the same issues relating to signal handling.
>>
>> Different than raise, all the signal are blocked so it would be
>> possible to implement pthread_cancel (which also should be
>> async-cancel safe).
> 
> What do you mean by implementing pthread_cancel?
> 
> Patch looks okay as far as I can see, but we'll eventually need a
> different approach to deal with the TID reuse race.
> 

For TID reuse case I don't see a easy way other than add a lock
on pthread_kill with synchronizes with pthread_exit.  It would
require some changes on how we synchronize the TID clear by the
kernel.
Adhemerval Zanella Dec. 4, 2020, 6:38 p.m. UTC | #3
On 04/12/2020 15:20, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> Simiar to raise (BZ #15368), pthread_kill is also defined as
>> async-signal-safe (POSIX.1-2008 TC1).  However, similar to raise
>> it has the same issues relating to signal handling.
>>
>> Different than raise, all the signal are blocked so it would be
>> possible to implement pthread_cancel (which also should be
>> async-cancel safe).
> 
> What do you mean by implementing pthread_cancel?

To avoid the second issue of the pthread_cancel async-signal-safe
issues [2].

[2] https://sourceware.org/pipermail/libc-alpha/2020-December/120423.html
Florian Weimer Dec. 4, 2020, 6:45 p.m. UTC | #4
* Adhemerval Zanella:

> On 04/12/2020 15:20, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> Simiar to raise (BZ #15368), pthread_kill is also defined as
>>> async-signal-safe (POSIX.1-2008 TC1).  However, similar to raise
>>> it has the same issues relating to signal handling.
>>>
>>> Different than raise, all the signal are blocked so it would be
>>> possible to implement pthread_cancel (which also should be
>>> async-cancel safe).
>> 
>> What do you mean by implementing pthread_cancel?
>
> To avoid the second issue of the pthread_cancel async-signal-safe
> issues [2].
>
> [2] https://sourceware.org/pipermail/libc-alpha/2020-December/120423.html

I think the signal blocking would have to happen on the target thread.
Blocking signals on the signal-sending thread does not help?
diff mbox series

Patch

diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
index 7ef68d1572..c547c5fe58 100644
--- a/nptl/pthread_kill.c
+++ b/nptl/pthread_kill.c
@@ -36,11 +36,17 @@  __pthread_kill (pthread_t threadid, int signo)
     /* Not a valid thread handle.  */
     return ESRCH;
 
+  sigset_t set;
+  __libc_signal_block_all (&set);
+
   /* We have a special syscall to do the work.  */
   pid_t pid = __getpid ();
 
   int val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo);
-  return (INTERNAL_SYSCALL_ERROR_P (val)
-	  ? INTERNAL_SYSCALL_ERRNO (val) : 0);
+  val = (INTERNAL_SYSCALL_ERROR_P (val) ? INTERNAL_SYSCALL_ERRNO (val) : 0);
+
+  __libc_signal_restore_set (&set);
+
+  return val;
 }
 strong_alias (__pthread_kill, pthread_kill)