Re: [PATCH v3 16/17] signal: enforce the user worker signal mask in __set_task_blocked()

[Oleg Nesterov <oleg@redhat.com>]

On 09/21, Christian Brauner wrote:
>
> The in-kernel sigprocmask() users block more signals for a critical
> section and restore the saved mask afterwards. cifs used to do that in
> __smb_send_rqst() and ocfs2 in ocfs2_block_signals(). Both were
> reachable from an io-wq worker. Both only add SIGKILL and SIGSTOP to the
> user workers's and then put the fork-time mask back.

Ooh, this reminds me... see below.

> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3212,6 +3212,16 @@ long do_no_restart_syscall(struct restart_block *param)
>
>  static void __set_task_blocked(struct task_struct *tsk, const sigset_t *newset)
>  {
> +	sigset_t floor, floored;
> +
> +	/* A user worker never unblocks anything but SIGKILL and SIGSTOP. */
> +	if (unlikely(tsk->flags & PF_USER_WORKER)) {
> +		siginitsetinv(&floor, SIG_KERNEL_ONLY_MASK);
> +		sigorsets(&floored, newset, &floor);
> +		WARN_ON_ONCE(!sigequalsets(&floored, newset));
> +		newset = &floored;
> +	}

OK, Acked-by: Oleg Nesterov <oleg@redhat.com>

although to me something like a more simple version

	/* A user worker never unblocks anything but SIGKILL and SIGSTOP. */
	if (unlikely(tsk->flags & PF_USER_WORKER)) {
		sigset_t xxx;
		siginitsetinv(&xxx, SIG_KERNEL_ONLY_MASK);
		if (WARN_ON_ONCE(has_pending_signals(&xxx, newset)))
			return;
	}

makes more sense. But this is minor.

In fact I think that __set_task_blocked() should simply do

	if (WARN_ON_ONCE(tsk->flags & PF_USER_WORKER))
		return;

but yes, we can't do this right now, we have in-kernel abusers of sigprocmask().

IMO, they should be changed to not rely on sigprocmask(). Lets look at
ocfs2_delete_inode() for example,

	/* We want to block signals in delete_inode as the lock and
	 * messaging paths may return us -ERESTARTSYS. Which would
	 * cause us to exit early, resulting in inodes being orphaned
	 * forever. */
	ocfs2_block_signals(&oldset);

ocfs2_block_signals() blocks everything including SIGKILL and SIGSTOP.
I guess to protect against signal_wake_up / TIF_SIGPENDING ?

But this can only help in the single-threaded case. And PF_USER_WORKER's
are never single-threaded.

Blocking SIGSTOP can't protect from SIGSTOP if another thread dequeues
SIGSTOP or another sig_kernel_stop() signal. This another thread will do
do_signal_stop() -> signal_wake_up().

Same for SIGKILL...

In short, I agree with this patch, but mostly because of WARN_ON_ONCE()
it adds.

Oleg.