[PATCH] module: set MODULE_STATE_GOING state when a module fails to load

[PATCH] module: set MODULE_STATE_GOING state when a module fails to load

[Miroslav Benes]

If a module fails to load due to an error in prepare_coming_module(),
the following error handling in load_module() runs with
MODULE_STATE_COMING in module's state. Fix it by correctly setting
MODULE_STATE_GOING under "bug_cleanup" label.

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

diff --git a/kernel/module.c b/kernel/module.c
index a4fa44a652a7..b34235082394 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3991,6 +3991,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 				     MODULE_STATE_GOING, mod);
 	klp_module_going(mod);
  bug_cleanup:
+	mod->state = MODULE_STATE_GOING;
 	/* module_bug_cleanup needs module_mutex protection */
 	mutex_lock(&module_mutex);
 	module_bug_cleanup(mod);
-- 
2.29.0

Re: [PATCH] module: set MODULE_STATE_GOING state when a module fails to load

[Jessica Yu]

+++ Miroslav Benes [27/10/20 15:03 +0100]:
>If a module fails to load due to an error in prepare_coming_module(),
>the following error handling in load_module() runs with
>MODULE_STATE_COMING in module's state. Fix it by correctly setting
>MODULE_STATE_GOING under "bug_cleanup" label.
>
>Signed-off-by: Miroslav Benes <mbenes@suse.cz>
>---
> kernel/module.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index a4fa44a652a7..b34235082394 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -3991,6 +3991,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> 				     MODULE_STATE_GOING, mod);
> 	klp_module_going(mod);
>  bug_cleanup:
>+	mod->state = MODULE_STATE_GOING;
> 	/* module_bug_cleanup needs module_mutex protection */
> 	mutex_lock(&module_mutex);
> 	module_bug_cleanup(mod);

Thanks for the fix! Hmm, I am wondering if we also need to set the
module to GOING if it happens to fail while it is still UNFORMED.

Currently, when a module is UNFORMED and encounters an error during
load_module(), it stays UNFORMED until it finally goes away. That
sounds fine, but try_module_get() technically permits you to get a
module while it's UNFORMED (but not if it's GOING). Theoretically
someone could increase the refcount of an unformed module that has
encountered an error condition and is in the process of going away.
This shouldn't happen if we properly set the module to GOING whenever
it encounters an error during load_module().

But - I cannot think of a scenario where someone could call
try_module_get() on an unformed module, since find_module() etc. do
not return unformed modules, so they shouldn't be visible outside of
the module loader. So in practice, I think we're probably safe here..

Re: [PATCH] module: set MODULE_STATE_GOING state when a module fails to load

[Miroslav Benes]

On Wed, 28 Oct 2020, Jessica Yu wrote:

> +++ Miroslav Benes [27/10/20 15:03 +0100]:
> >If a module fails to load due to an error in prepare_coming_module(),
> >the following error handling in load_module() runs with
> >MODULE_STATE_COMING in module's state. Fix it by correctly setting
> >MODULE_STATE_GOING under "bug_cleanup" label.
> >
> >Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> >---
> > kernel/module.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> >diff --git a/kernel/module.c b/kernel/module.c
> >index a4fa44a652a7..b34235082394 100644
> >--- a/kernel/module.c
> >+++ b/kernel/module.c
> >@@ -3991,6 +3991,7 @@ static int load_module(struct load_info *info, const
> >char __user *uargs,
> >  			     MODULE_STATE_GOING, mod);
> >  	klp_module_going(mod);
> >  bug_cleanup:
> >+	mod->state = MODULE_STATE_GOING;
> >  /* module_bug_cleanup needs module_mutex protection */
> >  mutex_lock(&module_mutex);
> >  module_bug_cleanup(mod);
> 
> Thanks for the fix! Hmm, I am wondering if we also need to set the
> module to GOING if it happens to fail while it is still UNFORMED.
> 
> Currently, when a module is UNFORMED and encounters an error during
> load_module(), it stays UNFORMED until it finally goes away. That
> sounds fine, but try_module_get() technically permits you to get a
> module while it's UNFORMED (but not if it's GOING). Theoretically
> someone could increase the refcount of an unformed module that has
> encountered an error condition and is in the process of going away.

