Re: [PATCH v3] kernel/signal.c: fix BUG_ON with SIG128 (MIPS)

[James Hogan]

On 26/06/13 18:15, Oleg Nesterov wrote:
> On 06/26, Ralf Baechle wrote:
>>
>> On Wed, Jun 26, 2013 at 06:14:52PM +0200, Oleg Nesterov wrote:
>>
>>> Or simply remove the BUG_ON(), this can equally confuse wait(status).
>>> 128 & 0x7f == 0.
>>>
>>> Still I think it would be better to change _NSIG on mips.
>>
>> If it was that easy.  That's going to outright break binary compatibility,
>> see kernel/signal.c:
>>
>> SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, nset,
>>                 sigset_t __user *, oset, size_t, sigsetsize)
>> {
>>         sigset_t old_set, new_set;
>>         int error;
>>
>>         /* XXX: Don't preclude handling different sized sigset_t's.  */
>>         if (sigsetsize != sizeof(sigset_t))
>>                 return -EINVAL;
> 
> I meant the minimal hack like
> 
> 	--- x/arch/mips/include/uapi/asm/signal.h
> 	+++ x/arch/mips/include/uapi/asm/signal.h
> 	@@ -11,9 +11,9 @@
> 	 
> 	 #include <linux/types.h>
> 	 
> 	-#define _NSIG		128
> 	+#define _NSIG		127
> 	 #define _NSIG_BPW	(sizeof(unsigned long) * 8)
> 	-#define _NSIG_WORDS	(_NSIG / _NSIG_BPW)
> 	+#define _NSIG_WORDS	DIV_ROUND_UP(_NSIG / _NSIG_BPW)
> 	 
> 	 typedef struct {
> 		unsigned long sig[_NSIG_WORDS];
> 
> just to avoid BUG_ON().
> 
> I agree that _NSIG == 126 or 64 needs more discussion. Although personally
> I think this is the only choice in the long term, or we should change ABI
> and break user-space completely.
> 
> And, just in case, the hack above doesn't kill SIG_128 completely.
> Say, the task can block/unblock it.

Well it prevents a handler being added or the signal being sent, so it
pretty much does kill it (patch v2 did this). Since glibc already has
SIGRTMAX=127, nothing running glibc should be affected unless it's being
really stupid (uClibc and bionic is a different matter).

I've booted debian with only 64 signals and an appropriate printk in
do_sigaction, and although various things try and set all the handlers,
I didn't found anything that breaks because of them being missing.

I've also audited all the binaries installed on my desktop (Fedora 17)
which contain libc_current_sigrtmax to see if I can find anything
problematic. Various things iterate all the signals (which doesn't do
any harm if a bunch are missing, especially when EINVAL is often
expected for SIGRTMIN..SIGRTMIN+2 because of libc using them). High
level language bindings open up some potential for breakage since
SIGRTMAX etc can be exposed to higher levels. The only real problem I've
found though is openjdk:

http://hg.openjdk.java.net/jdk7/jsn/jdk/file/tip/src/solaris/native/java/net/linux_close.c
57 /*
58  * Signal to unblock thread
59  */
60 static int sigWakeup = (__SIGRTMAX - 2);

It sets up a signal handler when the library is loaded (without any
error checking), and tries to send the signal to all threads blocked on
a file descriptor to wake them up (again without error checking), called
from NET_Dup2 and NET_SocketClose. Clearly this could easily break
backwards compatibility if SIGRTMAX is reduced any lower than 126.

Obviously this isn't exhaustive (I haven't tried android etc or actually
running many applications, and open source software probably isn't the
best example of badly written code), but it looks like it may be safe to
reduce _NSIG to 127 for a stable fix if nobody objects, and possibly to
126 in the future to avoid the wait exit status problem.

Cheers
James

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/