Message-ID: <20150501193813.GA2812@gmail.com>
Date:	Fri, 1 May 2015 21:38:13 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Neil Brown <neilb@...e.de>, Evgeniy Polyakov <zbr@...emap.net>,
	Stephen Smalley <sds@...ho.nsa.gov>
Cc:	Alex Williamson <alex.williamson@...hat.com>,
	Oleg Nesterov <oleg@...hat.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	kvm <kvm@...r.kernel.org>
Subject: [PATCH] signals: Generate warning when flush_signals() is called
 from non-kthread context


* Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Fri, May 1, 2015 at 10:40 AM, Alex Williamson
> <alex.williamson@...hat.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.

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

Couldn't convince myself it's safe, but it appears to be. (Call chains 
are obfuscated in various ways that makes it hard to tell where a 
given function execute.)

  drivers/md/md.c
  drivers/md/raid1.c
  drivers/md/raid5.c

Hm, so I'm not super sure about the flush_signals() in 
raid1.c:make_request() AFAICS we can do direct RAID1 writes in 
raid1_unplug(). That looks unsafe ... I've Cc:-ed Neil.

raid5.c seems safe: raid5_unplug() doesn't create requests directly, 
leaves it all for the mddev kthread.

  drivers/scsi/bnx2fc/bnx2fc_fcoe.c
  drivers/scsi/bnx2fc/bnx2fc_tgt.c
  drivers/scsi/bnx2i/bnx2i_iscsi.c
  drivers/scsi/libiscsi.c
  drivers/target/iscsi/iscsi_target_login.c
  drivers/target/iscsi/iscsi_target_nego.c

Couldn't fully check it due to excessive complexity, but seemed safe.

  drivers/staging/rtl8188eu/core/rtw_cmd.c
  drivers/staging/rtl8712/osdep_service.h

Looks safe: done in RTW_CMD_THREAD and 'padapter' kthreads.

  drivers/w1/w1_family.c
  drivers/w1/w1_int.c

Looks unsafe: called from various module exit handlers in:

  drivers/w1/slaves/w1_bq27000.c
  drivers/w1/slaves/w1_ds2406.c
  drivers/w1/slaves/w1_ds2408.c
  drivers/w1/slaves/w1_ds2413.c
  drivers/w1/slaves/w1_ds2423.c
  drivers/w1/slaves/w1_ds2431.c
  drivers/w1/slaves/w1_ds2433.c
  drivers/w1/slaves/w1_ds2760.c
  drivers/w1/slaves/w1_ds2780.c
  drivers/w1/slaves/w1_ds2781.c
  drivers/w1/slaves/w1_ds28e04.c
  drivers/w1/slaves/w1_smem.c
  drivers/w1/slaves/w1_therm.c

which would be executed in rmmod context, losing signals.
Cc:-ed Evgeniy.

  fs/lockd/svc.c
  fs/nfs/callback.c
  fs/nfsd/nfssvc.c

Looks safe: lockd, nfsd plus nfsv4.%u-svc kthreads.

  kernel/locking/rtmutex-tester.c

Looks safe: used within a kthread.

  include/linux/sched.h
  kernel/signal.c

Both safe ;-)

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.

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

Thanks,

	Ingo

---
 kernel/signal.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/signal.c b/kernel/signal.c
index d51c5ddd855c..100e30afe5d2 100644
--- 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;
+
 	spin_lock_irqsave(&t->sighand->siglock, flags);
 	__flush_signals(t);
 	spin_unlock_irqrestore(&t->sighand->siglock, flags);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists