[PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

classic Classic list List threaded Threaded
31 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Adhemerval Zanella-2
Changes from previous version:

  - Remove unrelated changes.

--

The a2e8aa0d9e shows two regressions on sparc64-linux-gnu:

  nptl/tst-cancel-self-canceltype
  nptl/tst-cancel5

This is not from the patch itself, but rather from an invalid
__NR_rt_sigprocmask issued by the loader:

  rt_sigprocmask(SIG_UNBLOCK, [RTMIN RT_1], NULL, 8) = 0
  rt_sigprocmask(0xffd07c60 /* SIG_??? */, ~[], 0x7feffd07d08, 8) = -1 EINVAL (Invalid argument)

Tracking the culprit it really seems a wrong code generation in the
INTERNAL_SYSCALL due the automatic sigset_t used on
__libc_signal_block_all:

  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
                          set, _NSIG / 8);

Where SIGALL_SET is defined as:

  ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })

Building the expanded __libc_signal_block_all on sparc64 with recent
compiler (gcc 8.3.1 and 9.1.1):

  #include <signal>

  int
  _libc_signal_block_all (sigset_t *set)
  {
    INTERNAL_SYSCALL_DECL (err);
    return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
                             set, _NSIG / 8);
  }

It seems that the first argument (SIG_BLOCK) is not correctly set on
'o0' register:

  __libc_signal_block_all:
        save    %sp, -304, %sp
        add     %fp, 1919, %o0
        mov     128, %o2
        sethi   %hi(.LC0), %o1
        call    memcpy, 0
         or     %o1, %lo(.LC0), %o1
        add     %fp, 1919, %o1
        mov     %i0, %o2
        mov     8, %o3
        mov     103, %g1
        ta      0x6d;
        bcc,pt  %xcc, 1f
        mov     0, %g1
        sub     %g0, %o0, %o0
        mov     1, %g1
     1: sra     %o0, 0, %i0
        return  %i7+8
         nop

Where is I define SIGALL_SET outside INTERNAL_SYSCALL macro, gcc
correctly sets the expected kernel argument in correct register:

        sethi   %hi(.LC0), %o1
        call    memcpy, 0
         or     %o1, %lo(.LC0), %o1
   ->   mov     1, %o0
        add     %fp, 1919, %o1

This patch fixes it by moving both sigset_t that represent all signals
sets and the application set to constant data objects.

Checked on x86_64-linux-gnu, i686-linux-gnu, and sparc64-linux-gnu.
---
 sysdeps/unix/sysv/linux/internal-signals.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
index 01d8bf0a4c..a496c7174c 100644
--- a/sysdeps/unix/sysv/linux/internal-signals.h
+++ b/sysdeps/unix/sysv/linux/internal-signals.h
@@ -22,6 +22,7 @@
 #include <signal.h>
 #include <sigsetops.h>
 #include <stdbool.h>
+#include <limits.h>
 #include <sysdep.h>
 
 /* The signal used for asynchronous cancelation.  */
@@ -53,15 +54,16 @@ __clear_internal_signals (sigset_t *set)
   __sigdelset (set, SIGSETXID);
 }
 
-#define SIGALL_SET \
-  ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
+static const sigset_t sigall_set = {
+   .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 }
+};
 
 /* Block all signals, including internal glibc ones.  */
 static inline int
 __libc_signal_block_all (sigset_t *set)
 {
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
+  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &sigall_set,
    set, _NSIG / 8);
 }
 
@@ -69,11 +71,11 @@ __libc_signal_block_all (sigset_t *set)
 static inline int
 __libc_signal_block_app (sigset_t *set)
 {
-  sigset_t allset = SIGALL_SET;
+  sigset_t allset = sigall_set;
   __clear_internal_signals (&allset);
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset, set,
-   _NSIG / 8);
+  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset,
+   set, _NSIG / 8);
 }
 
 /* Restore current process signal mask.  */
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/7] Add a constant data object for __libc_signal_block_app signal mask

Adhemerval Zanella-2
It allows some minor optimization and less stack usage.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 sysdeps/unix/sysv/linux/internal-signals.h | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
index a496c7174c..4b70109c14 100644
--- a/sysdeps/unix/sysv/linux/internal-signals.h
+++ b/sysdeps/unix/sysv/linux/internal-signals.h
@@ -58,6 +58,18 @@ static const sigset_t sigall_set = {
    .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 }
 };
 
+static const sigset_t sigapp_set = {
+#if ULONG_MAX == 0xffffffff
+  .__val = { [0]                      = ~0UL & ~(__sigmask (SIGCANCEL)),
+             [1]                      = ~0UL & ~(__sigmask (SIGSETXID)),
+             [2 ... _SIGSET_NWORDS-1] = ~0UL }
+#else
+  .__val = { [0]                      = ~0UL & ~(__sigmask (SIGCANCEL)
+                                                 | __sigmask (SIGSETXID)),
+             [1 ... _SIGSET_NWORDS-1] = ~0UL }
+#endif
+};
+
 /* Block all signals, including internal glibc ones.  */
 static inline int
 __libc_signal_block_all (sigset_t *set)
@@ -71,10 +83,8 @@ __libc_signal_block_all (sigset_t *set)
 static inline int
 __libc_signal_block_app (sigset_t *set)
 {
-  sigset_t allset = sigall_set;
-  __clear_internal_signals (&allset);
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset,
+  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &sigapp_set,
    set, _NSIG / 8);
 }
 
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 3/7] Fix return code for __libc_signal_* functions

Adhemerval Zanella-2
In reply to this post by Adhemerval Zanella-2
since the function should not fail if input argument is NULL.  Also,
for Linux the return value is not fully correct on some platforms due
the missing usage of INTERNAL_SYSCALL_ERROR_P / INTERNAL_SYSCALL_ERRNO
macros.

Checked on x86_64-linux-gnu, i686-linux-gnu, and sparc64-linux-gnu.
---
 sysdeps/generic/internal-signals.h         | 12 ++++++------
 sysdeps/unix/sysv/linux/internal-signals.h | 18 +++++++++---------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h
index a515e3e649..41c24dc4b3 100644
--- a/sysdeps/generic/internal-signals.h
+++ b/sysdeps/generic/internal-signals.h
@@ -34,28 +34,28 @@ __clear_internal_signals (sigset_t *set)
 {
 }
 
-static inline int
+static inline void
 __libc_signal_block_all (sigset_t *set)
 {
   sigset_t allset;
   __sigfillset (&allset);
-  return __sigprocmask (SIG_BLOCK, &allset, set);
+  __sigprocmask (SIG_BLOCK, &allset, set);
 }
 
-static inline int
+static inline void
 __libc_signal_block_app (sigset_t *set)
 {
   sigset_t allset;
   __sigfillset (&allset);
   __clear_internal_signals (&allset);
-  return __sigprocmask (SIG_BLOCK, &allset, set);
+  __sigprocmask (SIG_BLOCK, &allset, set);
 }
 
 /* Restore current process signal mask.  */
-static inline int
+static inline void
 __libc_signal_restore_set (const sigset_t *set)
 {
-  return __sigprocmask (SIG_SETMASK, set, NULL);
+  __sigprocmask (SIG_SETMASK, set, NULL);
 }
 
 
diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
index 4b70109c14..04e1ec4f0a 100644
--- a/sysdeps/unix/sysv/linux/internal-signals.h
+++ b/sysdeps/unix/sysv/linux/internal-signals.h
@@ -71,30 +71,30 @@ static const sigset_t sigapp_set = {
 };
 
 /* Block all signals, including internal glibc ones.  */
-static inline int
+static inline void
 __libc_signal_block_all (sigset_t *set)
 {
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &sigall_set,
-   set, _NSIG / 8);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, err, SIG_BLOCK, &sigall_set, set,
+ _NSIG / 8);
 }
 
 /* Block all application signals (excluding internal glibc ones).  */
