linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/2] Possible race between load_module() error handling and kprobe registration ?
@ 2016-10-20 16:18 Aaron Tomlin
  2016-10-20 16:18 ` [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code Aaron Tomlin
  2016-10-20 16:18 ` [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too Aaron Tomlin
  0 siblings, 2 replies; 16+ messages in thread
From: Aaron Tomlin @ 2016-10-20 16:18 UTC (permalink / raw)
  To: linux-kernel; +Cc: rusty, rostedt


I think there is a race (albeit a hard-to-hit one) between load_module()
error handling and kprobe registration which could cause a kernel page to
become read-only, panic due to protection fault.

In short, the protection that gets applied [at the bug_cleanup label] can be
overridden by another CPU when executing set_all_modules_text_ro().
Therefore creating the possibility for the kprobe registration code path to
touch a [formed] module that is being deallocated. Consequently we could
free a mapped page, that is not 'writable'. The same page, when later
accessed, will result in a page fault which cannot be handled. Below is an
attempt to illustrate the race. Please note we assume that:

      - kprobe uses ftrace

      - parse_args() or mod_sysfs_setup() would have to fail

      - CPU Y and X do not attempt to load the same module

      - CPU Y would have to sneak in *after* CPU X called the two 'unset'
	functions but before CPU X removes the module from the list of all
	modules

       CPU X
   ...
     load_module
       // Unknown/invalid module
       // parameter specified ...
       after_dashes = parse_args(...)
       if (IS_ERR(after_dashes))
         err = PTR_ERR(after_dashes)
         goto coming_cleanup:
       ...
      bug_cleanup:

       module_disable_ro(mod)
       module_disable_nx(mod)
       ...
       // set_all_modules_text_ro() on CPU Y sneaks in here <-----.
       // and overrides the effects of the previous 'unset'       |
       ...                                                        |
       list_del_rcu(&mod->list)                                   |
                                                                  |
                                                                  |
       CPU Y                                                      |
   ...                                                            |
     sys_finit_module                                             |
       load_module                                                |
         do_init_module                                           |
	   do_one_initcall                                        |
	     // mod->init                                         |
	     kprobe_example_init                                  |
	       register_kprobe                                    |
	         arm_kprobe                                       |
		   // kprobe uses ftrace                          |
		   arm_kprobe_ftrace                              |
		     register_ftrace_function                     |
		       ftrace_startup                             |
		         ftrace_startup_enable                    |
			   ftrace_run_update_code                 |
			     ftrace_arch_code_modify_post_process |
			     {                                    |
			       //                                 |
			       // Set all [formed] module's       |
                               // core and init pages as          |
                               // read-only under                 |
			       // module_mutex ...                |
			       //                                 |
	                       set_all_modules_text_ro() ---------'
			     }



The following patches (I hope) is an attempt to address this theoretical
race.  Please let me know your thoughts.


Aaron Tomlin (2):
  module: Ensure a module's state is set accordingly during module
    coming cleanup code
  module: When modifying a module's text ignore modules which are going
    away too

 kernel/module.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.5.5

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code
  2016-10-20 16:18 [RFC PATCH 0/2] Possible race between load_module() error handling and kprobe registration ? Aaron Tomlin
@ 2016-10-20 16:18 ` Aaron Tomlin
  2016-10-26  0:54   ` Rusty Russell
  2016-10-20 16:18 ` [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too Aaron Tomlin
  1 sibling, 1 reply; 16+ messages in thread
From: Aaron Tomlin @ 2016-10-20 16:18 UTC (permalink / raw)
  To: linux-kernel; +Cc: rusty, rostedt

In load_module() in the event of an error, for e.g. unknown module
parameter(s) specified we go to perform some module coming clean up
operations. At this point the module is still in a "formed" state
when it is actually going away.

This patch updates the module's state accordingly to ensure anyone on the
module_notify_list waiting for a module going away notification will be
notified accordingly.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/module.c b/kernel/module.c
index f57dd63..ff93ab8 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3708,6 +3708,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
  sysfs_cleanup:
 	mod_sysfs_teardown(mod);
  coming_cleanup:
+	mod->state = MODULE_STATE_GOING;
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
 	klp_module_going(mod);
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too
  2016-10-20 16:18 [RFC PATCH 0/2] Possible race between load_module() error handling and kprobe registration ? Aaron Tomlin
  2016-10-20 16:18 ` [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code Aaron Tomlin
@ 2016-10-20 16:18 ` Aaron Tomlin
  2016-10-26  1:05   ` Rusty Russell
  1 sibling, 1 reply; 16+ messages in thread
From: Aaron Tomlin @ 2016-10-20 16:18 UTC (permalink / raw)
  To: linux-kernel; +Cc: rusty, rostedt

By default, during the access permission modification of a module's core
and init pages, we only ignore modules that are malformed. There is no
reason not to extend this to modules which are going away too.

This patch makes both set_all_modules_text_rw() and
set_all_modules_text_ro() skip modules which are going away too.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index ff93ab8..09c386b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1953,7 +1953,8 @@ void set_all_modules_text_rw(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_GOING)
 			continue;
 
 		frob_text(&mod->core_layout, set_memory_rw);
@@ -1969,7 +1970,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_GOING)
 			continue;
 
 		frob_text(&mod->core_layout, set_memory_ro);
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code
  2016-10-20 16:18 ` [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code Aaron Tomlin
@ 2016-10-26  0:54   ` Rusty Russell
  2016-11-09 10:12     ` Jessica Yu
  0 siblings, 1 reply; 16+ messages in thread
From: Rusty Russell @ 2016-10-26  0:54 UTC (permalink / raw)
  To: Aaron Tomlin, linux-kernel; +Cc: rostedt, Jessica Yu

Aaron Tomlin <atomlin@redhat.com> writes:
> In load_module() in the event of an error, for e.g. unknown module
> parameter(s) specified we go to perform some module coming clean up
> operations. At this point the module is still in a "formed" state
> when it is actually going away.
>
> This patch updates the module's state accordingly to ensure anyone on the
> module_notify_list waiting for a module going away notification will be
> notified accordingly.

I recall a similar proposal before.

I've audited all the subscribers to check they didn't look at
mod->state; they seem OK.

We actually do this in the init-failed path, so this should be OK.

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks,
Rusty.

> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>  kernel/module.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index f57dd63..ff93ab8 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3708,6 +3708,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>   sysfs_cleanup:
>  	mod_sysfs_teardown(mod);
>   coming_cleanup:
> +	mod->state = MODULE_STATE_GOING;
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
>  	klp_module_going(mod);
> -- 
> 2.5.5

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too
  2016-10-20 16:18 ` [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too Aaron Tomlin
@ 2016-10-26  1:05   ` Rusty Russell
  2016-10-26 12:09     ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Rusty Russell @ 2016-10-26  1:05 UTC (permalink / raw)
  To: Aaron Tomlin, linux-kernel; +Cc: rostedt

Aaron Tomlin <atomlin@redhat.com> writes:
> By default, during the access permission modification of a module's core
> and init pages, we only ignore modules that are malformed. There is no
> reason not to extend this to modules which are going away too.

Well, it depends on all the callers (ie. ftrace): is that also ignoring
modules which are going away?

Otherwise, we set MODULE_STATE_GOING, ftrace walks all the modules and
this one is still RO...

Thanks,
Rusty.

> This patch makes both set_all_modules_text_rw() and
> set_all_modules_text_ro() skip modules which are going away too.
>
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>  kernel/module.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index ff93ab8..09c386b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1953,7 +1953,8 @@ void set_all_modules_text_rw(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_GOING)
>  			continue;
>  
>  		frob_text(&mod->core_layout, set_memory_rw);
> @@ -1969,7 +1970,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_GOING)
>  			continue;
>  
>  		frob_text(&mod->core_layout, set_memory_ro);
> -- 
> 2.5.5

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too
  2016-10-26  1:05   ` Rusty Russell
@ 2016-10-26 12:09     ` Steven Rostedt
  2016-10-27  9:36       ` [RFC PATCH v2 " Aaron Tomlin
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2016-10-26 12:09 UTC (permalink / raw)
  To: Rusty Russell; +Cc: Aaron Tomlin, linux-kernel

On Wed, 26 Oct 2016 11:35:18 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

> Aaron Tomlin <atomlin@redhat.com> writes:
> > By default, during the access permission modification of a module's core
> > and init pages, we only ignore modules that are malformed. There is no
> > reason not to extend this to modules which are going away too.  
> 
> Well, it depends on all the callers (ie. ftrace): is that also ignoring
> modules which are going away?
> 
> Otherwise, we set MODULE_STATE_GOING, ftrace walks all the modules and
> this one is still RO...
> 

Actually, looking into this more, you are correct. There's a
possibility in enabling ftrace after the module is about to go but
before ftrace_release_mod() is called (which will remove the module
text from the ftrace function list).

I don't see any reason for not allowing set_all_modules_text_rw() from
being called if a module is going. If a module is going, shouldn't its
text be rw anyway?

Perhaps just preventing it from turning into ro will be sufficient. And
remove the check from set_all_modules_text_rw().

-- Steve


> Thanks,
> Rusty.
> 
> > This patch makes both set_all_modules_text_rw() and
> > set_all_modules_text_ro() skip modules which are going away too.
> >
> > Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> > ---
> >  kernel/module.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index ff93ab8..09c386b 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -1953,7 +1953,8 @@ void set_all_modules_text_rw(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_GOING)
> >  			continue;
> >  
> >  		frob_text(&mod->core_layout, set_memory_rw);
> > @@ -1969,7 +1970,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_GOING)
> >  			continue;
> >  
> >  		frob_text(&mod->core_layout, set_memory_ro);
> > -- 
> > 2.5.5  

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [RFC PATCH v2 2/2] module: When modifying a module's text ignore modules which are going away too
  2016-10-26 12:09     ` Steven Rostedt
@ 2016-10-27  9:36       ` Aaron Tomlin
  2016-10-27 13:49         ` Steven Rostedt
  2016-11-18  6:15         ` [RFC PATCH v2 2/2] " Rusty Russell
  0 siblings, 2 replies; 16+ messages in thread
From: Aaron Tomlin @ 2016-10-27  9:36 UTC (permalink / raw)
  To: linux-kernel; +Cc: rusty, rostedt

By default, during the access permission modification of a module's core
and init pages, we only ignore modules that are malformed. Albeit for a
module which is going away, it does not make sense to change its text to
RO since the module should be RW, before deallocation.

This patch makes set_all_modules_text_ro() skip modules which are going
away too.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index ff93ab8..2a383df 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1969,7 +1969,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_GOING)
 			continue;
 
 		frob_text(&mod->core_layout, set_memory_ro);
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH v2 2/2] module: When modifying a module's text ignore modules which are going away too
  2016-10-27  9:36       ` [RFC PATCH v2 " Aaron Tomlin
@ 2016-10-27 13:49         ` Steven Rostedt
  2016-11-07 11:46           ` Aaron Tomlin
  2016-11-18  6:15         ` [RFC PATCH v2 2/2] " Rusty Russell
  1 sibling, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2016-10-27 13:49 UTC (permalink / raw)
  To: Aaron Tomlin; +Cc: linux-kernel, rusty, Jessica Yu


