Re: [PATCH v2] MIPS: Reduce _NSIG from 128 to 127 to avoid BUG_ON
On Fri, Jun 28, 2013 at 11:03:33PM +0100, James Hogan wrote: > On 28 June 2013 20:28, Denys Vlasenko wrote: > > On Monday 17 June 2013 12:36, James Hogan wrote: > >> On 14/06/13 17:03, James Hogan wrote: > >> > MIPS has 128 signals, the highest of which has the number 128 (they > >> > start from 1). The following command causes get_signal_to_deliver() to > >> > pass this signal number straight through to do_group_exit() as the exit > >> > code: > >> > > >> > strace sleep 10 & sleep 1 && kill -128 `pidof sleep` > >> > > >> > However do_group_exit() checks for the core dump bit (0x80) in the exit > >> > code which matches in this particular case and the kernel panics: > >> > > >> > BUG_ON(exit_code & 0x80); /* core dumps don't get here */ > >> > > >> > Lets avoid this by changing the ABI by reducing the number of signals to > >> > 127 (so that the maximum signal number is 127). Glibc incorrectly sets > >> > [__]SIGRTMAX to 127 already. uClibc sets it to 128 so it's conceivable > >> > that programs built against uClibc which intentionally uses RT signals > >> > from the top (SIGRTMAX-n, n>=0) would need an updated uClibc (and a > >> > rebuild if it's crazy enough to use __SIGRTMAX). > >> > >> Hmm, although this works around the BUG_ON, this doesn't actually seem > >> to be sufficient to behave correctly. > >> > >> So it appears the exit status is constructed like this: > >> bits purpose > >> 0x007fsignal number (0-127) > >> 0x0080core dump > >> 0xff00exit status > >> > >> but the macros in waitstatus.h and wait.h in libc > >> (see also "man 2 wait"): > >> WIFEXITED: status & 0x7f == 0 > >> WIFSIGNALED: status & 0x7f in [1..126] (i.e. not 0 or 127) > >> WIFSTOPPED: status & 0xff == 127 > >> > >> So termination due to SIG127 looks like it's been stopped instead of > >> terminated via a signal, unless a core dump occurs in which case none of > >> the above match. > >> > >> (And termination due to SIG128 hits BUG_ON, otherwise would appear to > >> have exited normally with core dump). > >> > >> > >> Reducing number of signals to 126 to avoid this will change the glibc > >> ABI too, in which case we may as well reduce to 64 to match other > >> arches, which is more likely to break something (I'm not really > >> comfortable making that change). > >> > >> Reducing to 127 (this patch) still leaves incorrect exit status codes > >> for SIG127 ... > >> > >> Any further thoughts/opinions? > > > > Strictly speaking, exit status of 0x007f isn't ambiguous. > > > > Currently userspace uses the following rules > > (assuming that status is 16-bit (IOW, dropping PTRACE_EVENT bits)): > > > > WIFEXITED(status)= (status & 0x7f) == 0 > > WIFSIGNALED(status) = (status & 0x7f) != 0 && (status & 0x7f) < 0x7f > > WIFSTOPPED(status) = (status & 0xff) == 0x7f > > WIFCONTINUED(status) = (status == 0x) > > > > WEXITSTATUS(status) = status >> 8 > > WSTOPSIG(status) = status >> 8 > > WCOREDUMP(status)= status & 0x80 > > WTERMSIG(status) = status & 0x7f > > > > When process dies from signal 127, status is 0x007f and it is not a valid > > "stopped by signal" indicator, since WSTOPSIG == 0 is an impossibility. > > > > Status 0x007f get misinterpreted by the rules above, namely, > > WIFSTOPPED is true, WIFSIGNALED is false. > > > > But an alternative definition exists which works correctly with > > all previous status codes, treats 0x007f as "killed by signal 127" > > and isn't more convoluted. > > In fact, while WIFSTOPPED needs one additional check, > > WIFSIGNALED gets simpler (loses one AND'ing operation): > > > > WIFSTOPPED(status) = (status & 0xff) == 0x7f && (status >> 8) != 0 > > WIFSIGNALED(status) = status != 0 && status <= 0xff > > > > All other rules need no change. > > > > I think it's feasible to ask {g,uc}libc to change their defines > > (on MIPS as a minimum), and live with 127 signals. > > Thanks for the explanation. This makes a lot of sense and if I > understand correctly it already describes the current behaviour of the > kernel up to SIG127 (I hadn't twigged WIFSTOPPED should imply > WSTOPSIG!=0 for some reason). I like it. One other note on this issue: SIG128 also aliases CLONE_VM, and it would be very bad if a program requesting SIG128 as its exit signal when calling clone instead ended up with the effects of CLONE_VM... Also, I have some improved macros for WIFSTOPPED and WIFSIGNALED which avoid multiple evaluation of their arguments: #define WIFSTOPPED(s) ((short)s)&0x)*0x10001)>>8) > 0x7f00) #define WIFSIGNALED(s) (((s)&0x)-1 < 0xffu) These are what we are using in musl libc now. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] MIPS: Reduce _NSIG from 128 to 127 to avoid BUG_ON
On 28 June 2013 20:28, Denys Vlasenko wrote: > On Monday 17 June 2013 12:36, James Hogan wrote: >> On 14/06/13 17:03, James Hogan wrote: >> > MIPS has 128 signals, the highest of which has the number 128 (they >> > start from 1). The following command causes get_signal_to_deliver() to >> > pass this signal number straight through to do_group_exit() as the exit >> > code: >> > >> > strace sleep 10 & sleep 1 && kill -128 `pidof sleep` >> > >> > However do_group_exit() checks for the core dump bit (0x80) in the exit >> > code which matches in this particular case and the kernel panics: >> > >> > BUG_ON(exit_code & 0x80); /* core dumps don't get here */ >> > >> > Lets avoid this by changing the ABI by reducing the number of signals to >> > 127 (so that the maximum signal number is 127). Glibc incorrectly sets >> > [__]SIGRTMAX to 127 already. uClibc sets it to 128 so it's conceivable >> > that programs built against uClibc which intentionally uses RT signals >> > from the top (SIGRTMAX-n, n>=0) would need an updated uClibc (and a >> > rebuild if it's crazy enough to use __SIGRTMAX). >> >> Hmm, although this works around the BUG_ON, this doesn't actually seem >> to be sufficient to behave correctly. >> >> So it appears the exit status is constructed like this: >> bits purpose >> 0x007fsignal number (0-127) >> 0x0080core dump >> 0xff00exit status >> >> but the macros in waitstatus.h and wait.h in libc >> (see also "man 2 wait"): >> WIFEXITED: status & 0x7f == 0 >> WIFSIGNALED: status & 0x7f in [1..126] (i.e. not 0 or 127) >> WIFSTOPPED: status & 0xff == 127 >> >> So termination due to SIG127 looks like it's been stopped instead of >> terminated via a signal, unless a core dump occurs in which case none of >> the above match. >> >> (And termination due to SIG128 hits BUG_ON, otherwise would appear to >> have exited normally with core dump). >> >> >> Reducing number of signals to 126 to avoid this will change the glibc >> ABI too, in which case we may as well reduce to 64 to match other >> arches, which is more likely to break something (I'm not really >> comfortable making that change). >> >> Reducing to 127 (this patch) still leaves incorrect exit status codes >> for SIG127 ... >> >> Any further thoughts/opinions? > > Strictly speaking, exit status of 0x007f isn't ambiguous. > > Currently userspace uses the following rules > (assuming that status is 16-bit (IOW, dropping PTRACE_EVENT bits)): > > WIFEXITED(status)= (status & 0x7f) == 0 > WIFSIGNALED(status) = (status & 0x7f) != 0 && (status & 0x7f) < 0x7f > WIFSTOPPED(status) = (status & 0xff) == 0x7f > WIFCONTINUED(status) = (status == 0x) > > WEXITSTATUS(status) = status >> 8 > WSTOPSIG(status) = status >> 8 > WCOREDUMP(status)= status & 0x80 > WTERMSIG(status) = status & 0x7f > > When process dies from signal 127, status is 0x007f and it is not a valid > "stopped by signal" indicator, since WSTOPSIG == 0 is an impossibility. > > Status 0x007f get misinterpreted by the rules above, namely, > WIFSTOPPED is true, WIFSIGNALED is false. > > But an alternative definition exists which works correctly with > all previous status codes, treats 0x007f as "killed by signal 127" > and isn't more convoluted. > In fact, while WIFSTOPPED needs one additional check, > WIFSIGNALED gets simpler (loses one AND'ing operation): > > WIFSTOPPED(status) = (status & 0xff) == 0x7f && (status >> 8) != 0 > WIFSIGNALED(status) = status != 0 && status <= 0xff > > All other rules need no change. > > I think it's feasible to ask {g,uc}libc to change their defines > (on MIPS as a minimum), and live with 127 signals. Thanks for the explanation. This makes a lot of sense and if I understand correctly it already describes the current behaviour of the kernel up to SIG127 (I hadn't twigged WIFSTOPPED should imply WSTOPSIG!=0 for some reason). I like it. Cheers James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] MIPS: Reduce _NSIG from 128 to 127 to avoid BUG_ON
On Monday 17 June 2013 12:36, James Hogan wrote: > On 14/06/13 17:03, James Hogan wrote: > > MIPS has 128 signals, the highest of which has the number 128 (they > > start from 1). The following command causes get_signal_to_deliver() to > > pass this signal number straight through to do_group_exit() as the exit > > code: > > > > strace sleep 10 & sleep 1 && kill -128 `pidof sleep` > > > > However do_group_exit() checks for the core dump bit (0x80) in the exit > > code which matches in this particular case and the kernel panics: > > > > BUG_ON(exit_code & 0x80); /* core dumps don't get here */ > > > > Lets avoid this by changing the ABI by reducing the number of signals to > > 127 (so that the maximum signal number is 127). Glibc incorrectly sets > > [__]SIGRTMAX to 127 already. uClibc sets it to 128 so it's conceivable > > that programs built against uClibc which intentionally uses RT signals > > from the top (SIGRTMAX-n, n>=0) would need an updated uClibc (and a > > rebuild if it's crazy enough to use __SIGRTMAX). > > Hmm, although this works around the BUG_ON, this doesn't actually seem > to be sufficient to behave correctly. > > So it appears the exit status is constructed like this: > bits purpose > 0x007fsignal number (0-127) > 0x0080core dump > 0xff00exit status > > but the macros in waitstatus.h and wait.h in libc > (see also "man 2 wait"): > WIFEXITED: status & 0x7f == 0 > WIFSIGNALED: status & 0x7f in [1..126] (i.e. not 0 or 127) > WIFSTOPPED: status & 0xff == 127 > > So termination due to SIG127 looks like it's been stopped instead of > terminated via a signal, unless a core dump occurs in which case none of > the above match. > > (And termination due to SIG128 hits BUG_ON, otherwise would appear to > have exited normally with core dump). > > > Reducing number of signals to 126 to avoid this will change the glibc > ABI too, in which case we may as well reduce to 64 to match other > arches, which is more likely to break something (I'm not really > comfortable making that change). > > Reducing to 127 (this patch) still leaves incorrect exit status codes > for SIG127 ... > > Any further thoughts/opinions? Strictly speaking, exit status of 0x007f isn't ambiguous. Currently userspace uses the following rules (assuming that status is 16-bit (IOW, dropping PTRACE_EVENT bits)): WIFEXITED(status)= (status & 0x7f) == 0 WIFSIGNALED(status) = (status & 0x7f) != 0 && (status & 0x7f) < 0x7f WIFSTOPPED(status) = (status & 0xff) == 0x7f WIFCONTINUED(status) = (status == 0x) WEXITSTATUS(status) = status >> 8 WSTOPSIG(status) = status >> 8 WCOREDUMP(status)= status & 0x80 WTERMSIG(status) = status & 0x7f When process dies from signal 127, status is 0x007f and it is not a valid "stopped by signal" indicator, since WSTOPSIG == 0 is an impossibility. Status 0x007f get misinterpreted by the rules above, namely, WIFSTOPPED is true, WIFSIGNALED is false. But an alternative definition exists which works correctly with all previous status codes, treats 0x007f as "killed by signal 127" and isn't more convoluted. In fact, while WIFSTOPPED needs one additional check, WIFSIGNALED gets simpler (loses one AND'ing operation): WIFSTOPPED(status) = (status & 0xff) == 0x7f && (status >> 8) != 0 WIFSIGNALED(status) = status != 0 && status <= 0xff All other rules need no change. I think it's feasible to ask {g,uc}libc to change their defines (on MIPS as a minimum), and live with 127 signals. -- vda -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] MIPS: Reduce _NSIG from 128 to 127 to avoid BUG_ON
On 14/06/13 17:03, James Hogan wrote: > MIPS has 128 signals, the highest of which has the number 128 (they > start from 1). The following command causes get_signal_to_deliver() to > pass this signal number straight through to do_group_exit() as the exit > code: > > strace sleep 10 & sleep 1 && kill -128 `pidof sleep` > > However do_group_exit() checks for the core dump bit (0x80) in the exit > code which matches in this particular case and the kernel panics: > > BUG_ON(exit_code & 0x80); /* core dumps don't get here */ > > Lets avoid this by changing the ABI by reducing the number of signals to > 127 (so that the maximum signal number is 127). Glibc incorrectly sets > [__]SIGRTMAX to 127 already. uClibc sets it to 128 so it's conceivable > that programs built against uClibc which intentionally uses RT signals > from the top (SIGRTMAX-n, n>=0) would need an updated uClibc (and a > rebuild if it's crazy enough to use __SIGRTMAX). Hmm, although this works around the BUG_ON, this doesn't actually seem to be sufficient to behave correctly. So it appears the exit status is constructed like this: bitspurpose 0x007f signal number (0-127) 0x0080 core dump 0xff00 exit status but the macros in waitstatus.h and wait.h in libc (see also "man 2 wait"): WIFEXITED: status & 0x7f == 0 WIFSIGNALED: status & 0x7f in [1..126] (i.e. not 0 or 127) WIFSTOPPED: status & 0xff == 127 So termination due to SIG127 looks like it's been stopped instead of terminated via a signal, unless a core dump occurs in which case none of the above match. (And termination due to SIG128 hits BUG_ON, otherwise would appear to have exited normally with core dump). Reducing number of signals to 126 to avoid this will change the glibc ABI too, in which case we may as well reduce to 64 to match other arches, which is more likely to break something (I'm not really comfortable making that change). Reducing to 127 (this patch) still leaves incorrect exit status codes for SIG127, in which case we may as well leave it at 128, workaround the BUG_ON and just accept that exit codes may refer to the wrong signal number in the "terminated by SIG127 or SIG128" cases (something like the first patch I sent, but with maximum reduced to 126). It would probably be sensible to then reduce number of signals hardcoded in the C libraries to avoid these problematic signals (which wouldn't be an ABI break). Any further thoughts/opinions? Cheers James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] MIPS: Reduce _NSIG from 128 to 127 to avoid BUG_ON
On 06/14/2013 09:03 AM, James Hogan wrote: MIPS has 128 signals, the highest of which has the number 128 (they start from 1). The following command causes get_signal_to_deliver() to pass this signal number straight through to do_group_exit() as the exit code: strace sleep 10 & sleep 1 && kill -128 `pidof sleep` However do_group_exit() checks for the core dump bit (0x80) in the exit code which matches in this particular case and the kernel panics: BUG_ON(exit_code & 0x80); /* core dumps don't get here */ Lets avoid this by changing the ABI by reducing the number of signals to 127 (so that the maximum signal number is 127). Glibc incorrectly sets [__]SIGRTMAX to 127 already. uClibc sets it to 128 so it's conceivable that programs built against uClibc which intentionally uses RT signals from the top (SIGRTMAX-n, n>=0) would need an updated uClibc (and a rebuild if it's crazy enough to use __SIGRTMAX). Note that the signals man page seems to make clear that signals should be referred to from SIGRTMIN, and it seems unlikely that any portable program would ever need to use 96 RT signals: "programs should never refer to real-time signals using hard-coded numbers, but instead should always refer to real-time signals using the notation SIGRTMIN+n, and include suitable (run-time) checks that SIGRTMIN+n does not exceed SIGRTMAX." As previously discussed, I think this is the way to go, Acked-by: David Daney Signed-off-by: James Hogan Cc: Ralf Baechle Cc: David Daney Cc: Oleg Nesterov Cc: Al Viro Cc: "Paul E. McKenney" Cc: David Howells Cc: Dave Jones Cc: linux-m...@linux-mips.org --- As discussed on IRC, another possibility is to reduce the number of signals down to 64 to match other arches and reduce the number of sigset_t words, but I think that's riskier as it would affect glibc too. arch/mips/include/uapi/asm/signal.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/mips/include/uapi/asm/signal.h b/arch/mips/include/uapi/asm/signal.h index addb9f5..40e944d 100644 --- a/arch/mips/include/uapi/asm/signal.h +++ b/arch/mips/include/uapi/asm/signal.h @@ -11,9 +11,9 @@ #include -#define _NSIG 128 +#define _NSIG 127 #define _NSIG_BPW (sizeof(unsigned long) * 8) -#define _NSIG_WORDS(_NSIG / _NSIG_BPW) +#define _NSIG_WORDS((_NSIG + _NSIG_BPW - 1) / _NSIG_BPW) typedef struct { unsigned long sig[_NSIG_WORDS]; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] MIPS: Reduce _NSIG from 128 to 127 to avoid BUG_ON
On 06/14, James Hogan wrote: > > However do_group_exit() checks for the core dump bit (0x80) in the exit > code which matches in this particular case and the kernel panics: > > BUG_ON(exit_code & 0x80); /* core dumps don't get here */ > > Lets avoid this by changing the ABI by reducing the number of signals to > 127 (so that the maximum signal number is 127). Agreed. Of course I can't ack the change in arch/mips, but to me this looks like a best solution. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] MIPS: Reduce _NSIG from 128 to 127 to avoid BUG_ON
MIPS has 128 signals, the highest of which has the number 128 (they start from 1). The following command causes get_signal_to_deliver() to pass this signal number straight through to do_group_exit() as the exit code: strace sleep 10 & sleep 1 && kill -128 `pidof sleep` However do_group_exit() checks for the core dump bit (0x80) in the exit code which matches in this particular case and the kernel panics: BUG_ON(exit_code & 0x80); /* core dumps don't get here */ Lets avoid this by changing the ABI by reducing the number of signals to 127 (so that the maximum signal number is 127). Glibc incorrectly sets [__]SIGRTMAX to 127 already. uClibc sets it to 128 so it's conceivable that programs built against uClibc which intentionally uses RT signals from the top (SIGRTMAX-n, n>=0) would need an updated uClibc (and a rebuild if it's crazy enough to use __SIGRTMAX). Note that the signals man page seems to make clear that signals should be referred to from SIGRTMIN, and it seems unlikely that any portable program would ever need to use 96 RT signals: "programs should never refer to real-time signals using hard-coded numbers, but instead should always refer to real-time signals using the notation SIGRTMIN+n, and include suitable (run-time) checks that SIGRTMIN+n does not exceed SIGRTMAX." Signed-off-by: James Hogan Cc: Ralf Baechle Cc: David Daney Cc: Oleg Nesterov Cc: Al Viro Cc: "Paul E. McKenney" Cc: David Howells Cc: Dave Jones Cc: linux-m...@linux-mips.org --- As discussed on IRC, another possibility is to reduce the number of signals down to 64 to match other arches and reduce the number of sigset_t words, but I think that's riskier as it would affect glibc too. arch/mips/include/uapi/asm/signal.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/mips/include/uapi/asm/signal.h b/arch/mips/include/uapi/asm/signal.h index addb9f5..40e944d 100644 --- a/arch/mips/include/uapi/asm/signal.h +++ b/arch/mips/include/uapi/asm/signal.h @@ -11,9 +11,9 @@ #include -#define _NSIG 128 +#define _NSIG 127 #define _NSIG_BPW (sizeof(unsigned long) * 8) -#define _NSIG_WORDS(_NSIG / _NSIG_BPW) +#define _NSIG_WORDS((_NSIG + _NSIG_BPW - 1) / _NSIG_BPW) typedef struct { unsigned long sig[_NSIG_WORDS]; -- 1.8.1.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/