From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752163AbbEBMGb (ORCPT ); Sat, 2 May 2015 08:06:31 -0400 Received: from forward20m.cmail.yandex.net ([5.255.216.151]:35450 "EHLO forward20m.cmail.yandex.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751159AbbEBMG2 (ORCPT ); Sat, 2 May 2015 08:06:28 -0400 X-Greylist: delayed 578 seconds by postgrey-1.27 at vger.kernel.org; Sat, 02 May 2015 08:06:27 EDT From: Evgeniy Polyakov Envelope-From: drustafa@yandex.ru To: Ingo Molnar , Linus Torvalds , Neil Brown , Stephen Smalley Cc: Alex Williamson , Oleg Nesterov , linux-kernel , kvm In-Reply-To: <20150501193813.GA2812@gmail.com> References: <1430502057.4472.255.camel@redhat.com> <20150501193813.GA2812@gmail.com> Subject: Re: [PATCH] signals: Generate warning when flush_signals() is called from non-kthread context MIME-Version: 1.0 Message-Id: <8198211430567803@web26m.yandex.ru> X-Mailer: Yamail [ http://yandex.ru ] 5.0 Date: Sat, 02 May 2015 14:56:43 +0300 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=koi8-r Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ingo 01.05.2015, 22:38, "Ingo Molnar" : > * 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. > 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. w1 uses a little bit strange refcnt logic, basically every w1 module waits for all its users to release their w1 resources and doesn't exit until its safe. To wait for resources to be freed at module exit time it checks its refcnt to drop to zero periodically and sleeps between the checks for a second. w1 flushes signals between these checks for no particular reason, it can be safely removed from w1_unregister_family() and interruptible sleep replaced with the normal one.