This looks line to me. Rusty, do you have any issues with this?

Maybe we should add a comment to why a going module shouldn't be
converted to ro (because of ftrace and kprobes). But other than that,
I have no issue with it.

I also added Jessica to the Cc as I notice she will be the new module
maintainer: http://lwn.net/Articles/704653/

-- Steve


On Thu, 27 Oct 2016 10:36:06 +0100
Aaron Tomlin <atomlin@redhat.com> wrote:

> By default, during the access permission modification of a module's core
> and init pages, we only ignore modules that are malformed. Albeit for a
> module which is going away, it does not make sense to change its text to
> RO since the module should be RW, before deallocation.
> 
> This patch makes set_all_modules_text_ro() skip modules which are going
> away too.
> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>  kernel/module.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index ff93ab8..2a383df 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1969,7 +1969,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_GOING)
>  			continue;
>  
>  		frob_text(&mod->core_layout, set_memory_ro);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH v2 2/2] module: When modifying a module's text ignore modules which are going away too
  2016-10-27 13:49         ` Steven Rostedt
@ 2016-11-07 11:46           ` Aaron Tomlin
  2016-11-09 10:40             ` Jessica Yu
  0 siblings, 1 reply; 16+ messages in thread
From: Aaron Tomlin @ 2016-11-07 11:46 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-kernel, rusty, Jessica Yu

On Thu 2016-10-27 09:49 -0400, Steven Rostedt wrote:
[ ... ]
> I also added Jessica to the Cc as I notice she will be the new module
> maintainer: http://lwn.net/Articles/704653/

Hi Jessica,

Any thoughts?

Thanks,

-- 
Aaron Tomlin

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: module: Ensure a module's state is set accordingly during module coming cleanup code
  2016-10-26  0:54   ` Rusty Russell