-static inline int
+static inline void
 __libc_signal_block_app (sigset_t *set)
 {
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &sigapp_set,
-   set, _NSIG / 8);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, err, SIG_BLOCK, &sigapp_set, set,
+ _NSIG / 8);
 }
 
 /* Restore current process signal mask.  */
-static inline int
+static inline void
 __libc_signal_restore_set (const sigset_t *set)
 {
   INTERNAL_SYSCALL_DECL (err);
-  return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, set, NULL,
-   _NSIG / 8);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, err, SIG_SETMASK, set, NULL,
+ _NSIG / 8);
 }
 
 /* Used to communicate with signal handler.  */
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 4/7] linux: Consolidate sigprocmask

Adhemerval Zanella-2
In reply to this post by Adhemerval Zanella-2
All architectures now uses the Linux generic implementation which
uses __NR_rt_sigprocmask.

Checked on x86_64-linux-gnu, sparc64-linux-gnu, ia64-linux-gnu,
s390x-linux-gnu, and alpha-linux-gnu.
---
 sysdeps/unix/sysv/linux/alpha/sigprocmask.c   | 58 -------------------
 sysdeps/unix/sysv/linux/ia64/sigprocmask.c    | 40 -------------
 .../sysv/linux/s390/s390-64/sigprocmask.c     | 38 ------------
 sysdeps/unix/sysv/linux/sigprocmask.c         | 14 +----
 .../sysv/linux/sparc/sparc64/sigprocmask.c    | 34 -----------
 sysdeps/unix/sysv/linux/x86_64/sigprocmask.c  | 39 -------------
 6 files changed, 3 insertions(+), 220 deletions(-)
 delete mode 100644 sysdeps/unix/sysv/linux/alpha/sigprocmask.c
 delete mode 100644 sysdeps/unix/sysv/linux/ia64/sigprocmask.c
 delete mode 100644 sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
 delete mode 100644 sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
 delete mode 100644 sysdeps/unix/sysv/linux/x86_64/sigprocmask.c

diff --git a/sysdeps/unix/sysv/linux/alpha/sigprocmask.c b/sysdeps/unix/sysv/linux/alpha/sigprocmask.c
deleted file mode 100644
index 0e807179bf..0000000000
--- a/sysdeps/unix/sysv/linux/alpha/sigprocmask.c
+++ /dev/null
@@ -1,58 +0,0 @@
-/* Copyright (C) 1993-2019 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by David Mosberger ([hidden email]).
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <errno.h>
-#include <sysdep.h>
-#include <signal.h>
-
-/* When there is kernel support for more than 64 signals, we'll have to
-   switch to a new system call convention here.  */
-
-int
-__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
-{
-  unsigned long int setval;
-  long result;
-
-  if (set)
-    setval = set->__val[0];
-  else
-    {
-      setval = 0;
-      how = SIG_BLOCK; /* ensure blocked mask doesn't get changed */
-    }
-
-  result = INLINE_SYSCALL (osf_sigprocmask, 2, how, setval);
-  if (result == -1)
-    /* If there are ever more than 63 signals, we need to recode this
-       in assembler since we wouldn't be able to distinguish a mask of
-       all 1s from -1, but for now, we're doing just fine... */
-    return result;
-
-  if (oset)
-    {
-      oset->__val[0] = result;
-      result = _SIGSET_NWORDS;
-      while (--result > 0)
- oset->__val[result] = 0;
-    }
-  return 0;
-}
-
-libc_hidden_def (__sigprocmask)
-weak_alias (__sigprocmask, sigprocmask);
diff --git a/sysdeps/unix/sysv/linux/ia64/sigprocmask.c b/sysdeps/unix/sysv/linux/ia64/sigprocmask.c
deleted file mode 100644
index 81c2d3cd8e..0000000000
--- a/sysdeps/unix/sysv/linux/ia64/sigprocmask.c
+++ /dev/null
@@ -1,40 +0,0 @@
-/* Copyright (C) 1997-2019 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Linux/IA64 specific sigprocmask
-   Written by Jes Sorensen, <[hidden email]>, April 1999.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-/* Linux/ia64 only has rt signals, thus we do not even want to try falling
-   back to the old style signals as the default Linux handler does. */
-
-#include <errno.h>
-#include <signal.h>
-#include <unistd.h>
-
-#include <sysdep.h>
-#include <sys/syscall.h>
-
-/* Get and/or change the set of blocked signals.  */
-int
-__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
-{
-
-  /* XXX The size argument hopefully will have to be changed to the
-     real size of the user-level sigset_t.  */
-  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
-}
-libc_hidden_def (__sigprocmask)
-weak_alias (__sigprocmask, sigprocmask)
diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c b/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
deleted file mode 100644
index f0eb099748..0000000000
--- a/sysdeps/unix/sysv/linux/s390/s390-64/sigprocmask.c
+++ /dev/null
@@ -1,38 +0,0 @@
-/* Copyright (C) 2001-2019 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-/* 64 bit Linux for S/390 only has rt signals, thus we do not even want to try
-   falling back to the old style signals as the default Linux handler does. */
-
-#include <errno.h>
-#include <signal.h>
-#include <unistd.h>
-
-#include <sysdep.h>
-#include <sys/syscall.h>
-
-/* Get and/or change the set of blocked signals.  */
-int
-__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
-{
-
-  /* XXX The size argument hopefully will have to be changed to the
-     real size of the user-level sigset_t.  */
-  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
-}
-libc_hidden_def (__sigprocmask)
-weak_alias (__sigprocmask, sigprocmask)
diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
index 01521c8107..73b0d0c19a 100644
--- a/sysdeps/unix/sysv/linux/sigprocmask.c
+++ b/sysdeps/unix/sysv/linux/sigprocmask.c
@@ -15,17 +15,9 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
 #include <signal.h>
-#include <string.h>  /* Needed for string function builtin redirection.  */
-#include <unistd.h>
-
-#include <sysdep.h>
-#include <sys/syscall.h>
-
 #include <nptl/pthreadP.h>              /* SIGCANCEL, SIGSETXID */
 
-
 /* Get and/or change the set of blocked signals.  */
 int
 __sigprocmask (int how, const sigset_t *set, sigset_t *oset)
@@ -35,8 +27,8 @@ __sigprocmask (int how, const sigset_t *set, sigset_t *oset)
   /* The only thing we have to make sure here is that SIGCANCEL and
      SIGSETXID are not blocked.  */
   if (set != NULL
-      && (__builtin_expect (__sigismember (set, SIGCANCEL), 0)
-  || __builtin_expect (__sigismember (set, SIGSETXID), 0)))
+      && __glibc_unlikely (__sigismember (set, SIGCANCEL)
+ || __glibc_unlikely (__sigismember (set, SIGSETXID))))
     {
       local_newmask = *set;
       __sigdelset (&local_newmask, SIGCANCEL);
@@ -44,7 +36,7 @@ __sigprocmask (int how, const sigset_t *set, sigset_t *oset)
       set = &local_newmask;
     }
 
-  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
+  return INLINE_SYSCALL_CALL (rt_sigprocmask, how, set, oset, _NSIG / 8);
 }
 libc_hidden_def (__sigprocmask)
 weak_alias (__sigprocmask, sigprocmask)
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c b/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
deleted file mode 100644
index 5823826ab2..0000000000
--- a/sysdeps/unix/sysv/linux/sparc/sparc64/sigprocmask.c
+++ /dev/null
@@ -1,34 +0,0 @@
-/* Copyright (C) 1997-2019 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <errno.h>
-#include <signal.h>
-#include <unistd.h>
-
-#include <sysdep.h>
-#include <sys/syscall.h>
-
-/* Get and/or change the set of blocked signals.  */
-int
-__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
-{
-  /* XXX The size argument hopefully will have to be changed to the
-     real size of the user-level sigset_t.  */
-  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
-}
-libc_hidden_def (__sigprocmask)
-weak_alias (__sigprocmask, sigprocmask)
diff --git a/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c b/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c
deleted file mode 100644
index c2e721d7b9..0000000000
--- a/sysdeps/unix/sysv/linux/x86_64/sigprocmask.c
+++ /dev/null
@@ -1,39 +0,0 @@
-/* Copyright (C) 1997-2019 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Written by Jes Sorensen, <[hidden email]>, April 1999.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-/* Linux/x86_64 only has rt signals, thus we do not even want to try falling
-   back to the old style signals as the default Linux handler does. */
-
-#include <errno.h>
-#include <signal.h>
-#include <unistd.h>
-
-#include <sysdep.h>
-#include <sys/syscall.h>
-
-/* Get and/or change the set of blocked signals.  */
-int
-__sigprocmask (int how, const sigset_t *set, sigset_t *oset)
-{
-
-  /* XXX The size argument hopefully will have to be changed to the
-     real size of the user-level sigset_t.  */
-  return INLINE_SYSCALL (rt_sigprocmask, 4, how, set, oset, _NSIG / 8);
-}
-libc_hidden_def (__sigprocmask)
-weak_alias (__sigprocmask, sigprocmask)
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 5/7] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask

