[PATCH -mm 0/3] coredump: make it freezable (almost)

[PATCH -mm 0/3] coredump: make it freezable (almost)

[Oleg Nesterov]

Hello.

On top of coredump-sanitize-the-setting-of-signal-group_exit_code.patch

Andrew, "almost" means we need a bit more changes, but these
changes should be absolutely trivial/straightforward. We need
to add the killable/freezeng checks in dump_write/seek. But this
depends on 2/3, lets discuss this series first.

Mandeep, assuming that you agree with this series, I would be
happy if you add your sob's, this all is based on our discussion.

Oleg.

 arch/x86/ia32/ia32_aout.c |    1 -
 fs/binfmt_aout.c          |    1 -
 fs/binfmt_elf.c           |    3 +--
 fs/binfmt_elf_fdpic.c     |    2 --
 fs/coredump.c             |   13 +++++++------
 kernel/freezer.c          |    2 +-
 6 files changed, 9 insertions(+), 13 deletions(-)

[PATCH 1/3] coredump: factor out the setting of PF_DUMPCORE

[Oleg Nesterov]

Cleanup and preparation. Every linux_binfmt->core_dump() sets
PF_DUMPCORE, move this into zap_threads() called by do_coredump().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/ia32/ia32_aout.c |    1 -
 fs/binfmt_aout.c          |    1 -
 fs/binfmt_elf.c           |    3 +--
 fs/binfmt_elf_fdpic.c     |    2 --
 fs/coredump.c             |    1 +
 5 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
index a703af1..24b787c 100644
--- a/arch/x86/ia32/ia32_aout.c
+++ b/arch/x86/ia32/ia32_aout.c
@@ -162,7 +162,6 @@ static int aout_core_dump(long signr, struct pt_regs *regs, struct file *file,
 	fs = get_fs();
 	set_fs(KERNEL_DS);
 	has_dumped = 1;
-	current->flags |= PF_DUMPCORE;
 	strncpy(dump.u_comm, current->comm, sizeof(current->comm));
 	dump.u_ar0 = offsetof(struct user32, regs);
 	dump.signal = signr;
diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index 6043567..f70aea2 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -62,7 +62,6 @@ static int aout_core_dump(struct coredump_params *cprm)
 	fs = get_fs();
 	set_fs(KERNEL_DS);
 	has_dumped = 1;
-	current->flags |= PF_DUMPCORE;
        	strncpy(dump.u_comm, current->comm, sizeof(dump.u_comm));
 	dump.u_ar0 = offsetof(struct user, regs);
 	dump.signal = cprm->siginfo->si_signo;
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 0c42cdb..1f52be1 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -2082,8 +2082,7 @@ static int elf_core_dump(struct coredump_params *cprm)
 		goto cleanup;
 
 	has_dumped = 1;
-	current->flags |= PF_DUMPCORE;
-  
+
 	fs = get_fs();
 	set_fs(KERNEL_DS);
 
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index dc84732..8f2c03d 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -1684,8 +1684,6 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
 	fill_elf_fdpic_header(elf, e_phnum);
 
 	has_dumped = 1;
-	current->flags |= PF_DUMPCORE;
-
 	/*
 	 * Set up the notes in similar form to SVR4 core dumps made
 	 * with info from their /proc.
diff --git a/fs/coredump.c b/fs/coredump.c
index 5503d94..4f3c8d1 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -290,6 +290,7 @@ 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->flags = PF_DUMPCORE;
 		tsk->signal->group_exit_task = tsk;
 		/* ignore all signals except SIGKILL, see prepare_signal() */
 		tsk->signal->flags = SIGNAL_GROUP_COREDUMP;
-- 
1.5.5.1

[PATCH 2/3] freezer: do not send a fake signal to a PF_DUMPCORE thread

[Oleg Nesterov]

A coredumping thread can't be frozen anyway but the fake signal sent
by freeze_task() can confuse dump_write/wait_for_dump_helpers/etc
and interrupt the coredump.

We are going to make the do_coredump() paths freezable but the fake
TIF_SIGPENDING doesn't help, it only makes sense when we assume that
the target can return to user-mode and call get_signal_to_deliver().

Change freeze_task() to check PF_DUMPCORE along with PF_KTHREAD.

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

diff --git a/kernel/freezer.c b/kernel/freezer.c
index c38893b..88d2644 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -116,7 +116,7 @@ bool freeze_task(struct task_struct *p)
 		return false;
 	}
 
