Re: [PATCH] ftrace: fix race between ftrace and insmod

[Zhang, Yanmin]

On 2015/12/14 23:51, Steven Rostedt wrote:
> On Mon, 14 Dec 2015 11:16:18 +0800
> "Qiu, PeiyangX" <peiyangx.qiu@xxxxxxxxx> wrote:
>
>> We hit ftrace_bug report when booting Android on a 64bit ATOM SOC chip.
>> Basically, there is a race between insmod and ftrace_run_update_code.
>>
>> After load_module=>ftrace_module_init, another thread jumps in to call
>> ftrace_run_update_code=>ftrace_arch_code_modify_prepare
>>                          =>set_all_modules_text_rw, to change all modules  
>> as RW. Since the new module is at MODULE_STATE_UNFORMED, the text attribute
>> is not changed. Then, the 2nd thread goes ahead to change codes.
>> However, load_module continues to call 
>> complete_formation=>set_section_ro_nx,  
>> then 2nd thread would fail when probing the module's TEXT.
>>
>> Below patch tries to resolve it.
>>
>> commit a949ae560a511fe4e3adf48fa44fefded93e5c2b
>> Author: Steven Rostedt (Red Hat) <rostedt@xxxxxxxxxxx>
>> Date:   Thu Apr 24 10:40:12 2014 -0400
>>
>>      ftrace/module: Hardcode ftrace_module_init() call into load_module()
>>
>> But it can't fully resolve the issue.
>>
>> THis patch holds ftrace_mutex across ftrace_module_init and
>> complete_formation.
>>
>> Signed-off-by: Qiu Peiyang <peiyangx.qiu@xxxxxxxxx>
>> Signed-off-by: Zhang Yanmin <yanmin.zhang@xxxxxxxxx>
> First, this patch has major whitespace damage. All tabs are now spaces!

Sorry. That's really bad. It's the 1st time for Peiyang to send patch to LKML.
He would fix such issue by reconfiguring email client.

