Peter Zijlstra <pet...@infradead.org> 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] [<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.
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