-	if (!(p->flags & PF_KTHREAD))
+	if (!(p->flags & (PF_KTHREAD | PF_DUMPCORE)))
 		fake_signal_wake_up(p);
 	else
 		wake_up_state(p, TASK_INTERRUPTIBLE);
-- 
1.5.5.1

[PATCH 3/3] coredump: make wait_for_dump_helpers() freezable

[Oleg Nesterov]

wait_for_dump_helpers() calls wake_up/kill_fasync from inside the
wait_event-like loop. This is not needed and in fact this is not
strictly correct, we can/should do this only once after we change
pipe->writers. We could even check if it becomes zero.

With this change it is trivial to convert this code to use
wait_event_freezable() and make this function freezable/killable,
only SIGKILL can set TIF_SIGPENDING.

With this patch we check pipe->readers without pipe_lock(), this
is fine. Once we see pipe->readers == 1 we know that the handler
decremented the counter, this is all we need.

Note: wait_event_freezable() is "strange", perhaps it should be
changed or simply removed. In the latter case we can change this
code again to use freezer_do_not_count + wait_event_interruptible.

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

diff --git a/fs/coredump.c b/fs/coredump.c
index 4f3c8d1..284d841 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>
@@ -428,17 +429,16 @@ static void wait_for_dump_helpers(struct file *file)
 	pipe_lock(pipe);
 	pipe->readers++;
 	pipe->writers--;
+	wake_up_interruptible_sync(&pipe->wait);
+	kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
+	pipe_unlock(pipe);
 
-	while ((pipe->readers > 1) && (!signal_pending(current))) {
-		wake_up_interruptible_sync(&pipe->wait);
-		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
-		pipe_wait(pipe);
-	}
+	wait_event_freezable(pipe->wait, pipe->readers == 1);
 
+	pipe_lock(pipe);
 	pipe->readers--;
 	pipe->writers++;
 	pipe_unlock(pipe);
