Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752159AbbHRCbn (ORCPT <rfc822;w@1wt.eu>);
	Mon, 17 Aug 2015 22:31:43 -0400
Received: from casper.infradead.org ([85.118.1.10]:41444 "EHLO
	casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751118AbbHRCbm (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 17 Aug 2015 22:31:42 -0400
Date: Tue, 18 Aug 2015 04:31:37 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Laura Abbott <labbott@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: 0be964be0 "module: Sanitize RCU usage and locking" breaks
 symbol_put_addr?
Message-ID: <20150818023137.GI20948@worktop>
References: <55D26C29.5040204@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <55D26C29.5040204@redhat.com>
User-Agent: Mutt/1.5.22.1 (2013-10-16)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2700
Lines: 67

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]  [<ffffffff81150529>] module_assert_mutex_or_preempt+0x49/0x90
> [   41.550664]  [<ffffffff81150822>] __module_address+0x32/0x150
> [   41.552684]  [<ffffffff81150956>] __module_text_address+0x16/0x70
> [   41.554701]  [<ffffffff81150f19>] symbol_put_addr+0x29/0x40
> [   41.555392]  [<ffffffffa04b77ad>] 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.

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/