Re: ftrace/kprobes: Warning when insmod two modules
On Thu, 24 Apr 2014 15:58:53 +0900
Takao Indoh wrote:
> 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(_lock);
>
> set_one_module_text_rw(mod);
> ftrace_process_locs(mod, start, end);
> set_one_module_text_ro(mod);
>
> mutex_unlock(_lock);
> }
>
I like Rusty's solution the best. Just hard code the call to ftrace's
module init code where it is still safe to do so
(MODULE_STATE_UNFORMED). This seems to be an ftrace only issue.
Thanks,
-- Steve
--
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/
Re: ftrace/kprobes: Warning when insmod two modules
On Thu, 24 Apr 2014 17:08:56 +0930 Rusty Russell wrote: > OTOH, if it's just ftrace (do tracepoints have an issue?) I'd rather > hardcode a ftrace_init_module() call in exactly the right place. > Notifiers which are sensitive to their exact call location tend give me > the creeps... I think I like this solution the best. I believe it was the original solution for ftrace until we realized that it can be also done by a notifier. It also makes it more in line with what the core kernel does, as I considered notifiers similar to initcalls and the init code for ftrace is hard coded in init/main.c and not done by initcalls, as it is important to be done before anything else. Yeah, a ftrace_init_module() hard coded in where the module state is still MODULE_STATE_UNFORMED, would work. -- Steve -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
Steven Rostedt writes: > On Tue, 22 Apr 2014 13:21:18 +0930 > Rusty Russell wrote: > > >> Sorry, was on paternity leave. > > Congratulations! Although having two teenage daughters myself, I'm more > inclined to say "my condolences". Thanks... I think! >> I'm always nervous about adding more states, since every place which >> examines the state has to be audited. > > I didn't really add a state but instead broke an existing state into > two. More importantly, this second part of the state doesn't get > exported to other parts of the kernel. That is, there is no notifier > for it. > > This means the only place you need to audit is in module.c itself. Any > other change requires auditing all module notifiers. Yes, we only need to check everywhere which looks at mod->state. >> We set the mod->state to MOD_STATE_COMING in complete_formation; >> why don't we set NX there instead? It also makes more sense to >> set NX before we hit parse_args() which can execute code in the module. > > Well, NX is a different issue here all together. Sure if you want to, > but that wont affect this. RO and NX get set together in the module code, but you're right, it's the RO which affects you. >> +/* Set RO and NX regions for core */ >> +set_section_ro_nx(mod->module_core, >> +mod->core_text_size, >> +mod->core_ro_size, >> +mod->core_size); >> + >> +/* Set RO and NX regions for init */ >> +set_section_ro_nx(mod->module_init, >> +mod->init_text_size, >> +mod->init_ro_size, >> +mod->init_size); >> + >> /* Mark state as coming so strong_try_module_get() ignores us, >> * but kallsyms etc. can see us. */ >> mod->state = MODULE_STATE_COMING; >> +mutex_unlock(_mutex); >> + >> +blocking_notifier_call_chain(_notify_list, >> + MODULE_STATE_COMING, mod); > > Here we break ftrace. ftrace expects that it can still replaces the > calls to mcount with nops here. If the text is RO, then it will crash. I think we should apply a patch like the above for other reasons, so... What if we call the notifier before setting ro_nx, and then set the state after the notifier? OTOH, if it's just ftrace (do tracepoints have an issue?) I'd rather hardcode a ftrace_init_module() call in exactly the right place. Notifiers which are sensitive to their exact call location tend give me the creeps... Cheers, Rusty. -- 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/
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 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(_lock);
>> ftrace_startup()
>>update_ftrace_function();
>> ftrace_arch_code_modify_prepare()
>> set_all_module_text_rw();
>>
>> 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(_lock);
set_one_module_text_rw(mod);
ftrace_process_locs(mod, start, end);
set_one_module_text_ro(mod);
mutex_unlock(_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/
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...@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/
Re: ftrace/kprobes: Warning when insmod two modules
Steven Rostedt rost...@goodmis.org writes: On Tue, 22 Apr 2014 13:21:18 +0930 Rusty Russell ru...@rustcorp.com.au wrote: Sorry, was on paternity leave. Congratulations! Although having two teenage daughters myself, I'm more inclined to say my condolences. Thanks... I think! I'm always nervous about adding more states, since every place which examines the state has to be audited. I didn't really add a state but instead broke an existing state into two. More importantly, this second part of the state doesn't get exported to other parts of the kernel. That is, there is no notifier for it. This means the only place you need to audit is in module.c itself. Any other change requires auditing all module notifiers. Yes, we only need to check everywhere which looks at mod-state. We set the mod-state to MOD_STATE_COMING in complete_formation; why don't we set NX there instead? It also makes more sense to set NX before we hit parse_args() which can execute code in the module. Well, NX is a different issue here all together. Sure if you want to, but that wont affect this. RO and NX get set together in the module code, but you're right, it's the RO which affects you. +/* Set RO and NX regions for core */ +set_section_ro_nx(mod-module_core, +mod-core_text_size, +mod-core_ro_size, +mod-core_size); + +/* Set RO and NX regions for init */ +set_section_ro_nx(mod-module_init, +mod-init_text_size, +mod-init_ro_size, +mod-init_size); + /* Mark state as coming so strong_try_module_get() ignores us, * but kallsyms etc. can see us. */ mod-state = MODULE_STATE_COMING; +mutex_unlock(module_mutex); + +blocking_notifier_call_chain(module_notify_list, + MODULE_STATE_COMING, mod); Here we break ftrace. ftrace expects that it can still replaces the calls to mcount with nops here. If the text is RO, then it will crash. I think we should apply a patch like the above for other reasons, so... What if we call the notifier before setting ro_nx, and then set the state after the notifier? OTOH, if it's just ftrace (do tracepoints have an issue?) I'd rather hardcode a ftrace_init_module() call in exactly the right place. Notifiers which are sensitive to their exact call location tend give me the creeps... Cheers, Rusty. -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
On Thu, 24 Apr 2014 17:08:56 +0930 Rusty Russell ru...@rustcorp.com.au wrote: OTOH, if it's just ftrace (do tracepoints have an issue?) I'd rather hardcode a ftrace_init_module() call in exactly the right place. Notifiers which are sensitive to their exact call location tend give me the creeps... I think I like this solution the best. I believe it was the original solution for ftrace until we realized that it can be also done by a notifier. It also makes it more in line with what the core kernel does, as I considered notifiers similar to initcalls and the init code for ftrace is hard coded in init/main.c and not done by initcalls, as it is important to be done before anything else. Yeah, a ftrace_init_module() hard coded in where the module state is still MODULE_STATE_UNFORMED, would work. -- Steve -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
On Thu, 24 Apr 2014 15:58:53 +0900
Takao Indoh indou.ta...@jp.fujitsu.com wrote:
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);
}
I like Rusty's solution the best. Just hard code the call to ftrace's
module init code where it is still safe to do so
(MODULE_STATE_UNFORMED). This seems to be an ftrace only issue.
Thanks,
-- Steve
--
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/
Re: ftrace/kprobes: Warning when insmod two modules
(2014/04/23 10:56), Steven Rostedt wrote: > On Wed, 23 Apr 2014 10:26:00 +0900 > Masami Hiramatsu 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(_lock); >ftrace_startup() > update_ftrace_function(); > ftrace_arch_code_modify_prepare() > set_all_module_text_rw(); > > 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. Thank you, -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
On Wed, 23 Apr 2014 10:26:00 +0900 Masami Hiramatsu 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? 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(_lock); ftrace_startup() update_ftrace_function(); ftrace_arch_code_modify_prepare() set_all_module_text_rw(); 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. Now, if text poke does something similar, we need to make another mutex that covers modifying text. Don't we have one 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 -- Steve -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
(2014/04/22 17:35), Takao Indoh wrote: >>> >> But the text is already RO, so it causes panic. We need to call notifier >>> >> before setting it RO. Or should we unset RO temporarily in >>> >> ftrace_process_locs()? >> > >> > Perhaps, IMHO, ftrace needs to change the module RW in ftrace_init_module >> > and >> > makes it RO after modifying the module text. > Hmm..., I think the same problem occurs if we set module RW in > ftrace_init_module(). > > > init_module > load_module > complete_formation > set_section_ro_nx -- (1) > set_section_ro_nx -- (2) > blocking_notifier_call_chain > ftrace_module_notify_enter > ftrace_init_module - (3) > ftrace_process_locs > mutex_lock(_lock) (4) > ftrace_update_code >__ftrace_replace_code > ftrace_make_nop >ftrace_modify_code_direct > do_ftrace_mod_code >probe_kernel_write (5) > > > The text of module B is set to RO at (1) and (2) by Rusty's patch. And > even if we change it to RW at (3), it set to RO again by another module > while module B is waiting at (4). > > So, we need to set module to RW somewhere after get ftrace_lock, maybe > in ftrace_update_code()? 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. Thank you, -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
On Tue, 22 Apr 2014 13:21:18 +0930 Rusty Russell wrote: > Sorry, was on paternity leave. Congratulations! Although having two teenage daughters myself, I'm more inclined to say "my condolences". > > I'm always nervous about adding more states, since every place which > examines the state has to be audited. I didn't really add a state but instead broke an existing state into two. More importantly, this second part of the state doesn't get exported to other parts of the kernel. That is, there is no notifier for it. This means the only place you need to audit is in module.c itself. Any other change requires auditing all module notifiers. > > We set the mod->state to MOD_STATE_COMING in complete_formation; > why don't we set NX there instead? It also makes more sense to > set NX before we hit parse_args() which can execute code in the module. Well, NX is a different issue here all together. Sure if you want to, but that wont affect this. > > In fact, we should probably call the notifier there too, so people > can breakpoint/tracepoint/etc parameter calls. > > Of course, this means that we set NX before the notifier; does anything > break? > > Subject: module: set nx before marking module MODULE_STATE_COMING. > > This prevents a WARN_ON() where ftrace calls set_all_modules_text_ro() > which races with the module setting its own set_section_ro_nx(). > > It also means we're NX protected before we call parse_args(), which > can execute module code. > > This means that the notifiers will be called on a module which > is already NX, so that may cause problems. > > Signed-off-by: Rusty Russell > > diff --git a/kernel/module.c b/kernel/module.c > index 11869408f79b..83a437e5d429 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -3023,21 +3023,6 @@ static int do_init_module(struct module *mod) >*/ > current->flags &= ~PF_USED_ASYNC; > > - blocking_notifier_call_chain(_notify_list, > - MODULE_STATE_COMING, mod); > - > - /* Set RO and NX regions for core */ > - set_section_ro_nx(mod->module_core, > - mod->core_text_size, > - mod->core_ro_size, > - mod->core_size); > - > - /* Set RO and NX regions for init */ > - set_section_ro_nx(mod->module_init, > - mod->init_text_size, > - mod->init_ro_size, > - mod->init_size); > - > do_mod_ctors(mod); > /* Start the module */ > if (mod->init != NULL) > @@ -3168,9 +3153,26 @@ static int complete_formation(struct module *mod, > struct load_info *info) > /* This relies on module_mutex for list integrity. */ > module_bug_finalize(info->hdr, info->sechdrs, mod); > > + /* Set RO and NX regions for core */ > + set_section_ro_nx(mod->module_core, > + mod->core_text_size, > + mod->core_ro_size, > + mod->core_size); > + > + /* Set RO and NX regions for init */ > + set_section_ro_nx(mod->module_init, > + mod->init_text_size, > + mod->init_ro_size, > + mod->init_size); > + > /* Mark state as coming so strong_try_module_get() ignores us, >* but kallsyms etc. can see us. */ > mod->state = MODULE_STATE_COMING; > + mutex_unlock(_mutex); > + > + blocking_notifier_call_chain(_notify_list, > + MODULE_STATE_COMING, mod); Here we break ftrace. ftrace expects that it can still replaces the calls to mcount with nops here. If the text is RO, then it will crash. -- Steve > + return 0; > > out: > mutex_unlock(_mutex); -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
(2014/04/22 16:28), Masami Hiramatsu wrote: > (2014/04/22 14:29), Takao Indoh wrote: >> (2014/04/22 12:51), Rusty Russell wrote: >>> Steven Rostedt writes: On Mon, 24 Mar 2014 20:26:05 +0900 Masami Hiramatsu wrote: > Thank you for reporting with this pretty backtrace :) > Steven, I think this is not the kprobe bug but ftrace (and perhaps, > module). Looks to be more of a module issue than a ftrace issue. > > If the ftrace can set loading module text read only before the module > subsystem > expected, I think it should be protected by the module subsystem itself > (e.g. set_all_modules_text_ro(rw) skips the modules which is > MODULE_STATE_COMING) > Does this patch fix it? In-review-off-by: Steven Rostedt >>> >>> Sorry, was on paternity leave. >>> >>> I'm always nervous about adding more states, since every place which >>> examines the state has to be audited. >>> >>> We set the mod->state to MOD_STATE_COMING in complete_formation; >>> why don't we set NX there instead? It also makes more sense to >>> set NX before we hit parse_args() which can execute code in the module. >>> >>> In fact, we should probably call the notifier there too, so people >>> can breakpoint/tracepoint/etc parameter calls. >>> >>> Of course, this means that we set NX before the notifier; does anything >>> break? >> >> This does not work. ftrace_process_locs() is called from the notifier, >> and it tries to change its text like this. >> >> load_module >>blocking_notifier_call_chain >> ftrace_module_notify_enter >>ftrace_init_module >> ftrace_process_locs >>sort >> ftrace_swap_ips >> >> But the text is already RO, so it causes panic. We need to call notifier >> before setting it RO. Or should we unset RO temporarily in >> ftrace_process_locs()? > > Perhaps, IMHO, ftrace needs to change the module RW in ftrace_init_module and > makes it RO after modifying the module text. Hmm..., I think the same problem occurs if we set module RW in ftrace_init_module(). init_module load_module complete_formation set_section_ro_nx -- (1) set_section_ro_nx -- (2) blocking_notifier_call_chain ftrace_module_notify_enter ftrace_init_module - (3) ftrace_process_locs mutex_lock(_lock) (4) ftrace_update_code __ftrace_replace_code ftrace_make_nop ftrace_modify_code_direct do_ftrace_mod_code probe_kernel_write (5) The text of module B is set to RO at (1) and (2) by Rusty's patch. And even if we change it to RW at (3), it set to RO again by another module while module B is waiting at (4). So, we need to set module to RW somewhere after get ftrace_lock, maybe in ftrace_update_code()? 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/
Re: Re: ftrace/kprobes: Warning when insmod two modules
(2014/04/22 14:29), Takao Indoh wrote: > (2014/04/22 12:51), Rusty Russell wrote: >> Steven Rostedt writes: >>> On Mon, 24 Mar 2014 20:26:05 +0900 >>> Masami Hiramatsu wrote: >>> >>> Thank you for reporting with this pretty backtrace :) Steven, I think this is not the kprobe bug but ftrace (and perhaps, module). >>> >>> Looks to be more of a module issue than a ftrace issue. >>> If the ftrace can set loading module text read only before the module subsystem expected, I think it should be protected by the module subsystem itself (e.g. set_all_modules_text_ro(rw) skips the modules which is MODULE_STATE_COMING) >>> >>> Does this patch fix it? >>> >>> In-review-off-by: Steven Rostedt >> >> Sorry, was on paternity leave. >> >> I'm always nervous about adding more states, since every place which >> examines the state has to be audited. >> >> We set the mod->state to MOD_STATE_COMING in complete_formation; >> why don't we set NX there instead? It also makes more sense to >> set NX before we hit parse_args() which can execute code in the module. >> >> In fact, we should probably call the notifier there too, so people >> can breakpoint/tracepoint/etc parameter calls. >> >> Of course, this means that we set NX before the notifier; does anything >> break? > > This does not work. ftrace_process_locs() is called from the notifier, > and it tries to change its text like this. > > load_module > blocking_notifier_call_chain > ftrace_module_notify_enter > ftrace_init_module > ftrace_process_locs > sort > ftrace_swap_ips > > But the text is already RO, so it causes panic. We need to call notifier > before setting it RO. Or should we unset RO temporarily in > ftrace_process_locs()? Perhaps, IMHO, ftrace needs to change the module RW in ftrace_init_module and makes it RO after modifying the module text. Thank you, -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- 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/
Re: Re: ftrace/kprobes: Warning when insmod two modules
(2014/04/22 14:29), Takao Indoh wrote: (2014/04/22 12:51), Rusty Russell wrote: Steven Rostedt rost...@goodmis.org writes: On Mon, 24 Mar 2014 20:26:05 +0900 Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: Thank you for reporting with this pretty backtrace :) Steven, I think this is not the kprobe bug but ftrace (and perhaps, module). Looks to be more of a module issue than a ftrace issue. If the ftrace can set loading module text read only before the module subsystem expected, I think it should be protected by the module subsystem itself (e.g. set_all_modules_text_ro(rw) skips the modules which is MODULE_STATE_COMING) Does this patch fix it? In-review-off-by: Steven Rostedt rost...@goodmis.org Sorry, was on paternity leave. I'm always nervous about adding more states, since every place which examines the state has to be audited. We set the mod-state to MOD_STATE_COMING in complete_formation; why don't we set NX there instead? It also makes more sense to set NX before we hit parse_args() which can execute code in the module. In fact, we should probably call the notifier there too, so people can breakpoint/tracepoint/etc parameter calls. Of course, this means that we set NX before the notifier; does anything break? This does not work. ftrace_process_locs() is called from the notifier, and it tries to change its text like this. load_module blocking_notifier_call_chain ftrace_module_notify_enter ftrace_init_module ftrace_process_locs sort ftrace_swap_ips But the text is already RO, so it causes panic. We need to call notifier before setting it RO. Or should we unset RO temporarily in ftrace_process_locs()? Perhaps, IMHO, ftrace needs to change the module RW in ftrace_init_module and makes it RO after modifying the module text. Thank you, -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
(2014/04/22 16:28), Masami Hiramatsu wrote: (2014/04/22 14:29), Takao Indoh wrote: (2014/04/22 12:51), Rusty Russell wrote: Steven Rostedt rost...@goodmis.org writes: On Mon, 24 Mar 2014 20:26:05 +0900 Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: Thank you for reporting with this pretty backtrace :) Steven, I think this is not the kprobe bug but ftrace (and perhaps, module). Looks to be more of a module issue than a ftrace issue. If the ftrace can set loading module text read only before the module subsystem expected, I think it should be protected by the module subsystem itself (e.g. set_all_modules_text_ro(rw) skips the modules which is MODULE_STATE_COMING) Does this patch fix it? In-review-off-by: Steven Rostedt rost...@goodmis.org Sorry, was on paternity leave. I'm always nervous about adding more states, since every place which examines the state has to be audited. We set the mod-state to MOD_STATE_COMING in complete_formation; why don't we set NX there instead? It also makes more sense to set NX before we hit parse_args() which can execute code in the module. In fact, we should probably call the notifier there too, so people can breakpoint/tracepoint/etc parameter calls. Of course, this means that we set NX before the notifier; does anything break? This does not work. ftrace_process_locs() is called from the notifier, and it tries to change its text like this. load_module blocking_notifier_call_chain ftrace_module_notify_enter ftrace_init_module ftrace_process_locs sort ftrace_swap_ips But the text is already RO, so it causes panic. We need to call notifier before setting it RO. Or should we unset RO temporarily in ftrace_process_locs()? Perhaps, IMHO, ftrace needs to change the module RW in ftrace_init_module and makes it RO after modifying the module text. Hmm..., I think the same problem occurs if we set module RW in ftrace_init_module(). insmod module B init_module load_module complete_formation set_section_ro_nx -- (1) set_section_ro_nx -- (2) blocking_notifier_call_chain ftrace_module_notify_enter ftrace_init_module - (3) ftrace_process_locs mutex_lock(ftrace_lock) (4) ftrace_update_code __ftrace_replace_code ftrace_make_nop ftrace_modify_code_direct do_ftrace_mod_code probe_kernel_write (5) The text of module B is set to RO at (1) and (2) by Rusty's patch. And even if we change it to RW at (3), it set to RO again by another module while module B is waiting at (4). So, we need to set module to RW somewhere after get ftrace_lock, maybe in ftrace_update_code()? 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/
Re: ftrace/kprobes: Warning when insmod two modules
On Tue, 22 Apr 2014 13:21:18 +0930 Rusty Russell ru...@rustcorp.com.au wrote: Sorry, was on paternity leave. Congratulations! Although having two teenage daughters myself, I'm more inclined to say my condolences. I'm always nervous about adding more states, since every place which examines the state has to be audited. I didn't really add a state but instead broke an existing state into two. More importantly, this second part of the state doesn't get exported to other parts of the kernel. That is, there is no notifier for it. This means the only place you need to audit is in module.c itself. Any other change requires auditing all module notifiers. We set the mod-state to MOD_STATE_COMING in complete_formation; why don't we set NX there instead? It also makes more sense to set NX before we hit parse_args() which can execute code in the module. Well, NX is a different issue here all together. Sure if you want to, but that wont affect this. In fact, we should probably call the notifier there too, so people can breakpoint/tracepoint/etc parameter calls. Of course, this means that we set NX before the notifier; does anything break? Subject: module: set nx before marking module MODULE_STATE_COMING. This prevents a WARN_ON() where ftrace calls set_all_modules_text_ro() which races with the module setting its own set_section_ro_nx(). It also means we're NX protected before we call parse_args(), which can execute module code. This means that the notifiers will be called on a module which is already NX, so that may cause problems. Signed-off-by: Rusty Russell ru...@rustcorp.com.au diff --git a/kernel/module.c b/kernel/module.c index 11869408f79b..83a437e5d429 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3023,21 +3023,6 @@ static int do_init_module(struct module *mod) */ current-flags = ~PF_USED_ASYNC; - blocking_notifier_call_chain(module_notify_list, - MODULE_STATE_COMING, mod); - - /* Set RO and NX regions for core */ - set_section_ro_nx(mod-module_core, - mod-core_text_size, - mod-core_ro_size, - mod-core_size); - - /* Set RO and NX regions for init */ - set_section_ro_nx(mod-module_init, - mod-init_text_size, - mod-init_ro_size, - mod-init_size); - do_mod_ctors(mod); /* Start the module */ if (mod-init != NULL) @@ -3168,9 +3153,26 @@ static int complete_formation(struct module *mod, struct load_info *info) /* This relies on module_mutex for list integrity. */ module_bug_finalize(info-hdr, info-sechdrs, mod); + /* Set RO and NX regions for core */ + set_section_ro_nx(mod-module_core, + mod-core_text_size, + mod-core_ro_size, + mod-core_size); + + /* Set RO and NX regions for init */ + set_section_ro_nx(mod-module_init, + mod-init_text_size, + mod-init_ro_size, + mod-init_size); + /* Mark state as coming so strong_try_module_get() ignores us, * but kallsyms etc. can see us. */ mod-state = MODULE_STATE_COMING; + mutex_unlock(module_mutex); + + blocking_notifier_call_chain(module_notify_list, + MODULE_STATE_COMING, mod); Here we break ftrace. ftrace expects that it can still replaces the calls to mcount with nops here. If the text is RO, then it will crash. -- Steve + return 0; out: mutex_unlock(module_mutex); -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
(2014/04/22 17:35), Takao Indoh wrote: But the text is already RO, so it causes panic. We need to call notifier before setting it RO. Or should we unset RO temporarily in ftrace_process_locs()? Perhaps, IMHO, ftrace needs to change the module RW in ftrace_init_module and makes it RO after modifying the module text. Hmm..., I think the same problem occurs if we set module RW in ftrace_init_module(). insmod module B init_module load_module complete_formation set_section_ro_nx -- (1) set_section_ro_nx -- (2) blocking_notifier_call_chain ftrace_module_notify_enter ftrace_init_module - (3) ftrace_process_locs mutex_lock(ftrace_lock) (4) ftrace_update_code __ftrace_replace_code ftrace_make_nop ftrace_modify_code_direct do_ftrace_mod_code probe_kernel_write (5) The text of module B is set to RO at (1) and (2) by Rusty's patch. And even if we change it to RW at (3), it set to RO again by another module while module B is waiting at (4). So, we need to set module to RW somewhere after get ftrace_lock, maybe in ftrace_update_code()? 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. Thank you, -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
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? 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. Now, if text poke does something similar, we need to make another mutex that covers modifying text. Don't we have one 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 -- Steve -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
(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. Thank you, -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
(2014/04/22 12:51), Rusty Russell wrote: > Steven Rostedt writes: >> On Mon, 24 Mar 2014 20:26:05 +0900 >> Masami Hiramatsu wrote: >> >> >>> Thank you for reporting with this pretty backtrace :) >>> Steven, I think this is not the kprobe bug but ftrace (and perhaps, module). >> >> Looks to be more of a module issue than a ftrace issue. >> >>> >>> If the ftrace can set loading module text read only before the module >>> subsystem >>> expected, I think it should be protected by the module subsystem itself >>> (e.g. set_all_modules_text_ro(rw) skips the modules which is >>> MODULE_STATE_COMING) >>> >> >> Does this patch fix it? >> >> In-review-off-by: Steven Rostedt > > Sorry, was on paternity leave. > > I'm always nervous about adding more states, since every place which > examines the state has to be audited. > > We set the mod->state to MOD_STATE_COMING in complete_formation; > why don't we set NX there instead? It also makes more sense to > set NX before we hit parse_args() which can execute code in the module. > > In fact, we should probably call the notifier there too, so people > can breakpoint/tracepoint/etc parameter calls. > > Of course, this means that we set NX before the notifier; does anything > break? This does not work. ftrace_process_locs() is called from the notifier, and it tries to change its text like this. load_module blocking_notifier_call_chain ftrace_module_notify_enter ftrace_init_module ftrace_process_locs sort ftrace_swap_ips But the text is already RO, so it causes panic. We need to call notifier before setting it RO. Or should we unset RO temporarily in ftrace_process_locs()? Thanks, Takao Indoh > > Subject: module: set nx before marking module MODULE_STATE_COMING. > > This prevents a WARN_ON() where ftrace calls set_all_modules_text_ro() > which races with the module setting its own set_section_ro_nx(). > > It also means we're NX protected before we call parse_args(), which > can execute module code. > > This means that the notifiers will be called on a module which > is already NX, so that may cause problems. > > Signed-off-by: Rusty Russell > > diff --git a/kernel/module.c b/kernel/module.c > index 11869408f79b..83a437e5d429 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -3023,21 +3023,6 @@ static int do_init_module(struct module *mod) >*/ > current->flags &= ~PF_USED_ASYNC; > > - blocking_notifier_call_chain(_notify_list, > - MODULE_STATE_COMING, mod); > - > - /* Set RO and NX regions for core */ > - set_section_ro_nx(mod->module_core, > - mod->core_text_size, > - mod->core_ro_size, > - mod->core_size); > - > - /* Set RO and NX regions for init */ > - set_section_ro_nx(mod->module_init, > - mod->init_text_size, > - mod->init_ro_size, > - mod->init_size); > - > do_mod_ctors(mod); > /* Start the module */ > if (mod->init != NULL) > @@ -3168,9 +3153,26 @@ static int complete_formation(struct module *mod, > struct load_info *info) > /* This relies on module_mutex for list integrity. */ > module_bug_finalize(info->hdr, info->sechdrs, mod); > > + /* Set RO and NX regions for core */ > + set_section_ro_nx(mod->module_core, > + mod->core_text_size, > + mod->core_ro_size, > + mod->core_size); > + > + /* Set RO and NX regions for init */ > + set_section_ro_nx(mod->module_init, > + mod->init_text_size, > + mod->init_ro_size, > + mod->init_size); > + > /* Mark state as coming so strong_try_module_get() ignores us, >* but kallsyms etc. can see us. */ > mod->state = MODULE_STATE_COMING; > + mutex_unlock(_mutex); > + > + blocking_notifier_call_chain(_notify_list, > + MODULE_STATE_COMING, mod); > + return 0; > > out: > mutex_unlock(_mutex); > > -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
Steven Rostedt writes: > On Mon, 24 Mar 2014 20:26:05 +0900 > Masami Hiramatsu wrote: > > >> Thank you for reporting with this pretty backtrace :) >> Steven, I think this is not the kprobe bug but ftrace (and perhaps, module). > > Looks to be more of a module issue than a ftrace issue. > >> >> If the ftrace can set loading module text read only before the module >> subsystem >> expected, I think it should be protected by the module subsystem itself >> (e.g. set_all_modules_text_ro(rw) skips the modules which is >> MODULE_STATE_COMING) >> > > Does this patch fix it? > > In-review-off-by: Steven Rostedt Sorry, was on paternity leave. I'm always nervous about adding more states, since every place which examines the state has to be audited. We set the mod->state to MOD_STATE_COMING in complete_formation; why don't we set NX there instead? It also makes more sense to set NX before we hit parse_args() which can execute code in the module. In fact, we should probably call the notifier there too, so people can breakpoint/tracepoint/etc parameter calls. Of course, this means that we set NX before the notifier; does anything break? Subject: module: set nx before marking module MODULE_STATE_COMING. This prevents a WARN_ON() where ftrace calls set_all_modules_text_ro() which races with the module setting its own set_section_ro_nx(). It also means we're NX protected before we call parse_args(), which can execute module code. This means that the notifiers will be called on a module which is already NX, so that may cause problems. Signed-off-by: Rusty Russell diff --git a/kernel/module.c b/kernel/module.c index 11869408f79b..83a437e5d429 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3023,21 +3023,6 @@ static int do_init_module(struct module *mod) */ current->flags &= ~PF_USED_ASYNC; - blocking_notifier_call_chain(_notify_list, - MODULE_STATE_COMING, mod); - - /* Set RO and NX regions for core */ - set_section_ro_nx(mod->module_core, - mod->core_text_size, - mod->core_ro_size, - mod->core_size); - - /* Set RO and NX regions for init */ - set_section_ro_nx(mod->module_init, - mod->init_text_size, - mod->init_ro_size, - mod->init_size); - do_mod_ctors(mod); /* Start the module */ if (mod->init != NULL) @@ -3168,9 +3153,26 @@ static int complete_formation(struct module *mod, struct load_info *info) /* This relies on module_mutex for list integrity. */ module_bug_finalize(info->hdr, info->sechdrs, mod); + /* Set RO and NX regions for core */ + set_section_ro_nx(mod->module_core, + mod->core_text_size, + mod->core_ro_size, + mod->core_size); + + /* Set RO and NX regions for init */ + set_section_ro_nx(mod->module_init, + mod->init_text_size, + mod->init_ro_size, + mod->init_size); + /* Mark state as coming so strong_try_module_get() ignores us, * but kallsyms etc. can see us. */ mod->state = MODULE_STATE_COMING; + mutex_unlock(_mutex); + + blocking_notifier_call_chain(_notify_list, +MODULE_STATE_COMING, mod); + return 0; out: mutex_unlock(_mutex); -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
Steven Rostedt rost...@goodmis.org writes: On Mon, 24 Mar 2014 20:26:05 +0900 Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: Thank you for reporting with this pretty backtrace :) Steven, I think this is not the kprobe bug but ftrace (and perhaps, module). Looks to be more of a module issue than a ftrace issue. If the ftrace can set loading module text read only before the module subsystem expected, I think it should be protected by the module subsystem itself (e.g. set_all_modules_text_ro(rw) skips the modules which is MODULE_STATE_COMING) Does this patch fix it? In-review-off-by: Steven Rostedt rost...@goodmis.org Sorry, was on paternity leave. I'm always nervous about adding more states, since every place which examines the state has to be audited. We set the mod-state to MOD_STATE_COMING in complete_formation; why don't we set NX there instead? It also makes more sense to set NX before we hit parse_args() which can execute code in the module. In fact, we should probably call the notifier there too, so people can breakpoint/tracepoint/etc parameter calls. Of course, this means that we set NX before the notifier; does anything break? Subject: module: set nx before marking module MODULE_STATE_COMING. This prevents a WARN_ON() where ftrace calls set_all_modules_text_ro() which races with the module setting its own set_section_ro_nx(). It also means we're NX protected before we call parse_args(), which can execute module code. This means that the notifiers will be called on a module which is already NX, so that may cause problems. Signed-off-by: Rusty Russell ru...@rustcorp.com.au diff --git a/kernel/module.c b/kernel/module.c index 11869408f79b..83a437e5d429 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3023,21 +3023,6 @@ static int do_init_module(struct module *mod) */ current-flags = ~PF_USED_ASYNC; - blocking_notifier_call_chain(module_notify_list, - MODULE_STATE_COMING, mod); - - /* Set RO and NX regions for core */ - set_section_ro_nx(mod-module_core, - mod-core_text_size, - mod-core_ro_size, - mod-core_size); - - /* Set RO and NX regions for init */ - set_section_ro_nx(mod-module_init, - mod-init_text_size, - mod-init_ro_size, - mod-init_size); - do_mod_ctors(mod); /* Start the module */ if (mod-init != NULL) @@ -3168,9 +3153,26 @@ static int complete_formation(struct module *mod, struct load_info *info) /* This relies on module_mutex for list integrity. */ module_bug_finalize(info-hdr, info-sechdrs, mod); + /* Set RO and NX regions for core */ + set_section_ro_nx(mod-module_core, + mod-core_text_size, + mod-core_ro_size, + mod-core_size); + + /* Set RO and NX regions for init */ + set_section_ro_nx(mod-module_init, + mod-init_text_size, + mod-init_ro_size, + mod-init_size); + /* Mark state as coming so strong_try_module_get() ignores us, * but kallsyms etc. can see us. */ mod-state = MODULE_STATE_COMING; + mutex_unlock(module_mutex); + + blocking_notifier_call_chain(module_notify_list, +MODULE_STATE_COMING, mod); + return 0; out: mutex_unlock(module_mutex); -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
(2014/04/22 12:51), Rusty Russell wrote: Steven Rostedt rost...@goodmis.org writes: On Mon, 24 Mar 2014 20:26:05 +0900 Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: Thank you for reporting with this pretty backtrace :) Steven, I think this is not the kprobe bug but ftrace (and perhaps, module). Looks to be more of a module issue than a ftrace issue. If the ftrace can set loading module text read only before the module subsystem expected, I think it should be protected by the module subsystem itself (e.g. set_all_modules_text_ro(rw) skips the modules which is MODULE_STATE_COMING) Does this patch fix it? In-review-off-by: Steven Rostedt rost...@goodmis.org Sorry, was on paternity leave. I'm always nervous about adding more states, since every place which examines the state has to be audited. We set the mod-state to MOD_STATE_COMING in complete_formation; why don't we set NX there instead? It also makes more sense to set NX before we hit parse_args() which can execute code in the module. In fact, we should probably call the notifier there too, so people can breakpoint/tracepoint/etc parameter calls. Of course, this means that we set NX before the notifier; does anything break? This does not work. ftrace_process_locs() is called from the notifier, and it tries to change its text like this. load_module blocking_notifier_call_chain ftrace_module_notify_enter ftrace_init_module ftrace_process_locs sort ftrace_swap_ips But the text is already RO, so it causes panic. We need to call notifier before setting it RO. Or should we unset RO temporarily in ftrace_process_locs()? Thanks, Takao Indoh Subject: module: set nx before marking module MODULE_STATE_COMING. This prevents a WARN_ON() where ftrace calls set_all_modules_text_ro() which races with the module setting its own set_section_ro_nx(). It also means we're NX protected before we call parse_args(), which can execute module code. This means that the notifiers will be called on a module which is already NX, so that may cause problems. Signed-off-by: Rusty Russell ru...@rustcorp.com.au diff --git a/kernel/module.c b/kernel/module.c index 11869408f79b..83a437e5d429 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3023,21 +3023,6 @@ static int do_init_module(struct module *mod) */ current-flags = ~PF_USED_ASYNC; - blocking_notifier_call_chain(module_notify_list, - MODULE_STATE_COMING, mod); - - /* Set RO and NX regions for core */ - set_section_ro_nx(mod-module_core, - mod-core_text_size, - mod-core_ro_size, - mod-core_size); - - /* Set RO and NX regions for init */ - set_section_ro_nx(mod-module_init, - mod-init_text_size, - mod-init_ro_size, - mod-init_size); - do_mod_ctors(mod); /* Start the module */ if (mod-init != NULL) @@ -3168,9 +3153,26 @@ static int complete_formation(struct module *mod, struct load_info *info) /* This relies on module_mutex for list integrity. */ module_bug_finalize(info-hdr, info-sechdrs, mod); + /* Set RO and NX regions for core */ + set_section_ro_nx(mod-module_core, + mod-core_text_size, + mod-core_ro_size, + mod-core_size); + + /* Set RO and NX regions for init */ + set_section_ro_nx(mod-module_init, + mod-init_text_size, + mod-init_ro_size, + mod-init_size); + /* Mark state as coming so strong_try_module_get() ignores us, * but kallsyms etc. can see us. */ mod-state = MODULE_STATE_COMING; + mutex_unlock(module_mutex); + + blocking_notifier_call_chain(module_notify_list, + MODULE_STATE_COMING, mod); + return 0; out: mutex_unlock(module_mutex); -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
(2014/03/24 23:59), Steven Rostedt wrote:
> On Mon, 24 Mar 2014 20:26:05 +0900
> Masami Hiramatsu wrote:
>
>
>> Thank you for reporting with this pretty backtrace :)
>> Steven, I think this is not the kprobe bug but ftrace (and perhaps, module).
>
> Looks to be more of a module issue than a ftrace issue.
>
>>
>> If the ftrace can set loading module text read only before the module
>> subsystem
>> expected, I think it should be protected by the module subsystem itself
>> (e.g. set_all_modules_text_ro(rw) skips the modules which is
>> MODULE_STATE_COMING)
>>
>
> Does this patch fix it?
Yep, I tested using my reproducer and confirmed that this patch fixed
this issue, thanks!
Thanks,
Takao Indoh
>
> In-review-off-by: Steven Rostedt
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 5a50539..a1acabf 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -207,10 +207,11 @@ struct module_use {
> };
>
> enum module_state {
> - MODULE_STATE_LIVE, /* Normal state. */
> - MODULE_STATE_COMING,/* Full formed, running module_init. */
> - MODULE_STATE_GOING, /* Going away. */
> - MODULE_STATE_UNFORMED, /* Still setting it up. */
> + MODULE_STATE_LIVE, /* Normal state. */
> + MODULE_STATE_COMING,/* Full formed, running module_init. */
> + MODULE_STATE_COMING_FINAL, /* Ready to be changed to read only. */
> + MODULE_STATE_GOING, /* Going away. */
> + MODULE_STATE_UNFORMED, /* Still setting it up. */
> };
>
> /**
> diff --git a/kernel/module.c b/kernel/module.c
> index d24fcf2..0905bed 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1805,7 +1805,8 @@ void set_all_modules_text_ro(void)
>
> mutex_lock(_mutex);
> list_for_each_entry_rcu(mod, , list) {
> - if (mod->state == MODULE_STATE_UNFORMED)
> + if (mod->state == MODULE_STATE_UNFORMED ||
> + mod->state == MODULE_STATE_COMING)
> continue;
> if ((mod->module_core) && (mod->core_text_size)) {
> set_page_attributes(mod->module_core,
> @@ -3020,6 +3021,13 @@ static int do_init_module(struct module *mod)
> blocking_notifier_call_chain(_notify_list,
> MODULE_STATE_COMING, mod);
>
> + /*
> + * This module must not be changed by set_all_modules_text_ro()
> + * until we get here. Otherwise notifiers that change text
> + * (like ftrace does) will break.
> + */
> + mod->state = MODULE_STATE_COMING_FINAL;
> +
> /* Set RO and NX regions for core */
> set_section_ro_nx(mod->module_core,
> mod->core_text_size,
>
>
>
--
印藤隆夫(INDOH Takao)
E-Mail : indou.ta...@jp.fujitsu.com
TEL: 7551-4832(055-924-7241)
富士通(株) PFソ事本)Linux開発統括部 開発部
--
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/
Re: ftrace/kprobes: Warning when insmod two modules
On Mon, 24 Mar 2014 20:26:05 +0900
Masami Hiramatsu wrote:
> Thank you for reporting with this pretty backtrace :)
> Steven, I think this is not the kprobe bug but ftrace (and perhaps, module).
Looks to be more of a module issue than a ftrace issue.
>
> If the ftrace can set loading module text read only before the module
> subsystem
> expected, I think it should be protected by the module subsystem itself
> (e.g. set_all_modules_text_ro(rw) skips the modules which is
> MODULE_STATE_COMING)
>
Does this patch fix it?
In-review-off-by: Steven Rostedt
diff --git a/include/linux/module.h b/include/linux/module.h
index 5a50539..a1acabf 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -207,10 +207,11 @@ struct module_use {
};
enum module_state {
- MODULE_STATE_LIVE, /* Normal state. */
- MODULE_STATE_COMING,/* Full formed, running module_init. */
- MODULE_STATE_GOING, /* Going away. */
- MODULE_STATE_UNFORMED, /* Still setting it up. */
+ MODULE_STATE_LIVE, /* Normal state. */
+ MODULE_STATE_COMING,/* Full formed, running module_init. */
+ MODULE_STATE_COMING_FINAL, /* Ready to be changed to read only. */
+ MODULE_STATE_GOING, /* Going away. */
+ MODULE_STATE_UNFORMED, /* Still setting it up. */
};
/**
diff --git a/kernel/module.c b/kernel/module.c
index d24fcf2..0905bed 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1805,7 +1805,8 @@ void set_all_modules_text_ro(void)
mutex_lock(_mutex);
list_for_each_entry_rcu(mod, , list) {
- if (mod->state == MODULE_STATE_UNFORMED)
+ if (mod->state == MODULE_STATE_UNFORMED ||
+ mod->state == MODULE_STATE_COMING)
continue;
if ((mod->module_core) && (mod->core_text_size)) {
set_page_attributes(mod->module_core,
@@ -3020,6 +3021,13 @@ static int do_init_module(struct module *mod)
blocking_notifier_call_chain(_notify_list,
MODULE_STATE_COMING, mod);
+ /*
+* This module must not be changed by set_all_modules_text_ro()
+* until we get here. Otherwise notifiers that change text
+* (like ftrace does) will break.
+*/
+ mod->state = MODULE_STATE_COMING_FINAL;
+
/* Set RO and NX regions for core */
set_section_ro_nx(mod->module_core,
mod->core_text_size,
--
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/
Re: ftrace/kprobes: Warning when insmod two modules
On Mon, 24 Mar 2014 20:26:05 +0900 Masami Hiramatsu wrote: > Thank you for reporting with this pretty backtrace :) > Steven, I think this is not the kprobe bug but ftrace (and perhaps, module). > > If the ftrace can set loading module text read only before the module > subsystem > expected, I think it should be protected by the module subsystem itself > (e.g. set_all_modules_text_ro(rw) skips the modules which is > MODULE_STATE_COMING) > I have to admit. I never thought about having the module init code enabling ftrace :-) Yeah, it looks like we need to handle this case. I'll take a look into it. Thanks for the report, -- Steve -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
(2014/03/24 14:10), Takao Indoh wrote: > Hi all, > > I noticed the following ftrace waring message when I insmod module. > > [ 409.337936] [ cut here ] > [ 409.337945] WARNING: CPU: 12 PID: 10028 at > /mnt/repos/linux/kernel/trace/ftrace.c:1716 ftrace_bug+0x206/0x270() > [ 409.337971] Modules linked in: test2(O+) test1(O+) ip6table_filter > ip6_tables iptable_filter ip_tables ebtable_nat ebtables coretemp kvm_intel > kvm crc32c_intel ghash_clmulni_intel aesni_intel sg aes_x86_64 glue_helper > nfsd lrw gf128mul ablk_helper auth_rpcgss oid_registry cryptd exportfs > nfs_acl lockd iTCO_wdt iTCO_vendor_support i7core_edac lpc_ich dm_mirror > microcode serio_raw pcspkr i2c_i801 ioatdma dm_region_hash mfd_core edac_core > ipmi_si dm_log shpchp tpm_infineon acpi_cpufreq dm_mod ipmi_msghandler uinput > sunrpc ext4 mbcache jbd2 igb ptp mptsas pps_core lpfc i2c_algo_bit > scsi_transport_sas scsi_transport_fc i2c_core mptscsih mptbase scsi_tgt > megaraid_sas dca ipv6 autofs4 [last unloaded: test2] > [ 409.337974] CPU: 12 PID: 10028 Comm: insmod Tainted: G O > 3.14.0-rc5 #6 > [ 409.337975] Hardware name: FUJITSU-SV PRIMERGY BX920 S2/D3030, BIOS 080015 > Rev.3D81.3030 02/10/2012 > [ 409.337979] 0009 88023f7efc50 81547bfe > > [ 409.337981] 88023f7efc88 81049a0d > a0364000 > [ 409.337983] 88023f6775c0 88023857 > 88023f7efc98 > [ 409.337983] Call Trace: > [ 409.337990] [] dump_stack+0x45/0x56 > [ 409.337993] [] warn_slowpath_common+0x7d/0xa0 > [ 409.337997] [] ? 0xa0363fff > [ 409.337999] [] warn_slowpath_null+0x1a/0x20 > [ 409.338001] [] ftrace_bug+0x206/0x270 > [ 409.338004] [] ? 0xa0363fff > [ 409.338006] [] ftrace_process_locs+0x32e/0x710 > [ 409.338008] [] ftrace_module_notify_enter+0x96/0xf0 > [ 409.338012] [] notifier_call_chain+0x4c/0x70 > [ 409.338018] [] __blocking_notifier_call_chain+0x4d/0x70 > [ 409.338020] [] blocking_notifier_call_chain+0x16/0x20 > [ 409.338026] [] load_module+0x1f54/0x25d0 > [ 409.338028] [] ? store_uevent+0x40/0x40 > [ 409.338031] [] SyS_finit_module+0x86/0xb0 > [ 409.338036] [] system_call_fastpath+0x16/0x1b > [ 409.338037] ---[ end trace e7e27c36e7a65831 ]--- > [ 409.338040] ftrace faulted on writing [] > handler_pre+0x0/0x10 [test2] > > To cause this problem, > o Insmod two modules almost at the same time > o At least one of them use kprobe. > > Let's say I'm trying to insmod module A and module B at the same time, > and module A calls register_kprobe() in it's module_init funciton. > > > init_module > load_module > do_init_module > do_one_initcall > kprobe_init > register_kprobe > arm_kprobe > arm_kprobe_ftrace > register_ftrace_function > mutex_lock(_lock) --- (1) > ftrace_startup > ftrace_startup_enable > ftrace_run_update_code > ftrace_arch_code_modify_post_process > set_all_modules_text_ro (2) > mutex_unlock(_lock) - (3) > > > > init_module > load_module > do_init_module > blocking_notifier_call_chain > ftrace_module_notify_enter > ftrace_init_module > ftrace_process_locs > mutex_lock(_lock) (4) > ftrace_update_code >__ftrace_replace_code > ftrace_make_nop >ftrace_modify_code_direct > do_ftrace_mod_code >probe_kernel_write (5) > > > o Frist of all, module A gets ftrace_lock at (1) > o Module B also tries to get ftrace_lock at (4) somewhat late, and wait > here because modules A got ftrace_lock. > o Module A sets all modules text to ReadOnly at (2), and then release > ftrace_lock at (3) > o Module B wakes up and tries to rewrite its text at (5), but it fails > because it is already changed to RO at (2) by modules A. The ftrace > waring message is outputted. Thank you for reporting with this pretty backtrace :) Steven, I think this is not the kprobe bug but ftrace (and perhaps, module). If the ftrace can set loading module text read only before the module subsystem expected, I think it should be protected by the module subsystem itself (e.g. set_all_modules_text_ro(rw) skips the modules which is MODULE_STATE_COMING) Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com -- 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
Re: ftrace/kprobes: Warning when insmod two modules
(2014/03/24 14:10), Takao Indoh wrote: Hi all, I noticed the following ftrace waring message when I insmod module. [ 409.337936] [ cut here ] [ 409.337945] WARNING: CPU: 12 PID: 10028 at /mnt/repos/linux/kernel/trace/ftrace.c:1716 ftrace_bug+0x206/0x270() [ 409.337971] Modules linked in: test2(O+) test1(O+) ip6table_filter ip6_tables iptable_filter ip_tables ebtable_nat ebtables coretemp kvm_intel kvm crc32c_intel ghash_clmulni_intel aesni_intel sg aes_x86_64 glue_helper nfsd lrw gf128mul ablk_helper auth_rpcgss oid_registry cryptd exportfs nfs_acl lockd iTCO_wdt iTCO_vendor_support i7core_edac lpc_ich dm_mirror microcode serio_raw pcspkr i2c_i801 ioatdma dm_region_hash mfd_core edac_core ipmi_si dm_log shpchp tpm_infineon acpi_cpufreq dm_mod ipmi_msghandler uinput sunrpc ext4 mbcache jbd2 igb ptp mptsas pps_core lpfc i2c_algo_bit scsi_transport_sas scsi_transport_fc i2c_core mptscsih mptbase scsi_tgt megaraid_sas dca ipv6 autofs4 [last unloaded: test2] [ 409.337974] CPU: 12 PID: 10028 Comm: insmod Tainted: G O 3.14.0-rc5 #6 [ 409.337975] Hardware name: FUJITSU-SV PRIMERGY BX920 S2/D3030, BIOS 080015 Rev.3D81.3030 02/10/2012 [ 409.337979] 0009 88023f7efc50 81547bfe [ 409.337981] 88023f7efc88 81049a0d a0364000 [ 409.337983] 88023f6775c0 88023857 88023f7efc98 [ 409.337983] Call Trace: [ 409.337990] [81547bfe] dump_stack+0x45/0x56 [ 409.337993] [81049a0d] warn_slowpath_common+0x7d/0xa0 [ 409.337997] [a0364000] ? 0xa0363fff [ 409.337999] [81049aea] warn_slowpath_null+0x1a/0x20 [ 409.338001] [810e79f6] ftrace_bug+0x206/0x270 [ 409.338004] [a0364000] ? 0xa0363fff [ 409.338006] [810e7d8e] ftrace_process_locs+0x32e/0x710 [ 409.338008] [810e8206] ftrace_module_notify_enter+0x96/0xf0 [ 409.338012] [81551dec] notifier_call_chain+0x4c/0x70 [ 409.338018] [8106fbfd] __blocking_notifier_call_chain+0x4d/0x70 [ 409.338020] [8106fc36] blocking_notifier_call_chain+0x16/0x20 [ 409.338026] [810bf3f4] load_module+0x1f54/0x25d0 [ 409.338028] [810baab0] ? store_uevent+0x40/0x40 [ 409.338031] [810bfbe6] SyS_finit_module+0x86/0xb0 [ 409.338036] [81556752] system_call_fastpath+0x16/0x1b [ 409.338037] ---[ end trace e7e27c36e7a65831 ]--- [ 409.338040] ftrace faulted on writing [a0364000] handler_pre+0x0/0x10 [test2] To cause this problem, o Insmod two modules almost at the same time o At least one of them use kprobe. Let's say I'm trying to insmod module A and module B at the same time, and module A calls register_kprobe() in it's module_init funciton. insmod module A init_module load_module do_init_module do_one_initcall kprobe_init register_kprobe arm_kprobe arm_kprobe_ftrace register_ftrace_function mutex_lock(ftrace_lock) --- (1) ftrace_startup ftrace_startup_enable ftrace_run_update_code ftrace_arch_code_modify_post_process set_all_modules_text_ro (2) mutex_unlock(ftrace_lock) - (3) insmod module B init_module load_module do_init_module blocking_notifier_call_chain ftrace_module_notify_enter ftrace_init_module ftrace_process_locs mutex_lock(ftrace_lock) (4) ftrace_update_code __ftrace_replace_code ftrace_make_nop ftrace_modify_code_direct do_ftrace_mod_code probe_kernel_write (5) o Frist of all, module A gets ftrace_lock at (1) o Module B also tries to get ftrace_lock at (4) somewhat late, and wait here because modules A got ftrace_lock. o Module A sets all modules text to ReadOnly at (2), and then release ftrace_lock at (3) o Module B wakes up and tries to rewrite its text at (5), but it fails because it is already changed to RO at (2) by modules A. The ftrace waring message is outputted. Thank you for reporting with this pretty backtrace :) Steven, I think this is not the kprobe bug but ftrace (and perhaps, module). If the ftrace can set loading module text read only before the module subsystem expected, I think it should be protected by the module subsystem itself (e.g. set_all_modules_text_ro(rw) skips the modules which is MODULE_STATE_COMING) Thank you, -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail:
Re: ftrace/kprobes: Warning when insmod two modules
On Mon, 24 Mar 2014 20:26:05 +0900 Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote: Thank you for reporting with this pretty backtrace :) Steven, I think this is not the kprobe bug but ftrace (and perhaps, module). If the ftrace can set loading module text read only before the module subsystem expected, I think it should be protected by the module subsystem itself (e.g. set_all_modules_text_ro(rw) skips the modules which is MODULE_STATE_COMING) I have to admit. I never thought about having the module init code enabling ftrace :-) Yeah, it looks like we need to handle this case. I'll take a look into it. Thanks for the report, -- Steve -- 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/
Re: ftrace/kprobes: Warning when insmod two modules
On Mon, 24 Mar 2014 20:26:05 +0900
Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote:
Thank you for reporting with this pretty backtrace :)
Steven, I think this is not the kprobe bug but ftrace (and perhaps, module).
Looks to be more of a module issue than a ftrace issue.
If the ftrace can set loading module text read only before the module
subsystem
expected, I think it should be protected by the module subsystem itself
(e.g. set_all_modules_text_ro(rw) skips the modules which is
MODULE_STATE_COMING)
Does this patch fix it?
In-review-off-by: Steven Rostedt rost...@goodmis.org
diff --git a/include/linux/module.h b/include/linux/module.h
index 5a50539..a1acabf 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -207,10 +207,11 @@ struct module_use {
};
enum module_state {
- MODULE_STATE_LIVE, /* Normal state. */
- MODULE_STATE_COMING,/* Full formed, running module_init. */
- MODULE_STATE_GOING, /* Going away. */
- MODULE_STATE_UNFORMED, /* Still setting it up. */
+ MODULE_STATE_LIVE, /* Normal state. */
+ MODULE_STATE_COMING,/* Full formed, running module_init. */
+ MODULE_STATE_COMING_FINAL, /* Ready to be changed to read only. */
+ MODULE_STATE_GOING, /* Going away. */
+ MODULE_STATE_UNFORMED, /* Still setting it up. */
};
/**
diff --git a/kernel/module.c b/kernel/module.c
index d24fcf2..0905bed 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1805,7 +1805,8 @@ void set_all_modules_text_ro(void)
mutex_lock(module_mutex);
list_for_each_entry_rcu(mod, modules, list) {
- if (mod-state == MODULE_STATE_UNFORMED)
+ if (mod-state == MODULE_STATE_UNFORMED ||
+ mod-state == MODULE_STATE_COMING)
continue;
if ((mod-module_core) (mod-core_text_size)) {
set_page_attributes(mod-module_core,
@@ -3020,6 +3021,13 @@ static int do_init_module(struct module *mod)
blocking_notifier_call_chain(module_notify_list,
MODULE_STATE_COMING, mod);
+ /*
+* This module must not be changed by set_all_modules_text_ro()
+* until we get here. Otherwise notifiers that change text
+* (like ftrace does) will break.
+*/
+ mod-state = MODULE_STATE_COMING_FINAL;
+
/* Set RO and NX regions for core */
set_section_ro_nx(mod-module_core,
mod-core_text_size,
--
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/
Re: ftrace/kprobes: Warning when insmod two modules
(2014/03/24 23:59), Steven Rostedt wrote:
On Mon, 24 Mar 2014 20:26:05 +0900
Masami Hiramatsu masami.hiramatsu...@hitachi.com wrote:
Thank you for reporting with this pretty backtrace :)
Steven, I think this is not the kprobe bug but ftrace (and perhaps, module).
Looks to be more of a module issue than a ftrace issue.
If the ftrace can set loading module text read only before the module
subsystem
expected, I think it should be protected by the module subsystem itself
(e.g. set_all_modules_text_ro(rw) skips the modules which is
MODULE_STATE_COMING)
Does this patch fix it?
Yep, I tested using my reproducer and confirmed that this patch fixed
this issue, thanks!
Thanks,
Takao Indoh
In-review-off-by: Steven Rostedt rost...@goodmis.org
diff --git a/include/linux/module.h b/include/linux/module.h
index 5a50539..a1acabf 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -207,10 +207,11 @@ struct module_use {
};
enum module_state {
- MODULE_STATE_LIVE, /* Normal state. */
- MODULE_STATE_COMING,/* Full formed, running module_init. */
- MODULE_STATE_GOING, /* Going away. */
- MODULE_STATE_UNFORMED, /* Still setting it up. */
+ MODULE_STATE_LIVE, /* Normal state. */
+ MODULE_STATE_COMING,/* Full formed, running module_init. */
+ MODULE_STATE_COMING_FINAL, /* Ready to be changed to read only. */
+ MODULE_STATE_GOING, /* Going away. */
+ MODULE_STATE_UNFORMED, /* Still setting it up. */
};
/**
diff --git a/kernel/module.c b/kernel/module.c
index d24fcf2..0905bed 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1805,7 +1805,8 @@ void set_all_modules_text_ro(void)
mutex_lock(module_mutex);
list_for_each_entry_rcu(mod, modules, list) {
- if (mod-state == MODULE_STATE_UNFORMED)
+ if (mod-state == MODULE_STATE_UNFORMED ||
+ mod-state == MODULE_STATE_COMING)
continue;
if ((mod-module_core) (mod-core_text_size)) {
set_page_attributes(mod-module_core,
@@ -3020,6 +3021,13 @@ static int do_init_module(struct module *mod)
blocking_notifier_call_chain(module_notify_list,
MODULE_STATE_COMING, mod);
+ /*
+ * This module must not be changed by set_all_modules_text_ro()
+ * until we get here. Otherwise notifiers that change text
+ * (like ftrace does) will break.
+ */
+ mod-state = MODULE_STATE_COMING_FINAL;
+
/* Set RO and NX regions for core */
set_section_ro_nx(mod-module_core,
mod-core_text_size,
--
印藤隆夫(INDOH Takao)
E-Mail : indou.ta...@jp.fujitsu.com
TEL: 7551-4832(055-924-7241)
富士通(株) PFソ事本)Linux開発統括部 開発部
--
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/
