Messages in this thread Patch in this message | | | Date | Fri, 1 May 2015 21:38:13 +0200 | From | Ingo Molnar <> | Subject | [PATCH] signals: Generate warning when flush_signals() is called from non-kthread context |
| |
* 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.
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(¤t->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);
| |