From patchwork Fri May 1 19:38:13 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ingo Molnar X-Patchwork-Id: 6312651 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 265799F3E9 for ; Fri, 1 May 2015 19:39:13 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 30062203EB for ; Fri, 1 May 2015 19:39:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1A8012041E for ; Fri, 1 May 2015 19:39:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752547AbbEATiY (ORCPT ); Fri, 1 May 2015 15:38:24 -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 Received: by widdi4 with SMTP id di4so56697864wid.0; Fri, 01 May 2015 12:38:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=FNKw0BapfXfAMLNz5sG4NAcKtZm1C/Fy7Q5L8qKnTPg=; b=uXENaSWYZj9IXixeNEMbjqyx05wlQ5AKQgch9JagfqhXwSM+L7DG0DYy1A7wTs3cdf 2XnFwBA8As6A7ePFnKXCO+2eWqthZMBNK6cTcYq43uznmmAbnlzNhTL8EEdfJbB990U2 nsfC0TZg2Lv4ZLRvSzBKqypIwJgnF6KBpAIhmkDkTZ7niA6DxszvwdD3BGqS61w9BFqX g70fhmng6sTrW/vtwXxE0v9kMuCob1n+1nDw04p3DOVI7mXZeeaQS728YMWDXobMg4jq b3owLSxYOHJ74cM6tfN72EXkgUNQBxl7Atq2jOzJgrTCpYUGkoiVaZ23YmwrfLZcn+YD aRyg== X-Received: by 10.194.187.243 with SMTP id fv19mr20855003wjc.26.1430509098819; Fri, 01 May 2015 12:38:18 -0700 (PDT) Received: from gmail.com (254C417F.nat.pool.telekom.hu. [37.76.65.127]) by mx.google.com with ESMTPSA id ao4sm8818844wjc.14.2015.05.01.12.38.16 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 01 May 2015 12:38:17 -0700 (PDT) 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-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, FSL_HELO_FAKE,RCVD_IN_DNSWL_HI,T_DKIM_INVALID,T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP * 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(+) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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);