| Message ID | 20230405022702.753323-4-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | module: avoid userspace pressure on unwanted allocations | expand |
On 05.04.23 04:26, Luis Chamberlain wrote: > The patient module check inside add_unformed_module() is large > enough as we need it. It is a bit hard to read too, so just > move it to a helper and do the inverse checks first to help > shift the code and make it easier to read. The new helper then > is module_patient_check_exists(). > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > kernel/module/main.c | 71 +++++++++++++++++++++++++------------------- > 1 file changed, 40 insertions(+), 31 deletions(-) > > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 98c261928325..8f382580195b 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -2638,6 +2638,43 @@ static bool finished_loading(const char *name) > return ret; > } > > +/* Must be called with module_mutex held */ > +static int module_patient_check_exists(const char *name) > +{ > + struct module *old; > + int err = 0; > + > + old = find_module_all(name, strlen(name), true); > + if (old == NULL) > + return 0; > + > + if (old->state == MODULE_STATE_COMING > + || old->state == MODULE_STATE_UNFORMED) { I never understood why people prefer to prefix the || on a newline. But it seems to be a thing in the module/ world :) > + /* Wait in case it fails to load. */ > + mutex_unlock(&module_mutex); > + err = wait_event_interruptible(module_wq, > + finished_loading(name)); > + if (err) > + return err; You return with the mutex unlocked. The caller will unlock again ... > + > + /* The module might have gone in the meantime. */ > + mutex_lock(&module_mutex); > + old = find_module_all(name, strlen(name), true); > + } > + > + /* > + * We are here only when the same module was being loaded. Do > + * not try to load it again right now. It prevents long delays > + * caused by serialized module load failures. It might happen > + * when more devices of the same type trigger load of > + * a particular module. > + */ > + if (old && old->state == MODULE_STATE_LIVE) > + return -EEXIST; > + else > + return -EBUSY; You can drop the else and return right away. > +} > + > /* > * We try to place it in the list now to make sure it's unique before Besides, LGTM.
On Wed, Apr 05, 2023 at 07:11:24PM +0200, David Hildenbrand wrote: > On 05.04.23 04:26, Luis Chamberlain wrote: > > The patient module check inside add_unformed_module() is large > > enough as we need it. It is a bit hard to read too, so just > > move it to a helper and do the inverse checks first to help > > shift the code and make it easier to read. The new helper then > > is module_patient_check_exists(). > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > --- > > kernel/module/main.c | 71 +++++++++++++++++++++++++------------------- > > 1 file changed, 40 insertions(+), 31 deletions(-) > > > > diff --git a/kernel/module/main.c b/kernel/module/main.c > > index 98c261928325..8f382580195b 100644 > > --- a/kernel/module/main.c > > +++ b/kernel/module/main.c > > @@ -2638,6 +2638,43 @@ static bool finished_loading(const char *name) > > return ret; > > } > > +/* Must be called with module_mutex held */ > > +static int module_patient_check_exists(const char *name) > > +{ > > + struct module *old; > > + int err = 0; > > + > > + old = find_module_all(name, strlen(name), true); > > + if (old == NULL) > > + return 0; > > + > > + if (old->state == MODULE_STATE_COMING > > + || old->state == MODULE_STATE_UNFORMED) { > > I never understood why people prefer to prefix the || on a newline. But it > seems to be a thing in the module/ world :) Yeah the other way seems better, I'll make it pretty. > > + /* Wait in case it fails to load. */ > > + mutex_unlock(&module_mutex); > > + err = wait_event_interruptible(module_wq, > > + finished_loading(name)); > > + if (err) > > + return err; > > You return with the mutex unlocked. The caller will unlock again ... Fixed now by moving the mutex below up after the wait_event_interruptible(), thanks. > > + > > + /* The module might have gone in the meantime. */ > > + mutex_lock(&module_mutex); > > + old = find_module_all(name, strlen(name), true); > > + } > > + > > + /* > > + * We are here only when the same module was being loaded. Do > > + * not try to load it again right now. It prevents long delays > > + * caused by serialized module load failures. It might happen > > + * when more devices of the same type trigger load of > > + * a particular module. > > + */ > > + if (old && old->state == MODULE_STATE_LIVE) > > + return -EEXIST; > > + else > > + return -EBUSY; > > You can drop the else and return right away. Will make it pretty like that, thanks. Luis
diff --git a/kernel/module/main.c b/kernel/module/main.c index 98c261928325..8f382580195b 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2638,6 +2638,43 @@ static bool finished_loading(const char *name) return ret; } +/* Must be called with module_mutex held */ +static int module_patient_check_exists(const char *name) +{ + struct module *old; + int err = 0; + + old = find_module_all(name, strlen(name), true); + if (old == NULL) + return 0; + + if (old->state == MODULE_STATE_COMING + || old->state == MODULE_STATE_UNFORMED) { + /* Wait in case it fails to load. */ + mutex_unlock(&module_mutex); + err = wait_event_interruptible(module_wq, + finished_loading(name)); + if (err) + return err; + + /* The module might have gone in the meantime. */ + mutex_lock(&module_mutex); + old = find_module_all(name, strlen(name), true); + } + + /* + * We are here only when the same module was being loaded. Do + * not try to load it again right now. It prevents long delays + * caused by serialized module load failures. It might happen + * when more devices of the same type trigger load of + * a particular module. + */ + if (old && old->state == MODULE_STATE_LIVE) + return -EEXIST; + else + return -EBUSY; +} + /* * We try to place it in the list now to make sure it's unique before * we dedicate too many resources. In particular, temporary percpu @@ -2646,41 +2683,14 @@ static bool finished_loading(const char *name) static int add_unformed_module(struct module *mod) { int err; - struct module *old; mod->state = MODULE_STATE_UNFORMED; mutex_lock(&module_mutex); - old = find_module_all(mod->name, strlen(mod->name), true); - if (old != NULL) { - if (old->state == MODULE_STATE_COMING - || old->state == MODULE_STATE_UNFORMED) { - /* Wait in case it fails to load. */ - mutex_unlock(&module_mutex); - err = wait_event_interruptible(module_wq, - finished_loading(mod->name)); - if (err) - goto out_unlocked; - - /* The module might have gone in the meantime. */ - mutex_lock(&module_mutex); - old = find_module_all(mod->name, strlen(mod->name), - true); - } - - /* - * We are here only when the same module was being loaded. Do - * not try to load it again right now. It prevents long delays - * caused by serialized module load failures. It might happen - * when more devices of the same type trigger load of - * a particular module. - */ - if (old && old->state == MODULE_STATE_LIVE) - err = -EEXIST; - else - err = -EBUSY; + err = module_patient_check_exists(mod->name); + if (err) goto out; - } + mod_update_bounds(mod); list_add_rcu(&mod->list, &modules); mod_tree_insert(mod); @@ -2688,7 +2698,6 @@ static int add_unformed_module(struct module *mod) out: mutex_unlock(&module_mutex); -out_unlocked: return err; }
The patient module check inside add_unformed_module() is large enough as we need it. It is a bit hard to read too, so just move it to a helper and do the inverse checks first to help shift the code and make it easier to read. The new helper then is module_patient_check_exists(). Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- kernel/module/main.c | 71 +++++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 31 deletions(-)