-
 }
 
 /*
-- 
1.5.5.1

Re: [PATCH -mm 0/3] coredump: make it freezable (almost)

[Oleg Nesterov]

On 02/24, Oleg Nesterov wrote:
>
> depends on 2/3, lets discuss this series first.

Cough. Not sure how I managed to convince myself that freeze_task()
can't race with zap_threads() and wrongly set TIF_SIGPENDING. I'll
send v2 in reply to 2/3.

Oleg.

[PATCH v2 2/3] freezer: do not send a fake signal to a PF_DUMPCORE thread

[Oleg Nesterov]

A coredumping thread can't be frozen anyway but the fake signal sent
by freeze_task() can confuse dump_write/wait_for_dump_helpers/etc
and interrupt the coredump.

We are going to make the do_coredump() paths freezable but the fake
TIF_SIGPENDING doesn't help, it only makes sense when we assume that
the target can return to user-mode and call get_signal_to_deliver().

Change freeze_task() to check PF_DUMPCORE along with PF_KTHREAD. We
need to recheck PF_DUMPCORE under ->siglock to avoid the race with
zap_threads() which can set this flag right before we take the lock.

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

diff --git a/kernel/freezer.c b/kernel/freezer.c
index c38893b..595afab 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -85,14 +85,21 @@ bool __refrigerator(bool check_kthr_stop)
 }
 EXPORT_SYMBOL(__refrigerator);
 
-static void fake_signal_wake_up(struct task_struct *p)
+static bool fake_signal_wake_up(struct task_struct *p)
 {
 	unsigned long flags;
+	bool ret = false;
+
+	if (p->flags & (PF_KTHREAD | PF_DUMPCORE))
+		return ret;
 
 	if (lock_task_sighand(p, &flags)) {
-		signal_wake_up(p, 0);
+		ret = !(p->flags & PF_DUMPCORE);
+		if (ret)
+			signal_wake_up(p, 0);
 		unlock_task_sighand(p, &flags);
 	}
+	return ret;
 }
 
 /**
@@ -100,8 +107,8 @@ static void fake_signal_wake_up(struct task_struct *p)
  * @p: task to send the request to
  *
  * If @p is freezing, the freeze request is sent either by sending a fake
- * signal (if it's not a kernel thread) or waking it up (if it's a kernel
- * thread).
+ * signal (if it's not a kernel thread or a coredumping thread) or waking
+ * it up otherwise.
  *
  * RETURNS:
  * %false, if @p is not freezing or already frozen; %true, otherwise
@@ -116,9 +123,7 @@ bool freeze_task(struct task_struct *p)
 		return false;
 	}
 
-	if (!(p->flags & PF_KTHREAD))
-		fake_signal_wake_up(p);
-	else
+	if (!fake_signal_wake_up(p))
 		wake_up_state(p, TASK_INTERRUPTIBLE);
 
 	spin_unlock_irqrestore(&freezer_lock, flags);
-- 
1.5.5.1

Re: [PATCH v2 2/3] freezer: do not send a fake signal to a PF_DUMPCORE thread

[Rafael J. Wysocki]

On Sunday, February 24, 2013 07:36:56 PM Oleg Nesterov wrote:
> A coredumping thread can't be frozen anyway but the fake signal sent
> by freeze_task() can confuse dump_write/wait_for_dump_helpers/etc
> and interrupt the coredump.
> 
> We are going to make the do_coredump() paths freezable but the fake
> TIF_SIGPENDING doesn't help, it only makes sense when we assume that
> the target can return to user-mode and call get_signal_to_deliver().
> 
> Change freeze_task() to check PF_DUMPCORE along with PF_KTHREAD. We
> need to recheck PF_DUMPCORE under ->siglock to avoid the race with
> zap_threads() which can set this flag right before we take the lock.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  kernel/freezer.c |   19 ++++++++++++-------
>  1 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index c38893b..595afab 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -85,14 +85,21 @@ bool __refrigerator(bool check_kthr_stop)
>  }
>  EXPORT_SYMBOL(__refrigerator);
>  
> -static void fake_signal_wake_up(struct task_struct *p)
> +static bool fake_signal_wake_up(struct task_struct *p)
>  {
>  	unsigned long flags;
> +	bool ret = false;
> +
> +	if (p->flags & (PF_KTHREAD | PF_DUMPCORE))
> +		return ret;
>  
>  	if (lock_task_sighand(p, &flags)) {
> -		signal_wake_up(p, 0);
> +		ret = !(p->flags & PF_DUMPCORE);
> +		if (ret)
> +			signal_wake_up(p, 0);
>  		unlock_task_sighand(p, &flags);
>  	}
> +	return ret;
>  }
>  
>  /**
> @@ -100,8 +107,8 @@ static void fake_signal_wake_up(struct task_struct *p)
>   * @p: task to send the request to
>   *
>   * If @p is freezing, the freeze request is sent either by sending a fake
> - * signal (if it's not a kernel thread) or waking it up (if it's a kernel
> - * thread).
> + * signal (if it's not a kernel thread or a coredumping thread) or waking
> + * it up otherwise.
>   *
>   * RETURNS:
>   * %false, if @p is not freezing or already frozen; %true, otherwise
> @@ -116,9 +123,7 @@ bool freeze_task(struct task_struct *p)
>  		return false;
>  	}
>  
> -	if (!(p->flags & PF_KTHREAD))
> -		fake_signal_wake_up(p);
> -	else
> +	if (!fake_signal_wake_up(p))
>  		wake_up_state(p, TASK_INTERRUPTIBLE);
>  
>  	spin_unlock_irqrestore(&freezer_lock, flags);
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH -mm 0/3] coredump: make it freezable (almost)

[Andrew Morton]

On Sun, 24 Feb 2013 18:31:44 +0100
Oleg Nesterov <oleg@redhat.com> wrote:

> Hello.
> 
> On top of coredump-sanitize-the-setting-of-signal-group_exit_code.patch
> 
> Andrew, "almost" means we need a bit more changes, but these
> changes should be absolutely trivial/straightforward. We need
> to add the killable/freezeng checks in dump_write/seek. But this
> depends on 2/3, lets discuss this series first.

Thanks.  I still have these marked "for 3.10".

coredump-only-sigkill-should-interrupt-the-coredumping-task.patch
coredump-ensure-that-sigkill-always-kills-the-dumping-thread.patch
coredump-sanitize-the-setting-of-signal-group_exit_code.patch
coredump-factor-out-the-setting-of-pf_dumpcore.patch
freezer-do-not-send-a-fake-signal-to-a-pf_dumpcore-thread.patch
coredump-make-wait_for_dump_helpers-freezable.patch

Re: [PATCH v2 2/3] freezer: do not send a fake signal to a PF_DUMPCORE thread

[Mandeep Singh Baines]

On Sun, Feb 24, 2013 at 10:36 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> A coredumping thread can't be frozen anyway but the fake signal sent
> by freeze_task() can confuse dump_write/wait_for_dump_helpers/etc
> and interrupt the coredump.
>
> We are going to make the do_coredump() paths freezable but the fake
> TIF_SIGPENDING doesn't help, it only makes sense when we assume that
> the target can return to user-mode and call get_signal_to_deliver().
>
> Change freeze_task() to check PF_DUMPCORE along with PF_KTHREAD. We
> need to recheck PF_DUMPCORE under ->siglock to avoid the race with
> zap_threads() which can set this flag right before we take the lock.
>

Won't this prevent suspend?

If there is a wait_event_interruptible in the coredump path, you'll
wake it up but it will simply go back to sleep. So try_to_freeze_tasks
will fail waiting for the coredump thread to enter the freezer.

Regards,
Mandeep

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/freezer.c |   19 ++++++++++++-------
>  1 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index c38893b..595afab 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -85,14 +85,21 @@ bool __refrigerator(bool check_kthr_stop)
>  }
>  EXPORT_SYMBOL(__refrigerator);
>
> -static void fake_signal_wake_up(struct task_struct *p)
> +static bool fake_signal_wake_up(struct task_struct *p)
>  {
>         unsigned long flags;
> +       bool ret = false;
> +
> +       if (p->flags & (PF_KTHREAD | PF_DUMPCORE))
> +               return ret;
>
>         if (lock_task_sighand(p, &flags)) {
> -               signal_wake_up(p, 0);
> +               ret = !(p->flags & PF_DUMPCORE);
> +               if (ret)
> +                       signal_wake_up(p, 0);
>                 unlock_task_sighand(p, &flags);
>         }
> +       return ret;
>  }
>
>  /**
> @@ -100,8 +107,8 @@ static void fake_signal_wake_up(struct task_struct *p)
>   * @p: task to send the request to
>   *
>   * If @p is freezing, the freeze request is sent either by sending a fake
> - * signal (if it's not a kernel thread) or waking it up (if it's a kernel
> - * thread).
> + * signal (if it's not a kernel thread or a coredumping thread) or waking
> + * it up otherwise.
>   *
>   * RETURNS:
>   * %false, if @p is not freezing or already frozen; %true, otherwise
> @@ -116,9 +123,7 @@ bool freeze_task(struct task_struct *p)
>                 return false;
>         }
>
> -       if (!(p->flags & PF_KTHREAD))
> -               fake_signal_wake_up(p);
> -       else
> +       if (!fake_signal_wake_up(p))
>                 wake_up_state(p, TASK_INTERRUPTIBLE);
>
>         spin_unlock_irqrestore(&freezer_lock, flags);
> --
> 1.5.5.1
>
>

Re: [PATCH v2 2/3] freezer: do not send a fake signal to a PF_DUMPCORE thread

[Mandeep Singh Baines]

On Tue, Feb 26, 2013 at 8:37 AM, Mandeep Singh Baines <msb@chromium.org> wrote:
> On Sun, Feb 24, 2013 at 10:36 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> A coredumping thread can't be frozen anyway but the fake signal sent
>> by freeze_task() can confuse dump_write/wait_for_dump_helpers/etc
>> and interrupt the coredump.
>>
>> We are going to make the do_coredump() paths freezable but the fake
>> TIF_SIGPENDING doesn't help, it only makes sense when we assume that
>> the target can return to user-mode and call get_signal_to_deliver().
>>
>> Change freeze_task() to check PF_DUMPCORE along with PF_KTHREAD. We
>> need to recheck PF_DUMPCORE under ->siglock to avoid the race with
>> zap_threads() which can set this flag right before we take the lock.
>>
>
> Won't this prevent suspend?
>
> If there is a wait_event_interruptible in the coredump path, you'll
> wake it up but it will simply go back to sleep. So try_to_freeze_tasks
> will fail waiting for the coredump thread to enter the freezer.
>

You'd rather have reliable suspend than coredumps that aren't
truncated so you need to set TIF_SIGPENDING to break waits in the
dump_write path.

But it would be nice to have both so you'd like to avoid terminating
on a signal. I think you'll need to fix each wait independent by
making it freezable or adding try_to_freeze?

To fix wait_for_dump_helpers:

static void wait_for_dump_helpers(struct file *file)
{
        struct pipe_inode_info *pipe;

        pipe = file->f_path.dentry->d_inode->i_pipe;

        pipe_lock(pipe);
        pipe->readers++;
        pipe->writers--;

        while (pipe->readers > 1) {
                unsigned long flags;

                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);

                if (fatal_signal_pending(current))
                        break;

                /* Clear fake signal from freeze_task(). */
                spin_lock_irqsave(&current->sighand->siglock, flags);
                recalc_sigpending();
                spin_unlock_irqrestore(&current->sighand->siglock, flags);
        }

        pipe->readers--;
        pipe->writers++;
        pipe_unlock(pipe);

}