Adhemerval Zanella-2
In reply to this post by Adhemerval Zanella-2
The sigfillset already does it, and this is the canonical way to operate
on sigset_t.  The only way to actually broke this assumption is if caller
initialize sigset with memset or something similar, i.e, bypassing glibc
(and again this is not a valid construction).

Checked on x86_64-linux-gnu.
---
 sysdeps/unix/sysv/linux/sigprocmask.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/sigprocmask.c b/sysdeps/unix/sysv/linux/sigprocmask.c
index 73b0d0c19a..c6961a8ac4 100644
--- a/sysdeps/unix/sysv/linux/sigprocmask.c
+++ b/sysdeps/unix/sysv/linux/sigprocmask.c
@@ -16,26 +16,12 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <signal.h>
-#include <nptl/pthreadP.h>              /* SIGCANCEL, SIGSETXID */
+#include <sysdep.h>
 
 /* Get and/or change the set of blocked signals.  */
 int
 __sigprocmask (int how, const sigset_t *set, sigset_t *oset)
 {
-  sigset_t local_newmask;
-
-  /* The only thing we have to make sure here is that SIGCANCEL and
-     SIGSETXID are not blocked.  */
-  if (set != NULL
-      && __glibc_unlikely (__sigismember (set, SIGCANCEL)
- || __glibc_unlikely (__sigismember (set, SIGSETXID))))
-    {
-      local_newmask = *set;
-      __sigdelset (&local_newmask, SIGCANCEL);
-      __sigdelset (&local_newmask, SIGSETXID);
-      set = &local_newmask;
-    }
-
   return INLINE_SYSCALL_CALL (rt_sigprocmask, how, set, oset, _NSIG / 8);
 }
 libc_hidden_def (__sigprocmask)
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 6/7] Block all signals on timer_create thread (BZ#10815)

Adhemerval Zanella-2
In reply to this post by Adhemerval Zanella-2
The behavior of the signal mask on threads created by timer_create
for SIGEV_THREAD timers are implementation defined and glibc explicit
unblocks all signals before calling the user-defined function.

This behavior, although not incorrect standard-wise, opens a race if a
program using a blocked rt-signal plus sigwaitinfo (and without an
installed signal handler for the rt-signal) receives the signal while
executing the timer threads created by SIGEV_THREAD.

A better alternative discussed in bug report is to rather block all
signals (besides the internal ones not available to application
usage).

This patch fixes by only unblock the SIGSETXID (used on set*uid).
The SIGTIMER is the same as SIGCANCEL and it will be handled by
sigwaitinfo on the helper thread (thus it can be masked as well).

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 rt/Makefile                                   |  7 +-
 rt/tst-timer-sigmask.c                        | 96 +++++++++++++++++++
 sysdeps/unix/sysv/linux/internal-signals.h    | 14 +++
 sysdeps/unix/sysv/linux/kernel-posix-timers.h |  1 -
 sysdeps/unix/sysv/linux/timer_routines.c      | 96 +++++++------------
 5 files changed, 148 insertions(+), 66 deletions(-)
 create mode 100644 rt/tst-timer-sigmask.c

diff --git a/rt/Makefile b/rt/Makefile
index 6c8365e0c0..a7a82879c7 100644
--- a/rt/Makefile
+++ b/rt/Makefile
@@ -47,6 +47,7 @@ tests := tst-shm tst-timer tst-timer2 \
  tst-timer3 tst-timer4 tst-timer5 \
  tst-cpuclock2 tst-cputimer1 tst-cputimer2 tst-cputimer3 \
  tst-shm-cancel
+tests-internal := tst-timer-sigmask
 
 extra-libs := librt
 extra-libs-others := $(extra-libs)
@@ -63,9 +64,11 @@ LDFLAGS-rt.so = -Wl,--enable-new-dtags,-z,nodelete
 $(objpfx)librt.so: $(shared-thread-library)
 
 ifeq (yes,$(build-shared))
-$(addprefix $(objpfx),$(tests)): $(objpfx)librt.so $(shared-thread-library)
+$(addprefix $(objpfx),$(tests) $(tests-internal)): \
+ $(objpfx)librt.so $(shared-thread-library)
 else
-$(addprefix $(objpfx),$(tests)): $(objpfx)librt.a $(static-thread-library)
+$(addprefix $(objpfx),$(tests)) $(tests-internal): \
+ $(objpfx)librt.a $(static-thread-library)
 endif
 
 tst-mqueue7-ARGS = -- $(host-test-program-cmd)
diff --git a/rt/tst-timer-sigmask.c b/rt/tst-timer-sigmask.c
new file mode 100644
index 0000000000..1030e4c79f
--- /dev/null
+++ b/rt/tst-timer-sigmask.c
@@ -0,0 +1,96 @@
+/* Check resulting signal mask from POSIX time using SIGEV_THREAD.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <time.h>
+#include <signal.h>
+#include <pthread.h>
+#include <stdbool.h>
+
+#include <support/check.h>
+#include <support/test-driver.h>
+
+#include <internal-signals.h>
+
+static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
+static enum
+{
+  THREAD_SIGNAL_CHECK_INIT = 0,
+  THREAD_SIGNAL_CHECK_OK,
+  THREAD_SIGNAL_CHECK_FAIL
+} thread_handler_check = THREAD_SIGNAL_CHECK_INIT;
+
+static void
+thread_handler (union sigval sv)
+{
+  bool failure = false;
+
+  sigset_t ss;
+  sigprocmask (SIG_BLOCK, NULL, &ss);
+  if (test_verbose > 0)
+    printf ("%s: blocked signal mask = { ", __func__);
+  for (int sig = 1; sig < NSIG; sig++)
+    {
+#ifdef __linux__
+      /* POSIX timers threads created to handle SIGEV_THREAD blocks
+ all signals except SIGKILL, SIGSTOP, and SIGSETXID.  */
+      if (!sigismember (&ss, sig)
+  && (sig != SIGSETXID && sig != SIGKILL && sig != SIGSTOP))
+ {
+  failure = true;
+ }
+#endif
+      if (test_verbose && sigismember (&ss, sig))
+ printf ("%d, ", sig);
+    }
+  if (test_verbose > 0)
+    printf ("}\n");
+
+  pthread_mutex_lock (&lock);
+  thread_handler_check = failure
+ ? THREAD_SIGNAL_CHECK_FAIL : THREAD_SIGNAL_CHECK_OK;
+  pthread_cond_signal (&cond);
+  pthread_mutex_unlock (&lock);
+}
+
+static int
+do_test (void)
+{
+  struct sigevent sev = { 0 };
+  sev.sigev_notify = SIGEV_THREAD;
+  sev.sigev_notify_function = &thread_handler;
+
+  timer_t timerid;
+  TEST_COMPARE (timer_create (CLOCK_REALTIME, &sev, &timerid), 0);
+
+  struct itimerspec trigger = { 0 };
+  trigger.it_value.tv_nsec = 1000000;
+  TEST_COMPARE (timer_settime (timerid, 0, &trigger, NULL), 0);
+
+  pthread_mutex_lock (&lock);
+  while (thread_handler_check == THREAD_SIGNAL_CHECK_INIT)
+    pthread_cond_wait (&cond, &lock);
+  pthread_mutex_unlock (&lock);
+
+  TEST_COMPARE (thread_handler_check, THREAD_SIGNAL_CHECK_OK);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
index 04e1ec4f0a..f155a8cd32 100644
--- a/sysdeps/unix/sysv/linux/internal-signals.h
+++ b/sysdeps/unix/sysv/linux/internal-signals.h
@@ -70,6 +70,11 @@ static const sigset_t sigapp_set = {
 #endif
 };
 
+static const sigset_t sigtimer_set = {
+  .__val = { [0]                      = __sigmask (SIGTIMER),
+     [1 ... _SIGSET_NWORDS-1] = 0 }
+};
+
 /* Block all signals, including internal glibc ones.  */
 static inline void
 __libc_signal_block_all (sigset_t *set)
@@ -88,6 +93,15 @@ __libc_signal_block_app (sigset_t *set)
  _NSIG / 8);
 }
 
