Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752200AbbHSDM4 (ORCPT ); Tue, 18 Aug 2015 23:12:56 -0400 Received: from ozlabs.org ([103.22.144.67]:35840 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751435AbbHSDMz (ORCPT ); Tue, 18 Aug 2015 23:12:55 -0400 From: Rusty Russell To: Peter Zijlstra , Laura Abbott Cc: "Paul E. McKenney" , Linux Kernel Mailing List Subject: Re: 0be964be0 "module: Sanitize RCU usage and locking" breaks symbol_put_addr? In-Reply-To: <20150818023137.GI20948@worktop> References: <55D26C29.5040204@redhat.com> <20150818023137.GI20948@worktop> User-Agent: Notmuch/0.17 (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Wed, 19 Aug 2015 06:19:43 +0930 Message-ID: <87lhd8uxso.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2935 Lines: 76 Peter Zijlstra writes: > On Mon, Aug 17, 2015 at 04:20:09PM -0700, Laura Abbott wrote: >> Hi, >> >> We received a few bug backtraces: >> >> [ 41.536933] ------------[ cut here ]------------ >> [ 41.537545] WARNING: CPU: 1 PID: 813 at kernel/module.c:291 module_assert_mutex_or_preempt+0x49/0x90() >> [ 41.538174] Modules linked in: mxl5007t af9013 ... dvb_usb_af9015(+) ... dvb_usb_v2 dvb_core rc_core ... >> [ 41.542457] CPU: 1 PID: 813 Comm: systemd-udevd Not tainted 4.2.0-0.rc6.git0.1.fc24.x86_64+debug #1 >> ... >> [ 41.549994] [] module_assert_mutex_or_preempt+0x49/0x90 >> [ 41.550664] [] __module_address+0x32/0x150 >> [ 41.552684] [] __module_text_address+0x16/0x70 >> [ 41.554701] [] symbol_put_addr+0x29/0x40 >> [ 41.555392] [] dvb_frontend_detach+0x7d/0x90 [dvb_core] > >> Based on my understanding, this is spitting a warning that the module_mutex >> isn't held. There's a nice comment in symbol_put_addr right before the call: >> >> /* module_text_address is safe here: we're supposed to have reference >> * to module from symbol_get, so it can't go away. */ >> modaddr = __module_text_address(a); >> >> so it looks like this is an erroneous warning which shouldn't need the mutex held. >> Any ideas or am I off base here? > > That comment is wrong, you still need either preempt disabled or hold > the module mutex because you're going to iterate the data structure in > order to look up the module. Indeed! That comment is wrong, and your fix is good. Care to S-O-B on it? Thanks, Rusty. > > The fact that you hold a reference on the module you're going to > (hopefully) find, doesn't stabilize the data structure. > > So I think maybe symbol_put_addr() is broken and it wants something > like: > > --- > kernel/module.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/kernel/module.c b/kernel/module.c > index b86b7bf1be38..8f051a106676 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -1063,11 +1063,15 @@ void symbol_put_addr(void *addr) > if (core_kernel_text(a)) > return; > > - /* module_text_address is safe here: we're supposed to have reference > - * to module from symbol_get, so it can't go away. */ > + /* > + * Even though we hold a reference on the module; we still need to > + * disable preemption in order to safely traverse the data structure. > + */ > + preempt_disable(); > modaddr = __module_text_address(a); > BUG_ON(!modaddr); > module_put(modaddr); > + preempt_enable(); > } > EXPORT_SYMBOL_GPL(symbol_put_addr); > -- 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/