(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...@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
--
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/