@ 2016-11-09 10:12     ` Jessica Yu
  2016-11-16 15:49       ` Miroslav Benes
  0 siblings, 1 reply; 16+ messages in thread
From: Jessica Yu @ 2016-11-09 10:12 UTC (permalink / raw)
  To: Rusty Russell; +Cc: Aaron Tomlin, linux-kernel, rostedt

+++ Rusty Russell [26/10/16 11:24 +1030]:
>Aaron Tomlin <atomlin@redhat.com> writes:
>> In load_module() in the event of an error, for e.g. unknown module
>> parameter(s) specified we go to perform some module coming clean up
>> operations. At this point the module is still in a "formed" state
>> when it is actually going away.
>>
>> This patch updates the module's state accordingly to ensure anyone on the
>> module_notify_list waiting for a module going away notification will be
>> notified accordingly.
>
>I recall a similar proposal before.
>
>I've audited all the subscribers to check they didn't look at
>mod->state; they seem OK.
>
>We actually do this in the init-failed path, so this should be OK.

We did discuss a similar proposal before:

    https://lkml.kernel.org/r/87a8m7ko6j.fsf@rustcorp.com.au

The complaint back then was that we need to be in the COMING state for
strong_try_module_get() to fail. But it will also correctly fail for GOING
modules in the module_is_live() check in the subsequent call to
try_module_get(), so I believe we are still OK here.

