All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>,
	Ingo Molnar <mingo@redhat.com>,
	Mandeep Singh Baines <msb@chromium.org>,
	Neil Horman <nhorman@redhat.com>,
	"Rafael J. Wysocki" <rjw@sisk.pl>,
	Roland McGrath <roland@hack.frob.com>, Tejun Heo <tj@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: [PATCH 1/3] coredump: only SIGKILL should interrupt the coredumping task
Date: Sun, 17 Feb 2013 20:18:49 +0100	[thread overview]
Message-ID: <20130217191849.GA21806@redhat.com> (raw)
In-Reply-To: <20130217191819.GA21778@redhat.com>

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


  reply	other threads:[~2013-02-17 19:20 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-17 19:18 [PATCH 0/3] coredump: fix the ancient signal problems Oleg Nesterov
2013-02-17 19:18 ` Oleg Nesterov [this message]
2013-02-17 19:19 ` [PATCH 2/3] coredump: ensure that SIGKILL always kills the dumping thread Oleg Nesterov
2013-02-17 19:19 ` [PATCH 3/3] coredump: sanitize the setting of signal->group_exit_code Oleg Nesterov
2013-02-17 19:34 ` [PATCH 0/3] coredump: fix the ancient signal problems Linus Torvalds
2013-02-17 19:50   ` Oleg Nesterov
2013-02-17 20:01     ` Oleg Nesterov
2013-02-20  1:29 ` Mandeep Singh Baines
2013-02-20 22:32   ` Oleg Nesterov
2013-02-20 23:14     ` Andrew Morton
2013-02-23 20:21       ` Oleg Nesterov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130217191849.GA21806@redhat.com \
    --to=oleg@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=alan@lxorguk.ukuu.org.uk \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=msb@chromium.org \
    --cc=nhorman@redhat.com \
    --cc=rjw@sisk.pl \
    --cc=roland@hack.frob.com \
    --cc=tj@kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.