+/* Block only the SIGTIMER set.  */
+static inline void
+__libc_signal_block_sigtimer (sigset_t *set)
+{
+  INTERNAL_SYSCALL_DECL (err);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, err, SIG_BLOCK, &sigtimer_set, set,
+ _NSIG / 8);
+}
+
 /* Restore current process signal mask.  */
 static inline void
 __libc_signal_restore_set (const sigset_t *set)
diff --git a/sysdeps/unix/sysv/linux/kernel-posix-timers.h b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
index 1ded4df51a..d60cb95f80 100644
--- a/sysdeps/unix/sysv/linux/kernel-posix-timers.h
+++ b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
@@ -43,7 +43,6 @@ extern pthread_mutex_t __active_timer_sigev_thread_lock attribute_hidden;
 /* Type of timers in the kernel.  */
 typedef int kernel_timer_t;
 
-
 /* Internal representation of timer.  */
 struct timer
 {
diff --git a/sysdeps/unix/sysv/linux/timer_routines.c b/sysdeps/unix/sysv/linux/timer_routines.c
index d96d963177..98bbe077fa 100644
--- a/sysdeps/unix/sysv/linux/timer_routines.c
+++ b/sysdeps/unix/sysv/linux/timer_routines.c
@@ -42,14 +42,6 @@ struct thread_start_data
 static void *
 timer_sigev_thread (void *arg)
 {
-  /* The parent thread has all signals blocked.  This is a bit
-     surprising for user code, although valid.  We unblock all
-     signals.  */
-  sigset_t ss;
-  sigemptyset (&ss);
-  INTERNAL_SYSCALL_DECL (err);
-  INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, &ss, NULL, _NSIG / 8);
-
   struct thread_start_data *td = (struct thread_start_data *) arg;
 
   void (*thrfunc) (sigval_t) = td->thrfunc;
@@ -69,65 +61,49 @@ timer_sigev_thread (void *arg)
 static void *
 timer_helper_thread (void *arg)
 {
-  /* Wait for the SIGTIMER signal, allowing the setXid signal, and
-     none else.  */
-  sigset_t ss;
-  sigemptyset (&ss);
-  __sigaddset (&ss, SIGTIMER);
-
   /* Endless loop of waiting for signals.  The loop is only ended when
      the thread is canceled.  */
   while (1)
     {
       siginfo_t si;
 
-      /* sigwaitinfo cannot be used here, since it deletes
- SIGCANCEL == SIGTIMER from the set.  */
-
-      /* XXX The size argument hopefully will have to be changed to the
- real size of the user-level sigset_t.  */
-      int result = SYSCALL_CANCEL (rt_sigtimedwait, &ss, &si, NULL, _NSIG / 8);
-
-      if (result > 0)
+      while (sigwaitinfo (&sigtimer_set, &si) < 0);
+      if (si.si_code == SI_TIMER)
  {
-  if (si.si_code == SI_TIMER)
-    {
-      struct timer *tk = (struct timer *) si.si_ptr;
+  struct timer *tk = (struct timer *) si.si_ptr;
+
+  /* Check the timer is still used and will not go away
+     while we are reading the values here.  */
+  pthread_mutex_lock (&__active_timer_sigev_thread_lock);
 
-      /* Check the timer is still used and will not go away
- while we are reading the values here.  */
-      pthread_mutex_lock (&__active_timer_sigev_thread_lock);
+  struct timer *runp = __active_timer_sigev_thread;
+  while (runp != NULL)
+    if (runp == tk)
+      break;
+  else
+    runp = runp->next;
 
-      struct timer *runp = __active_timer_sigev_thread;
-      while (runp != NULL)
- if (runp == tk)
-  break;
- else
-  runp = runp->next;
+  if (runp != NULL)
+    {
+      struct thread_start_data *td = malloc (sizeof (*td));
 
-      if (runp != NULL)
+      /* There is not much we can do if the allocation fails.  */
+      if (td != NULL)
  {
-  struct thread_start_data *td = malloc (sizeof (*td));
-
-  /* There is not much we can do if the allocation fails.  */
-  if (td != NULL)
-    {
-      /* This is the signal we are waiting for.  */
-      td->thrfunc = tk->thrfunc;
-      td->sival = tk->sival;
-
-      pthread_t th;
-      (void) pthread_create (&th, &tk->attr,
-     timer_sigev_thread, td);
-    }
- }
+  /* This is the signal we are waiting for.  */
+  td->thrfunc = tk->thrfunc;
+  td->sival = tk->sival;
 
-      pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
+  pthread_t th;
+  pthread_create (&th, &tk->attr, timer_sigev_thread, td);
+ }
     }
-  else if (si.si_code == SI_TKILL)
-    /* The thread is canceled.  */
-    pthread_exit (NULL);
+
+  pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
  }
+      else if (si.si_code == SI_TKILL)
+ /* The thread is canceled.  */
+ pthread_exit (NULL);
     }
 }
 
@@ -161,15 +137,10 @@ __start_helper_thread (void)
 
   /* Block all signals in the helper thread but SIGSETXID.  To do this
      thoroughly we temporarily have to block all signals here.  The
-     helper can lose wakeups if SIGCANCEL is not blocked throughout,
-     but sigfillset omits it SIGSETXID.  So, we add SIGCANCEL back
-     explicitly here.  */
+     helper can lose wakeups if SIGCANCEL is not blocked throughout.  */
   sigset_t ss;
-  sigset_t oss;
-  sigfillset (&ss);
-  __sigaddset (&ss, SIGCANCEL);
-  INTERNAL_SYSCALL_DECL (err);
-  INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, &ss, &oss, _NSIG / 8);
+  __libc_signal_block_app (&ss);
+  __libc_signal_block_sigtimer (NULL);
 
   /* Create the helper thread for this timer.  */
   pthread_t th;
@@ -179,8 +150,7 @@ __start_helper_thread (void)
     __helper_tid = ((struct pthread *) th)->tid;
 
   /* Restore the signal mask.  */
-  INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, &oss, NULL,
-    _NSIG / 8);
+  __libc_signal_restore_set (&ss);
 
   /* No need for the attribute anymore.  */
   (void) pthread_attr_destroy (&attr);
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

[PATCH 7/7] Cleanup timer_* routines

Adhemerval Zanella-2
In reply to this post by Adhemerval Zanella-2
  - Two function are added that manipulate the list of active
    timers, __{add,remove}_active_timer_sigev.   This allows make
    both __active_timer_sigev_thread and
    __active_timer_sigev_thread_lock internal on timer_routine
    implementation.

  - Remove ununsed __no_posix_timers defition.

  - Use INLINE_SYSCALL_CALL on timer_* functions.

  - Simplify some code on timer_create and timer_routines, such as
    thread attribute handling and invalid notification method.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 sysdeps/unix/sysv/linux/kernel-posix-timers.h |  13 +-
 sysdeps/unix/sysv/linux/timer_create.c        | 116 +++++++-----------
 sysdeps/unix/sysv/linux/timer_delete.c        |  42 ++-----
 sysdeps/unix/sysv/linux/timer_getoverr.c      |   2 +-
 sysdeps/unix/sysv/linux/timer_routines.c      |  50 ++++++--
 5 files changed, 100 insertions(+), 123 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/kernel-posix-timers.h b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
