Re: [PATCH] module: set MODULE_STATE_GOING state when a module fails to load
+++ 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 >--- > 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
Re: [PATCH] module: set MODULE_STATE_GOING state when a module fails to load
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 > >--- > > 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
+++ 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 --- 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..
[PATCH] module: set MODULE_STATE_GOING state when a module fails to load
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 --- 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