login
Header Space

 
 

Re: [RFC, PATCH] signals: print_fatal_signal: fix the signr "calculation"

Previous thread: Lockdep problem by Dmitry on Saturday, March 8, 2008 - 1:10 pm. (3 messages)

Next thread: [PATCH] keyring: Incorrect permissions checking in __keyring_search_one() by Arun Raghavan on Saturday, March 8, 2008 - 1:22 pm. (2 messages)
To: Andrew Morton <akpm@...>, Arjan van de Ven <arjan@...>, Ingo Molnar <mingo@...>, Roland McGrath <roland@...>
Cc: <linux-kernel@...>
Date: Saturday, March 8, 2008 - 1:24 pm

Not sure this is needed, please review. Afaics, print_fatal_signal() is needed
to detect the early segfaults, and we don not transform the coredump signals
into the group-wide SIGKILL. With this change, for example, ^C will be reported.
I don't know whether this is good or bad. Most probably we don't need this, but
please clarify what is the desirable behaviour.

The task can dequeue SIGKILL while in fact it was killed by another fatal signal.
Consult -&gt;group_exit_code to figure out the right signr.

Signed-off-by: Oleg Nesterov &lt;oleg@tv-sign.ru&gt;

--- 25/kernel/signal.c~7_PFS_FIX	2008-03-08 17:38:50.000000000 +0300
+++ 25/kernel/signal.c	2008-03-08 19:15:00.000000000 +0300
@@ -834,6 +834,13 @@ int print_fatal_signals;
 
 static void print_fatal_signal(struct pt_regs *regs, int signr)
 {
+	if (signr == SIGKILL) {
+		if (current-&gt;signal-&gt;flags &amp; SIGNAL_GROUP_EXIT)
+			signr = current-&gt;signal-&gt;group_exit_code &amp; 0x7f;
+		if (!signr || signr == SIGKILL)
+			return;
+	}
+
 	printk("%s/%d: potentially unexpected fatal signal %d.\n",
 		current-&gt;comm, task_pid_nr(current), signr);
 
@@ -855,7 +862,7 @@ static void print_fatal_signal(struct pt
 
 static int __init setup_print_fatal_signals(char *str)
 {
-	get_option (&amp;str, &amp;print_fatal_signals);
+	get_option(&amp;str, &amp;print_fatal_signals);
 
 	return 1;
 }
@@ -1768,7 +1775,7 @@ relock:
 		 * Anything else is fatal, maybe with a core dump.
 		 */
 		current-&gt;flags |= PF_SIGNALED;
-		if ((signr != SIGKILL) &amp;&amp; print_fatal_signals)
+		if (print_fatal_signals)
 			print_fatal_signal(regs, signr);
 		if (sig_kernel_coredump(signr)) {
 			/*

--
To: Oleg Nesterov <oleg@...>
Cc: Andrew Morton <akpm@...>, Arjan van de Ven <arjan@...>, Ingo Molnar <mingo@...>, <linux-kernel@...>
Date: Saturday, March 8, 2008 - 3:03 pm

The intent of your change is to get the printk for each thread, right?
I don't really see the point.  The thread that actually had the fault will
dequeue a non-SIGKILL signal and report its status.  We only need one
thread per signal to the print-out.

Hmm.  I see that non-coredump signals that hit the optimized fatal case in
__group_complete_signal will cause every thread to have a pending SIGKILL.
So that will be seen first and prevent the print-out.  So that's what you
intend to change?

I'm not sure print-fatal-signals was really ever intended for non-coredump
signals.  It doesn't seem like it would be all that useful.  It's probably
even undesireable for every normal C-c killing something to cause a printk.


Thanks,
Roland
--
To: Roland McGrath <roland@...>
Cc: Oleg Nesterov <oleg@...>, Andrew Morton <akpm@...>, Arjan van de Ven <arjan@...>, <linux-kernel@...>
Date: Sunday, March 9, 2008 - 7:05 am

correct. We used to have them for SIGKILL but even that was confusing to 
users - so the intent very much is to only have them for truly 
unexpected, non-user generated and 'fatal', coredump-generating signals.

	Ingo
--
To: Ingo Molnar <mingo@...>
Cc: Roland McGrath <roland@...>, Andrew Morton <akpm@...>, Arjan van de Ven <arjan@...>, <linux-kernel@...>
Date: Sunday, March 9, 2008 - 12:11 pm

Yes, I see now, thanks.

Let me clarify why I started this thread. I'm thinking how we can "improve"
fatal signals. Let's look at this patch

	http://marc.info/?l=linux-kernel&amp;m=120498888702466

(under discussion, let's suppose it will be accepted). In short: with this
patch the thread-specific SIGKILL shutdowns the whole thread group early on
signal delivery, the same way like the group-wide SIGKILL does.

For various reasons we can't currently do the same for sig_kernel_coredump()
signals. But, when rlim[RLIMIT_CORE] == 0, we don't actually need coredumping?
So, we could do something like


	-	if (!sig_kernel_coredump(sig)) {
	+	if (!sig_kernel_coredump(sig) || !signal-&gt;rlim[RLIMIT_CORE]) {

			// shutdown the whole group,
			// send SIGKILL to each thread

to speedup the processing of fatal coredump signals (to clarify: this issue
is minor, just for example. and the change above is not exactly right).

However, this breaks print_fatal_signal(), because with this change nobody
will dequeue the "right" signal to report, it was transformed to the "global"
SIGKILL.

So, if we change the behaviour of thread-specific coredump signals, then
we should "fix" print_fatal_signal(). At least, now I know which signals
should be reported.

Thanks!

Oleg.

--
To: Oleg Nesterov <oleg@...>
Cc: Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Arjan van de Ven <arjan@...>, <linux-kernel@...>
Date: Monday, March 10, 2008 - 10:19 pm

I don't like this.  I think it's better to leave logic like RLIMIT_CORE
checks to the core dump code itself.


Thanks,
Roland
--
To: Roland McGrath <roland@...>
Cc: Ingo Molnar <mingo@...>, Andrew Morton <akpm@...>, Arjan van de Ven <arjan@...>, <linux-kernel@...>
Date: Tuesday, March 11, 2008 - 2:22 pm

As I said, this was just for illustration.

What I am thinking about is how to restore the "sig_kernel_coredump() signal
freezes the whole group on delivery" behaviour, we had this before
198466b41d11dd062fb26ee0376080458d7bfcaf.

Afaics this is possible, we could add another SIGNAL_XXX flag but send SIGKILL,
this also unifies the fatal signals processing.

Oleg.

--
To: Roland McGrath <roland@...>
Cc: Andrew Morton <akpm@...>, Arjan van de Ven <arjan@...>, Ingo Molnar <mingo@...>, <linux-kernel@...>
Date: Saturday, March 8, 2008 - 4:30 pm

Great, thanks! Please ignore this patch then. I was afraid something was
missed here.

Oleg.

--
Previous thread: Lockdep problem by Dmitry on Saturday, March 8, 2008 - 1:10 pm. (3 messages)

Next thread: [PATCH] keyring: Incorrect permissions checking in __keyring_search_one() by Arun Raghavan on Saturday, March 8, 2008 - 1:22 pm. (2 messages)
speck-geostationary