Jessica

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: module: When modifying a module's text ignore modules which are going away too
  2016-11-07 11:46           ` Aaron Tomlin
@ 2016-11-09 10:40             ` Jessica Yu
  2016-11-23 16:00               ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Jessica Yu @ 2016-11-09 10:40 UTC (permalink / raw)
  To: Aaron Tomlin; +Cc: Steven Rostedt, linux-kernel, rusty

+++ Aaron Tomlin [07/11/16 11:46 +0000]:
>Hi Jessica,
>
>Any thoughts?

Hi Aaron,

Thanks for your patience as I slowly work through a large swath of emails :-)

Anyway, this looks fine to me. A going module's text should be (or
soon will be) rw anyway, so checking for going modules in the ro
case should be enough.

Rusty, if you give your ack for the second patch, I can apply both
patches to my modules-next branch. I'll also incorporate Steven's
suggestion for a comment explaining why going modules shouldn't be
converted to ro in this context.

Thanks,
Jessica

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: module: Ensure a module's state is set accordingly during module coming cleanup code
  2016-11-09 10:12     ` Jessica Yu
@ 2016-11-16 15:49       ` Miroslav Benes
  2016-11-23 23:41         ` Jessica Yu
  0 siblings, 1 reply; 16+ messages in thread
From: Miroslav Benes @ 2016-11-16 15:49 UTC (permalink / raw)
  To: Jessica Yu; +Cc: Rusty Russell, Aaron Tomlin, linux-kernel, rostedt

On Wed, 9 Nov 2016, Jessica Yu wrote:

> +++ Rusty Russell [26/10/16 11:24 +1030]:
> > Aaron Tomlin <atomlin@redhat.com> writes:
> > > In load_module() in the event of an error, for e.g. unknown module
> > > parameter(s) specified we go to perform some module coming clean up
> > > operations. At this point the module is still in a "formed" state
> > > when it is actually going away.
> > > 
> > > This patch updates the module's state accordingly to ensure anyone on the
> > > module_notify_list waiting for a module going away notification will be
> > > notified accordingly.
> > 
> > I recall a similar proposal before.
> > 
> > I've audited all the subscribers to check they didn't look at
> > mod->state; they seem OK.
> > 
> > We actually do this in the init-failed path, so this should be OK.
> 
> We did discuss a similar proposal before:
> 
>    https://lkml.kernel.org/r/87a8m7ko6j.fsf@rustcorp.com.au
> 
> The complaint back then was that we need to be in the COMING state for
> strong_try_module_get() to fail. But it will also correctly fail for GOING
> modules in the module_is_live() check in the subsequent call to
> try_module_get(), so I believe we are still OK here.

FWIW, I looked and this is true. Even the error -ENOENT could be better in 
this case than -EBUSY (since the module is going away).

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

for the patch, if you want it.

Anyway, the comment above strong_try_module_get() is not true for almost 9 
nine years. So how about something like:

-->8--

>From 872e11394fdaba8fb9a333e114dc92273d2d1bf5 Mon Sep 17 00:00:00 2001
From: Miroslav Benes <mbenes@suse.cz>
Date: Wed, 16 Nov 2016 16:45:48 +0100
Subject: [PATCH] module: Fix a comment above strong_try_module_get()

The comment above strong_try_module_get() function is not true anymore.
Return values changed with commit c9a3ba55bb5d ("module: wait for
dependent modules doing init.").

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/module.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index f57dd63186e6..67160ca8110e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -313,8 +313,9 @@ struct load_info {
 	} index;
 };
 
-/* We require a truly strong try_module_get(): 0 means failure due to
-   ongoing or failed initialization etc. */
+/* We require a truly strong try_module_get(): 0 means success.
+ * Otherwise an error is returned due to ongoing or failed
+ * initialization etc. */
 static inline int strong_try_module_get(struct module *mod)
 {
 	BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED);
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH v2 2/2] module: When modifying a module's text ignore modules which are going away too
  2016-10-27  9:36       ` [RFC PATCH v2 " Aaron Tomlin
  2016-10-27 13:49         ` Steven Rostedt
@ 2016-11-18  6:15         ` Rusty Russell
  1 sibling, 0 replies; 16+ messages in thread
From: Rusty Russell @ 2016-11-18  6:15 UTC (permalink / raw)
  To: Aaron Tomlin, linux-kernel; +Cc: rostedt, Jessica Yu

Aaron Tomlin <atomlin@redhat.com> writes:
> By default, during the access permission modification of a module's core
> and init pages, we only ignore modules that are malformed. Albeit for a
> module which is going away, it does not make sense to change its text to
> RO since the module should be RW, before deallocation.
>
> This patch makes set_all_modules_text_ro() skip modules which are going
> away too.
>
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks!
Rusty.

> ---
>  kernel/module.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index ff93ab8..2a383df 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1969,7 +1969,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_GOING)
>  			continue;
>  
>  		frob_text(&mod->core_layout, set_memory_ro);
> -- 
> 2.5.5

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: module: When modifying a module's text ignore modules which are going away too
  2016-11-09 10:40             ` Jessica Yu
@ 2016-11-23 16:00               ` Steven Rostedt
  2016-11-23 18:33                 ` Jessica Yu
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2016-11-23 16:00 UTC (permalink / raw)
  To: Jessica Yu; +Cc: Aaron Tomlin, linux-kernel, rusty

On Wed, 9 Nov 2016 05:40:58 -0500
Jessica Yu <jeyu@redhat.com> wrote:

> +++ Aaron Tomlin [07/11/16 11:46 +0000]:
> >Hi Jessica,
> >
> >Any thoughts?  
> 
> Hi Aaron,
> 
> Thanks for your patience as I slowly work through a large swath of emails :-)
> 
> Anyway, this looks fine to me. A going module's text should be (or
> soon will be) rw anyway, so checking for going modules in the ro
> case should be enough.
> 
> Rusty, if you give your ack for the second patch, I can apply both
> patches to my modules-next branch. I'll also incorporate Steven's
> suggestion for a comment explaining why going modules shouldn't be
> converted to ro in this context.
> 

Hi Jessica,

Have you pulled these in? I haven't noticed them in linux-next.

-- Steve

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: module: When modifying a module's text ignore modules which are going away too
  2016-11-23 16:00               ` Steven Rostedt
