Re: [PATCH] signals: Generate warning when flush_signals() is called from non-kthread context

[Oleg Nesterov <oleg@redhat.com>]

On 05/01, Ingo Molnar wrote:
>
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > On Fri, May 1, 2015 at 10:40 AM, Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > >
> > > - Flush signals on interrupted wait to retain polling interval (Alex Williamson)
> >
> > This cannot *possibly* be right. If I read this patch right, you're
> > randomly just getting rid of signals. No way in hell is that correct.
> >
> > "flush_signals()" is only for kernel threads, where it's a hacky
> > alternative to actually handling them (since kernel threads never
> > rreturn to user space and cannot really "handle" a signal). But you're
> > doing it in the ->remove handler for the device, which can be called
> > by arbitrary system processes. This is not a kernel thread thing, as
> > far as I can see.
> >
> > If you cannot handle signals, you damn well shouldn't be using
> > "wait_event_interruptible_timeout()" to begin with. Get rid of the
> > "interruptible", since it apparently *isn't* interruptible.
> >
> > So I'm not pulling this.
> >
> > Now I'm worried that other drivers do insane things like this. I
> > wonder if we should add some sanity test to flush_signals() to make
> > sure that it can only ever get called from a kernel thread.
> >
> > Oleg?
>
> So there are these uses:
>
>   triton:~/tip> git grep -lw flush_signals
>   arch/arm/common/bL_switcher.c
>
>
> Looks safe: used within the bL_switcher_thread() kthread.

safe but wrong at first glance... I mean, it is pointless. Looks like,
bL_switcher_thread() wrongly thinks that wait_event_interruptible() can
lead to signal_pending().

>   drivers/block/drbd/drbd_main.c
>   drivers/block/drbd/drbd_nl.c
>   drivers/block/drbd/drbd_receiver.c
>   drivers/block/drbd/drbd_worker.c


Oh, I didn't know this helper is abused so much. I'll try to recheck
tomorrow, but it seems that it should die...


>   drivers/md/md.c

I can't understand this code... The comment says:

	/* We need to wait INTERRUPTIBLE so that
	 * we don't add to the load-average.
	 * That means we need to be sure no signals are
	 * pending
	 */
	if (signal_pending(current))
		flush_signals(current);

and this is wrong. However, signal_pending() can be true because of
allow_signal(SIGKILL) above. But why it does allow_signal() ?


>   fs/lockd/svc.c
>   fs/nfs/callback.c
>   fs/nfsd/nfssvc.c
>
> Looks safe: lockd, nfsd plus nfsv4.%u-svc kthreads.

Yes, this case looks fine. But perhaps it makes sense to add another
helper... Or not, I'll try to think more.

> I also found a __flush_signals() use in:
>
>   security/selinux/hooks.c
>
> Now that's selinux_bprm_committed_creds(), apparently executed on
> exec(). Also does stuff like:
>
>                 memset(&itimer, 0, sizeof itimer);
>                 for (i = 0; i < 3; i++)
>                         do_setitimer(i, &itimer, NULL);
>
> and unblocks signals as well:
>
>                         sigemptyset(&current->blocked);
>
> but this appears to be kind of legit: the task failed to get the
> required permissions, and guns go off.

and I simply can't understand this code... but I feel that it can't
be correct ;) Will try to re-read tomorrow.


> In any case, it seems to me that the patch below would be justified?
> Totally untested and so. __flush_signals() not affected.

Yes, agreed, it can't be right if the caller is not kthread.


> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -427,6 +427,10 @@ void flush_signals(struct task_struct *t)
>  {
>  	unsigned long flags;
>
> +	/* Only kthreads are allowed to destroy signals: */
> +	if (WARN_ON_ONCE(!(current->flags & PF_KTHREAD)))
> +		return;
> +

But I am not sure this can't make some buggy driver even more buggy.
Just suppose it does something

	do {
		if (signal_pending())
			flush_signals();
	} while (wait_event_interruptible(...));

and this change will turn this into busy-wait loop.

So perhaps another change which just adds WARN_ON_RATELIMIT() without
"return" will be safer?

Oleg.