Right.

> This shouldn't happen if we properly set the module to GOING whenever
> it encounters an error during load_module().

That's correct.
 
> But - I cannot think of a scenario where someone could call
> try_module_get() on an unformed module, since find_module() etc. do
> not return unformed modules, so they shouldn't be visible outside of
> the module loader. So in practice, I think we're probably safe here..

Hopefully yes. I haven't found anything that would contradict it.

I think it is even safer to leave UNFORMED there. free_module() explicitly 
sets UNFORMED state too while going through the similar process.

ftrace_release_mod() is the only inconsistency there. It is called with 
UNFORMED in load_module() if going through ddebug_cleanup label 
directly, and with GOING in both do_init_module() before free_module() is 
called and delete_module syscall. But it probably does not care.

Miroslav

Re: [PATCH] module: set MODULE_STATE_GOING state when a module fails to load

[Jessica Yu]

+++ Miroslav Benes [29/10/20 13:31 +0100]:
>On Wed, 28 Oct 2020, Jessica Yu wrote:
>
>> +++ Miroslav Benes [27/10/20 15:03 +0100]:
>> >If a module fails to load due to an error in prepare_coming_module(),
>> >the following error handling in load_module() runs with
>> >MODULE_STATE_COMING in module's state. Fix it by correctly setting
>> >MODULE_STATE_GOING under "bug_cleanup" label.
>> >
>> >Signed-off-by: Miroslav Benes <mbenes@suse.cz>
>> >---
>> > kernel/module.c | 1 +
>> > 1 file changed, 1 insertion(+)
>> >
>> >diff --git a/kernel/module.c b/kernel/module.c
>> >index a4fa44a652a7..b34235082394 100644
>> >--- a/kernel/module.c
>> >+++ b/kernel/module.c
>> >@@ -3991,6 +3991,7 @@ static int load_module(struct load_info *info, const
>> >char __user *uargs,
>> >  			     MODULE_STATE_GOING, mod);
>> >  	klp_module_going(mod);
>> >  bug_cleanup:
>> >+	mod->state = MODULE_STATE_GOING;
>> >  /* module_bug_cleanup needs module_mutex protection */
>> >  mutex_lock(&module_mutex);
>> >  module_bug_cleanup(mod);
>>
>> Thanks for the fix! Hmm, I am wondering if we also need to set the
>> module to GOING if it happens to fail while it is still UNFORMED.
>>
>> Currently, when a module is UNFORMED and encounters an error during
>> load_module(), it stays UNFORMED until it finally goes away. That
>> sounds fine, but try_module_get() technically permits you to get a
>> module while it's UNFORMED (but not if it's GOING). Theoretically
>> someone could increase the refcount of an unformed module that has
>> encountered an error condition and is in the process of going away.
>
>Right.
>
>> This shouldn't happen if we properly set the module to GOING whenever
>> it encounters an error during load_module().
>
>That's correct.
>
>> But - I cannot think of a scenario where someone could call
>> try_module_get() on an unformed module, since find_module() etc. do
>> not return unformed modules, so they shouldn't be visible outside of
>> the module loader. So in practice, I think we're probably safe here..
>
>Hopefully yes. I haven't found anything that would contradict it.
>
>I think it is even safer to leave UNFORMED there. free_module() explicitly
>sets UNFORMED state too while going through the similar process.

That's true, and agreed.

>ftrace_release_mod() is the only inconsistency there. It is called with
>UNFORMED in load_module() if going through ddebug_cleanup label
>directly, and with GOING in both do_init_module() before free_module() is
>called and delete_module syscall. But it probably does not care.

Yes, I guess that inconsistency with ftrace_release_mod() has been
there long before this patch :-/ A quick look through ftrace.c doesn't
suggest to me there are any direct dependencies on module state there
(other than that comment above ftrace_module_init()). In practice
there hasn't been any issues..

I have applied this to modules-next. Thanks!

Jessica