Messages in this thread | | | Date | Thu, 24 Apr 2014 15:58:53 +0900 | From | Takao Indoh <> | Subject | Re: ftrace/kprobes: Warning when insmod two modules |
| |
(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
| |