[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87lhd8uxso.fsf@rustcorp.com.au>
Date: Wed, 19 Aug 2015 06:19:43 +0930
From: Rusty Russell <rusty@...tcorp.com.au>
To: Peter Zijlstra <peterz@...radead.org>,
Laura Abbott <labbott@...hat.com>
Cc: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: 0be964be0 "module: Sanitize RCU usage and locking" breaks symbol_put_addr?
Peter Zijlstra <peterz@...radead.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 majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists