Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1302305 > unrolled thread

Re: [PATCH 2/2] module: deal with the failure of complete_formation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] module: deal with the failure of complete_formation Steven Rostedt <rostedt@goodmis.org> - 2016-01-06 02:10 +0100
    Re: [PATCH 2/2] module: deal with the failure of complete_formation "Zhang, Yanmin" <yanmin_zhang@linux.intel.com> - 2016-01-06 02:20 +0100
      Re: [PATCH 2/2] module: deal with the failure of complete_formation Steven Rostedt <rostedt@goodmis.org> - 2016-01-06 02:30 +0100
        Re: [PATCH 2/2] module: deal with the failure of complete_formation "Zhang, Yanmin" <yanmin_zhang@linux.intel.com> - 2016-01-06 02:50 +0100
          Re: [PATCH 2/2] module: deal with the failure of complete_formation Steven Rostedt <rostedt@goodmis.org> - 2016-01-06 03:00 +0100

#1302305 — Re: [PATCH 2/2] module: deal with the failure of complete_formation

On Fri, 25 Dec 2015 15:03:13 +0800
"Qiu, PeiyangX" <peiyangx.qiu@intel.com> wrote:

> From: Qiu Peiyang <peiyangx.qiu@intel.com>
> 
> complete_formation might fail. kernel need clean up
> ftrace records of the module.
> 
> The patch fixes it by tuning the operation sequence in
> complete_formation. After complete_formation checks
> verify_export_symbols, call ftrace_module_init to init
> ftrace records.
> 
> Signed-off-by: Qiu Peiyang <peiyangx.qiu@intel.com>
> Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
> ---
>  kernel/module.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 8f051a1..0a67c4e 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3373,6 +3373,11 @@ 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);
>  
> +	mutex_unlock(&module_mutex);
> +

First of all, this is buggy. You can't just move the locking of
module_mutex. That is needed to modify mod->state.

Second of all, you are solving this wrong. I guess we should add
ftrace_module_init_fail() function to cover the cases where the module
can fail before calling do_init_module(), as if that happens it does
not call the module going notifiers.

I'll be sending this to stable too.

-- Steve

---
 include/linux/ftrace.h |    2 ++
 kernel/module.c        |    1 +
 kernel/trace/ftrace.c  |    5 +++++
 3 files changed, 8 insertions(+)

