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

[Jessica Yu <jeyu@kernel.org>]

+++ 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