Re: user workers as coredumpers [Re: [PATCH 1/6] coredump: don't switch a dumper that has no files] table

[Oleg Nesterov <oleg@redhat.com>]

On 09/18, Christian Brauner wrote:
>
> So I thought a bit about this yesterday and had some brief discussions
> bout this as well. I think we should kill this whole bug class by
> ensuring that PF_USER_WORKERs never participate in a coredump.

Agreed,

> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3021,6 +3021,21 @@ bool get_signal(struct ksignal *ksig)
>  		 */
>  		current->flags |= PF_SIGNALED;
>
> +		/*
> +		 * PF_USER_WORKER threads will catch and exit on fatal signals
> +		 * themselves. They have cleanup that must be performed, so we
> +		 * cannot call do_exit() on their behalf. Note that ksig won't
> +		 * be properly initialized, PF_USER_WORKER's shouldn't use it.
> +		 *
> +		 * They must not dump core either. The dumper waits for every
> +		 * other thread in the group to exit, and a sibling's exit path
> +		 * may in turn wait for this worker's own exit, which only runs
> +		 * after get_signal() returns: io_sq_thread() ends up in
> +		 * io_wq_exit_workers() and waits there for its io-wq workers.
> +		 */
> +		if (current->flags & PF_USER_WORKER)
> +			goto out;

This probably makes sense anyway... but see below.

Perhaps it makes sense to change ptrace(PTRACE_SETSIGMASK) to fail if
child->flags & PF_USER_WORKER ?

Currently get_signal() from PF_USER_WORKER can only return SIGKILL,
I think we should keep this rule.



I don't think your change can fix all problems. Suppose we have a main
thread T and a PF_USER_WORKER sub-thread W.

Some signal, say, SIGHUP has a handler. Debugger unblocks SIGHUP for W.

Now, kill(SIGHUP, T) can choose W as a target, complete_signal() can
choose any thread if wants_signal(t) is true. W will "drop" this signal,
its signal handler won't be called.


In the longer term it would be nice to rework this logic somehow to not
rely on ->blocked...

Oleg.