Fix __libc_signal_block_all on sparc64
Commit Message
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(-)
Comments
* 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
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.
* 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
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.
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.
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
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.
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).
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.
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
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.
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.
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.
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.
@@ -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);
}
@@ -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. */