On 05/01, Ingo Molnar wrote:
>
> * Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> > On Fri, May 1, 2015 at 10:40 AM, Alex Williamson
> > <alex.williamson@xxxxxxxxxx> 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(¤t->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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
