Message ID | 20210503210005.2891859-1-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | [v3,1/6] nptl: Make pthread_kill async-signal-safe | expand |
* Adhemerval Zanella via Libc-alpha: > Changes from v2: > * Rebase against master. > > Changes from v1: > * Move __libc_signal_block_all prior TID read. > > --- > > 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). I think it would make sense to spell out what the actual bug fix is. It's hard to tell whether pthread_kill is now truly async-signal-safe, given that there is still a race with regards to thread exit and TID reuse. Thanks, Florian
On 11/05/2021 11:25, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> Changes from v2: >> * Rebase against master. >> >> Changes from v1: >> * Move __libc_signal_block_all prior TID read. >> >> --- >> >> 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). > > I think it would make sense to spell out what the actual bug fix is. > > It's hard to tell whether pthread_kill is now truly async-signal-safe, > given that there is still a race with regards to thread exit and TID > reuse. But this is another issue [1] and it would require a different fix than this one (possibly a additional per-thread lock to synchronize with pthread_create and pthread_kill). Blocking the signal help regarding cancellation, but we will need a different to handle the pthread_exit. [1] https://sourceware.org/bugzilla/show_bug.cgi?id=12889
* Adhemerval Zanella: > On 11/05/2021 11:25, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> Changes from v2: >>> * Rebase against master. >>> >>> Changes from v1: >>> * Move __libc_signal_block_all prior TID read. >>> >>> --- >>> >>> 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). >> >> I think it would make sense to spell out what the actual bug fix is. >> >> It's hard to tell whether pthread_kill is now truly async-signal-safe, >> given that there is still a race with regards to thread exit and TID >> reuse. > > But this is another issue [1] and it would require a different fix > than this one (possibly a additional per-thread lock to synchronize > with pthread_create and pthread_kill). > > Blocking the signal help regarding cancellation, but we will need a > different to handle the pthread_exit. > > [1] https://sourceware.org/bugzilla/show_bug.cgi?id=12889 Sure, but I don't really see how this improves async-signal-safety given that we do not block signals around fork (which we should perhaps do; I think there's nothing preventing us from doing that). Thanks, Florian
On 11/05/2021 11:44, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 11/05/2021 11:25, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> Changes from v2: >>>> * Rebase against master. >>>> >>>> Changes from v1: >>>> * Move __libc_signal_block_all prior TID read. >>>> >>>> --- >>>> >>>> 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). >>> >>> I think it would make sense to spell out what the actual bug fix is. >>> >>> It's hard to tell whether pthread_kill is now truly async-signal-safe, >>> given that there is still a race with regards to thread exit and TID >>> reuse. >> >> But this is another issue [1] and it would require a different fix >> than this one (possibly a additional per-thread lock to synchronize >> with pthread_create and pthread_kill). >> >> Blocking the signal help regarding cancellation, but we will need a >> different to handle the pthread_exit. >> >> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=12889 > > Sure, but I don't really see how this improves async-signal-safety given > that we do not block signals around fork (which we should perhaps do; I > think there's nothing preventing us from doing that). Right, I think this patch came from a previous attempt to fix bz12889, but thinking twice it does not add much without fixing the race condition in pthread_kill first.
diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c index ad7e011779..71c362adce 100644 --- a/nptl/pthread_kill.c +++ b/nptl/pthread_kill.c @@ -28,6 +28,11 @@ __pthread_kill (pthread_t threadid, int signo) if (__is_internal_signal (signo)) return EINVAL; + sigset_t set; + __libc_signal_block_all (&set); + + int val; + /* Force load of pd->tid into local variable or register. Otherwise if a thread exits between ESRCH test and tgkill, we might return EINVAL, because pd->tid would be cleared by the kernel. */ @@ -35,14 +40,20 @@ __pthread_kill (pthread_t threadid, int signo) pid_t tid = atomic_forced_read (pd->tid); if (__glibc_unlikely (tid <= 0)) /* Not a valid thread handle. */ - return ESRCH; + val = ESRCH; + else + { + /* We have a special syscall to do the work. */ + pid_t pid = __getpid (); + + val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo); + val = (INTERNAL_SYSCALL_ERROR_P (val) + ? INTERNAL_SYSCALL_ERRNO (val) : 0); + } - /* We have a special syscall to do the work. */ - pid_t pid = __getpid (); + __libc_signal_restore_set (&set); - int val = INTERNAL_SYSCALL_CALL (tgkill, pid, tid, signo); - return (INTERNAL_SYSCALL_ERROR_P (val) - ? INTERNAL_SYSCALL_ERRNO (val) : 0); + return val; } versioned_symbol (libc, __pthread_kill, pthread_kill, GLIBC_2_34);