index d60cb95f80..bf17d8c915 100644
--- a/sysdeps/unix/sysv/linux/kernel-posix-timers.h
+++ b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
@@ -21,10 +21,6 @@
 #include <signal.h>
 #include <sys/types.h>
 
-
-/* Nonzero if the system calls are not available.  */
-extern int __no_posix_timers attribute_hidden;
-
 /* Callback to start helper thread.  */
 extern void __start_helper_thread (void) attribute_hidden;
 
@@ -34,12 +30,6 @@ extern pthread_once_t __helper_once attribute_hidden;
 /* TID of the helper thread.  */
 extern pid_t __helper_tid attribute_hidden;
 
-/* List of active SIGEV_THREAD timers.  */
-extern struct timer *__active_timer_sigev_thread attribute_hidden;
-/* Lock for the __active_timer_sigev_thread.  */
-extern pthread_mutex_t __active_timer_sigev_thread_lock attribute_hidden;
-
-
 /* Type of timers in the kernel.  */
 typedef int kernel_timer_t;
 
@@ -64,3 +54,6 @@ struct timer
   /* Next element in list of active SIGEV_THREAD timers.  */
   struct timer *next;
 };
+
+void __add_active_timer_sigev (struct timer *tk) attribute_hidden;
+void __remove_active_timer_sigev (struct timer *tk) attribute_hidden;
diff --git a/sysdeps/unix/sysv/linux/timer_create.c b/sysdeps/unix/sysv/linux/timer_create.c
index 4e41ca011f..e5dcdaab65 100644
--- a/sysdeps/unix/sysv/linux/timer_create.c
+++ b/sysdeps/unix/sysv/linux/timer_create.c
@@ -16,17 +16,12 @@
    License along with the GNU C Library; see the file COPYING.LIB.  If
    not, see <https://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <pthread.h>
 #include <signal.h>
-#include <stdlib.h>
-#include <string.h>
 #include <time.h>
-#include <sysdep.h>
-#include <internaltypes.h>
-#include <nptl/pthreadP.h>
-#include "kernel-posix-timers.h"
-#include "kernel-posix-cpu-timers.h"
+
+#include <kernel-posix-timers.h>
+#include <kernel-posix-cpu-timers.h>
+#include <internal-signals.h>
 
 
 #ifdef timer_create_alias
@@ -38,17 +33,16 @@ int
 timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
 {
 #undef timer_create
-  {
-    clockid_t syscall_clockid = (clock_id == CLOCK_PROCESS_CPUTIME_ID
- ? MAKE_PROCESS_CPUCLOCK (0, CPUCLOCK_SCHED)
- : clock_id == CLOCK_THREAD_CPUTIME_ID
- ? MAKE_THREAD_CPUCLOCK (0, CPUCLOCK_SCHED)
- : clock_id);
-
-    /* If the user wants notification via a thread we need to handle
-       this special.  */
-    if (evp == NULL
- || __builtin_expect (evp->sigev_notify != SIGEV_THREAD, 1))
+  clockid_t syscall_clockid = (clock_id == CLOCK_PROCESS_CPUTIME_ID
+       ? MAKE_PROCESS_CPUCLOCK (0, CPUCLOCK_SCHED)
+       : clock_id == CLOCK_THREAD_CPUTIME_ID
+       ? MAKE_THREAD_CPUCLOCK (0, CPUCLOCK_SCHED)
+       : clock_id);
+
+  switch (evp != NULL ? evp->sigev_notify : SIGEV_SIGNAL)
+    {
+    case SIGEV_NONE:
+    case SIGEV_SIGNAL:
       {
  struct sigevent local_evp;
 
@@ -75,27 +69,22 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
   }
 
  kernel_timer_t ktimerid;
- int retval = INLINE_SYSCALL (timer_create, 3, syscall_clockid, evp,
-     &ktimerid);
-
- if (retval != -1)
-  {
-    newp->sigev_notify = (evp != NULL
-  ? evp->sigev_notify : SIGEV_SIGNAL);
-    newp->ktimerid = ktimerid;
-
-    *timerid = (timer_t) newp;
-  }
- else
+ int retval = INLINE_SYSCALL_CALL (timer_create, syscall_clockid, evp,
+  &ktimerid);
+ if (retval != 0)
   {
     /* Cannot allocate the timer, fail.  */
     free (newp);
-    retval = -1;
+    return-1;
   }
 
- return retval;
+ newp->sigev_notify = (evp != NULL ? evp->sigev_notify : SIGEV_SIGNAL);
+ newp->ktimerid = ktimerid;
+ *timerid = (timer_t) newp;
       }
-    else
+      break;
+
+   case SIGEV_THREAD:
       {
  /* Create the helper thread.  */
  pthread_once (&__helper_once, __start_helper_thread);
@@ -116,25 +105,10 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
  newp->thrfunc = evp->sigev_notify_function;
  newp->sigev_notify = SIGEV_THREAD;
 
- /* We cannot simply copy the thread attributes since the
-   implementation might keep internal information for
-   each instance.  */
- (void) pthread_attr_init (&newp->attr);
  if (evp->sigev_notify_attributes != NULL)
-  {
-    struct pthread_attr *nattr;
-    struct pthread_attr *oattr;
-
-    nattr = (struct pthread_attr *) &newp->attr;
-    oattr = (struct pthread_attr *) evp->sigev_notify_attributes;
-
-    nattr->schedparam = oattr->schedparam;
-    nattr->schedpolicy = oattr->schedpolicy;
-    nattr->flags = oattr->flags;
-    nattr->guardsize = oattr->guardsize;
-    nattr->stackaddr = oattr->stackaddr;
-    nattr->stacksize = oattr->stacksize;
-  }
+  newp->attr = *evp->sigev_notify_attributes;
+ else
+  pthread_attr_init (&newp->attr);
 
  /* In any case set the detach flag.  */
  (void) pthread_attr_setdetachstate (&newp->attr,
@@ -148,29 +122,25 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
     ._sigev_un = { ._pad = { [0] = __helper_tid } } };
 
  /* Create the timer.  */
- INTERNAL_SYSCALL_DECL (err);
- int res;
- res = INTERNAL_SYSCALL (timer_create, err, 3,
- syscall_clockid, &sev, &newp->ktimerid);
- if (! INTERNAL_SYSCALL_ERROR_P (res, err))
+ int res = INLINE_SYSCALL_CALL (timer_create, syscall_clockid, &sev,
+       &newp->ktimerid);
+ if (res != 0)
   {
-    /* Add to the queue of active timers with thread
-       delivery.  */
-    pthread_mutex_lock (&__active_timer_sigev_thread_lock);
-    newp->next = __active_timer_sigev_thread;
-    __active_timer_sigev_thread = newp;
-    pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
-
-    *timerid = (timer_t) newp;
-    return 0;
+    free (newp);
+    return -1;
   }
 
- /* Free the resources.  */
- free (newp);
+ /* Add to the queue of active timers with thread delivery.  */
+ __add_active_timer_sigev (newp);
 
- __set_errno (INTERNAL_SYSCALL_ERRNO (res, err));
-
- return -1;
+ *timerid = (timer_t) newp;
       }
-  }
+     break;
+
+   default:
+     __set_errno (-1);
+     return -1;
+   }
+
+  return 0;
 }
diff --git a/sysdeps/unix/sysv/linux/timer_delete.c b/sysdeps/unix/sysv/linux/timer_delete.c
index 3aec393132..ee5deb8739 100644
--- a/sysdeps/unix/sysv/linux/timer_delete.c
+++ b/sysdeps/unix/sysv/linux/timer_delete.c
@@ -16,7 +16,6 @@
    License along with the GNU C Library; see the file COPYING.LIB.  If
    not, see <https://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
 #include <stdlib.h>
 #include <time.h>
 #include <sysdep.h>
