From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932277AbcHIMRz (ORCPT ); Tue, 9 Aug 2016 08:17:55 -0400 Received: from smtprelay0250.hostedemail.com ([216.40.44.250]:59274 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752046AbcHIMRw (ORCPT ); Tue, 9 Aug 2016 08:17:52 -0400 X-Session-Marker: 726F737465647440676F6F646D69732E6F7267 X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::,RULES_HIT:41:355:379:541:599:800:960:966:973:988:989:1260:1277:1311:1313:1314:1345:1359:1437:1515:1516:1518:1534:1542:1593:1594:1711:1730:1747:1777:1792:2196:2199:2393:2553:2559:2562:2692:2693:3138:3139:3140:3141:3142:3355:3622:3865:3866:3867:3868:3870:3871:3872:3873:3874:4385:4559:5007:6261:7875:8660:9040:10004:10400:10848:10967:11232:11658:11914:12296:12517:12519:12740:13148:13200:13229:13230:13439:14093:14096:14097:14181:14659:14721:21080:21433:30012:30054:30060:30070:30090:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:4,LUA_SUMMARY:none X-HE-Tag: magic52_a27dd8ed5001 X-Filterd-Recvd-Size: 3453 Date: Tue, 9 Aug 2016 08:17:48 -0400 From: Steven Rostedt To: Miroslav Benes Cc: Linux Kernel Mailing List Subject: Re: A bug in ftrace - dynamic fops Message-ID: <20160809081748.5c8e7d1b@gandalf.local.home> In-Reply-To: References: <20160808105303.16bf85ed@gandalf.local.home> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 9 Aug 2016 10:16:00 +0200 (CEST) Miroslav Benes wrote: > I agree it is kind of shooting oneself in the foot bug, because explicit > call to a sleeping function may not be the brightest thing to do. However > I see two (closely related) issues with this. > > 1. It is a change in behaviour. Ftrace silently relies on an atomicity of > ops->func(). I don't see it documented anywhere, but it did not matter > because the atomicity was always guaranteed as described above. Now there > is a possibility to achieve a situation which breaks the assumption. It > makes me worried. Why? It's something that a kernel developer should be aware of. I mean, that ops->func can easily be called from *any* context, like irq, softirq, or even an NMI. One who hooks into any function of the kernel should understand that it has special requirements, just like we don't document that you can't sleep in an NMI. And if you only hook to functions that can sleep, then great! You are allowed to do that too. Just like calling a module function that can sleep. You need to make sure nothing is calling your function when you unload the module. I don't see anything that is deceptive here. > > 2. Previously if someone called a function which could sleep he was > immediately warned not to do so via "sleeping in atomic context" BUG. Now > he wouldn't know. That's because in_atomic() and might_sleep() > infrastructure does not work in ops->func(). in_atomic() gives 0 even if > it is an atomic context in fact. But well, the comment for in_atomic() in > linux/preempt.h warns about exactly this situation I guess. It will warn if you hook to a function that can sleep. And if you never do, then there's nothing wrong. If the only functions you hook to can sleep, then it is fine for you to sleep in your code too. But if you do, you must synchronize that logic. You must make sure all functions are out of the sleep when you unresgister. Just like you must make sure all functions are out of a sleeping function in a module. This is kernel programming 101. I never saw a need to have sleeping functions being called by ops->func() and I don't know of a case that would. If there is a legitimate case (not hypothetical) and then I could add a way to postpone freeing of an ops if need be. Because note, that TASK_RCU will only be called when CONFIG_PREEMPT is enabled. It would be overkill to do it for !CONFIG_PREEMPT, thus it will not solve what you want here. -- Steve