[PATCH] signal: Unexport sigsuspend()

[PATCH] signal: Unexport sigsuspend()

[Richard Weinberger]

sigsuspend() is nowhere used except in signal.c itself,
so we can mark it static do not pollute the global namespace.

But this patch is more than a boring cleanup patch,
it fixes a real issue on UserModeLinux.
UML has a special console driver to display ttys using xterm,
or other terminal emulators, on the host side.
Vegard reported that sometimes UML is unable to spawn a xterm
and he's facing the following warning:
WARNING: CPU: 0 PID: 908 at include/linux/thread_info.h:128 sigsuspend+0xab/0xc0()
It turned out that this warning makes absolutely no sense as
the UML xterm code calls sigsuspend() on the host side, at least it tries.
But as the kernel itself offers a sigsuspend() symbol the linker choose
this one instead of the glibc wrapper. Interestingly this code used to
work since ever but always blocked signals on the wrong side.
Some recent kernel change made the WARN_ON() trigger and uncovered the bug.

It is a wonderful example of how much works by chance on computers. :-)

Reported-and-tested-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 include/linux/signal.h | 1 -
 kernel/signal.c        | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/signal.h b/include/linux/signal.h
index ab1e039..92557bb 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -239,7 +239,6 @@ extern int sigprocmask(int, sigset_t *, sigset_t *);
 extern void set_current_blocked(sigset_t *);
 extern void __set_current_blocked(const sigset_t *);
 extern int show_unhandled_signals;
-extern int sigsuspend(sigset_t *);
 
 struct sigaction {
 #ifndef __ARCH_HAS_IRIX_SIGACTION
diff --git a/kernel/signal.c b/kernel/signal.c
index c0b01fe..f3f1f7a 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3503,7 +3503,7 @@ SYSCALL_DEFINE0(pause)
 
 #endif
 
-int sigsuspend(sigset_t *set)
+static int sigsuspend(sigset_t *set)
 {
 	current->saved_sigmask = current->blocked;
 	set_current_blocked(set);
-- 
2.5.0

Re: [PATCH] signal: Unexport sigsuspend()

[Oleg Nesterov]

On 11/16, Richard Weinberger wrote:
>
> sigsuspend() is nowhere used except in signal.c itself,
> so we can mark it static do not pollute the global namespace.
>
> But this patch is more than a boring cleanup patch,
> it fixes a real issue on UserModeLinux.
> UML has a special console driver to display ttys using xterm,
> or other terminal emulators, on the host side.
> Vegard reported that sometimes UML is unable to spawn a xterm
> and he's facing the following warning:
> WARNING: CPU: 0 PID: 908 at include/linux/thread_info.h:128 sigsuspend+0xab/0xc0()
> It turned out that this warning makes absolutely no sense as
> the UML xterm code calls sigsuspend() on the host side, at least it tries.
> But as the kernel itself offers a sigsuspend() symbol the linker choose
> this one instead of the glibc wrapper. Interestingly this code used to
> work since ever but always blocked signals on the wrong side.
> Some recent kernel change made the WARN_ON() trigger and uncovered the bug.
>
> It is a wonderful example of how much works by chance on computers. :-)

You know, initially I didn't bother to read the changelog, I was going
to nack this change because git-grep reports that sigsuspend() is called
by arch/um/drivers/chan_user.c ;)

> Reported-and-tested-by: Vegard Nossum <vegard.nossum@oracle.com>
> Signed-off-by: Richard Weinberger <richard@nod.at>

Acked-by: Oleg Nesterov <oleg@redhat.com>