@@ -35,38 +34,17 @@ timer_delete (timer_t timerid)
   struct timer *kt = (struct timer *) timerid;
 
   /* Delete the kernel timer object.  */
-  int res = INLINE_SYSCALL (timer_delete, 1, kt->ktimerid);
+  int r = INLINE_SYSCALL_CALL (timer_delete, kt->ktimerid);
+  if (r != 0)
+    /* The kernel timer is not known or something else bad happened.
+       Return the error.  */
+    return -1;
 
-  if (res == 0)
-    {
-      if (kt->sigev_notify == SIGEV_THREAD)
- {
-  /* Remove the timer from the list.  */
-  pthread_mutex_lock (&__active_timer_sigev_thread_lock);
-  if (__active_timer_sigev_thread == kt)
-    __active_timer_sigev_thread = kt->next;
-  else
-    {
-      struct timer *prevp = __active_timer_sigev_thread;
-      while (prevp->next != NULL)
- if (prevp->next == kt)
-  {
-    prevp->next = kt->next;
-    break;
-  }
- else
-  prevp = prevp->next;
-    }
-  pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
- }
+  if (kt->sigev_notify == SIGEV_THREAD)
+    __remove_active_timer_sigev (kt);
 
-      /* Free the memory.  */
-      (void) free (kt);
+  /* Free the memory.  */
+  free (kt);
 
-      return 0;
-    }
-
-  /* The kernel timer is not known or something else bad happened.
-     Return the error.  */
-  return -1;
+  return 0;
 }
diff --git a/sysdeps/unix/sysv/linux/timer_getoverr.c b/sysdeps/unix/sysv/linux/timer_getoverr.c
index 04122a9878..82627e4ee3 100644
--- a/sysdeps/unix/sysv/linux/timer_getoverr.c
+++ b/sysdeps/unix/sysv/linux/timer_getoverr.c
@@ -34,7 +34,7 @@ timer_getoverrun (timer_t timerid)
   struct timer *kt = (struct timer *) timerid;
 
   /* Get the information from the kernel.  */
-  int res = INLINE_SYSCALL (timer_getoverrun, 1, kt->ktimerid);
+  int res = INLINE_SYSCALL_CALL (timer_getoverrun, kt->ktimerid);
 
   return res;
 }
diff --git a/sysdeps/unix/sysv/linux/timer_routines.c b/sysdeps/unix/sysv/linux/timer_routines.c
index 98bbe077fa..97882f036b 100644
--- a/sysdeps/unix/sysv/linux/timer_routines.c
+++ b/sysdeps/unix/sysv/linux/timer_routines.c
@@ -16,19 +16,20 @@
    License along with the GNU C Library; see the file COPYING.LIB.  If
    not, see <https://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <setjmp.h>
+#include <pthread.h>
 #include <signal.h>
-#include <stdbool.h>
-#include <sysdep-cancel.h>
+#include <stdlib.h>
+
+#include <kernel-posix-timers.h>
+#include <internal-signals.h>
 #include <nptl/pthreadP.h>
-#include "kernel-posix-timers.h"
 
 
 /* List of active SIGEV_THREAD timers.  */
-struct timer *__active_timer_sigev_thread;
+static struct timer *__active_timer_sigev_thread;
 /* Lock for the __active_timer_sigev_thread.  */
-pthread_mutex_t __active_timer_sigev_thread_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t __active_timer_sigev_thread_lock
+  = PTHREAD_MUTEX_INITIALIZER;
 
 
 struct thread_start_data
@@ -56,6 +57,41 @@ timer_sigev_thread (void *arg)
   return NULL;
 }
 
+/* Append a new time KT on the on the active list __active_timer_sigev_thread
+   list in a thread safe way.  */
+void
+__add_active_timer_sigev (struct timer *kt)
+{
+  pthread_mutex_lock (&__active_timer_sigev_thread_lock);
+  kt->next = __active_timer_sigev_thread;
+  __active_timer_sigev_thread = kt;
+  pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
+}
+
+/* Remove the active timer KT on the active list __active_timer_sigev_thread
+   list in a thread safe way.  */
+void
+__remove_active_timer_sigev (struct timer *kt)
+{
+  /* Remove the timer from the list.  */
+  pthread_mutex_lock (&__active_timer_sigev_thread_lock);
+  if (__active_timer_sigev_thread == kt)
+    __active_timer_sigev_thread = kt->next;
+  else
+    {
+      struct timer *prevp = __active_timer_sigev_thread;
+      while (prevp->next != NULL)
+ if (prevp->next == kt)
+  {
+    prevp->next = kt->next;
+    break;
+  }
+ else
+  prevp = prevp->next;
+    }
+  pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
+}
+
 
 /* Helper function to support starting threads for SIGEV_THREAD.  */
 static void *
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Andreas Schwab
In reply to this post by Adhemerval Zanella-2
On Dez 10 2019, Adhemerval Zanella wrote:

> @@ -53,15 +54,16 @@ __clear_internal_signals (sigset_t *set)
>    __sigdelset (set, SIGSETXID);
>  }
>  
> -#define SIGALL_SET \
> -  ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
> +static const sigset_t sigall_set = {
> +   .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 }
> +};

Why do you need that static object?  Doesn't it suffice to make the
compound literal const?

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Adhemerval Zanella-2


On 11/12/2019 05:51, Andreas Schwab wrote:

> On Dez 10 2019, Adhemerval Zanella wrote:
>
>> @@ -53,15 +54,16 @@ __clear_internal_signals (sigset_t *set)
>>    __sigdelset (set, SIGSETXID);
>>  }
>>  
>> -#define SIGALL_SET \
>> -  ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
>> +static const sigset_t sigall_set = {
>> +   .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 }
>> +};
>
> Why do you need that static object?  Doesn't it suffice to make the
> compound literal const?

It is a suggestion from Florian to use less stack usage since the
gcc with compound literal materialize the object on the stack; and
slight compat code on some architecture (where coping the compiler
create compound object might incur in a memcpy call).
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Andreas Schwab
On Dez 11 2019, Adhemerval Zanella wrote:

> It is a suggestion from Florian to use less stack usage since the
> gcc with compound literal materialize the object on the stack; and
> slight compat code on some architecture (where coping the compiler
> create compound object might incur in a memcpy call).

It shouldn't do that for a const literal.

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 5/7] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask

Florian Weimer-5
In reply to this post by Adhemerval Zanella-2
* Adhemerval Zanella:

> The sigfillset already does it, and this is the canonical way to operate
> on sigset_t.  The only way to actually broke this assumption is if caller
> initialize sigset with memset or something similar, i.e, bypassing glibc
> (and again this is not a valid construction).

Is you argument that sigfillset already does this, and there is no way
to compute the complement of a signal set, so the bits for
SIGCANCEL and SIGSETXID can never become set?

I think it's possible to set them directly using sigaddset.  I don't see
why using SIGCANCEL/SIGSETXID with that function would be undefined.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 5/7] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask

Adhemerval Zanella-2


On 12/12/2019 09:54, Florian Weimer wrote:

> * Adhemerval Zanella:
>
>> The sigfillset already does it, and this is the canonical way to operate
>> on sigset_t.  The only way to actually broke this assumption is if caller
>> initialize sigset with memset or something similar, i.e, bypassing glibc
>> (and again this is not a valid construction).
>
> Is you argument that sigfillset already does this, and there is no way
> to compute the complement of a signal set, so the bits for
> SIGCANCEL and SIGSETXID can never become set?

Yes.

>
> I think it's possible to set them directly using sigaddset.  I don't see
> why using SIGCANCEL/SIGSETXID with that function would be undefined.

