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

[Rusty Russell <rusty@rustcorp.com.au>]

Peter Zijlstra <peterz@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);
>