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

[Oleg Nesterov <oleg@redhat.com>]

On 02/16, Mandeep Singh Baines wrote:
>
> 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 &

As I already said, this is not enough. And if we change send_signal() paths
to "ignore" the non-fatal signals sent to the dumping process (and I think
we should do this anyway), this change is not needed.

Except we have other problems with the freezer.

> +static int sigkill_pending(struct task_struct *tsk)
> +{
> +	return	signal_pending(tsk) &&
> +		(sigismember(&tsk->pending.signal, SIGKILL) ||
> +		 sigismember(&tsk->signal->shared_pending.signal, SIGKILL));
> +}

Why? __fatal_signal_pending() is enough, you do not need to check
->shared_pending. And once again, ignoring the freezer problems I
do not think we need this check at all.

IOW. Yes, we will probably need to do this change but only to be
freezer-friendly.

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

(sigprocmask() must die, please never use, we have set_current_blocked().
 Although in this particular case this doesn't matter...)

Heh. When I suggested this change a looong ago, my attempt was NACK'ed
because the core handler looks at /proc/pid/status.

If we could do this, we could simply ignore all signals except SIGKILL
at the start, in zap_threads(). This could solve almost all problems
with the signals/SIGKILL.

But see above, we can't.

Anyway. Please look at the patch below. I need to recheck it, and I was
going to send it later, along with other changes I am _trying_ to do. But
if it is correct, it looks certainly better and perhaps it can go ahead.

Afaics it could equally fix the mentioned problems (again, if correct ;).
"equally" also means it is equally incomplete.

Oleg.

--- x/fs/coredump.c
+++ x/fs/coredump.c
@@ -416,17 +416,17 @@ static void wait_for_dump_helpers(struct
 	pipe_lock(pipe);
 	pipe->readers++;
 	pipe->writers--;
+	// TODO: wake_up_interruptible_sync_poll ?
+	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_freezekillable(&pipe->wait, pipe->readers == 1);
 
+	pipe_lock(pipe);
 	pipe->readers--;
 	pipe->writers++;
 	pipe_unlock(pipe);
-
 }
 
 /*