sigaddset filter out SIGCANCEL/SIGSETXID through __is_internal_signal,
returning EINVAL for such case.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/7] Block all signals on timer_create thread (BZ#10815)

Florian Weimer-5
In reply to this post by Adhemerval Zanella-2
* Adhemerval Zanella:

> The behavior of the signal mask on threads created by timer_create
> for SIGEV_THREAD timers are implementation defined and glibc explicit
                              implementation-defined           explictlly

> unblocks all signals before calling the user-defined function.
>
> This behavior, although not incorrect standard-wise, opens a race if a
> program using a blocked rt-signal plus sigwaitinfo (and without an
> installed signal handler for the rt-signal) receives the signal while
> executing the timer threads created by SIGEV_THREAD.

while executing the user-defined function for SIGEV_THREAD?

> A better alternative discussed in bug report is to rather block all
> signals (besides the internal ones not available to application
> usage).
>
> This patch fixes by only unblock the SIGSETXID (used on set*uid).
             fixes this issiue by only unblicking SIGSETXID

> The SIGTIMER is the same as SIGCANCEL and it will be handled by

No “The”?

> sigwaitinfo on the helper thread (thus it can be masked as well).
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.

Please also try to build this on Hurd.

> diff --git a/rt/tst-timer-sigmask.c b/rt/tst-timer-sigmask.c
> new file mode 100644
> index 0000000000..1030e4c79f
> --- /dev/null
> +++ b/rt/tst-timer-sigmask.c
> @@ -0,0 +1,96 @@

> +static void
> +thread_handler (union sigval sv)
> +{
> +  bool failure = false;
> +
> +  sigset_t ss;
> +  sigprocmask (SIG_BLOCK, NULL, &ss);
> +  if (test_verbose > 0)
> +    printf ("%s: blocked signal mask = { ", __func__);
> +  for (int sig = 1; sig < NSIG; sig++)
> +    {
> +#ifdef __linux__
> +      /* POSIX timers threads created to handle SIGEV_THREAD blocks
> + all signals except SIGKILL, SIGSTOP, and SIGSETXID.  */
> +      if (!sigismember (&ss, sig)
> +  && (sig != SIGSETXID && sig != SIGKILL && sig != SIGSTOP))
> + {
> +  failure = true;
> + }
> +#endif
> +      if (test_verbose && sigismember (&ss, sig))
> + printf ("%d, ", sig);
> +    }
> +  if (test_verbose > 0)
> +    printf ("}\n");
> +
> +  pthread_mutex_lock (&lock);
> +  thread_handler_check = failure
> + ? THREAD_SIGNAL_CHECK_FAIL : THREAD_SIGNAL_CHECK_OK;
> +  pthread_cond_signal (&cond);
> +  pthread_mutex_unlock (&lock);

Should you use the x variants?  xpthread_sigmask, xpthread_mutex_lock
etc.?

The synchronization might more easily expressed using a POSIX barrier.

> diff --git a/sysdeps/unix/sysv/linux/kernel-posix-timers.h b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
> index 1ded4df51a..d60cb95f80 100644
> --- a/sysdeps/unix/sysv/linux/kernel-posix-timers.h
> +++ b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
> @@ -43,7 +43,6 @@ extern pthread_mutex_t __active_timer_sigev_thread_lock attribute_hidden;
>  /* Type of timers in the kernel.  */
>  typedef int kernel_timer_t;
>  
> -
>  /* Internal representation of timer.  */
>  struct timer
>  {

Unrelated change?

> +  /* This is the signal we are waiting for.  */
> +  td->thrfunc = tk->thrfunc;
> +  td->sival = tk->sival;
>  
> +  pthread_t th;
> +  pthread_create (&th, &tk->attr, timer_sigev_thread, td);
> + }

I think this does not unblock SIGCANCEL on the new thread with the
current pthread_create implementation.  This will change with the
block-signals-around-clone patch I submitted, though.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 5/7] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask

Florian Weimer-5
In reply to this post by Adhemerval Zanella-2
* Adhemerval Zanella:

> On 12/12/2019 09:54, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>> The sigfillset already does it, and this is the canonical way to operate
>>> on sigset_t.  The only way to actually broke this assumption is if caller
>>> initialize sigset with memset or something similar, i.e, bypassing glibc
>>> (and again this is not a valid construction).
>>
>> Is you argument that sigfillset already does this, and there is no way
>> to compute the complement of a signal set, so the bits for
>> SIGCANCEL and SIGSETXID can never become set?
>
> Yes.
>
>>
>> I think it's possible to set them directly using sigaddset.  I don't see
>> why using SIGCANCEL/SIGSETXID with that function would be undefined.
>
> sigaddset filter out SIGCANCEL/SIGSETXID through __is_internal_signal,
> returning EINVAL for such case.

Oh, I see.

It's still not clear to me whether it is not in fact better to allow
appplications to block internal signals (from a compatibility
perspective, e.g. if the application knows that the stack pointer is
problematic).

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 5/7] linux: Remove SIGCANCEL/SIGSETXID handling on sigprocmask

Adhemerval Zanella-2


On 12/12/2019 10:12, Florian Weimer wrote:

> * Adhemerval Zanella:
>
>> On 12/12/2019 09:54, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> The sigfillset already does it, and this is the canonical way to operate
>>>> on sigset_t.  The only way to actually broke this assumption is if caller
>>>> initialize sigset with memset or something similar, i.e, bypassing glibc
>>>> (and again this is not a valid construction).
>>>
>>> Is you argument that sigfillset already does this, and there is no way
>>> to compute the complement of a signal set, so the bits for
>>> SIGCANCEL and SIGSETXID can never become set?
>>
>> Yes.
>>
>>>
>>> I think it's possible to set them directly using sigaddset.  I don't see
>>> why using SIGCANCEL/SIGSETXID with that function would be undefined.
>>
>> sigaddset filter out SIGCANCEL/SIGSETXID through __is_internal_signal,
>> returning EINVAL for such case.
>
> Oh, I see.
>
> It's still not clear to me whether it is not in fact better to allow
> appplications to block internal signals (from a compatibility
> perspective, e.g. if the application knows that the stack pointer is
> problematic).

My view is the semantic of the signals are not exported to userspace
(we could use a different signal for SIGCANCEL in a future version,
for instance) and we can eventually phase out the signal usage if
either POSIX deprecate some functionality or if kernel provides a
cleanly way to accomplish the required functionality (for instance,
if it provides a syscall that change the xid of all threads).

