Re: 0be964be0 "module: Sanitize RCU usage and locking" breaks symbol_put_addr?

[Peter Zijlstra]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/