What do you think? That would fix most cases. You'll still get a
truncated core if you were to receive the signal during pipe_write or
something.

Regards,
Mandeep

> Regards,
> Mandeep
>
>> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>> ---
>>  kernel/freezer.c |   19 ++++++++++++-------
>>  1 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/kernel/freezer.c b/kernel/freezer.c
>> index c38893b..595afab 100644
>> --- a/kernel/freezer.c
>> +++ b/kernel/freezer.c
>> @@ -85,14 +85,21 @@ bool __refrigerator(bool check_kthr_stop)
>>  }
>>  EXPORT_SYMBOL(__refrigerator);
>>
>> -static void fake_signal_wake_up(struct task_struct *p)
>> +static bool fake_signal_wake_up(struct task_struct *p)
>>  {
>>         unsigned long flags;
>> +       bool ret = false;
>> +
>> +       if (p->flags & (PF_KTHREAD | PF_DUMPCORE))
>> +               return ret;
>>
>>         if (lock_task_sighand(p, &flags)) {
>> -               signal_wake_up(p, 0);
>> +               ret = !(p->flags & PF_DUMPCORE);
>> +               if (ret)
>> +                       signal_wake_up(p, 0);
>>                 unlock_task_sighand(p, &flags);
>>         }
>> +       return ret;
>>  }
>>
>>  /**
>> @@ -100,8 +107,8 @@ static void fake_signal_wake_up(struct task_struct *p)
>>   * @p: task to send the request to
>>   *
>>   * If @p is freezing, the freeze request is sent either by sending a fake
>> - * signal (if it's not a kernel thread) or waking it up (if it's a kernel
>> - * thread).
>> + * signal (if it's not a kernel thread or a coredumping thread) or waking
>> + * it up otherwise.
>>   *
>>   * RETURNS:
>>   * %false, if @p is not freezing or already frozen; %true, otherwise
>> @@ -116,9 +123,7 @@ bool freeze_task(struct task_struct *p)
>>                 return false;
>>         }
>>
>> -       if (!(p->flags & PF_KTHREAD))
>> -               fake_signal_wake_up(p);
>> -       else
>> +       if (!fake_signal_wake_up(p))
>>                 wake_up_state(p, TASK_INTERRUPTIBLE);
>>
>>         spin_unlock_irqrestore(&freezer_lock, flags);
>> --
>> 1.5.5.1
>>
>>

Re: [PATCH v2 2/3] freezer: do not send a fake signal to a PF_DUMPCORE thread

[Oleg Nesterov]

On 02/26, Mandeep Singh Baines wrote:
>
> >> Change freeze_task() to check PF_DUMPCORE along with PF_KTHREAD. We
> >> need to recheck PF_DUMPCORE under ->siglock to avoid the race with
> >> zap_threads() which can set this flag right before we take the lock.
> >>
> >
> > Won't this prevent suspend?

Hmm. I guess you mean that pipe_write() can hang in pipe_wait() if the
user-space handler was already freezed... Damn, and I even mentioned
this race when we discussed this 2 weeks ago.

I need to think, but most probably you are right, and we need another
solution...

> You'd rather have reliable suspend than coredumps that aren't
> truncated so you need to set TIF_SIGPENDING to break waits in the
> dump_write path.

Oh, I agree. In this case the necessary changes look simple.

> static void wait_for_dump_helpers(struct file *file)
> {
>         struct pipe_inode_info *pipe;
>
>         pipe = file->f_path.dentry->d_inode->i_pipe;
>
>         pipe_lock(pipe);
>         pipe->readers++;
>         pipe->writers--;
>
>         while (pipe->readers > 1) {
>                 unsigned long flags;
>
>                 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);
>
>                 if (fatal_signal_pending(current))
>                         break;
>
>                 /* Clear fake signal from freeze_task(). */
>                 spin_lock_irqsave(&current->sighand->siglock, flags);
>                 recalc_sigpending();
>                 spin_unlock_irqrestore(&current->sighand->siglock, flags);