Application can still block internal signals, but they will to actually
statically initialize a sigprocmask in a non standard way.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/7] Block all signals on timer_create thread (BZ#10815)

Adhemerval Zanella-2
In reply to this post by Florian Weimer-5


On 12/12/2019 10:10, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> The behavior of the signal mask on threads created by timer_create
>> for SIGEV_THREAD timers are implementation defined and glibc explicit
>                               implementation-defined           explictlly

Ack.

>
>> unblocks all signals before calling the user-defined function.
>>
>> This behavior, although not incorrect standard-wise, opens a race if a
>> program using a blocked rt-signal plus sigwaitinfo (and without an
>> installed signal handler for the rt-signal) receives the signal while
>> executing the timer threads created by SIGEV_THREAD.
>
> while executing the user-defined function for SIGEV_THREAD?

Ack.

>
>> A better alternative discussed in bug report is to rather block all
>> signals (besides the internal ones not available to application
>> usage).
>>
>> This patch fixes by only unblock the SIGSETXID (used on set*uid).
>              fixes this issiue by only unblicking SIGSETXID
>
>> The SIGTIMER is the same as SIGCANCEL and it will be handled by
>
> No “The”?

Ack.

>
>> sigwaitinfo on the helper thread (thus it can be masked as well).
>>
>> Checked on x86_64-linux-gnu and i686-linux-gnu.
>
> Please also try to build this on Hurd.

I usually do, both build and rt/tests builds fine on i686-gnu.

>
>> diff --git a/rt/tst-timer-sigmask.c b/rt/tst-timer-sigmask.c
>> new file mode 100644
>> index 0000000000..1030e4c79f
>> --- /dev/null
>> +++ b/rt/tst-timer-sigmask.c
>> @@ -0,0 +1,96 @@
>
>> +static void
>> +thread_handler (union sigval sv)
>> +{
>> +  bool failure = false;
>> +
>> +  sigset_t ss;
>> +  sigprocmask (SIG_BLOCK, NULL, &ss);
>> +  if (test_verbose > 0)
>> +    printf ("%s: blocked signal mask = { ", __func__);
>> +  for (int sig = 1; sig < NSIG; sig++)
>> +    {
>> +#ifdef __linux__
>> +      /* POSIX timers threads created to handle SIGEV_THREAD blocks
>> + all signals except SIGKILL, SIGSTOP, and SIGSETXID.  */
>> +      if (!sigismember (&ss, sig)
>> +  && (sig != SIGSETXID && sig != SIGKILL && sig != SIGSTOP))
>> + {
>> +  failure = true;
>> + }
>> +#endif
>> +      if (test_verbose && sigismember (&ss, sig))
>> + printf ("%d, ", sig);
>> +    }
>> +  if (test_verbose > 0)
>> +    printf ("}\n");
>> +
>> +  pthread_mutex_lock (&lock);
>> +  thread_handler_check = failure
>> + ? THREAD_SIGNAL_CHECK_FAIL : THREAD_SIGNAL_CHECK_OK;
>> +  pthread_cond_signal (&cond);
>> +  pthread_mutex_unlock (&lock);
>
> Should you use the x variants?  xpthread_sigmask, xpthread_mutex_lock
> etc.?
>
> The synchronization might more easily expressed using a POSIX barrier.

Ack, I changed to use xpthread_barrier_*.

>
>> diff --git a/sysdeps/unix/sysv/linux/kernel-posix-timers.h b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
>> index 1ded4df51a..d60cb95f80 100644
>> --- a/sysdeps/unix/sysv/linux/kernel-posix-timers.h
>> +++ b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
>> @@ -43,7 +43,6 @@ extern pthread_mutex_t __active_timer_sigev_thread_lock attribute_hidden;
>>  /* Type of timers in the kernel.  */
>>  typedef int kernel_timer_t;
>>  
>> -
>>  /* Internal representation of timer.  */
>>  struct timer
>>  {
>
> Unrelated change?

Ack.

>
>> +  /* This is the signal we are waiting for.  */
>> +  td->thrfunc = tk->thrfunc;
>> +  td->sival = tk->sival;
>>  
>> +  pthread_t th;
>> +  pthread_create (&th, &tk->attr, timer_sigev_thread, td);
>> + }
>
> I think this does not unblock SIGCANCEL on the new thread with the
> current pthread_create implementation.  This will change with the
> block-signals-around-clone patch I submitted, though.

It does not indeed, although not sure how valid is the scenario to
pthread cancel a thread not created explicitly by a pthread_create
issued by the user (since POSIX also state it is not valid to call
pthread_join() timer thread).

The sigcancel is explicit blocked with __libc_signal_block_sigtimer
on __start_helper_thread, so sigwaitinfo would be the only point
where the timer is triggered.  I don't think it is correct to unblock
it before calling pthread_create, so the created thread inherit the
unblock SIGCANCEL, for the same reason the SIGCANCEL is blocked on the
helper thread that calls sigwaitinfo (possible lose wakeups).

Another possibility is decouple SIGTIMER from SIGCANCEL, so the
helper sigwaitinfo thread is created with SIGTIMER blocked, but
not SIGCANCEL.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/7] Block all signals on timer_create thread (BZ#10815)

Florian Weimer-5
* Adhemerval Zanella:

>> I think this does not unblock SIGCANCEL on the new thread with the
>> current pthread_create implementation.  This will change with the
>> block-signals-around-clone patch I submitted, though.
>
> It does not indeed, although not sure how valid is the scenario to
> pthread cancel a thread not created explicitly by a pthread_create
> issued by the user (since POSIX also state it is not valid to call
> pthread_join() timer thread).

I don't see any restrictions on calling pthread_cancel or pthread_exit.

> The sigcancel is explicit blocked with __libc_signal_block_sigtimer
> on __start_helper_thread, so sigwaitinfo would be the only point
> where the timer is triggered.  I don't think it is correct to unblock
> it before calling pthread_create, so the created thread inherit the
> unblock SIGCANCEL, for the same reason the SIGCANCEL is blocked on the
> helper thread that calls sigwaitinfo (possible lose wakeups).

We can wrap the thread start function with another function that
unblocks SIGCANCEL.  Or note the semantic difference on the other change
(which I assume we still want for other reasons).

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/7] Block all signals on timer_create thread (BZ#10815)

Adhemerval Zanella-2


On 12/12/2019 12:14, Florian Weimer wrote:

> * Adhemerval Zanella:
>
>>> I think this does not unblock SIGCANCEL on the new thread with the
>>> current pthread_create implementation.  This will change with the
>>> block-signals-around-clone patch I submitted, though.
>>
>> It does not indeed, although not sure how valid is the scenario to
>> pthread cancel a thread not created explicitly by a pthread_create
>> issued by the user (since POSIX also state it is not valid to call
>> pthread_join() timer thread).
>
> I don't see any restrictions on calling pthread_cancel or pthread_exit.
>
>> The sigcancel is explicit blocked with __libc_signal_block_sigtimer
>> on __start_helper_thread, so sigwaitinfo would be the only point
>> where the timer is triggered.  I don't think it is correct to unblock
>> it before calling pthread_create, so the created thread inherit the
>> unblock SIGCANCEL, for the same reason the SIGCANCEL is blocked on the
>> helper thread that calls sigwaitinfo (possible lose wakeups).
>
> We can wrap the thread start function with another function that
> unblocks SIGCANCEL.  Or note the semantic difference on the other change
> (which I assume we still want for other reasons).

We can add such call ontimer_sigev_thread, but it does not really
help the 'rt' side of the function with another syscall. Do you
consider this a block? Should we reinstate the sigprocmask to
unblock SIGCANCEL on timer_sigev_thread?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Adhemerval Zanella-2
In reply to this post by Andreas Schwab


On 11/12/2019 11:06, Andreas Schwab wrote:
> On Dez 11 2019, Adhemerval Zanella wrote:
>
>> It is a suggestion from Florian to use less stack usage since the
>> gcc with compound literal materialize the object on the stack; and
>> slight compat code on some architecture (where coping the compiler
>> create compound object might incur in a memcpy call).
>
> It shouldn't do that for a const literal.

Well I am seeing such behaviour with gcc 9.2.1 on x86_64 at least:

$ cat sigmask.c
#include <signal.h>

#ifdef COMPOUND
#define sigall \
  ((const __sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
#else
static const sigset_t sigall = {
   .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 }
};
#endif

int foo (void)
{
  return sigprocmask (SIG_BLOCK, &sigall, 0);
}
$ x86_64-glibc-linux-gnu-gcc -O2 -std=gnu11 sigmask.c -S -o -
[...]
foo:
.LFB0:
        .cfi_startproc
        xorl    %edx, %edx
        movl    $sigall, %esi
        xorl    %edi, %edi
        jmp     sigprocmask
[...]
$ x86_64-glibc-linux-gnu-gcc -O2 -std=gnu11 sigmask.c -S -o - -DCOMPOUND
[...]foo:
.LFB0:
        .cfi_startproc
        subq    $136, %rsp
        .cfi_def_cfa_offset 144
        xorl    %edx, %edx
        xorl    %edi, %edi
        movq    %rsp, %rsi
        movq    $-1, (%rsp)
        [...]
        call    sigprocmask
        addq    $136, %rsp
[...]

Do you consider this a blocker? Should we use the compound literal? The
advantage of the static global is it slighter easier to define different
mask for the different ABI (64-bit, 32-bit, and mips with its outlier
number of signals).
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/7] Fix __libc_signal_block_all on sparc64

Andreas Schwab
On Dez 12 2019, Adhemerval Zanella wrote:

> Well I am seeing such behaviour with gcc 9.2.1 on x86_64 at least:

That's a missed optimisation bug in gcc then.  There should not be a
difference between a const compound literal and a static const object,
if only constant expressions are used for initialisation.

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
12