Re: ftrace/kprobes: Warning when insmod two modules

[Takao Indoh <indou.takao@jp.fujitsu.com>]

(2014/04/23 11:37), Masami Hiramatsu wrote:
> (2014/04/23 10:56), Steven Rostedt wrote:
>> On Wed, 23 Apr 2014 10:26:00 +0900
>> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>>
>>
>>> Agreed. That should be done in a protected (critical) region,
>>> and the region must be protected by correct lock. It seems that
>>> the ftrace_lock is not a correct one.
>>
>> The setting of RO to RW done by ftrace before doing the normal
>> modification is under the ftrace_lock mutex. Why wouldn't that be the
>> correct lock?
> 
> Hmm, Ok. I checked that currently ftrace is the only user of
> set_all_modules_text_rw(), so until another user appears,
> ftrace_lock mutex can work.  (and also, we need a comment
> on the top of such functions, about by what it is protected. )
> 
>> The issue today is with the loading of a module and ftrace
>> expecting its code to be RW. Here's the current race:
>>
>>
>> 	CPU 1				CPU 2
>> 	-----				-----
>>    load_module()
>>     module->state = MODULE_STATE_COMING
>>
>> 				register_ftrace_function()
>> 				 mutex_lock(&ftrace_lock);
>> 				 ftrace_startup()
>> 				  update_ftrace_function();
>> 				   ftrace_arch_code_modify_prepare()
>> 				    set_all_module_text_rw();
>> 				   <enables-ftrace>
>> 				    ftrace_arch_code_modify_post_process()
>> 				     set_all_module_text_ro();
>>
>> 				[ here all module text is set to RO,
>> 				  including the module that is
>> 				  loading!! ]
>>
>>     blocking_notifier_call_chain(MODULE_STATE_COMING);
>>      ftrace_init_module()
>>
>>
>>       [ tries to modify code, but it's RO, and fails! ]
>>
>> One solution is to add a way to set a single module text to ro and rw,
>> and then we can encapsulate ftrace_init_module() under ftrace_lock
>> mutex and have the ftrace_init_module() set the text to RW and then
>> back to RO, and this will keep ftrace from having issues with the
>> loaded module.
> 
> It sounds nicer solution, less side-effect.
> 
>> Now, if text poke does something similar, we need to make another mutex
>> that covers modifying text. Don't we have one already?
> 
> We have the text_mutex already :).
> 
>> The worry I have here, and why I still prefer the simple split state of
>> MODULE_STATE_COMING, is that once you add another mutex, we now have to
>> fight mutex ordering. Not to mention where else things might do this :-p
> 
> I see, however, we should take care of it, at least comment level.

Ok, I'll do this. Something like this, right?

static void ftrace_init_module(struct module *mod,
                               unsigned long *start, unsigned long *end)
{
	if (ftrace_disabled || start == end)
		return;

	/*
	 * Need ftrace_lock here to prevent someone from changing the module
	 * text to RO by set_all_modules_text_ro(). Currently ftrace is the
	 * only user of set_all_modules_text_ro(), so until another user
	 * appears, ftrace_lock mutex can work.
	 */
	mutex_lock(&ftrace_lock);

	set_one_module_text_rw(mod);
	ftrace_process_locs(mod, start, end);
	set_one_module_text_ro(mod);

	mutex_unlock(&ftrace_lock);
}

Thanks,
Takao Indoh