>
>> ---
>>   include/linux/ftrace.h |  6 ++++--
>>   kernel/module.c        |  3 ++-
>>   kernel/trace/ftrace.c  | 33 ++++++++++++++++++++++-----------
>>   3 files changed, 28 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
>> index eae6548..4adc279 100644
>> --- a/include/linux/ftrace.h
>> +++ b/include/linux/ftrace.h
>> @@ -585,7 +585,8 @@ static inline int ftrace_modify_call(struct 
>> dyn_ftrace *rec, unsigned long old_a
>>   extern int ftrace_arch_read_dyn_info(char *buf, int size);
>>
>>   extern int skip_trace(unsigned long ip);
>> -extern void ftrace_module_init(struct module *mod);
>> +extern void ftrace_module_init_start(struct module *mod);
>> +extern void ftrace_module_init_end(struct module *mod);
>>
>>   extern void ftrace_disable_daemon(void);
>>   extern void ftrace_enable_daemon(void);
>> @@ -595,7 +596,8 @@ static inline int ftrace_force_update(void) { return 
>> 0; }
>>   static inline void ftrace_disable_daemon(void) { }
>>   static inline void ftrace_enable_daemon(void) { }
>>   static inline void ftrace_release_mod(struct module *mod) {}
>> -static inline void ftrace_module_init(struct module *mod) {}
>> +static inline void ftrace_module_init_start(struct module *mod) {}
>> +static inline void ftrace_module_init_end(struct module *mod) {}
>>   static inline __init int register_ftrace_command(struct 
>> ftrace_func_command *cmd)
>>   {
>>       return -EINVAL;
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 8f051a1..324c5c6 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3506,10 +3506,11 @@ static int load_module(struct load_info *info, 
>> const char __user *uargs,
>>       dynamic_debug_setup(info->debug, info->num_debug);
>>
>>       /* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
>> -    ftrace_module_init(mod);
>> +    ftrace_module_init_start(mod);
>>
>>       /* Finally it's fully formed, ready to start executing. */
>>       err = complete_formation(mod, info);
>> +    ftrace_module_init_end(mod);
> Instead of modifying the module code even more, why not just put this
> into the module notifier when done?
>
> Then this would only need to change the ftrace code. Although, holding
> a lock throughout seems rather hacky.

We did consider notifier. When ftrace_module_init is called, start_pg of the
module is linked to ftrace_pages. To fix the issue, we have to either add a
new var to struct module to save its start_pg, or change ftrace to save
module start_pg before MODULE_STATE_COMING notification is delivered.
In addition, function complete_formation need consider cleanup the
start_pg if init fails.

>
>
>
>>       if (err)
>>           goto ddebug_cleanup;
>>
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index 3f743b1..436c199 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -4795,7 +4795,7 @@ static int ftrace_cmp_ips(const void *a, const 
>> void *b)
>>       return 0;
>>   }
>>
>> -static int ftrace_process_locs(struct module *mod,
>> +static int ftrace_process_locs_nolock(struct module *mod,
> Note, the usually method for functions in the tracing code that do not
> take locks is to simply prepend a double underscore (__) in front of
> the function name: __ftrace_process_locs()

Yes. We would change the function name.

>
>
>>                      unsigned long *start,
>>                      unsigned long *end)
>>   {
>> @@ -4820,8 +4820,6 @@ static int ftrace_process_locs(struct module *mod,
>>       if (!start_pg)
>>           return -ENOMEM;
>>
>> -    mutex_lock(&ftrace_lock);
>> -
>>       /*
>>        * Core and each module needs their own pages, as
>>        * modules will free them when they are removed.
>> @@ -4889,8 +4887,18 @@ static int ftrace_process_locs(struct module *mod,
>>           local_irq_restore(flags);
>>       ret = 0;
>>    out:
>> -    mutex_unlock(&ftrace_lock);
>> +    return ret;
>> +}
> We can remove the out, and all the "goto out" can be changed to return.

Yes, indeed. We would add such cleanup codes.

>
>> +static int ftrace_process_locs(struct module *mod,
>> +                   unsigned long *start,
>> +                   unsigned long *end)
>> +{
>> +    int ret;
>> +
>> +    mutex_lock(&ftrace_lock);
>> +    ret = ftrace_process_locs_nolock(mod, start, end);
>> +    mutex_unlock(&ftrace_lock);
>>       return ret;
>>   }
>>
>> @@ -4940,19 +4948,22 @@ void ftrace_release_mod(struct module *mod)
>>       mutex_unlock(&ftrace_lock);
>>   }
>>
>> -static void ftrace_init_module(struct module *mod,
>> -                   unsigned long *start, unsigned long *end)
>> +void ftrace_module_init_start(struct module *mod)
>>   {
>> +    unsigned long *start = mod->ftrace_callsites;
>> +    unsigned long *end = mod->ftrace_callsites + mod->num_ftrace_callsites;
>> +
>> +    mutex_lock(&ftrace_lock);
>> +
>>       if (ftrace_disabled || start == end)
>>           return;
>> -    ftrace_process_locs(mod, start, end);
>> +
>> +    ftrace_process_locs_nolock(mod, start, end);
>>   }
>>
>> -void ftrace_module_init(struct module *mod)
>> +void ftrace_module_init_end(struct module *mod)
>>   {
>> -    ftrace_init_module(mod, mod->ftrace_callsites,
>> -               mod->ftrace_callsites +
>> -               mod->num_ftrace_callsites);
>> +    mutex_unlock(&ftrace_lock);
>>   }
>>
>>   static int ftrace_module_notify_exit(struct notifier_block *self,
> This seems very hackish, although I can't think of a better way at the
> moment. But I would like not to add more code into module.c if
> possible, and just use a notifier unless there's a real reason we can't
> (as there was when we added the hook in the first place).

We would double-check it again. It's not a good idea to change common
module codes.

Thanks for your kind comments.

Yanmin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/