[PATCH 1/5] vfork: don't freezer_count() for in-kernel users of CLONE_VFORK

[PATCH 1/5] vfork: don't freezer_count() for in-kernel users of CLONE_VFORK

[Mandeep Singh Baines]

We don't need to call freezer_do_not_count() for in-kernel users
of CLONE_VFORK since exec will get called in bounded time.

We don't want to call freezer_count() for in-kernel users because
they may be holding locks. freezer_count() calls try_to_freeze().
We don't want to freeze an in-kernel user because it may be
holding locks.

In a follow-up patch, I call debug_check_no_locks_held() from
try_to_freeze(). After applying this patch, I get no lockdep
warnings with that patch.

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Tejun Heo <tj@kernel.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Ingo Molnar <mingo@redhat.com>
---
 kernel/fork.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index c535f33..a7cd973 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -722,9 +722,11 @@ static int wait_for_vfork_done(struct task_struct *child,
 {
 	int killed;
 
-	freezer_do_not_count();
+	if (current->mm)
+		freezer_do_not_count();
 	killed = wait_for_completion_killable(vfork);
-	freezer_count();
+	if (current->mm)
+		freezer_count();
 
 	if (killed) {
 		task_lock(child);
-- 
1.8.1

[PATCH 2/5] lockdep: check that no locks held at freeze time

[Mandeep Singh Baines]

We shouldn't try_to_freeze if locks are held. Verified that
I get no lockdep warnings after applying this patch and
"vfork: don't freezer_count() for in-kernel users of CLONE_VFORK".

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Tejun Heo <tj@kernel.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Ingo Molnar <mingo@redhat.com>
---
 include/linux/freezer.h | 2 ++
 kernel/lockdep.c        | 6 +++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index e4238ce..1538cfc 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -3,6 +3,7 @@
 #ifndef FREEZER_H_INCLUDED
 #define FREEZER_H_INCLUDED
 
+#include <linux/debug_locks.h>
 #include <linux/sched.h>
 #include <linux/wait.h>
 #include <linux/atomic.h>
@@ -43,6 +44,7 @@ extern void thaw_kernel_threads(void);
 
 static inline bool try_to_freeze(void)
 {
+	debug_check_no_locks_held(current);
 	might_sleep();
 	if (likely(!freezing(current)))
 		return false;
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 7981e5b..e3ee8af 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -4091,10 +4091,10 @@ static void print_held_locks_bug(struct task_struct *curr)
 		return;
 
 	printk("\n");
-	printk("=====================================\n");
-	printk("[ BUG: lock held at task exit time! ]\n");
+	printk("=======================================\n");
+	printk("[ BUG: lock held at exit/freeze time! ]\n");
 	print_kernel_ident();
-	printk("-------------------------------------\n");
+	printk("---------------------------------------\n");
 	printk("%s/%d is exiting with locks still held!\n",
 		curr->comm, task_pid_nr(curr));
 	lockdep_print_held_locks(curr);
-- 
1.8.1

[PATCH 3/5] coredump: cleanup the waiting for coredump_finish code

[Mandeep Singh Baines]

Replace the for loop with a simple if.

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Tejun Heo <tj@kernel.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Ingo Molnar <mingo@redhat.com>
---
 kernel/exit.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index b4df219..f215198 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -479,12 +479,9 @@ static void exit_mm(struct task_struct * tsk)
 		if (atomic_dec_and_test(&core_state->nr_threads))
 			complete(&core_state->startup);
 
-		for (;;) {
-			set_task_state(tsk, TASK_UNINTERRUPTIBLE);
-			if (!self.task) /* see coredump_finish() */
-				break;
+		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+		if (self.task) /* see coredump_finish() */
 			schedule();
-		}
 		__set_task_state(tsk, TASK_RUNNING);
 		down_read(&mm->mmap_sem);
 	}
-- 
1.8.1

[PATCH 4/5] coredump: use a freezable_schedule for the coredump_finish wait

[Mandeep Singh Baines]

Prevents hung_task detector from panicing the machine. This is also
needed to prevent this wait from blocking suspend.

(It doesnt' currently block suspend but it would once the next
patch in this series is applied.)

Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Tejun Heo <tj@kernel.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Ingo Molnar <mingo@redhat.com>
---
 kernel/exit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index f215198..04d9db0 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -20,6 +20,7 @@
 #include <linux/tsacct_kern.h>
 #include <linux/file.h>
 #include <linux/fdtable.h>
+#include <linux/freezer.h>
 #include <linux/binfmts.h>
 #include <linux/nsproxy.h>
 #include <linux/pid_namespace.h>
@@ -481,7 +482,7 @@ static void exit_mm(struct task_struct * tsk)
 
 		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
 		if (self.task) /* see coredump_finish() */
-			schedule();
+			freezable_schedule();
 		__set_task_state(tsk, TASK_RUNNING);
 		down_read(&mm->mmap_sem);
 	}
-- 
1.8.1

[PATCH 5/5] coredump: abort core dump piping only due to a fatal signal

[Mandeep Singh Baines]

From: Ben Chan <benchan@chromium.org>

This patch makes wait_for_dump_helpers() not to abort piping the core
dump data when the crashing process has received any but a fatal signal
(SIGKILL). The rationale is that a crashing process may still receive
uninteresting signals such as SIGCHLD when its core dump data is being
redirected to a helper application. While it's necessary to allow
terminating the core dump piping via SIGKILL, it's practically more
useful for the purpose of debugging and crash reporting if the core dump
piping is not aborted due to other non-fatal signals.

Signed-off-by: Ben Chan <benchan@chromium.org>
Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Tejun Heo <tj@kernel.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Ingo Molnar <mingo@redhat.com>
---
 fs/coredump.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 1774932..54e31a3 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -32,6 +32,7 @@
 #include <linux/pipe_fs_i.h>
 #include <linux/oom.h>
 #include <linux/compat.h>
+#include <linux/freezer.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -417,10 +418,13 @@ static void wait_for_dump_helpers(struct file *file)
 	pipe->readers++;
 	pipe->writers--;
 
-	while ((pipe->readers > 1) && (!signal_pending(current))) {
+	while ((pipe->readers > 1) && (!fatal_signal_pending(current))) {
 		wake_up_interruptible_sync(&pipe->wait);
 		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
 		pipe_wait(pipe);
+		pipe_unlock(pipe);
+		try_to_freeze();
+		pipe_lock(pipe);
 	}
 
 	pipe->readers--;
-- 
1.8.1

Re: [PATCH 2/5] lockdep: check that no locks held at freeze time

[Ingo Molnar]

* Mandeep Singh Baines <msb@chromium.org> wrote:

> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -4091,10 +4091,10 @@ static void print_held_locks_bug(struct task_struct *curr)
>  		return;
>  
>  	printk("\n");
> -	printk("=====================================\n");
> -	printk("[ BUG: lock held at task exit time! ]\n");
> +	printk("=======================================\n");
> +	printk("[ BUG: lock held at exit/freeze time! ]\n");

Looks good, but to make it more apparent which case triggers, 
please pass in a 'msg' string that gets printed - and the msg is 
either "exit" or "freeze", depending on from where it's called.

Thanks,

	Ingo

Re: [PATCH 3/5] coredump: cleanup the waiting for coredump_finish code

[Oleg Nesterov]

On 02/14, Mandeep Singh Baines wrote:
>
> Replace the for loop with a simple if.

Why?

> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -479,12 +479,9 @@ static void exit_mm(struct task_struct * tsk)
>  		if (atomic_dec_and_test(&core_state->nr_threads))
>  			complete(&core_state->startup);
>
> -		for (;;) {
> -			set_task_state(tsk, TASK_UNINTERRUPTIBLE);
> -			if (!self.task) /* see coredump_finish() */
> -				break;
> +		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
> +		if (self.task) /* see coredump_finish() */
>  			schedule();
> -		}

If you think we should not worry about spurious wakeups you can simplify
this code even more, you do not need mb() to set TASK_UNINTERRUPTIBLE
(just move it before dec_and_test), you do not need "if (self.task)",
you do not need __set_task_state(tsk, TASK_RUNNING).

But I think we should always worry, so why?

Oleg.

Re: [PATCH 5/5] coredump: abort core dump piping only due to a fatal signal

[Oleg Nesterov]

On 02/14, Mandeep Singh Baines wrote:
>
> This patch makes wait_for_dump_helpers() not to abort piping the core
> dump data when the crashing process has received any but a fatal signal
> (SIGKILL). The rationale is that a crashing process may still receive
> uninteresting signals such as SIGCHLD when its core dump data is being
> redirected to a helper application.

You already sent this change in the past ;) and I already reviewed it.

It is not enough and imho not good. Damn, I'll try very much to make the
patches on weekend...

> -	while ((pipe->readers > 1) && (!signal_pending(current))) {
> +	while ((pipe->readers > 1) && (!fatal_signal_pending(current))) {

This turns pipe_wait() belowe into the busy-wait loop if signal_pending().
Not good. And not enough, there are other reasons why coredump can fail
if the signal is pending.

>  		wake_up_interruptible_sync(&pipe->wait);
>  		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
>  		pipe_wait(pipe);
> +		pipe_unlock(pipe);
> +		try_to_freeze();

Oh, yes. One of the problems with coredump/signals is freezer. Not sure
what should we do...

But if we add try_to_freeze() here, we need to add more try_to_freeze's,
think about dumping the huge core on the slow media.

Oleg.

Re: [PATCH 1/5] vfork: don't freezer_count() for in-kernel users of CLONE_VFORK

[Oleg Nesterov]

On 02/14, Mandeep Singh Baines wrote:
>
> We don't need to call freezer_do_not_count() for in-kernel users
> of CLONE_VFORK since exec will get called in bounded time.

OK,

> We don't want to call freezer_count() for in-kernel users because
> they may be holding locks.

Who? We should not do this anyway. And __call_usermodehelper() doesn't
afaics.

OK, its caller (process_one_work) does lock_map_acquire() for debugging
purposes, this can "confuse" print_held_locks_bug(). But this thread is
PF_NOFREEZE ?

> @@ -722,9 +722,11 @@ static int wait_for_vfork_done(struct task_struct *child,
>  {
>  	int killed;
>
> -	freezer_do_not_count();
> +	if (current->mm)
> +		freezer_do_not_count();

And if you want to exclude in-kernel users, then perhaps PF_KTHREAD check
will look a bit better.

Oleg.

Re: [PATCH 2/5] lockdep: check that no locks held at freeze time

[Oleg Nesterov]

On 02/14, Mandeep Singh Baines wrote:
>
> We shouldn't try_to_freeze if locks are held. Verified that
> I get no lockdep warnings after applying this patch and
> "vfork: don't freezer_count() for in-kernel users of CLONE_VFORK".

Ah. Now I understand why you did 1/5. Beacuse you call
debug_check_no_locks_held() right at the start of try_to_freeze(),
even if the caller can't be frozen.

>  static inline bool try_to_freeze(void)
>  {
> +	debug_check_no_locks_held(current);
>  	might_sleep();
>  	if (likely(!freezing(current)))
>  		return false;

Up to maintainers, but perhaps you should check !PF_NOFREEZE at least?

Oleg.

Re: [PATCH 5/5] coredump: abort core dump piping only due to a fatal signal

[Andrew Morton]

On Thu, 14 Feb 2013 15:38:16 -0800
Mandeep Singh Baines <msb@chromium.org> wrote:

> From: Ben Chan <benchan@chromium.org>
> 
> This patch makes wait_for_dump_helpers() not to abort piping the core
> dump data when the crashing process has received any but a fatal signal
> (SIGKILL). The rationale is that a crashing process may still receive
> uninteresting signals such as SIGCHLD when its core dump data is being
> redirected to a helper application. While it's necessary to allow
> terminating the core dump piping via SIGKILL, it's practically more
> useful for the purpose of debugging and crash reporting if the core dump
> piping is not aborted due to other non-fatal signals.

Sounds sensible.  The changelog is rather hard to understand.  Is the
below accurate?

: Make wait_for_dump_helpers() not abort piping the core dump data when the
: crashing process has received a non-fatal signal.  The abort still occurs
: in the case of SIGKILL.
: 
: The rationale is that a crashing process may still receive uninteresting
: signals such as SIGCHLD when its core dump data is being redirected to a
: helper application.  While it's necessary to allow terminating the core
: dump piping via SIGKILL, it's practically more useful for the purpose of
: debugging and crash reporting if the core dump piping is not aborted due
: to other non-fatal signals.

> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -32,6 +32,7 @@
>  #include <linux/pipe_fs_i.h>
>  #include <linux/oom.h>
>  #include <linux/compat.h>
> +#include <linux/freezer.h>
>  
>  #include <asm/uaccess.h>
>  #include <asm/mmu_context.h>
> @@ -417,10 +418,13 @@ static void wait_for_dump_helpers(struct file *file)
>  	pipe->readers++;
>  	pipe->writers--;
>  
> -	while ((pipe->readers > 1) && (!signal_pending(current))) {
> +	while ((pipe->readers > 1) && (!fatal_signal_pending(current))) {
>  		wake_up_interruptible_sync(&pipe->wait);
>  		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
>  		pipe_wait(pipe);
> +		pipe_unlock(pipe);
> +		try_to_freeze();
> +		pipe_lock(pipe);
>  	}

The new call to try_to_freeze() is unchangelogged and uncommented.  Can
we please fix both these things?

Re: [PATCH 3/5] coredump: cleanup the waiting for coredump_finish code

[Andrew Morton]

On Thu, 14 Feb 2013 15:38:14 -0800
Mandeep Singh Baines <msb@chromium.org> wrote:

> Replace the for loop with a simple if.

Well OK, but why?  Presumably the loop was added for a reason and
presumably you believe that reason to be (no longer?) correct.  Please
describe all these things.

> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -479,12 +479,9 @@ static void exit_mm(struct task_struct * tsk)
>  		if (atomic_dec_and_test(&core_state->nr_threads))
>  			complete(&core_state->startup);
>  
> -		for (;;) {
> -			set_task_state(tsk, TASK_UNINTERRUPTIBLE);
> -			if (!self.task) /* see coredump_finish() */
> -				break;
> +		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
> +		if (self.task) /* see coredump_finish() */
>  			schedule();
> -		}

[PATCH v3] coredump: ignore non-fatal signals when core dumping to a pipe

[Mandeep Singh Baines]

From: Ben Chan <benchan@chromium.org>

Make wait_for_dump_helpers() not abort piping the core dump data when the
crashing process has received a non-fatal signal.  The abort still occurs
in the case of SIGKILL.

Addresses http://crosbug.com/21559

Changes since v1:
* Mandeep Singh Baines
  * To prevent blocking suspend, add try_to_freeze().
Changes since v2:
* LKML: <20130215150117.GB30829@redhat.com> Oleg Nestorov
  * Block non-fatal signals to avoid poll_wait busy waiting.
* LKML: <20130215152538.9a61a44e.akpm@linux-foundation.org> Andrew Morton
  * Added comment re: try_to_freeze and clarified commit message.

Signed-off-by: Ben Chan <benchan@chromium.org>
Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Tejun Heo <tj@kernel.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Ingo Molnar <mingo@redhat.com>
---
 fs/coredump.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/coredump.c b/fs/coredump.c
index 1774932..976f6cc 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -32,6 +32,7 @@
 #include <linux/pipe_fs_i.h>
 #include <linux/oom.h>
 #include <linux/compat.h>
+#include <linux/freezer.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -410,6 +411,11 @@ static void coredump_finish(struct mm_struct *mm)
 static void wait_for_dump_helpers(struct file *file)
 {
 	struct pipe_inode_info *pipe;
+	sigset_t blocked, previous;
+
+	/* Block all but fatal signals. */
+	siginitsetinv(&blocked, sigmask(SIGKILL));
+	sigprocmask(SIG_BLOCK, &blocked, &previous);
 
 	pipe = file->f_path.dentry->d_inode->i_pipe;
 
@@ -421,12 +427,22 @@ static void wait_for_dump_helpers(struct file *file)
 		wake_up_interruptible_sync(&pipe->wait);
 		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
 		pipe_wait(pipe);
+
+		/*
+		 * Non-fatal signals are blocked. So we need to try
+		 * to freeze in order to not block suspend.
+		 */
+		pipe_unlock(pipe);
+		try_to_freeze();
+		pipe_lock(pipe);
 	}
 
 	pipe->readers--;
 	pipe->writers++;
 	pipe_unlock(pipe);
 
+	/* Restore signals. */
+	sigprocmask(SIG_SETMASK, &previous, NULL);
 }
 
 /*
-- 
1.7.12.4

[PATCH v4] coredump: ignore non-fatal signals when core dumping to a pipe

[Mandeep Singh Baines]

From: Ben Chan <benchan@chromium.org>

Make wait_for_dump_helpers() not abort piping the core dump data when the
crashing process has received a non-fatal signal.  The abort still occurs
in the case of SIGKILL.

Testing:

localhost ~ # echo "|/usr/bin/sleep 1d" > /proc/sys/kernel/core_pattern
localhost ~ # sleep 1d &
[1] 2514
localhost ~ # kill -ABRT $! # Cause coredump
localhost ~ # kill -USR1 $! # Send non-fatal signal
localhost ~ # top -p $! -n1 -b # Verify that we aren't dead or busy waiting
top - 16:45:34 up 2 min,  0 users,  load average: 0.71, 0.42, 0.17
Tasks:   1 total,   0 running,   1 sleeping,   0 stopped,   0 zombie
Cpu(s): 26.0%us,  8.5%sy,  0.0%ni, 65.1%id,  0.2%wa,  0.0%hi,  0.1%si,  0.0%st
Mem:    991516k total,   418556k used,   572960k free,     5948k buffers
Swap:        0k total,        0k used,        0k free,   289928k cached

  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
 2514 root      20   0  1868  392  336 S    0  0.0   0:00.00 sleep

localhost ~ # echo mem > /sys/power/state # Suspend
localhost ~ # top -p $! -n1 -b # Verify that we aren't dead or busy waiting
top - 16:46:46 up 3 min,  0 users,  load average: 1.68, 0.69, 0.28
Tasks:   1 total,   0 running,   1 sleeping,   0 stopped,   0 zombie
Cpu(s): 24.1%us,  7.7%sy,  0.0%ni, 67.9%id,  0.2%wa,  0.0%hi,  0.1%si,  0.0%st
Mem:    991516k total,   419956k used,   571560k free,     5996k buffers
Swap:        0k total,        0k used,        0k free,   290208k cached

  PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
 2514 root      20   0  1868  392  336 S    0  0.0   0:00.00 sleep

Addresses http://crosbug.com/21559

Changes since v1:
* Mandeep Singh Baines
  * To prevent blocking suspend, add try_to_freeze().
Changes since v2:
* LKML: <20130215150117.GB30829@redhat.com> Oleg Nestorov
  * Block non-fatal signals to avoid poll_wait busy waiting.
* LKML: <20130215152538.9a61a44e.akpm@linux-foundation.org> Andrew Morton
  * Added comment re: try_to_freeze and clarified commit message.
Changes since v3:
* Mandeep Singh Baines
  * Clear signal pending caused by fake signal from freeze_task().
  * Document how the patch was tested.

Signed-off-by: Ben Chan <benchan@chromium.org>
Signed-off-by: Mandeep Singh Baines <msb@chromium.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Tejun Heo <tj@kernel.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Rafael J. Wysocki <rjw@sisk.pl>
CC: Ingo Molnar <mingo@redhat.com>
---
 fs/coredump.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 1774932..3c5a08f 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -32,6 +32,7 @@
 #include <linux/pipe_fs_i.h>
 #include <linux/oom.h>
 #include <linux/compat.h>
+#include <linux/freezer.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -410,6 +411,11 @@ static void coredump_finish(struct mm_struct *mm)
 static void wait_for_dump_helpers(struct file *file)
 {
 	struct pipe_inode_info *pipe;
+	sigset_t blocked, previous;
+
+	/* Block all but fatal signals. */
+	siginitsetinv(&blocked, sigmask(SIGKILL));
+	sigprocmask(SIG_BLOCK, &blocked, &previous);
 
 	pipe = file->f_path.dentry->d_inode->i_pipe;
 
@@ -417,16 +423,29 @@ static void wait_for_dump_helpers(struct file *file)
 	pipe->readers++;
 	pipe->writers--;
 
-	while ((pipe->readers > 1) && (!signal_pending(current))) {
+	while ((pipe->readers > 1) && (!fatal_signal_pending(current))) {
 		wake_up_interruptible_sync(&pipe->wait);
 		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
 		pipe_wait(pipe);
+
+		/*
+		 * Non-fatal signals are blocked. So we need to try
+		 * to freeze in order to not block suspend.
+		 */
+		pipe_unlock(pipe);
+		try_to_freeze();
+		pipe_lock(pipe);
+
+		/* Clear fake signal from freeze_task. */
+		recalc_sigpending();
 	}
 
 	pipe->readers--;
 	pipe->writers++;
 	pipe_unlock(pipe);
 
+	/* Restore signals. */
+	sigprocmask(SIG_SETMASK, &previous, NULL);
 }
 
 /*
-- 
1.7.12.4

Re: [PATCH 5/5] coredump: abort core dump piping only due to a fatal signal

[Mandeep Singh Baines]

On Fri, Feb 15, 2013 at 7:01 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 02/14, Mandeep Singh Baines wrote:
>>
>> This patch makes wait_for_dump_helpers() not to abort piping the core
>> dump data when the crashing process has received any but a fatal signal
>> (SIGKILL). The rationale is that a crashing process may still receive
>> uninteresting signals such as SIGCHLD when its core dump data is being
>> redirected to a helper application.
>
> You already sent this change in the past ;) and I already reviewed it.
>
> It is not enough and imho not good. Damn, I'll try very much to make the
> patches on weekend...
>
>> -     while ((pipe->readers > 1) && (!signal_pending(current))) {
>> +     while ((pipe->readers > 1) && (!fatal_signal_pending(current))) {
>
> This turns pipe_wait() belowe into the busy-wait loop if signal_pending().

D'oh. Thanks for catching that.

Fixed in v3 by blocking non-fatal signals.

> Not good. And not enough, there are other reasons why coredump can fail
> if the signal is pending.

What other reasons did you have in mind?

Since applying an earlier version of this patch, truncated/missing
coredumps are no longer any issue for us.

Maybe it could fail in binfmt->core_dump().

Could the other reasons be addressed in another patch?

>
>>               wake_up_interruptible_sync(&pipe->wait);
>>               kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
>>               pipe_wait(pipe);
>> +             pipe_unlock(pipe);
>> +             try_to_freeze();
>
> Oh, yes. One of the problems with coredump/signals is freezer. Not sure
> what should we do...
>
> But if we add try_to_freeze() here, we need to add more try_to_freeze's,
> think about dumping the huge core on the slow media.
>

We could add more try_to_freeze()s in the dump_write paths to work
even better with freezer. Do you see any issues with just adding it
here for a start. It fixes the non-slow media case.

Thanks much for reviewing this patch.

Regards,
Mandeep

> Oleg.
>

Re: [PATCH 5/5] coredump: abort core dump piping only due to a fatal signal

[Oleg Nesterov]

On 02/15, Mandeep Singh Baines wrote:
>
> On Fri, Feb 15, 2013 at 7:01 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > It is not enough and imho not good. Damn, I'll try very much to make the
> > patches on weekend...
> >
> >> -     while ((pipe->readers > 1) && (!signal_pending(current))) {
> >> +     while ((pipe->readers > 1) && (!fatal_signal_pending(current))) {
> >
> > This turns pipe_wait() belowe into the busy-wait loop if signal_pending().
>
> D'oh. Thanks for catching that.
>
> Fixed in v3 by blocking non-fatal signals.

Doesn't look correct...

> > Not good. And not enough, there are other reasons why coredump can fail
> > if the signal is pending.
>
> What other reasons did you have in mind?

Say, pipe_write() can fail if signal_pending() == T.

> Since applying an earlier version of this patch, truncated/missing
> coredumps are no longer any issue for us.

Sure, this "almost works". But this is doesn't really work.

And more importantly, we should fix another problem, SIGKILL should
really stop the coredumping, and I do not see a simple solution, the
main problem is the races with the exiting threads...

> Could the other reasons be addressed in another patch?

Well. Personally I believe we should fix the problems with signals
first, then add the freezer changes...

> >>               wake_up_interruptible_sync(&pipe->wait);
> >>               kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
> >>               pipe_wait(pipe);
> >> +             pipe_unlock(pipe);
> >> +             try_to_freeze();
> >
> > Oh, yes. One of the problems with coredump/signals is freezer. Not sure
> > what should we do...
> >
> > But if we add try_to_freeze() here, we need to add more try_to_freeze's,
> > think about dumping the huge core on the slow media.
> >
>
> We could add more try_to_freeze()s in the dump_write paths to work
> even better with freezer. Do you see any issues with just adding it
> here for a start. It fixes the non-slow media case.

The only issue is that, again, this change pretends to work but it doesn't ;)
IOW, imho you fix the symptom only.

Lets forget about the slow media, consider the piped coredump (the case
you are trying to fix). Suppose that try_to_freeze_tasks() is in progress,
the user-space coredump handler is already frozen, and the dumping thread
does pipe_write()->pipe_wait().

If only we could change pipe_wait() to do freezable_schedule()...

Oleg.