[PATCH 0/3] coredump: fix the ancient signal problems

[PATCH 0/3] coredump: fix the ancient signal problems

[Oleg Nesterov]

Hello.

These problems are really annoying. I reported and tried to fix
them in 2008 (see http://marc.info/?l=linux-kernel&m=121665710711931)
but nobody was interested.

Since then I had a lot of (to some degree contradictory) bug reports:
we do not want the interrupted coredumps (this is what the current code
tries to achieve but the logic is very wrong), but at the same time some
people blame the coredump because it is not interruptible.

And every time the discussion was confusing, it is not clear what should
we actually do to make everyone happy.

Linus, et al, could you please ack/nack the intent? Of course I will
appreciate if you can review the code, but what I am actually worried
about is the user-visible change: the coredumping becomes killable but
only by the _explicit_ SIGKILL, other fatal signals are "ignored".

The changes were not tested at all, I'll try to recheck everything and
test this tomorrow. I am sending this series right now because I strongly
believe that the recent -mm patches in this area are not right and should
be dropped, and I also disagree with the pending v2.

Mandeep, just in case please note that 1/3 alone should fix the problems
with non-fatal signals and wait_dump_helper(). As for the freezer, afaics
we are almost ready for the (slightly modified) fix proposed in
http://marc.info/?l=linux-kernel&m=136103469831268.

Oleg.

 fs/coredump.c         |   33 +++++++++++++++++++--------------
 include/linux/sched.h |    1 +
 kernel/signal.c       |    6 ++++--
 3 files changed, 24 insertions(+), 16 deletions(-)

[PATCH 1/3] coredump: only SIGKILL should interrupt the coredumping task

[Oleg Nesterov]

There are 2 well known and ancient problems with coredump/signals,
and a lot of related bug reports:

- do_coredump() clears TIF_SIGPENDING but of course this can't help
  if, say, SIGCHLD comes after that.

  In this case the coredump can fail unexpectedly. See for example
  wait_for_dump_helper()->signal_pending() check but there are other
  reasons.

- At the same time, dumping a huge core on the slow media can take a
  lot of time/resources and there is no way to kill the coredumping
  task reliably. In particular this is not oom_kill-friendly.

This patch tries to fix the 1st problem, and makes the preparation
for the next changes.

We add the new SIGNAL_GROUP_COREDUMP flag set by zap_threads() to
indicate that this process dumps the core. prepare_signal() checks
this flag and nacks any signal except SIGKILL.

Note that this check tries to be conservative, in the long term we
should probably treat the SIGNAL_GROUP_EXIT case equally but this
needs more discussion. See marc.info/?l=linux-kernel&m=120508897917439

Notes:
	- recalc_sigpending() doesn't check SIGNAL_GROUP_COREDUMP.
	  The patch assumes that dump_write/etc paths should never
	  call it, but we can change it as well.

	- There is another source of TIF_SIGPENDING, freezer. This
	  will be addressed separately.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/coredump.c         |   13 +++++--------
 include/linux/sched.h |    1 +
 kernel/signal.c       |    6 ++++--
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 1774932..2c1ef6a 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -280,8 +280,8 @@ static int zap_process(struct task_struct *start, int exit_code)
 	return nr;
 }
 