@ 2016-11-23 18:33                 ` Jessica Yu
  0 siblings, 0 replies; 16+ messages in thread
From: Jessica Yu @ 2016-11-23 18:33 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Aaron Tomlin, linux-kernel, rusty

+++ Steven Rostedt [23/11/16 11:00 -0500]:
>On Wed, 9 Nov 2016 05:40:58 -0500
>Jessica Yu <jeyu@redhat.com> wrote:
>
>> +++ Aaron Tomlin [07/11/16 11:46 +0000]:
>> >Hi Jessica,
>> >
>> >Any thoughts?
>>
>> Hi Aaron,
>>
>> Thanks for your patience as I slowly work through a large swath of emails :-)
>>
>> Anyway, this looks fine to me. A going module's text should be (or
>> soon will be) rw anyway, so checking for going modules in the ro
>> case should be enough.
>>
>> Rusty, if you give your ack for the second patch, I can apply both
>> patches to my modules-next branch. I'll also incorporate Steven's
>> suggestion for a comment explaining why going modules shouldn't be
>> converted to ro in this context.
>>
>
>Hi Jessica,
>
>Have you pulled these in? I haven't noticed them in linux-next.

I currently have this queued up for 4.10. I'm still clearing up some
(unrelated to this patch) maintainership transition questions, but
expect the new modules tree to be pushed out and included in -next
by this week.

Thanks,
Jessica

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: module: Ensure a module's state is set accordingly during module coming cleanup code
  2016-11-16 15:49       ` Miroslav Benes
