diff mbox series

Fix __libc_signal_block_all on sparc64

Message ID 20191205143526.27478-1-adhemerval.zanella@linaro.org
State New
Headers show
Series Fix __libc_signal_block_all on sparc64 | expand

Commit Message

Adhemerval Zanella Dec. 5, 2019, 2:35 p.m. UTC 
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 also changes the return value of __libc_signal_block_all,
__libc_signal_block_app, and __libc_signal_restore_set to 'void'
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 sparc64-linux-gnu.
---
 sysdeps/generic/internal-signals.h         | 12 ++++++------
 sysdeps/unix/sysv/linux/internal-signals.h | 19 ++++++++++---------
 2 files changed, 16 insertions(+), 15 deletions(-)

-- 
2.17.1

Comments

Florian Weimer Dec. 5, 2019, 2:45 p.m. UTC | #1 
* Adhemerval Zanella:

> Where SIGALL_SET is defined as:

>

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


Shouldn't this refer to a global constant data object?  Then we wouldn't
have to emit many local copies of the same object and then copy it onto
the stack.

(GCC cannot know that the system call will not modify the object.)

Thanks,
Florian
Adhemerval Zanella Dec. 5, 2019, 4:51 p.m. UTC | #2 
On 05/12/2019 11:45, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

>> Where SIGALL_SET is defined as:

>>

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

> 

> Shouldn't this refer to a global constant data object?  Then we wouldn't

> have to emit many local copies of the same object and then copy it onto

> the stack.

> 

> (GCC cannot know that the system call will not modify the object.)


Do we really need to add a sigset_t object on ld (it won't require it
any longer since you sent a reverted patch to the signal block on dlopen),
libc, and libpthread?

In fact I think we can simplify it a bit an just get rid of these
block/unblock signals and just use sigprocmask directly. I haven't done
on posix_spawn because the sigprocmask semantic cleared the internal
signals prior issuing rt_sigprocmask. 

However 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 imho).

So I what I am thinking is to consolidate the sigprocmask, remove the
SIGCANCEL/SIGSETXID handling (which is not done on all architectures
btw), and replace __libc_signal_block_all, __libc_signal_block_app,
and __libc_signal_restore_set with straight sigprocmask calls.
Florian Weimer Dec. 5, 2019, 5:05 p.m. UTC | #3 
* Adhemerval Zanella:

> On 05/12/2019 11:45, Florian Weimer wrote:

>> * Adhemerval Zanella:

>> 

>>> Where SIGALL_SET is defined as:

>>>

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

>> 

>> Shouldn't this refer to a global constant data object?  Then we wouldn't

>> have to emit many local copies of the same object and then copy it onto

>> the stack.

>> 

>> (GCC cannot know that the system call will not modify the object.)

>

> Do we really need to add a sigset_t object on ld (it won't require it

> any longer since you sent a reverted patch to the signal block on dlopen),

> libc, and libpthread?


It's already there, see the .LC0 label.  GCC should be using memset
instead of memcpy for the initialization, but it currently does not.

> In fact I think we can simplify it a bit an just get rid of these

> block/unblock signals and just use sigprocmask directly. I haven't done

> on posix_spawn because the sigprocmask semantic cleared the internal

> signals prior issuing rt_sigprocmask. 

>

> However 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 imho).


Another alternative would be to remove the restriction on blocking
internal signals altogether.  I don't think this would be very
problematic for SIGCANCEL.  For SIGSETXID, I think the code would just
block in the setxid caller due to the missing futex wakeup, and not
continue to run with partially unchanged credentials in case of a
blocked SIGSETXID signal on some thread.

Thanks,
Florian
Adhemerval Zanella Dec. 5, 2019, 5:13 p.m. UTC | #4 
On 05/12/2019 14:05, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

>> On 05/12/2019 11:45, Florian Weimer wrote:

>>> * Adhemerval Zanella:

>>>

>>>> Where SIGALL_SET is defined as:

>>>>

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

>>>

>>> Shouldn't this refer to a global constant data object?  Then we wouldn't