-static inline int zap_threads(struct task_struct *tsk, struct mm_struct *mm,
-				struct core_state *core_state, int exit_code)
+static int zap_threads(struct task_struct *tsk, struct mm_struct *mm,
+			struct core_state *core_state, int exit_code)
 {
 	struct task_struct *g, *p;
 	unsigned long flags;
@@ -291,6 +291,9 @@ static inline int zap_threads(struct task_struct *tsk, struct mm_struct *mm,
 	if (!signal_group_exit(tsk->signal)) {
 		mm->core_state = core_state;
 		nr = zap_process(tsk, exit_code);
+		/* ignore all signals except SIGKILL, see prepare_signal() */
+		tsk->signal->flags |= SIGNAL_GROUP_COREDUMP;
+		clear_tsk_thread_flag(tsk, TIF_SIGPENDING);
 	}
 	spin_unlock_irq(&tsk->sighand->siglock);
 	if (unlikely(nr < 0))
@@ -514,12 +517,6 @@ void do_coredump(siginfo_t *siginfo)
 
 	old_cred = override_creds(cred);
 
-	/*
-	 * Clear any false indication of pending signals that might
-	 * be seen by the filesystem code called to write the core file.
-	 */
-	clear_thread_flag(TIF_SIGPENDING);
-
 	ispipe = format_corename(&cn, &cprm);
 
  	if (ispipe) {
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d211247..932a90c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -672,6 +672,7 @@ struct signal_struct {
 #define SIGNAL_STOP_STOPPED	0x00000001 /* job control stop in effect */
 #define SIGNAL_STOP_CONTINUED	0x00000002 /* SIGCONT since WCONTINUED reap */
 #define SIGNAL_GROUP_EXIT	0x00000004 /* group exit in progress */
+#define SIGNAL_GROUP_COREDUMP	0x00000008 /* coredump in progress */
 /*
  * Pending notifications to parent.
  */
diff --git a/kernel/signal.c b/kernel/signal.c
index 3d09cf6..ebae2e0 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -851,12 +851,14 @@ static void ptrace_trap_notify(struct task_struct *t)
  * Returns true if the signal should be actually delivered, otherwise
  * it should be dropped.
  */
-static int prepare_signal(int sig, struct task_struct *p, bool force)
+static bool prepare_signal(int sig, struct task_struct *p, bool force)
 {
 	struct signal_struct *signal = p->signal;
 	struct task_struct *t;
 
-	if (unlikely(signal->flags & SIGNAL_GROUP_EXIT)) {
+	if (signal->flags & (SIGNAL_GROUP_EXIT | SIGNAL_GROUP_COREDUMP)) {
+		if (signal->flags & SIGNAL_GROUP_COREDUMP)
+			return sig == SIGKILL;
 		/*
 		 * The process is in the middle of dying, nothing to do.
 		 */
-- 
1.5.5.1

[PATCH 2/3] coredump: ensure that SIGKILL always kills the dumping thread

[Oleg Nesterov]

prepare_signal() blesses SIGKILL sent to the dumping process but
this signal can be "lost" anyway. The problems is, complete_signal()
sees SIGNAL_GROUP_EXIT and skips the "kill them all" logic. And even
if the dumping process is single-threaded (so the target is always
"correct"), the group-wide SIGKILL is not recorded in task->pending
and thus __fatal_signal_pending() won't be true. A multi-threaded
case has even more problems.

And even ignoring all technical details, SIGNAL_GROUP_EXIT doesn't
look right to me. This coredumping process is not exiting yet, it can
do a lot of work dumping the core.

With this patch the dumping process doesn't have SIGNAL_GROUP_EXIT,
we set signal->group_exit_task instead. This makes signal_group_exit()
true and thus this should equally close the races with exit/exec/stop
but allows to kill the dumping thread reliably.

Notes:
	- It is not clear what should we do with ->group_exit_code
	  if the dumper was killed, see the next change.

	- we need more (hopefully straightforward) changes to ensure
	  that SIGKILL actually interrupts the coredump. Basically we
	  need to check __fatal_signal_pending() in dump_write() and
	  dump_seek().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/coredump.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 2c1ef6a..835d731 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -263,7 +263,6 @@ static int zap_process(struct task_struct *start, int exit_code)
 	struct task_struct *t;
 	int nr = 0;
 
-	start->signal->flags = SIGNAL_GROUP_EXIT;
 	start->signal->group_exit_code = exit_code;
 	start->signal->group_stop_count = 0;
 
@@ -291,8 +290,9 @@ static int zap_threads(struct task_struct *tsk, struct mm_struct *mm,
 	if (!signal_group_exit(tsk->signal)) {
 		mm->core_state = core_state;
 		nr = zap_process(tsk, exit_code);
+		tsk->signal->group_exit_task = tsk;
 		/* ignore all signals except SIGKILL, see prepare_signal() */
-		tsk->signal->flags |= SIGNAL_GROUP_COREDUMP;
+		tsk->signal->flags = SIGNAL_GROUP_COREDUMP;
 		clear_tsk_thread_flag(tsk, TIF_SIGPENDING);
 	}
 	spin_unlock_irq(&tsk->sighand->siglock);
@@ -343,6 +343,7 @@ static int zap_threads(struct task_struct *tsk, struct mm_struct *mm,
 				if (unlikely(p->mm == mm)) {
 					lock_task_sighand(p, &flags);
 					nr += zap_process(p, exit_code);
+					p->signal->flags = SIGNAL_GROUP_EXIT;
 					unlock_task_sighand(p, &flags);
 				}
 				break;
@@ -394,6 +395,11 @@ static void coredump_finish(struct mm_struct *mm)
 	struct core_thread *curr, *next;
 	struct task_struct *task;
 
+	spin_lock_irq(&current->sighand->siglock);
+	current->signal->group_exit_task = NULL;
+	current->signal->flags = SIGNAL_GROUP_EXIT;
+	spin_unlock_irq(&current->sighand->siglock);
+
 	next = mm->core_state->dumper.next;
 	while ((curr = next) != NULL) {
 		next = curr->next;
-- 
1.5.5.1

[PATCH 3/3] coredump: sanitize the setting of signal->group_exit_code

[Oleg Nesterov]

Now that the coredumping process can be SIGKILL'ed, the setting of
->group_exit_code in do_coredump() can race with complete_signal()
and SIGKILL or 0x80 can be "lost", or wait(status) can report
status == SIGKILL | 0x80.

But the main problem is that it is not clear to me what should we
do if binfmt->core_dump() succeeds but SIGKILL was sent, that is
why this patch comes as a separate change.

This patch adds 0x80 if ->core_dump() succeeds and the process was
not killed. But perhaps we can (should?) re-set ->group_exit_code
changed by SIGKILL back to "siginfo->si_signo |= 0x80" in case when
core_dumped == T.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/coredump.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 835d731..5503d94 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -390,12 +390,14 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
 	return core_waiters;
 }
 
-static void coredump_finish(struct mm_struct *mm)
+static void coredump_finish(struct mm_struct *mm, bool core_dumped)
 {
 	struct core_thread *curr, *next;
 	struct task_struct *task;
 
 	spin_lock_irq(&current->sighand->siglock);
+	if (core_dumped && !__fatal_signal_pending(current))
+		current->signal->group_exit_code |= 0x80;
 	current->signal->group_exit_task = NULL;
 	current->signal->flags = SIGNAL_GROUP_EXIT;
 	spin_unlock_irq(&current->sighand->siglock);
@@ -480,6 +482,7 @@ void do_coredump(siginfo_t *siginfo)
 	int ispipe;
 	struct files_struct *displaced;
 	bool need_nonrelative = false;
+	bool core_dumped = false;
 	static atomic_t core_dump_count = ATOMIC_INIT(0);
 	struct coredump_params cprm = {
 		.siginfo = siginfo,
@@ -632,9 +635,8 @@ void do_coredump(siginfo_t *siginfo)
 		goto close_fail;
 	if (displaced)
 		put_files_struct(displaced);
-	retval = binfmt->core_dump(&cprm);
-	if (retval)
-		current->signal->group_exit_code |= 0x80;
+
+	core_dumped = binfmt->core_dump(&cprm);
 
 	if (ispipe && core_pipe_limit)
 		wait_for_dump_helpers(cprm.file);
@@ -647,7 +649,7 @@ fail_dropcount:
 fail_unlock:
 	kfree(cn.corename);
 fail_corename:
-	coredump_finish(mm);
+	coredump_finish(mm, core_dumped);
 	revert_creds(old_cred);
 fail_creds:
 	put_cred(cred);
-- 
1.5.5.1

Re: [PATCH 0/3] coredump: fix the ancient signal problems

[Linus Torvalds]

On Sun, Feb 17, 2013 at 11:18 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Linus, et al, could you please ack/nack the intent? Of course I will
> appreciate if you can review the code, but what I am actually worried
> about is the user-visible change: the coredumping becomes killable but
> only by the _explicit_ SIGKILL, other fatal signals are "ignored".

That isn't a problem. In fact, we already have logic that makes the
act of writing a file be killable by SIGKILL (because you really
really want that for network filesystems, for example), so I suspect
that core-dumping was interruptible by SIGKILL even before you made it
explicitly so - simply because the IO itself was.

And even if it wasn't (because maybe the SIGKILL logic doesn't get
triggered due to all the special-case core-dumping code in signal
handling), SIGKILL really is very very special. Having it kill a
coredump in progress sounds fine to me. SIGKILL is used by
users/admins as a last-case thing to get rid of processes that are
problematic for the system, and we should really try to honor it over
just about anything else for that reason. It's the user saying "get
this thing away from me", we should honor it.

That said, I'm not convinced about your particular split of patches.
The first patch introduces that new SIGNAL_GROUP_COREDUMP, and then
the second patch modifies one of the new use cases:

-               tsk->signal->flags |= SIGNAL_GROUP_COREDUMP;
+               tsk->signal->flags = SIGNAL_GROUP_COREDUMP;


and that just smells to me like you tried too hard to split things
into two patches.

And obviously, it will need more testing and review. I wonder if Al
Viro hould be on the cc. He doesn't always get involved, but when it
comes to core-dumping and signal handling, he *sometimes* does, and so
just checking if he has any comments might be worth it.

                         Linus

Re: [PATCH 0/3] coredump: fix the ancient signal problems

[Oleg Nesterov]

On 02/17, Linus Torvalds wrote:
>
> On Sun, Feb 17, 2013 at 11:18 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Linus, et al, could you please ack/nack the intent? Of course I will
> > appreciate if you can review the code, but what I am actually worried
> > about is the user-visible change: the coredumping becomes killable but
> > only by the _explicit_ SIGKILL, other fatal signals are "ignored".
>
> That isn't a problem. In fact, we already have logic that makes the
> act of writing a file be killable by SIGKILL (because you really
> really want that for network filesystems, for example), so I suspect
> that core-dumping was interruptible by SIGKILL even before you made it
> explicitly so - simply because the IO itself was.

Yes, and even pipe_write() can fail if signal_pending() == T.

> And even if it wasn't (because maybe the SIGKILL logic doesn't get
> triggered due to all the special-case core-dumping code in signal
> handling),

Yes, SIGKILL can wakeup (or can miss) the dumping thread sleeping in
->write() but this is not enough. See 2/3.

> SIGKILL really is very very special. Having it kill a
> coredump in progress sounds fine to me.

Great.

> That said, I'm not convinced about your particular split of patches.
> The first patch introduces that new SIGNAL_GROUP_COREDUMP, and then
> the second patch modifies one of the new use cases:
>
> -               tsk->signal->flags |= SIGNAL_GROUP_COREDUMP;
> +               tsk->signal->flags = SIGNAL_GROUP_COREDUMP;
>
>
> and that just smells to me like you tried too hard to split things
> into two patches.

Oh, I disagree, but I wouldn't mind to join these changes assuming
they pass the review (including my self-review tomorrow).

To me, the splitting is "natural". 1/3 protects the dumping thread
from !SIGKILL signals, 2/3 makes makes the dumping thread killable.

Another reason for 1/3 in a separate patch is the documentation,
I think we need more changes in prepare_signal(SIGNAL_GROUP_EXIT)
case.

But I won't insist.

> I wonder if Al
> Viro hould be on the cc.

Hello Al.

I'll send you mbox with this series privately.

Oleg.

Re: [PATCH 0/3] coredump: fix the ancient signal problems

[Oleg Nesterov]

On 02/17, Oleg Nesterov wrote:
>
> On 02/17, Linus Torvalds wrote:
> >
> > SIGKILL really is very very special. Having it kill a
> > coredump in progress sounds fine to me.
>
> Great.

Forgot to mention just in case...

We could probably make a simpler patch. do_coredump() can ignore
all signals except SIGKILL from the start. But when I suggested
this change in the past I was told that the dump handler looks
(or may look) at /proc/pid/status so we shouldn't do this.

And instead of s/SIGNAL_GROUP_EXIT/group_exit_task/ in 2/3 we
could add the ugly but simple checks in __send_signal() paths.
Say we could rely on PF_DUMPCORE (which btw asks for cleanup
anyway).

However, I think that using ->group_exit_task is better (if
correct!) and simply more logical.

Oleg.

Re: [PATCH 0/3] coredump: fix the ancient signal problems

[Mandeep Singh Baines]

On Sun, Feb 17, 2013 at 11:18 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> Hello.
>
> These problems are really annoying. I reported and tried to fix
> them in 2008 (see http://marc.info/?l=linux-kernel&m=121665710711931)
> but nobody was interested.
>
> Since then I had a lot of (to some degree contradictory) bug reports:
> we do not want the interrupted coredumps (this is what the current code
> tries to achieve but the logic is very wrong), but at the same time some
> people blame the coredump because it is not interruptible.
>
> And every time the discussion was confusing, it is not clear what should
> we actually do to make everyone happy.
>
> Linus, et al, could you please ack/nack the intent? Of course I will
> appreciate if you can review the code, but what I am actually worried
> about is the user-visible change: the coredumping becomes killable but
> only by the _explicit_ SIGKILL, other fatal signals are "ignored".
>
> The changes were not tested at all, I'll try to recheck everything and
> test this tomorrow. I am sending this series right now because I strongly
> believe that the recent -mm patches in this area are not right and should
> be dropped, and I also disagree with the pending v2.
>
> Mandeep, just in case please note that 1/3 alone should fix the problems
> with non-fatal signals and wait_dump_helper(). As for the freezer, afaics
> we are almost ready for the (slightly modified) fix proposed in
> http://marc.info/?l=linux-kernel&m=136103469831268.
>
> Oleg.

For the whole series:

Tested-by: Mandeep Singh Baines <msb@chromium.org>

This was an important issue for us so I'm in the process of merging
these into the ChromiumOS kernel tree.

Thanks,
Mandeep

>
>  fs/coredump.c         |   33 +++++++++++++++++++--------------
>  include/linux/sched.h |    1 +
>  kernel/signal.c       |    6 ++++--
>  3 files changed, 24 insertions(+), 16 deletions(-)
>

Re: [PATCH 0/3] coredump: fix the ancient signal problems

[Oleg Nesterov]

On 02/19, Mandeep Singh Baines wrote:
>
> For the whole series:
>
> Tested-by: Mandeep Singh Baines <msb@chromium.org>

Oh, thanks a lot Mandeep.

I tried to test it too, seems to work.

Andrew, could you take it? We will make more fixes on top.

Oleg.

Re: [PATCH 0/3] coredump: fix the ancient signal problems

[Andrew Morton]

On Wed, 20 Feb 2013 23:32:26 +0100
Oleg Nesterov <oleg@redhat.com> wrote:

> On 02/19, Mandeep Singh Baines wrote:
> >
> > For the whole series:
> >
> > Tested-by: Mandeep Singh Baines <msb@chromium.org>
> 
> Oh, thanks a lot Mandeep.
> 
> I tried to test it too, seems to work.
> 
> Andrew, could you take it? We will make more fixes on top.
> 

Did that, but the timing and somewhat tentative nature of the changes
makes me think "3.10-rc1".  Is that a problem?

Re: [PATCH 0/3] coredump: fix the ancient signal problems

[Oleg Nesterov]

On 02/20, Andrew Morton wrote:
>
> On Wed, 20 Feb 2013 23:32:26 +0100
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Andrew, could you take it? We will make more fixes on top.
>
> Did that, but the timing and somewhat tentative nature of the changes
> makes me think "3.10-rc1".

Oh, yes, I will sleep much better if these changes get more testing
in -mm, I do not think this is 3.9 material.

Oleg.