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@xxxxxxx>
> > >---
> > > 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