>>> have to emit many local copies of the same object and then copy it onto

>>> the stack.

>>>

>>> (GCC cannot know that the system call will not modify the object.)

>>

>> Do we really need to add a sigset_t object on ld (it won't require it

>> any longer since you sent a reverted patch to the signal block on dlopen),

>> libc, and libpthread?

> 

> It's already there, see the .LC0 label.  GCC should be using memset

> instead of memcpy for the initialization, but it currently does not.


I think we can use something like:

static const sigset_t sigall_set =
  { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } };

It would give compiler/linker more information to remove duplicate
definitions.

> 

>> In fact I think we can simplify it a bit an just get rid of these

>> block/unblock signals and just use sigprocmask directly. I haven't done

>> on posix_spawn because the sigprocmask semantic cleared the internal

>> signals prior issuing rt_sigprocmask. 

>>

>> However 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 imho).

> 

> Another alternative would be to remove the restriction on blocking

> internal signals altogether.  I don't think this would be very

> problematic for SIGCANCEL.  For SIGSETXID, I think the code would just

> block in the setxid caller due to the missing futex wakeup, and not

> continue to run with partially unchanged credentials in case of a

> blocked SIGSETXID signal on some thread.


I think for sigfillset there is no much gain in allowing all signals,
blocking the internal ones adds some consistency at least.
Andreas Schwab Dec. 9, 2019, 11 a.m. UTC | #5 
On Dez 05 2019, Adhemerval Zanella wrote:

> Where SIGALL_SET is defined as:

>

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


Why is that not const?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Adhemerval Zanella Dec. 9, 2019, 11:27 a.m. UTC | #6 
On 09/12/2019 08:00, Andreas Schwab wrote:
> On Dez 05 2019, Adhemerval Zanella wrote:

> 

>> Where SIGALL_SET is defined as:

>>

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

> 

> Why is that not const?

> 

> Andreas.

> 


It is on the updated version [1].

[1] https://sourceware.org/ml/libc-alpha/2019-12/msg00190.html
Andreas Schwab Dec. 9, 2019, 11:59 a.m. UTC | #7 
On Dez 09 2019, Adhemerval Zanella wrote:

> On 09/12/2019 08:00, Andreas Schwab wrote:

>> On Dez 05 2019, Adhemerval Zanella wrote:

>> 

>>> Where SIGALL_SET is defined as:

>>>

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

>> 

>> Why is that not const?

>> 

>> Andreas.

>> 

>

> It is on the updated version [1].


Does it help to make it const?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Adhemerval Zanella Dec. 9, 2019, 12:24 p.m. UTC | #8 
On 09/12/2019 08:59, Andreas Schwab wrote:
> On Dez 09 2019, Adhemerval Zanella wrote:

> 

>> On 09/12/2019 08:00, Andreas Schwab wrote:

>>> On Dez 05 2019, Adhemerval Zanella wrote:

>>>

>>>> Where SIGALL_SET is defined as:

>>>>

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

>>>

>>> Why is that not const?

>>>

>>> Andreas.

>>>

>>

>> It is on the updated version [1].

> 

> Does it help to make it const?

> 


It does, that's the main reason for the updated version (besides
the sigprocmask changes).
Andreas Schwab Dec. 9, 2019, 12:53 p.m. UTC | #9 
On Dez 09 2019, Adhemerval Zanella wrote:

> On 09/12/2019 08:59, Andreas Schwab wrote:

>> On Dez 09 2019, Adhemerval Zanella wrote:

>> 

>>> On 09/12/2019 08:00, Andreas Schwab wrote:

>>>> On Dez 05 2019, Adhemerval Zanella wrote:

>>>>

>>>>> Where SIGALL_SET is defined as:

>>>>>

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

>>>>

>>>> Why is that not const?

>>>>

>>>> Andreas.

>>>>

>>>

>>> It is on the updated version [1].

>> 

>> Does it help to make it const?

>> 

>

> It does, that's the main reason for the updated version