> ---
>  include/linux/signal.h | 1 -
>  kernel/signal.c        | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index ab1e039..92557bb 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -239,7 +239,6 @@ extern int sigprocmask(int, sigset_t *, sigset_t *);
>  extern void set_current_blocked(sigset_t *);
>  extern void __set_current_blocked(const sigset_t *);
>  extern int show_unhandled_signals;
> -extern int sigsuspend(sigset_t *);
>  
>  struct sigaction {
>  #ifndef __ARCH_HAS_IRIX_SIGACTION
> diff --git a/kernel/signal.c b/kernel/signal.c
> index c0b01fe..f3f1f7a 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3503,7 +3503,7 @@ SYSCALL_DEFINE0(pause)
>  
>  #endif
>  
> -int sigsuspend(sigset_t *set)
> +static int sigsuspend(sigset_t *set)
>  {
>  	current->saved_sigmask = current->blocked;
>  	set_current_blocked(set);
> -- 
> 2.5.0
> 

Re: [PATCH] signal: Unexport sigsuspend()

[Andrew Morton]

On Mon, 16 Nov 2015 19:18:21 +0100 Richard Weinberger <richard@nod.at> wrote:

> sigsuspend() is nowhere used except in signal.c itself,
> so we can mark it static do not pollute the global namespace.
> 
> But this patch is more than a boring cleanup patch,
> it fixes a real issue on UserModeLinux.
> UML has a special console driver to display ttys using xterm,
> or other terminal emulators, on the host side.
> Vegard reported that sometimes UML is unable to spawn a xterm
> and he's facing the following warning:
> WARNING: CPU: 0 PID: 908 at include/linux/thread_info.h:128 sigsuspend+0xab/0xc0()
> It turned out that this warning makes absolutely no sense as
> the UML xterm code calls sigsuspend() on the host side, at least it tries.
> But as the kernel itself offers a sigsuspend() symbol the linker choose
> this one instead of the glibc wrapper. Interestingly this code used to
> work since ever but always blocked signals on the wrong side.
> Some recent kernel change made the WARN_ON() trigger and uncovered the bug.
> 

So we don't know what caused this or when it started happening.  hrm. 
I guess I'll stick a cc:stable in there as it's likely to affect 4.3
and perhaps earlier, OK?

Re: [PATCH] signal: Unexport sigsuspend()

[Richard Weinberger]

Am 18.11.2015 um 21:44 schrieb Andrew Morton:
> On Mon, 16 Nov 2015 19:18:21 +0100 Richard Weinberger <richard@nod.at> wrote:
> 
>> sigsuspend() is nowhere used except in signal.c itself,
>> so we can mark it static do not pollute the global namespace.
>>
>> But this patch is more than a boring cleanup patch,
>> it fixes a real issue on UserModeLinux.
>> UML has a special console driver to display ttys using xterm,
>> or other terminal emulators, on the host side.
>> Vegard reported that sometimes UML is unable to spawn a xterm
>> and he's facing the following warning:
>> WARNING: CPU: 0 PID: 908 at include/linux/thread_info.h:128 sigsuspend+0xab/0xc0()
>> It turned out that this warning makes absolutely no sense as
>> the UML xterm code calls sigsuspend() on the host side, at least it tries.
>> But as the kernel itself offers a sigsuspend() symbol the linker choose
>> this one instead of the glibc wrapper. Interestingly this code used to
>> work since ever but always blocked signals on the wrong side.
>> Some recent kernel change made the WARN_ON() trigger and uncovered the bug.
>>
> 
> So we don't know what caused this or when it started happening.  hrm. 
> I guess I'll stick a cc:stable in there as it's likely to affect 4.3
> and perhaps earlier, OK?

I fear it has been this way for ever.
CC'ing stable is a good idea!

Thanks,
//richard

Re: [PATCH] signal: Unexport sigsuspend()

[Richard Weinberger]

Am 18.11.2015 um 21:47 schrieb Richard Weinberger:
> Am 18.11.2015 um 21:44 schrieb Andrew Morton:
>> On Mon, 16 Nov 2015 19:18:21 +0100 Richard Weinberger <richard@nod.at> wrote:
>>
>>> sigsuspend() is nowhere used except in signal.c itself,
>>> so we can mark it static do not pollute the global namespace.
>>>
>>> But this patch is more than a boring cleanup patch,
>>> it fixes a real issue on UserModeLinux.
>>> UML has a special console driver to display ttys using xterm,
>>> or other terminal emulators, on the host side.
>>> Vegard reported that sometimes UML is unable to spawn a xterm
>>> and he's facing the following warning:
>>> WARNING: CPU: 0 PID: 908 at include/linux/thread_info.h:128 sigsuspend+0xab/0xc0()
>>> It turned out that this warning makes absolutely no sense as
>>> the UML xterm code calls sigsuspend() on the host side, at least it tries.
>>> But as the kernel itself offers a sigsuspend() symbol the linker choose
>>> this one instead of the glibc wrapper. Interestingly this code used to
>>> work since ever but always blocked signals on the wrong side.
>>> Some recent kernel change made the WARN_ON() trigger and uncovered the bug.
>>>
>>
>> So we don't know what caused this or when it started happening.  hrm. 
>> I guess I'll stick a cc:stable in there as it's likely to affect 4.3
>> and perhaps earlier, OK?
> 
> I fear it has been this way for ever.

Did some research. It is not that bad.
It broke in v3.5 because of this commit:

commit 68f3f16d9ad0f1e28ab3fd0001ab5798c41f15a3
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Mon May 21 21:42:32 2012 -0400

    new helper: sigsuspend()

    guts of saved_sigmask-based sigsuspend/rt_sigsuspend.  Takes
    kernel sigset_t *.

    Open-coded instances replaced with calling it

Thanks,
//richard