Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751307AbbECReg (ORCPT ); Sun, 3 May 2015 13:34:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59835 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750918AbbECRe2 (ORCPT ); Sun, 3 May 2015 13:34:28 -0400 Date: Sun, 3 May 2015 19:34:02 +0200 From: Oleg Nesterov To: Ingo Molnar Cc: Linus Torvalds , Neil Brown , Evgeniy Polyakov , Stephen Smalley , Alex Williamson , linux-kernel , kvm Subject: Re: [PATCH] signals: Generate warning when flush_signals() is called from non-kthread context Message-ID: <20150503173401.GA22052@redhat.com> References: <1430502057.4472.255.camel@redhat.com> <20150501193813.GA2812@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150501193813.GA2812@gmail.com> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4231 Lines: 137 On 05/01, Ingo Molnar wrote: > > * 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. 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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/