@ 2016-11-23 23:41         ` Jessica Yu
  0 siblings, 0 replies; 16+ messages in thread
From: Jessica Yu @ 2016-11-23 23:41 UTC (permalink / raw)
  To: Miroslav Benes; +Cc: Rusty Russell, Aaron Tomlin, linux-kernel, rostedt

+++ Miroslav Benes [16/11/16 16:49 +0100]:
>On Wed, 9 Nov 2016, Jessica Yu wrote:
>
>> +++ Rusty Russell [26/10/16 11:24 +1030]:
>> > Aaron Tomlin <atomlin@redhat.com> writes:
>> > > In load_module() in the event of an error, for e.g. unknown module
>> > > parameter(s) specified we go to perform some module coming clean up
>> > > operations. At this point the module is still in a "formed" state
>> > > when it is actually going away.
>> > >
>> > > This patch updates the module's state accordingly to ensure anyone on the
>> > > module_notify_list waiting for a module going away notification will be
>> > > notified accordingly.
>> >
>> > I recall a similar proposal before.
>> >
>> > I've audited all the subscribers to check they didn't look at
>> > mod->state; they seem OK.
>> >
>> > We actually do this in the init-failed path, so this should be OK.
>>
>> We did discuss a similar proposal before:
>>
>>    https://lkml.kernel.org/r/87a8m7ko6j.fsf@rustcorp.com.au
>>
>> The complaint back then was that we need to be in the COMING state for
>> strong_try_module_get() to fail. But it will also correctly fail for GOING
>> modules in the module_is_live() check in the subsequent call to
>> try_module_get(), so I believe we are still OK here.
>
>FWIW, I looked and this is true. Even the error -ENOENT could be better in
>this case than -EBUSY (since the module is going away).
>
>Reviewed-by: Miroslav Benes <mbenes@suse.cz>
>
>for the patch, if you want it.
>
>Anyway, the comment above strong_try_module_get() is not true for almost 9
>nine years. So how about something like:
>
>-->8--
>
>From 872e11394fdaba8fb9a333e114dc92273d2d1bf5 Mon Sep 17 00:00:00 2001
>From: Miroslav Benes <mbenes@suse.cz>
>Date: Wed, 16 Nov 2016 16:45:48 +0100
>Subject: [PATCH] module: Fix a comment above strong_try_module_get()
>
>The comment above strong_try_module_get() function is not true anymore.
>Return values changed with commit c9a3ba55bb5d ("module: wait for
>dependent modules doing init.").
>
>Signed-off-by: Miroslav Benes <mbenes@suse.cz>

Thanks Miroslav, that comment was confusing and needed updating. I've
queued this on top of the other patches.

Jessica

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2016-11-23 23:42 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-20 16:18 [RFC PATCH 0/2] Possible race between load_module() error handling and kprobe registration ? Aaron Tomlin
2016-10-20 16:18 ` [RFC PATCH 1/2] module: Ensure a module's state is set accordingly during module coming cleanup code Aaron Tomlin
2016-10-26  0:54   ` Rusty Russell
2016-11-09 10:12     ` Jessica Yu
2016-11-16 15:49       ` Miroslav Benes
2016-11-23 23:41         ` Jessica Yu
2016-10-20 16:18 ` [RFC PATCH 2/2] module: When modifying a module's text ignore modules which are going away too Aaron Tomlin
2016-10-26  1:05   ` Rusty Russell
2016-10-26 12:09     ` Steven Rostedt
2016-10-27  9:36       ` [RFC PATCH v2 " Aaron Tomlin
2016-10-27 13:49         ` Steven Rostedt
2016-11-07 11:46           ` Aaron Tomlin
2016-11-09 10:40             ` Jessica Yu
2016-11-23 16:00               ` Steven Rostedt
2016-11-23 18:33                 ` Jessica Yu
2016-11-18  6:15         ` [RFC PATCH v2 2/2] " Rusty Russell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).