IIRC, this is what you added into your tree. But note that
recalc_sigpending() is wrong, exactly because (say) SIGCHLD can
be pending if it was sent before we set SIGNAL_GROUP_COREDUMP.

So this code needs something like

	spin_lock_irq(siglock);
	if (!fatal_signal_pending)
		clear_thread_flag(TIF_SIGPENDING);
	spin_unlock_irq(siglock);

Or we need to change recalc_sigpending() to check SIGNAL_GROUP_COREDUMP
or PF_DUMPCORE. I'd like to avoid this, but perhaps we have to do this...

(Btw, this is offtopic, but whatever we do 3/3 still looks like a nice
 cleanup to me, although it probably needs more changes)

> What do you think? That would fix most cases. You'll still get a
> truncated core if you were to receive the signal during pipe_write or
> something.

Let me think a bit...

Right now I can only say that personally I do not really like the
idea to fix wait_for_dump_helpers() but not pipe_write(). I mean,
if pipe_write() can fail due to freezing(), then why should we care
about wait_for_dump_helpers() ? Let them all fail, suspend is not
that often.

Or we should try to make everything freezer-friendly. But if
freeze_task() sets TIF_SIGPENDING then we need the ugly "retry"
logic in dump_write()... Not good.

Thanks Mandeep. If you have other ideas please tell me ;)

