Steven Rostedt <[email protected]> writes:
> On Mon, 24 Mar 2014 20:26:05 +0900
> Masami Hiramatsu <[email protected]> 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 <[email protected]>
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 <[email protected]>
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);
(2014/04/22 12:51), Rusty Russell wrote:
> Steven Rostedt <[email protected]> writes:
>> On Mon, 24 Mar 2014 20:26:05 +0900
>> Masami Hiramatsu <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
>
> 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);
>
>
(2014/04/22 14:29), Takao Indoh wrote:
> (2014/04/22 12:51), Rusty Russell wrote:
>> Steven Rostedt <[email protected]> writes:
>>> On Mon, 24 Mar 2014 20:26:05 +0900
>>> Masami Hiramatsu <[email protected]> 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 <[email protected]>
>>
>> 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: [email protected]
(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 <[email protected]> writes:
>>>> On Mon, 24 Mar 2014 20:26:05 +0900
>>>> Masami Hiramatsu <[email protected]> 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 <[email protected]>
>>>
>>> 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
On Tue, 22 Apr 2014 13:21:18 +0930
Rusty Russell <[email protected]> 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 <[email protected]>
>
> 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);
(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: [email protected]
On Wed, 23 Apr 2014 10:26:00 +0900
Masami Hiramatsu <[email protected]> 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
(2014/04/23 10:56), Steven Rostedt wrote:
> On Wed, 23 Apr 2014 10:26:00 +0900
> Masami Hiramatsu <[email protected]> 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: [email protected]
(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 <[email protected]> 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
Steven Rostedt <[email protected]> writes:
> On Tue, 22 Apr 2014 13:21:18 +0930
> Rusty Russell <[email protected]> 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.
On Thu, 24 Apr 2014 17:08:56 +0930
Rusty Russell <[email protected]> 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
On Thu, 24 Apr 2014 15:58:53 +0900
Takao Indoh <[email protected]> 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