No, I mean without the other changes.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Adhemerval Zanella Dec. 9, 2019, 1:18 p.m. UTC | #10 
On 09/12/2019 09:53, Andreas Schwab wrote:
> On Dez 09 2019, Adhemerval Zanella wrote:

> 

>> On 09/12/2019 08:59, Andreas Schwab wrote:

>>> On Dez 09 2019, Adhemerval Zanella wrote:

>>>

>>>> On 09/12/2019 08:00, Andreas Schwab wrote:

>>>>> On Dez 05 2019, Adhemerval Zanella wrote:

>>>>>

>>>>>> Where SIGALL_SET is defined as:

>>>>>>

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

>>>>>

>>>>> Why is that not const?

>>>>>

>>>>> Andreas.

>>>>>

>>>>

>>>> It is on the updated version [1].

>>>

>>> Does it help to make it const?

>>>

>>

>> It does, that's the main reason for the updated version

> 

> No, I mean without the other changes.

> 


The const fixes the issues, the other changes on [1] are essentially 
changing __libc_signal_ to return void.

[1] https://sourceware.org/ml/libc-alpha/2019-12/msg00190.html
Andreas Schwab Dec. 9, 2019, 1:23 p.m. UTC | #11 
On Dez 09 2019, Adhemerval Zanella wrote:

> The const fixes the issues, the other changes on [1] are essentially 

> changing __libc_signal_ to return void.


So the other changes are not necessary?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Adhemerval Zanella Dec. 9, 2019, 2:18 p.m. UTC | #12 
On 09/12/2019 10:23, Andreas Schwab wrote:
> On Dez 09 2019, Adhemerval Zanella wrote:

> 

>> The const fixes the issues, the other changes on [1] are essentially 

>> changing __libc_signal_ to return void.

> 

> So the other changes are not necessary?


Not strictly, the 'Fix __libc_signal_block_all on sparc64' can be applied
independently.
Andreas Schwab Dec. 9, 2019, 2:54 p.m. UTC | #13 
On Dez 09 2019, Adhemerval Zanella wrote:

> On 09/12/2019 10:23, Andreas Schwab wrote:

>> On Dez 09 2019, Adhemerval Zanella wrote:

>> 

>>> The const fixes the issues, the other changes on [1] are essentially 

>>> changing __libc_signal_ to return void.

>> 

>> So the other changes are not necessary?

>

> Not strictly, the 'Fix __libc_signal_block_all on sparc64' can be applied

> independently. 


But that patch contains the other changes.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Adhemerval Zanella Dec. 9, 2019, 4:52 p.m. UTC | #14 
On 09/12/2019 11:54, Andreas Schwab wrote:
> On Dez 09 2019, Adhemerval Zanella wrote:

> 

>> On 09/12/2019 10:23, Andreas Schwab wrote:

>>> On Dez 09 2019, Adhemerval Zanella wrote:

>>>

>>>> The const fixes the issues, the other changes on [1] are essentially 

>>>> changing __libc_signal_ to return void.

>>>

>>> So the other changes are not necessary?

>>

>> Not strictly, the 'Fix __libc_signal_block_all on sparc64' can be applied

>> independently. 

> 

> But that patch contains the other changes.


Ok, I will send a v3 with the fix only for the referenced issue.
diff mbox series

Patch

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 01d8bf0a4c..0a5faf175f 100644
--- a/sysdeps/unix/sysv/linux/internal-signals.h
+++ b/sysdeps/unix/sysv/linux/internal-signals.h
@@ -57,32 +57,33 @@  __clear_internal_signals (sigset_t *set)
   ((__sigset_t) { .__val = {[0 ...  _SIGSET_NWORDS-1 ] =  -1 } })
 
 /* Block all signals, including internal glibc ones.  */
-static inline int
+static inline void
 __libc_signal_block_all (sigset_t *set)
 {
+  sigset_t allset = SIGALL_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, &allset, set,
+			 _NSIG / 8);
 }
 
 /* Block all application signals (excluding internal glibc ones).  */
-static inline int
+static inline void
 __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, set,
-			   _NSIG / 8);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, err, SIG_BLOCK, &allset, 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.  */