Oleg.

Re: [PATCH v2 2/3] freezer: do not send a fake signal to a PF_DUMPCORE thread

[Oleg Nesterov]

On 02/27, Oleg Nesterov wrote:
>
> On 02/26, Mandeep Singh Baines wrote:
> >
> > You'd rather have reliable suspend than coredumps that aren't
> > truncated so you need to set TIF_SIGPENDING to break waits in the
> > dump_write path.
>
> Oh, I agree. In this case the necessary changes look simple.

Really. What if we simply add

	if (signal_pending())
		// SIGKILL or freezing()
		return -EINTR;

into dump_write() and change 3/3 to use wait_event_interruptible?

At least for the start. This is at least consistent, we do not
prevent suspend but the coredumping can be truncated (with the
current code "truncated" can happen anyway).

Then we can try to make it freezable or simply forget about this
imho minor problem.

What do you think?

Oleg.

Re: [PATCH v2 2/3] freezer: do not send a fake signal to a PF_DUMPCORE thread

[Mandeep Singh Baines]

On Wed, Feb 27, 2013 at 10:08 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 02/26, Mandeep Singh Baines wrote:
>>
>> >> Change freeze_task() to check PF_DUMPCORE along with PF_KTHREAD. We
>> >> need to recheck PF_DUMPCORE under ->siglock to avoid the race with
>> >> zap_threads() which can set this flag right before we take the lock.
>> >>
>> >
>> > Won't this prevent suspend?
>
> Hmm. I guess you mean that pipe_write() can hang in pipe_wait() if the
> user-space handler was already freezed... Damn, and I even mentioned
> this race when we discussed this 2 weeks ago.
>
> I need to think, but most probably you are right, and we need another
> solution...
>
>> You'd rather have reliable suspend than coredumps that aren't
>> truncated so you need to set TIF_SIGPENDING to break waits in the
>> dump_write path.
>
> Oh, I agree. In this case the necessary changes look simple.
>
>> static void wait_for_dump_helpers(struct file *file)
>> {
>>         struct pipe_inode_info *pipe;
>>
>>         pipe = file->f_path.dentry->d_inode->i_pipe;
>>
>>         pipe_lock(pipe);
>>         pipe->readers++;
>>         pipe->writers--;
>>
>>         while (pipe->readers > 1) {
>>                 unsigned long flags;
>>
>>                 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);
>>
>>                 if (fatal_signal_pending(current))
>>                         break;
>>
>>                 /* Clear fake signal from freeze_task(). */
>>                 spin_lock_irqsave(&current->sighand->siglock, flags);
>>                 recalc_sigpending();
>>                 spin_unlock_irqrestore(&current->sighand->siglock, flags);
>
> IIRC, this is what you added into your tree. But note that
> recalc_sigpending() is wrong, exactly because (say) SIGCHLD can
> be pending if it was sent before we set SIGNAL_GROUP_COREDUMP.
>
> So this code needs something like
>
>         spin_lock_irq(siglock);
>         if (!fatal_signal_pending)
>                 clear_thread_flag(TIF_SIGPENDING);
>         spin_unlock_irq(siglock);
>
> Or we need to change recalc_sigpending() to check SIGNAL_GROUP_COREDUMP
> or PF_DUMPCORE. I'd like to avoid this, but perhaps we have to do this...
>
> (Btw, this is offtopic, but whatever we do 3/3 still looks like a nice
>  cleanup to me, although it probably needs more changes)
>
>> What do you think? That would fix most cases. You'll still get a
>> truncated core if you were to receive the signal during pipe_write or
>> something.
>
> Let me think a bit...
>
> Right now I can only say that personally I do not really like the
> idea to fix wait_for_dump_helpers() but not pipe_write(). I mean,
> if pipe_write() can fail due to freezing(), then why should we care
> about wait_for_dump_helpers() ? Let them all fail, suspend is not
> that often.
>

I agree. This does seem like a minor problem since its likely very
rare. Probably not worth fixing unless there is an elegant,
non-invasive solution.

As long as suspend is reliable, a very small chance of a truncated
core dumps is OK.

Regards,
Mandeep

> Or we should try to make everything freezer-friendly. But if
> freeze_task() sets TIF_SIGPENDING then we need the ugly "retry"
> logic in dump_write()... Not good.
>
> Thanks Mandeep. If you have other ideas please tell me ;)
>
> Oleg.
>

