From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752584AbbEATiZ (ORCPT ); Fri, 1 May 2015 15:38:25 -0400 Received: from mail-wi0-f175.google.com ([209.85.212.175]:36882 "EHLO mail-wi0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750951AbbEATiU (ORCPT ); Fri, 1 May 2015 15:38:20 -0400 Date: Fri, 1 May 2015 21:38:13 +0200 From: Ingo Molnar To: Linus Torvalds , Neil Brown , Evgeniy Polyakov , Stephen Smalley Cc: Alex Williamson , Oleg Nesterov , linux-kernel , kvm Subject: [PATCH] signals: Generate warning when flush_signals() is called from non-kthread context Message-ID: <20150501193813.GA2812@gmail.com> References: <1430502057.4472.255.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Linus Torvalds wrote: > On Fri, May 1, 2015 at 10:40 AM, Alex Williamson > 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);