Index: linux-trace.git/include/linux/ftrace.h
===================================================================
--- linux-trace.git.orig/include/linux/ftrace.h	2016-01-05 18:00:11.686173290 -0500
+++ linux-trace.git/include/linux/ftrace.h	2016-01-05 18:00:47.815631131 -0500
@@ -602,6 +602,7 @@ extern int ftrace_arch_read_dyn_info(cha
 
 extern int skip_trace(unsigned long ip);
 extern void ftrace_module_init(struct module *mod);
+extern void ftrace_module_init_fail(struct module *mod);
 
 extern void ftrace_disable_daemon(void);
 extern void ftrace_enable_daemon(void);
@@ -612,6 +613,7 @@ static inline void ftrace_disable_daemon
 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_fail(struct module *mod) {}
 static inline __init int register_ftrace_command(struct ftrace_func_command *cmd)
 {
 	return -EINVAL;
Index: linux-trace.git/kernel/module.c
===================================================================
--- linux-trace.git.orig/kernel/module.c	2016-01-05 18:00:11.687173275 -0500
+++ linux-trace.git/kernel/module.c	2016-01-05 18:00:47.816631117 -0500
@@ -3571,6 +3571,7 @@ static int load_module(struct load_info
 	synchronize_sched();
 	mutex_unlock(&module_mutex);
  free_module:
+	ftrace_module_init_fail(mod);
 	/* Free lock-classes; relies on the preceding sync_rcu() */
 	lockdep_free_key_range(mod->module_core, mod->core_size);
 
Index: linux-trace.git/kernel/trace/ftrace.c
===================================================================
--- linux-trace.git.orig/kernel/trace/ftrace.c	2016-01-05 18:00:47.817631101 -0500
+++ linux-trace.git/kernel/trace/ftrace.c	2016-01-05 18:01:10.418291635 -0500
@@ -4989,6 +4989,11 @@ void ftrace_module_init(struct module *m
 			    mod->ftrace_callsites + mod->num_ftrace_callsites);
 }
 
+void ftrace_module_init_fail(struct module *mod)
+{
+	ftrace_release_mod(mod);
+}
+
 static int ftrace_module_notify_exit(struct notifier_block *self,
 				     unsigned long val, void *data)
 {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1302309

On 2016/1/6 9:01, Steven Rostedt wrote:
> On Fri, 25 Dec 2015 15:03:13 +0800
> "Qiu, PeiyangX" <peiyangx.qiu@intel.com> wrote:
>
>> From: Qiu Peiyang <peiyangx.qiu@intel.com>
>>
>> complete_formation might fail. kernel need clean up
>> ftrace records of the module.
>>
>> The patch fixes it by tuning the operation sequence in
>> complete_formation. After complete_formation checks
>> verify_export_symbols, call ftrace_module_init to init
>> ftrace records.
>>
>> Signed-off-by: Qiu Peiyang <peiyangx.qiu@intel.com>
>> Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
>> ---
>>  kernel/module.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 8f051a1..0a67c4e 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3373,6 +3373,11 @@ 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);
>>  
>> +	mutex_unlock(&module_mutex);
>> +
> First of all, this is buggy. You can't just move the locking of
> module_mutex. That is needed to modify mod->state.
>
> Second of all, you are solving this wrong. I guess we should add
> ftrace_module_init_fail() function to cover the cases where the module
> can fail before calling do_init_module(), as if that happens it does
> not call the module going notifiers.

It's a good idea although ftrace_module_init_fail might be complicated.

Thanks,
Yanmin

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

[toc] | [prev] | [next] | [standalone]

#1302313

On Wed, 06 Jan 2016 09:14:42 +0800
"Zhang, Yanmin" <yanmin_zhang@linux.intel.com> wrote:


> It's a good idea although ftrace_module_init_fail might be complicated.

How so? I posted the patch. All it does is to call
ftrace_release_mod(), which will only remove pages that are included
with the module. If the pages were not added yet, nothing is done.

-- Steve

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

[toc] | [prev] | [next] | [standalone]

#1302321

On 2016/1/6 9:29, Steven Rostedt wrote:
> On Wed, 06 Jan 2016 09:14:42 +0800
> "Zhang, Yanmin" <yanmin_zhang@linux.intel.com> wrote:
>
>
>> It's a good idea although ftrace_module_init_fail might be complicated.
> How so? I posted the patch. All it does is to call
> ftrace_release_mod(), which will only remove pages that are included
> with the module. If the pages were not added yet, nothing is done.

You are right. ftrace_release_mod can be used.

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

[toc] | [prev] | [next] | [standalone]

#1302331

On Wed, 06 Jan 2016 09:48:42 +0800
"Zhang, Yanmin" <yanmin_zhang@linux.intel.com> wrote:

> On 2016/1/6 9:29, Steven Rostedt wrote:
> > On Wed, 06 Jan 2016 09:14:42 +0800
> > "Zhang, Yanmin" <yanmin_zhang@linux.intel.com> wrote:
> >
> >  
> >> It's a good idea although ftrace_module_init_fail might be complicated.  
> > How so? I posted the patch. All it does is to call
> > ftrace_release_mod(), which will only remove pages that are included
> > with the module. If the pages were not added yet, nothing is done.  
> 
> You are right. ftrace_release_mod can be used.

I was going to call that directly, but for clarity, documentation and
robustness I added the new call. Hmm, but on second thought, I noticed
that it is already shown globally. I think I'll change that back to call
ftrace_release_mod() directly.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web