Re: [PATCH -mm 0/3] coredump: make it freezable (almost)

[Oleg Nesterov]

On 02/25, Andrew Morton wrote:
>
> On Sun, 24 Feb 2013 18:31:44 +0100
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Hello.
> >
> > On top of coredump-sanitize-the-setting-of-signal-group_exit_code.patch
> >
> > Andrew, "almost" means we need a bit more changes, but these
> > changes should be absolutely trivial/straightforward.
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Andrew, I lied :/

> Thanks.  I still have these marked "for 3.10".
>
> coredump-only-sigkill-should-interrupt-the-coredumping-task.patch
> coredump-ensure-that-sigkill-always-kills-the-dumping-thread.patch
> coredump-sanitize-the-setting-of-signal-group_exit_code.patch

Yes, thanks. But please drop the last series:

> coredump-factor-out-the-setting-of-pf_dumpcore.patch
> freezer-do-not-send-a-fake-signal-to-a-pf_dumpcore-thread.patch
> coredump-make-wait_for_dump_helpers-freezable.patch

As Mandeep pointed out (thanks!), this is not enough to make
dump_write/etc freezer-friendly.

I'll send another version.

Oleg.

Re: [PATCH 3/3] coredump: make wait_for_dump_helpers() freezable

[Mandeep Singh Baines]

On Sun, Feb 24, 2013 at 9:32 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> wait_for_dump_helpers() calls wake_up/kill_fasync from inside the
> wait_event-like loop. This is not needed and in fact this is not
> strictly correct, we can/should do this only once after we change
> pipe->writers. We could even check if it becomes zero.
>
> With this change it is trivial to convert this code to use
> wait_event_freezable() and make this function freezable/killable,
> only SIGKILL can set TIF_SIGPENDING.
>
> With this patch we check pipe->readers without pipe_lock(), this
> is fine. Once we see pipe->readers == 1 we know that the handler
> decremented the counter, this is all we need.
>
> Note: wait_event_freezable() is "strange", perhaps it should be
> changed or simply removed. In the latter case we can change this
> code again to use freezer_do_not_count + wait_event_interruptible.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

> ---
>  fs/coredump.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 4f3c8d1..284d841 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>
> @@ -428,17 +429,16 @@ static void wait_for_dump_helpers(struct file *file)
>         pipe_lock(pipe);
>         pipe->readers++;
>         pipe->writers--;
> +       wake_up_interruptible_sync(&pipe->wait);
> +       kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
> +       pipe_unlock(pipe);
>
> -       while ((pipe->readers > 1) && (!signal_pending(current))) {
> -               wake_up_interruptible_sync(&pipe->wait);
> -               kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
> -               pipe_wait(pipe);
> -       }
> +       wait_event_freezable(pipe->wait, pipe->readers == 1);
>
> +       pipe_lock(pipe);
>         pipe->readers--;
>         pipe->writers++;
>         pipe_unlock(pipe);
> -
>  }
>
>  /*
> --
> 1.5.5.1
>