| Message ID | 20221016123031.3963-4-petr.pavlu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | module: Merge same-name module load requests | expand |
On 16.10.22 14:30, Petr Pavlu wrote: > During a system boot, it can happen that the kernel receives a burst of > requests to insert the same module but loading it eventually fails > during its init call. For instance, udev can make a request to insert > a frequency module for each individual CPU when another frequency module > is already loaded which causes the init function of the new module to > return an error. > > The module loader currently serializes all such requests, with the > barrier in add_unformed_module(). This creates a lot of unnecessary work > and delays the boot. It can prevent udev from loading drivers for other > devices and might cause timeouts of services waiting on them and > subsequently a failed boot. > > The mentioned serialization was introduced as a side-effect of commit > 6e6de3dee51a. The kernel before that merged some of same load requests > although it was more by accident and relied on specific timing. The > patch brings this behavior back in a more explicit form. > > The logic is improved as follows: > * A check whether a module load matches an already loaded module is > moved right after a module name is determined. -EEXIST continues to be > returned if the module exists and is live, -EBUSY is returned if > a same-name module is going. Can you clarify why the EBUSY change is needed? Why not simply return EEXIST? If you have thread 0 loading the module and thread 1 unloading the module concurrently, then it's pretty much unpredictable what the outcome will be either way, no? Add a random sleep to thread 1 (such that the module is *not* going yet) and the result will be EEXIST. I suggest avoiding a EBUSY change unless there is real reason to do so. User space that concurrently loads and unloads the same module is shaky already, no? > * A new reference-counted shared_load_info structure is introduced to > keep track of duplicate load requests. Two loads are considered > equivalent if their module name matches. In case a load duplicates > another running insert, the code waits for its completion and then > returns -EEXIST or -EBUSY depending on whether it succeeded. > > Moving the check for same-name module loads earlier has also a positive > effect on reducing memory pressure. For instance, David Hildenbrand and > Lin Liu reported [1] that when KASAN_INLINE is enabled (resulting in > large module size), with plenty of devices that udev wants to probe and > with plenty of CPUs that can carry out that probing concurrently, the > system can actually run out of module vmap space and trigger vmap > allocation errors. This is fixed by the patch too as it avoids duplicate > layout_and_allocate() work. It might we reasonable to add the kernel messages here. Can you also add the Reported-by? > > [1] https://lore.kernel.org/all/20221013180518.217405-1-david@redhat.com/ > > Fixes: 6e6de3dee51a ("kernel/module.c: Only return -EEXIST for modules that have finished loading") > Reviewed-by: Petr Mladek <pmladek@suse.com> > Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> > --- > kernel/module/main.c | 217 ++++++++++++++++++++++++++++++------------- > 1 file changed, 155 insertions(+), 62 deletions(-) > > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 5288843ca40f..2228c0f725e7 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -66,11 +66,28 @@ > * uses RCU list operations). > * 2) module_use links, > * 3) mod_tree.addr_min/mod_tree.addr_max, > - * 4) list of unloaded_tainted_modules. > + * 4) list of unloaded_tainted_modules, > + * 5) list of running_loads. > */ > DEFINE_MUTEX(module_mutex); > LIST_HEAD(modules); > > +/* Shared information to track duplicate module loads. */ > +struct shared_load_info { > + char name[MODULE_NAME_LEN]; > + refcount_t refcnt; > + struct list_head list; > + struct completion done; > + int err; > +}; > +static LIST_HEAD(running_loads); > + > +/* > + * Waiting for a module load when the exact module name is not known, for > + * example, when resolving symbols from another modules. > + */ > +static DECLARE_WAIT_QUEUE_HEAD(module_wq); > + > /* Work queue for freeing init sections in success case */ > static void do_free_init(struct work_struct *w); > static DECLARE_WORK(init_free_wq, do_free_init); > @@ -124,9 +141,6 @@ static void mod_update_bounds(struct module *mod) > int modules_disabled; > core_param(nomodule, modules_disabled, bint, 0); > > -/* Waiting for a module to finish initializing? */ > -static DECLARE_WAIT_QUEUE_HEAD(module_wq); > - > static BLOCKING_NOTIFIER_HEAD(module_notify_list); > > int register_module_notifier(struct notifier_block *nb) > @@ -764,8 +778,6 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user, > strscpy(last_unloaded_module.taints, module_flags(mod, buf, false), sizeof(last_unloaded_module.taints)); > > free_module(mod); > - /* someone could wait for the module in add_unformed_module() */ > - wake_up_interruptible(&module_wq); > return 0; > out: > mutex_unlock(&module_mutex); > @@ -2373,26 +2385,6 @@ static int post_relocation(struct module *mod, const struct load_info *info) > return module_finalize(info->hdr, info->sechdrs, mod); > } > > -/* Is this module of this name done loading? No locks held. */ > -static bool finished_loading(const char *name) > -{ > - struct module *mod; > - bool ret; > - > - /* > - * The module_mutex should not be a heavily contended lock; > - * if we get the occasional sleep here, we'll go an extra iteration > - * in the wait_event_interruptible(), which is harmless. > - */ > - sched_annotate_sleep(); > - mutex_lock(&module_mutex); > - mod = find_module_all(name, strlen(name), true); > - ret = !mod || mod->state == MODULE_STATE_LIVE; > - mutex_unlock(&module_mutex); > - > - return ret; > -} > - > /* Call module constructors. */ > static void do_mod_ctors(struct module *mod) > { > @@ -2523,7 +2515,6 @@ static noinline int do_init_module(struct module *mod) > schedule_work(&init_free_wq); > > mutex_unlock(&module_mutex); > - wake_up_interruptible(&module_wq); > > return 0; > > @@ -2539,7 +2530,6 @@ static noinline int do_init_module(struct module *mod) > klp_module_going(mod); > ftrace_release_mod(mod); > free_module(mod); > - wake_up_interruptible(&module_wq); > return ret; > } > > @@ -2551,43 +2541,138 @@ static int may_init_module(void) > return 0; > } > > +static struct shared_load_info * > +shared_load_info_alloc(const struct load_info *info) > +{ > + struct shared_load_info *shared_info = > + kzalloc(sizeof(*shared_info), GFP_KERNEL); > + if (shared_info == NULL) > + return ERR_PTR(-ENOMEM); > + > + strscpy(shared_info->name, info->name, sizeof(shared_info->name)); > + refcount_set(&shared_info->refcnt, 1); > + INIT_LIST_HEAD(&shared_info->list); > + init_completion(&shared_info->done); > + return shared_info; > +} > + > +static void shared_load_info_get(struct shared_load_info *shared_info) > +{ > + refcount_inc(&shared_info->refcnt); > +} > + > +static void shared_load_info_put(struct shared_load_info *shared_info) > +{ > + if (refcount_dec_and_test(&shared_info->refcnt)) > + kfree(shared_info); > +} > + > /* > - * 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 > + * Check that a module load is unique and make it visible to others. The code > + * looks for parallel running inserts and already loaded modules. Two inserts > + * are considered equivalent if their module name matches. In case this load > + * duplicates another running insert, the code waits for its completion and > + * then returns -EEXIST or -EBUSY depending on whether it succeeded. > + * > + * Detecting early that a load is unique avoids dedicating too many cycles and > + * resources to bring up the module. In particular, it prevents temporary percpu > * memory exhaustion. > + * > + * Merging same load requests then primarily helps during the boot process. It > + * can happen that the kernel receives a burst of requests to load the same > + * module (for example, a same module for each individual CPU) and loading it > + * eventually fails during its init call. Merging the requests allows that only > + * one full attempt to load the module is made. > + * > + * On a non-error return, it is guaranteed that this load is unique. > */ > -static int add_unformed_module(struct module *mod) > +static struct shared_load_info *add_running_load(const struct load_info *info) > { > - int err; > struct module *old; > + struct shared_load_info *shared_info; > > - mod->state = MODULE_STATE_UNFORMED; > - > -again: > mutex_lock(&module_mutex); > - old = find_module_all(mod->name, strlen(mod->name), true); > - if (old != NULL) { > - if (old->state != MODULE_STATE_LIVE) { > - /* Wait in case it fails to load. */ > + > + /* Search if there is a running load of a module with the same name. */ > + list_for_each_entry(shared_info, &running_loads, list) > + if (strcmp(shared_info->name, info->name) == 0) { > + int err; > + > + shared_load_info_get(shared_info); > mutex_unlock(&module_mutex); > - err = wait_event_interruptible(module_wq, > - finished_loading(mod->name)); > - if (err) > - goto out_unlocked; > - goto again; > + > + err = wait_for_completion_interruptible( > + &shared_info->done); > + /* > + * Return -EBUSY when the parallel load failed for any > + * reason. This load might end up another way but we are > + * not going to try. Why not? Usually "-EAGAIN" signals that user space should retry. But I hope that we can avoid EBUSY altogether and simply retry here. I'd suggest shared_load_info_put()+retry. No need to optimize for corner cases (concurrent load failing so we don't retry ourselves). > + */ > + if (!err) > + err = shared_info->err ? -EBUSY : -EEXIST; > + shared_load_info_put(shared_info); > + shared_info = ERR_PTR(err); > + goto out_unlocked; > + } > + > + /* Search if there is a live module with the given name already. */ > + old = find_module_all(info->name, strlen(info->name), true); > + if (old != NULL) { > + if (old->state == MODULE_STATE_LIVE) { > + shared_info = ERR_PTR(-EEXIST); > + goto out; > } > - err = -EEXIST; > + > + /* > + * Any active load always has its record in running_loads and so > + * would be found above. This applies independent whether such > + * a module is currently in MODULE_STATE_UNFORMED, > + * MODULE_STATE_COMING, or even in MODULE_STATE_GOING if its > + * initialization failed. It therefore means this must be an > + * older going module and the caller should try later once it is > + * gone. > + */ > + WARN_ON(old->state != MODULE_STATE_GOING); > + shared_info = ERR_PTR(-EBUSY); As raised above, why not EEXIST? Concurrent loading+unloading is racy either way. > goto out; > } > - mod_update_bounds(mod); > - list_add_rcu(&mod->list, &modules); > - mod_tree_insert(mod); > - err = 0; > + > + /* The load is unique, make it visible to others. */ > + shared_info = shared_load_info_alloc(info); > + if (IS_ERR(shared_info)) > + goto out; > + list_add(&shared_info->list, &running_loads); > > out: > mutex_unlock(&module_mutex); > out_unlocked: > - return err; > + return shared_info; > +} > + > +static void finalize_running_load(struct shared_load_info *shared_info, int err) s/finalize/release? It would be nice if the name could correspond to an opposite action of "add_running_load". > +{ > + /* Inform other duplicate inserts that the load finished. */ > + mutex_lock(&module_mutex); > + list_del(&shared_info->list); > + shared_info->err = err; > + mutex_unlock(&module_mutex); > + > + complete_all(&shared_info->done); > + shared_load_info_put(shared_info); > + > + /* Tell other modules waiting on this one that it completed loading. */ > + wake_up_interruptible(&module_wq); > +} > + [...] > sysfs_cleanup: > mod_sysfs_teardown(mod); > @@ -2880,15 +2973,15 @@ static int load_module(struct load_info *info, const char __user *uargs, > /* Unlink carefully: kallsyms could be walking list. */ > list_del_rcu(&mod->list); > mod_tree_remove(mod); > - wake_up_interruptible(&module_wq); > /* Wait for RCU-sched synchronizing before releasing mod->list. */ > synchronize_rcu(); > mutex_unlock(&module_mutex); > - free_module: > /* Free lock-classes; relies on the preceding sync_rcu() */ > lockdep_free_key_range(mod->data_layout.base, mod->data_layout.size); > > module_deallocate(mod, info); > + free_shared: Ideally, the label matches what's actually being done. So maybe "release_shared" if you go with "release_..." > + finalize_running_load(shared_info, err); > free_copy: > free_copy(info, flags); > return err;
On Sun 2022-10-16 14:30:30, Petr Pavlu wrote: > During a system boot, it can happen that the kernel receives a burst of > requests to insert the same module but loading it eventually fails > during its init call. For instance, udev can make a request to insert > a frequency module for each individual CPU when another frequency module > is already loaded which causes the init function of the new module to > return an error. > > The module loader currently serializes all such requests, with the > barrier in add_unformed_module(). This creates a lot of unnecessary work > and delays the boot. It can prevent udev from loading drivers for other > devices and might cause timeouts of services waiting on them and > subsequently a failed boot. > > The mentioned serialization was introduced as a side-effect of commit > 6e6de3dee51a. The kernel before that merged some of same load requests > although it was more by accident and relied on specific timing. The > patch brings this behavior back in a more explicit form. > > The logic is improved as follows: > * A check whether a module load matches an already loaded module is > moved right after a module name is determined. -EEXIST continues to be > returned if the module exists and is live, -EBUSY is returned if > a same-name module is going. > * A new reference-counted shared_load_info structure is introduced to > keep track of duplicate load requests. Two loads are considered > equivalent if their module name matches. In case a load duplicates > another running insert, the code waits for its completion and then > returns -EEXIST or -EBUSY depending on whether it succeeded. > > Moving the check for same-name module loads earlier has also a positive > effect on reducing memory pressure. For instance, David Hildenbrand and > Lin Liu reported [1] that when KASAN_INLINE is enabled (resulting in > large module size), with plenty of devices that udev wants to probe and > with plenty of CPUs that can carry out that probing concurrently, the > system can actually run out of module vmap space and trigger vmap > allocation errors. This is fixed by the patch too as it avoids duplicate > layout_and_allocate() work. > > [1] https://lore.kernel.org/all/20221013180518.217405-1-david@redhat.com/ > > Fixes: 6e6de3dee51a ("kernel/module.c: Only return -EEXIST for modules that have finished loading") > Reviewed-by: Petr Mladek <pmladek@suse.com> > Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> All these tags should be in the order of (logical) appearance. Somethings like: Suggested-by: if any Signed-off-by: author of the patch Acked-by: in order of appearance Revived-by: in order of appearance Tested-by: in order of appearance Signed-off-by: committer (upstream maintainer) I know that I provided Reviewed-by before you did the final changes. Well, you could use my tag only when you did only cosmetic changes or changes that I requested. You expect that I would agree with the patch even after it was updated. Best Regards, Petr
On 10/17/22 09:43, David Hildenbrand wrote: > On 16.10.22 14:30, Petr Pavlu wrote: >> During a system boot, it can happen that the kernel receives a burst of >> requests to insert the same module but loading it eventually fails >> during its init call. For instance, udev can make a request to insert >> a frequency module for each individual CPU when another frequency module >> is already loaded which causes the init function of the new module to >> return an error. >> >> The module loader currently serializes all such requests, with the >> barrier in add_unformed_module(). This creates a lot of unnecessary work >> and delays the boot. It can prevent udev from loading drivers for other >> devices and might cause timeouts of services waiting on them and >> subsequently a failed boot. >> >> The mentioned serialization was introduced as a side-effect of commit >> 6e6de3dee51a. The kernel before that merged some of same load requests >> although it was more by accident and relied on specific timing. The >> patch brings this behavior back in a more explicit form. >> >> The logic is improved as follows: >> * A check whether a module load matches an already loaded module is >> moved right after a module name is determined. -EEXIST continues to be >> returned if the module exists and is live, -EBUSY is returned if >> a same-name module is going. > > Can you clarify why the EBUSY change is needed? Why not simply return > EEXIST? > > If you have thread 0 loading the module and thread 1 unloading the > module concurrently, then it's pretty much unpredictable what the > outcome will be either way, no? > > Add a random sleep to thread 1 (such that the module is *not* going yet) > and the result will be EEXIST. > > I suggest avoiding a EBUSY change unless there is real reason to do so. > > User space that concurrently loads and unloads the same module is shaky > already, no? I cannot quite think of a scenario where it would practically matter how this corner case is handled. Prior to 6e6de3dee51a ("kernel/module.c: Only return -EEXIST for modules that have finished loading"), an init_module() call would have returned EEXIST in this case. After the mentioned commit, the loader waits for the old module to be gone and then proceeds with the load. Finally, this patch changes init_module() to immediately return EBUSY. With the proposed changes, EEXIST and EBUSY is used as follows: * EEXIST is returned from init_module() if a given module is already loaded or becomes live by a parallel load. * EBUSY is returned if a concurrent operation is detected on a module with the same name and the module is not live. This applies to both init_module() and delete_module(). I think it is generally a good idea to return EEXIST from init_module() only if a given module is fully operational. Userspace (udev) typically handles EEXIST as "success" and so there is some potential for confusion otherwise. However, I don't feel strongly about this particular case. >> * A new reference-counted shared_load_info structure is introduced to >> keep track of duplicate load requests. Two loads are considered >> equivalent if their module name matches. In case a load duplicates >> another running insert, the code waits for its completion and then >> returns -EEXIST or -EBUSY depending on whether it succeeded. >> >> Moving the check for same-name module loads earlier has also a positive >> effect on reducing memory pressure. For instance, David Hildenbrand and >> Lin Liu reported [1] that when KASAN_INLINE is enabled (resulting in >> large module size), with plenty of devices that udev wants to probe and >> with plenty of CPUs that can carry out that probing concurrently, the >> system can actually run out of module vmap space and trigger vmap >> allocation errors. This is fixed by the patch too as it avoids duplicate >> layout_and_allocate() work. > > It might we reasonable to add the kernel messages here. Can you also add > the Reported-by? Ok, I avoided adding the Reported-by tag because I was not sure how to properly record that it applies only to the vmap allocation issue. I suspect it can be clarified after the tag in a "[...]" note. My plan is to add the following: [ 165.842123] vmap allocation for size 2498560 failed: use vmalloc=<size> to increase size [ 165.843359] vmap allocation for size 2498560 failed: use vmalloc=<size> to increase size [ 165.844894] vmap allocation for size 2498560 failed: use vmalloc=<size> to increase size [ 165.847028] CPU: 253 PID: 4995 Comm: systemd-udevd Not tainted 5.19.0 #2 [ 165.935689] Hardware name: Lenovo ThinkSystem SR950 -[7X12ABC1WW]-/-[7X12ABC1WW]-, BIOS -[PSE130O-1.81]- 05/20/2020 [ 165.947343] Call Trace: [ 165.950075] <TASK> [ 165.952425] dump_stack_lvl+0x57/0x81 [ 165.956532] warn_alloc.cold+0x95/0x18a [ 165.981219] __vmalloc_node_range+0x291/0x560 [ 166.003741] module_alloc+0xe7/0x170 [ 166.011840] move_module+0x4c/0x630 [ 166.015751] layout_and_allocate+0x32c/0x560 [ 166.020519] load_module+0x8e0/0x25c0 [ 166.053854] __do_sys_finit_module+0x11a/0x1c0 [ 166.068494] do_syscall_64+0x59/0x90 [ 166.095855] entry_SYSCALL_64_after_hwframe+0x63/0xcd [ 165.818200] vmap allocation for size 2498560 failed: use vmalloc=<size> to increase size Reported-by: Lin Liu <linl@redhat.com> Reported-by: David Hildenbrand <david@redhat.com> [the vmap allocation issue] >> >> [1] https://lore.kernel.org/all/20221013180518.217405-1-david@redhat.com/ >> >> Fixes: 6e6de3dee51a ("kernel/module.c: Only return -EEXIST for modules that have finished loading") >> Reviewed-by: Petr Mladek <pmladek@suse.com> >> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> >> --- >> kernel/module/main.c | 217 ++++++++++++++++++++++++++++++------------- >> 1 file changed, 155 insertions(+), 62 deletions(-) >> >> diff --git a/kernel/module/main.c b/kernel/module/main.c >> index 5288843ca40f..2228c0f725e7 100644 >> --- a/kernel/module/main.c >> +++ b/kernel/module/main.c >> @@ -66,11 +66,28 @@ >> * uses RCU list operations). >> * 2) module_use links, >> * 3) mod_tree.addr_min/mod_tree.addr_max, >> - * 4) list of unloaded_tainted_modules. >> + * 4) list of unloaded_tainted_modules, >> + * 5) list of running_loads. >> */ >> DEFINE_MUTEX(module_mutex); >> LIST_HEAD(modules); >> >> +/* Shared information to track duplicate module loads. */ >> +struct shared_load_info { >> + char name[MODULE_NAME_LEN]; >> + refcount_t refcnt; >> + struct list_head list; >> + struct completion done; >> + int err; >> +}; >> +static LIST_HEAD(running_loads); >> + >> +/* >> + * Waiting for a module load when the exact module name is not known, for >> + * example, when resolving symbols from another modules. >> + */ >> +static DECLARE_WAIT_QUEUE_HEAD(module_wq); >> + >> /* Work queue for freeing init sections in success case */ >> static void do_free_init(struct work_struct *w); >> static DECLARE_WORK(init_free_wq, do_free_init); >> @@ -124,9 +141,6 @@ static void mod_update_bounds(struct module *mod) >> int modules_disabled; >> core_param(nomodule, modules_disabled, bint, 0); >> >> -/* Waiting for a module to finish initializing? */ >> -static DECLARE_WAIT_QUEUE_HEAD(module_wq); >> - >> static BLOCKING_NOTIFIER_HEAD(module_notify_list); >> >> int register_module_notifier(struct notifier_block *nb) >> @@ -764,8 +778,6 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user, >> strscpy(last_unloaded_module.taints, module_flags(mod, buf, false), sizeof(last_unloaded_module.taints)); >> >> free_module(mod); >> - /* someone could wait for the module in add_unformed_module() */ >> - wake_up_interruptible(&module_wq); >> return 0; >> out: >> mutex_unlock(&module_mutex); >> @@ -2373,26 +2385,6 @@ static int post_relocation(struct module *mod, const struct load_info *info) >> return module_finalize(info->hdr, info->sechdrs, mod); >> } >> >> -/* Is this module of this name done loading? No locks held. */ >> -static bool finished_loading(const char *name) >> -{ >> - struct module *mod; >> - bool ret; >> - >> - /* >> - * The module_mutex should not be a heavily contended lock; >> - * if we get the occasional sleep here, we'll go an extra iteration >> - * in the wait_event_interruptible(), which is harmless. >> - */ >> - sched_annotate_sleep(); >> - mutex_lock(&module_mutex); >> - mod = find_module_all(name, strlen(name), true); >> - ret = !mod || mod->state == MODULE_STATE_LIVE; >> - mutex_unlock(&module_mutex); >> - >> - return ret; >> -} >> - >> /* Call module constructors. */ >> static void do_mod_ctors(struct module *mod) >> { >> @@ -2523,7 +2515,6 @@ static noinline int do_init_module(struct module *mod) >> schedule_work(&init_free_wq); >> >> mutex_unlock(&module_mutex); >> - wake_up_interruptible(&module_wq); >> >> return 0; >> >> @@ -2539,7 +2530,6 @@ static noinline int do_init_module(struct module *mod) >> klp_module_going(mod); >> ftrace_release_mod(mod); >> free_module(mod); >> - wake_up_interruptible(&module_wq); >> return ret; >> } >> >> @@ -2551,43 +2541,138 @@ static int may_init_module(void) >> return 0; >> } >> >> +static struct shared_load_info * >> +shared_load_info_alloc(const struct load_info *info) >> +{ >> + struct shared_load_info *shared_info = >> + kzalloc(sizeof(*shared_info), GFP_KERNEL); >> + if (shared_info == NULL) >> + return ERR_PTR(-ENOMEM); >> + >> + strscpy(shared_info->name, info->name, sizeof(shared_info->name)); >> + refcount_set(&shared_info->refcnt, 1); >> + INIT_LIST_HEAD(&shared_info->list); >> + init_completion(&shared_info->done); >> + return shared_info; >> +} >> + >> +static void shared_load_info_get(struct shared_load_info *shared_info) >> +{ >> + refcount_inc(&shared_info->refcnt); >> +} >> + >> +static void shared_load_info_put(struct shared_load_info *shared_info) >> +{ >> + if (refcount_dec_and_test(&shared_info->refcnt)) >> + kfree(shared_info); >> +} >> + >> /* >> - * 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 >> + * Check that a module load is unique and make it visible to others. The code >> + * looks for parallel running inserts and already loaded modules. Two inserts >> + * are considered equivalent if their module name matches. In case this load >> + * duplicates another running insert, the code waits for its completion and >> + * then returns -EEXIST or -EBUSY depending on whether it succeeded. >> + * >> + * Detecting early that a load is unique avoids dedicating too many cycles and >> + * resources to bring up the module. In particular, it prevents temporary percpu >> * memory exhaustion. >> + * >> + * Merging same load requests then primarily helps during the boot process. It >> + * can happen that the kernel receives a burst of requests to load the same >> + * module (for example, a same module for each individual CPU) and loading it >> + * eventually fails during its init call. Merging the requests allows that only >> + * one full attempt to load the module is made. >> + * >> + * On a non-error return, it is guaranteed that this load is unique. >> */ >> -static int add_unformed_module(struct module *mod) >> +static struct shared_load_info *add_running_load(const struct load_info *info) >> { >> - int err; >> struct module *old; >> + struct shared_load_info *shared_info; >> >> - mod->state = MODULE_STATE_UNFORMED; >> - >> -again: >> mutex_lock(&module_mutex); >> - old = find_module_all(mod->name, strlen(mod->name), true); >> - if (old != NULL) { >> - if (old->state != MODULE_STATE_LIVE) { >> - /* Wait in case it fails to load. */ >> + >> + /* Search if there is a running load of a module with the same name. */ >> + list_for_each_entry(shared_info, &running_loads, list) >> + if (strcmp(shared_info->name, info->name) == 0) { >> + int err; >> + >> + shared_load_info_get(shared_info); >> mutex_unlock(&module_mutex); >> - err = wait_event_interruptible(module_wq, >> - finished_loading(mod->name)); >> - if (err) >> - goto out_unlocked; >> - goto again; >> + >> + err = wait_for_completion_interruptible( >> + &shared_info->done); >> + /* >> + * Return -EBUSY when the parallel load failed for any >> + * reason. This load might end up another way but we are >> + * not going to try. > > Why not? Usually "-EAGAIN" signals that user space should retry. But I > hope that we can avoid EBUSY altogether and simply retry here. > > I'd suggest shared_load_info_put()+retry. > > No need to optimize for corner cases (concurrent load failing so we > don't retry ourselves). Avoiding a retry in this case is actually the main motivation for this patch. It looks I'm still failing to explain this in the commit message, but please see my replies on previous versions of the patch where I provided more details about the observed issue [1, 2]. Worth noting is that both your scenario and my case are situations where a same module is attempted to be loaded multiple times, once per each CPU. Even if only one attempt is eventually fully processed, the decision that other parallel loads are not needed happens quite late. In particular, udev (libkmod) still has to load and decompress a given module binary multiple times. Ideally, I think this should be prevented altogether by improving other parts of the whole process. Udev could be made smarter to avoid duplicate loads or the kernel could model uevents related to CPUs differently. This is something that I was also considering but eventually settled on trying to fix only the immediate kernel regression. >> + */ >> + if (!err) >> + err = shared_info->err ? -EBUSY : -EEXIST; >> + shared_load_info_put(shared_info); >> + shared_info = ERR_PTR(err); >> + goto out_unlocked; >> + } >> + >> + /* Search if there is a live module with the given name already. */ >> + old = find_module_all(info->name, strlen(info->name), true); >> + if (old != NULL) { >> + if (old->state == MODULE_STATE_LIVE) { >> + shared_info = ERR_PTR(-EEXIST); >> + goto out; >> } >> - err = -EEXIST; >> + >> + /* >> + * Any active load always has its record in running_loads and so >> + * would be found above. This applies independent whether such >> + * a module is currently in MODULE_STATE_UNFORMED, >> + * MODULE_STATE_COMING, or even in MODULE_STATE_GOING if its >> + * initialization failed. It therefore means this must be an >> + * older going module and the caller should try later once it is >> + * gone. >> + */ >> + WARN_ON(old->state != MODULE_STATE_GOING); >> + shared_info = ERR_PTR(-EBUSY); > > As raised above, why not EEXIST? Concurrent loading+unloading is racy > either way. Discussed above. >> goto out; >> } >> - mod_update_bounds(mod); >> - list_add_rcu(&mod->list, &modules); >> - mod_tree_insert(mod); >> - err = 0; >> + >> + /* The load is unique, make it visible to others. */ >> + shared_info = shared_load_info_alloc(info); >> + if (IS_ERR(shared_info)) >> + goto out; >> + list_add(&shared_info->list, &running_loads); >> >> out: >> mutex_unlock(&module_mutex); >> out_unlocked: >> - return err; >> + return shared_info; >> +} >> + >> +static void finalize_running_load(struct shared_load_info *shared_info, int err) > > s/finalize/release? It would be nice if the name could correspond to an > opposite action of "add_running_load". > Ok, I can change this. Note though that functions add_running_load() and finalize_running_load() are not exactly opposite actions. I tried to avoid using "release" because it typically suggests to me a simple free of resources while finalize_running_load() still does some important work. >> +{ >> + /* Inform other duplicate inserts that the load finished. */ >> + mutex_lock(&module_mutex); >> + list_del(&shared_info->list); >> + shared_info->err = err; >> + mutex_unlock(&module_mutex); >> + >> + complete_all(&shared_info->done); >> + shared_load_info_put(shared_info); >> + >> + /* Tell other modules waiting on this one that it completed loading. */ >> + wake_up_interruptible(&module_wq); >> +} >> + > > > [...] > >> sysfs_cleanup: >> mod_sysfs_teardown(mod); >> @@ -2880,15 +2973,15 @@ static int load_module(struct load_info *info, const char __user *uargs, >> /* Unlink carefully: kallsyms could be walking list. */ >> list_del_rcu(&mod->list); >> mod_tree_remove(mod); >> - wake_up_interruptible(&module_wq); >> /* Wait for RCU-sched synchronizing before releasing mod->list. */ >> synchronize_rcu(); >> mutex_unlock(&module_mutex); >> - free_module: >> /* Free lock-classes; relies on the preceding sync_rcu() */ >> lockdep_free_key_range(mod->data_layout.base, mod->data_layout.size); >> >> module_deallocate(mod, info); >> + free_shared: > > Ideally, the label matches what's actually being done. So maybe > "release_shared" if you go with "release_..." Ok. [1] https://lore.kernel.org/linux-modules/0ccb384f-bbd5-f0fd-3832-c2255df505b2@suse.com/ [2] https://lore.kernel.org/linux-modules/aa8d9456-b260-d999-0296-8e6ab876af7a@suse.com/ Thanks, Petr
>> User space that concurrently loads and unloads the same module is shaky >> already, no? > > I cannot quite think of a scenario where it would practically matter how this > corner case is handled. Prior to 6e6de3dee51a ("kernel/module.c: Only return > -EEXIST for modules that have finished loading"), an init_module() call would > have returned EEXIST in this case. After the mentioned commit, the loader > waits for the old module to be gone and then proceeds with the load. Finally, > this patch changes init_module() to immediately return EBUSY. > > With the proposed changes, EEXIST and EBUSY is used as follows: > * EEXIST is returned from init_module() if a given module is already loaded or > becomes live by a parallel load. > * EBUSY is returned if a concurrent operation is detected on a module with the > same name and the module is not live. This applies to both init_module() and > delete_module(). > > I think it is generally a good idea to return EEXIST from init_module() only > if a given module is fully operational. Userspace (udev) typically handles > EEXIST as "success" and so there is some potential for confusion otherwise. > > However, I don't feel strongly about this particular case. > I'd vote to keep it simple and not change the way errors are returned unless there is real reason. EBUSY is currently documented to be only returned for "Timeout while trying to resolve a symbol reference by this module.". Your're changing that. EEXIST: "A module with this name is already loaded." -- which includes IMHO if the module is concurrently going away. Again, it's all racy either way. >>> * A new reference-counted shared_load_info structure is introduced to >>> keep track of duplicate load requests. Two loads are considered >>> equivalent if their module name matches. In case a load duplicates >>> another running insert, the code waits for its completion and then >>> returns -EEXIST or -EBUSY depending on whether it succeeded. >>> >>> Moving the check for same-name module loads earlier has also a positive >>> effect on reducing memory pressure. For instance, David Hildenbrand and >>> Lin Liu reported [1] that when KASAN_INLINE is enabled (resulting in >>> large module size), with plenty of devices that udev wants to probe and >>> with plenty of CPUs that can carry out that probing concurrently, the >>> system can actually run out of module vmap space and trigger vmap >>> allocation errors. This is fixed by the patch too as it avoids duplicate >>> layout_and_allocate() work. >> >> It might we reasonable to add the kernel messages here. Can you also add >> the Reported-by? > > Ok, I avoided adding the Reported-by tag because I was not sure how to > properly record that it applies only to the vmap allocation issue. I suspect > it can be clarified after the tag in a "[...]" note. > > My plan is to add the following: > > [ 165.842123] vmap allocation for size 2498560 failed: use vmalloc=<size> to increase size > [ 165.843359] vmap allocation for size 2498560 failed: use vmalloc=<size> to increase size > [ 165.844894] vmap allocation for size 2498560 failed: use vmalloc=<size> to increase size > [ 165.847028] CPU: 253 PID: 4995 Comm: systemd-udevd Not tainted 5.19.0 #2 > [ 165.935689] Hardware name: Lenovo ThinkSystem SR950 -[7X12ABC1WW]-/-[7X12ABC1WW]-, BIOS -[PSE130O-1.81]- 05/20/2020 > [ 165.947343] Call Trace: > [ 165.950075] <TASK> > [ 165.952425] dump_stack_lvl+0x57/0x81 > [ 165.956532] warn_alloc.cold+0x95/0x18a > [ 165.981219] __vmalloc_node_range+0x291/0x560 > [ 166.003741] module_alloc+0xe7/0x170 > [ 166.011840] move_module+0x4c/0x630 > [ 166.015751] layout_and_allocate+0x32c/0x560 > [ 166.020519] load_module+0x8e0/0x25c0 > [ 166.053854] __do_sys_finit_module+0x11a/0x1c0 > [ 166.068494] do_syscall_64+0x59/0x90 > [ 166.095855] entry_SYSCALL_64_after_hwframe+0x63/0xcd > [ 165.818200] vmap allocation for size 2498560 failed: use vmalloc=<size> to increase size > > Reported-by: Lin Liu <linl@redhat.com> > Reported-by: David Hildenbrand <david@redhat.com> > [the vmap allocation issue] Sounds good, you can also mention which issue was reported by whom in the text in addition. [...] >>> -static int add_unformed_module(struct module *mod) >>> +static struct shared_load_info *add_running_load(const struct load_info *info) >>> { >>> - int err; >>> struct module *old; >>> + struct shared_load_info *shared_info; >>> >>> - mod->state = MODULE_STATE_UNFORMED; >>> - >>> -again: >>> mutex_lock(&module_mutex); >>> - old = find_module_all(mod->name, strlen(mod->name), true); >>> - if (old != NULL) { >>> - if (old->state != MODULE_STATE_LIVE) { >>> - /* Wait in case it fails to load. */ >>> + >>> + /* Search if there is a running load of a module with the same name. */ >>> + list_for_each_entry(shared_info, &running_loads, list) >>> + if (strcmp(shared_info->name, info->name) == 0) { >>> + int err; >>> + >>> + shared_load_info_get(shared_info); >>> mutex_unlock(&module_mutex); >>> - err = wait_event_interruptible(module_wq, >>> - finished_loading(mod->name)); >>> - if (err) >>> - goto out_unlocked; >>> - goto again; >>> + >>> + err = wait_for_completion_interruptible( >>> + &shared_info->done); >>> + /* >>> + * Return -EBUSY when the parallel load failed for any >>> + * reason. This load might end up another way but we are >>> + * not going to try. >> >> Why not? Usually "-EAGAIN" signals that user space should retry. But I >> hope that we can avoid EBUSY altogether and simply retry here. >> >> I'd suggest shared_load_info_put()+retry. >> >> No need to optimize for corner cases (concurrent load failing so we >> don't retry ourselves). > > Avoiding a retry in this case is actually the main motivation for this patch. > It looks I'm still failing to explain this in the commit message, but please > see my replies on previous versions of the patch where I provided more details > about the observed issue [1, 2]. How is it the common case we care about that a parallel load *failed* (not with -EEXIST but via some other error)? That would mean we're optimizing for the case that 400 CPUs try loading the same module and loading the module essentially always fails. Is this really what we want to optimize? Isn't there a way to not report EBUSY in that case as well? Return the error from the other load that failed? > > Worth noting is that both your scenario and my case are situations where > a same module is attempted to be loaded multiple times, once per each CPU. > Even if only one attempt is eventually fully processed, the decision that > other parallel loads are not needed happens quite late. In particular, udev > (libkmod) still has to load and decompress a given module binary multiple > times. Ideally, I think this should be prevented altogether by improving other > parts of the whole process. Udev could be made smarter to avoid duplicate > loads or the kernel could model uevents related to CPUs differently. This is > something that I was also considering but eventually settled on trying to fix > only the immediate kernel regression. Yes, exactly same thoughts here.
diff --git a/kernel/module/main.c b/kernel/module/main.c index 5288843ca40f..2228c0f725e7 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -66,11 +66,28 @@ * uses RCU list operations). * 2) module_use links, * 3) mod_tree.addr_min/mod_tree.addr_max, - * 4) list of unloaded_tainted_modules. + * 4) list of unloaded_tainted_modules, + * 5) list of running_loads. */ DEFINE_MUTEX(module_mutex); LIST_HEAD(modules); +/* Shared information to track duplicate module loads. */ +struct shared_load_info { + char name[MODULE_NAME_LEN]; + refcount_t refcnt; + struct list_head list; + struct completion done; + int err; +}; +static LIST_HEAD(running_loads); + +/* + * Waiting for a module load when the exact module name is not known, for + * example, when resolving symbols from another modules. + */ +static DECLARE_WAIT_QUEUE_HEAD(module_wq); + /* Work queue for freeing init sections in success case */ static void do_free_init(struct work_struct *w); static DECLARE_WORK(init_free_wq, do_free_init); @@ -124,9 +141,6 @@ static void mod_update_bounds(struct module *mod) int modules_disabled; core_param(nomodule, modules_disabled, bint, 0); -/* Waiting for a module to finish initializing? */ -static DECLARE_WAIT_QUEUE_HEAD(module_wq); - static BLOCKING_NOTIFIER_HEAD(module_notify_list); int register_module_notifier(struct notifier_block *nb) @@ -764,8 +778,6 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user, strscpy(last_unloaded_module.taints, module_flags(mod, buf, false), sizeof(last_unloaded_module.taints)); free_module(mod); - /* someone could wait for the module in add_unformed_module() */ - wake_up_interruptible(&module_wq); return 0; out: mutex_unlock(&module_mutex); @@ -2373,26 +2385,6 @@ static int post_relocation(struct module *mod, const struct load_info *info) return module_finalize(info->hdr, info->sechdrs, mod); } -/* Is this module of this name done loading? No locks held. */ -static bool finished_loading(const char *name) -{ - struct module *mod; - bool ret; - - /* - * The module_mutex should not be a heavily contended lock; - * if we get the occasional sleep here, we'll go an extra iteration - * in the wait_event_interruptible(), which is harmless. - */ - sched_annotate_sleep(); - mutex_lock(&module_mutex); - mod = find_module_all(name, strlen(name), true); - ret = !mod || mod->state == MODULE_STATE_LIVE; - mutex_unlock(&module_mutex); - - return ret; -} - /* Call module constructors. */ static void do_mod_ctors(struct module *mod) { @@ -2523,7 +2515,6 @@ static noinline int do_init_module(struct module *mod) schedule_work(&init_free_wq); mutex_unlock(&module_mutex); - wake_up_interruptible(&module_wq); return 0; @@ -2539,7 +2530,6 @@ static noinline int do_init_module(struct module *mod) klp_module_going(mod); ftrace_release_mod(mod); free_module(mod); - wake_up_interruptible(&module_wq); return ret; } @@ -2551,43 +2541,138 @@ static int may_init_module(void) return 0; } +static struct shared_load_info * +shared_load_info_alloc(const struct load_info *info) +{ + struct shared_load_info *shared_info = + kzalloc(sizeof(*shared_info), GFP_KERNEL); + if (shared_info == NULL) + return ERR_PTR(-ENOMEM); + + strscpy(shared_info->name, info->name, sizeof(shared_info->name)); + refcount_set(&shared_info->refcnt, 1); + INIT_LIST_HEAD(&shared_info->list); + init_completion(&shared_info->done); + return shared_info; +} + +static void shared_load_info_get(struct shared_load_info *shared_info) +{ + refcount_inc(&shared_info->refcnt); +} + +static void shared_load_info_put(struct shared_load_info *shared_info) +{ + if (refcount_dec_and_test(&shared_info->refcnt)) + kfree(shared_info); +} + /* - * 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 + * Check that a module load is unique and make it visible to others. The code + * looks for parallel running inserts and already loaded modules. Two inserts + * are considered equivalent if their module name matches. In case this load + * duplicates another running insert, the code waits for its completion and + * then returns -EEXIST or -EBUSY depending on whether it succeeded. + * + * Detecting early that a load is unique avoids dedicating too many cycles and + * resources to bring up the module. In particular, it prevents temporary percpu * memory exhaustion. + * + * Merging same load requests then primarily helps during the boot process. It + * can happen that the kernel receives a burst of requests to load the same + * module (for example, a same module for each individual CPU) and loading it + * eventually fails during its init call. Merging the requests allows that only + * one full attempt to load the module is made. + * + * On a non-error return, it is guaranteed that this load is unique. */ -static int add_unformed_module(struct module *mod) +static struct shared_load_info *add_running_load(const struct load_info *info) { - int err; struct module *old; + struct shared_load_info *shared_info; - mod->state = MODULE_STATE_UNFORMED; - -again: mutex_lock(&module_mutex); - old = find_module_all(mod->name, strlen(mod->name), true); - if (old != NULL) { - if (old->state != MODULE_STATE_LIVE) { - /* Wait in case it fails to load. */ + + /* Search if there is a running load of a module with the same name. */ + list_for_each_entry(shared_info, &running_loads, list) + if (strcmp(shared_info->name, info->name) == 0) { + int err; + + shared_load_info_get(shared_info); mutex_unlock(&module_mutex); - err = wait_event_interruptible(module_wq, - finished_loading(mod->name)); - if (err) - goto out_unlocked; - goto again; + + err = wait_for_completion_interruptible( + &shared_info->done); + /* + * Return -EBUSY when the parallel load failed for any + * reason. This load might end up another way but we are + * not going to try. + */ + if (!err) + err = shared_info->err ? -EBUSY : -EEXIST; + shared_load_info_put(shared_info); + shared_info = ERR_PTR(err); + goto out_unlocked; + } + + /* Search if there is a live module with the given name already. */ + old = find_module_all(info->name, strlen(info->name), true); + if (old != NULL) { + if (old->state == MODULE_STATE_LIVE) { + shared_info = ERR_PTR(-EEXIST); + goto out; } - err = -EEXIST; + + /* + * Any active load always has its record in running_loads and so + * would be found above. This applies independent whether such + * a module is currently in MODULE_STATE_UNFORMED, + * MODULE_STATE_COMING, or even in MODULE_STATE_GOING if its + * initialization failed. It therefore means this must be an + * older going module and the caller should try later once it is + * gone. + */ + WARN_ON(old->state != MODULE_STATE_GOING); + shared_info = ERR_PTR(-EBUSY); goto out; } - mod_update_bounds(mod); - list_add_rcu(&mod->list, &modules); - mod_tree_insert(mod); - err = 0; + + /* The load is unique, make it visible to others. */ + shared_info = shared_load_info_alloc(info); + if (IS_ERR(shared_info)) + goto out; + list_add(&shared_info->list, &running_loads); out: mutex_unlock(&module_mutex); out_unlocked: - return err; + return shared_info; +} + +static void finalize_running_load(struct shared_load_info *shared_info, int err) +{ + /* Inform other duplicate inserts that the load finished. */ + mutex_lock(&module_mutex); + list_del(&shared_info->list); + shared_info->err = err; + mutex_unlock(&module_mutex); + + complete_all(&shared_info->done); + shared_load_info_put(shared_info); + + /* Tell other modules waiting on this one that it completed loading. */ + wake_up_interruptible(&module_wq); +} + +static void add_unformed_module(struct module *mod) +{ + mod->state = MODULE_STATE_UNFORMED; + + mutex_lock(&module_mutex); + mod_update_bounds(mod); + list_add_rcu(&mod->list, &modules); + mod_tree_insert(mod); + mutex_unlock(&module_mutex); } static int complete_formation(struct module *mod, struct load_info *info) @@ -2672,6 +2757,7 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname, static int load_module(struct load_info *info, const char __user *uargs, int flags) { + struct shared_load_info *shared_info; struct module *mod; long err = 0; char *after_dashes; @@ -2709,38 +2795,43 @@ static int load_module(struct load_info *info, const char __user *uargs, goto free_copy; /* - * Now that we know we have the correct module name, check - * if it's blacklisted. + * Now that we know we have the correct module name, check if there is + * another load of the same name in progress. */ + shared_info = add_running_load(info); + if (IS_ERR(shared_info)) { + err = PTR_ERR(shared_info); + goto free_copy; + } + + /* Check if the module is blacklisted. */ if (blacklisted(info->name)) { err = -EPERM; pr_err("Module %s is blacklisted\n", info->name); - goto free_copy; + goto free_shared; } err = rewrite_section_headers(info, flags); if (err) - goto free_copy; + goto free_shared; /* Check module struct version now, before we try to use module. */ if (!check_modstruct_version(info, info->mod)) { err = -ENOEXEC; - goto free_copy; + goto free_shared; } /* Figure out module layout, and allocate all the memory. */ mod = layout_and_allocate(info, flags); if (IS_ERR(mod)) { err = PTR_ERR(mod); - goto free_copy; + goto free_shared; } audit_log_kern_module(mod->name); /* Reserve our place in the list. */ - err = add_unformed_module(mod); - if (err) - goto free_module; + add_unformed_module(mod); #ifdef CONFIG_MODULE_SIG mod->sig_ok = info->sig_ok; @@ -2847,7 +2938,9 @@ static int load_module(struct load_info *info, const char __user *uargs, /* Done! */ trace_module_load(mod); - return do_init_module(mod); + err = do_init_module(mod); + finalize_running_load(shared_info, err); + return err; sysfs_cleanup: mod_sysfs_teardown(mod); @@ -2880,15 +2973,15 @@ static int load_module(struct load_info *info, const char __user *uargs, /* Unlink carefully: kallsyms could be walking list. */ list_del_rcu(&mod->list); mod_tree_remove(mod); - wake_up_interruptible(&module_wq); /* Wait for RCU-sched synchronizing before releasing mod->list. */ synchronize_rcu(); mutex_unlock(&module_mutex); - free_module: /* Free lock-classes; relies on the preceding sync_rcu() */ lockdep_free_key_range(mod->data_layout.base, mod->data_layout.size); module_deallocate(mod, info); + free_shared: + finalize_running_load(shared_info, err); free_copy: free_copy(info, flags); return err;
During a system boot, it can happen that the kernel receives a burst of requests to insert the same module but loading it eventually fails during its init call. For instance, udev can make a request to insert a frequency module for each individual CPU when another frequency module is already loaded which causes the init function of the new module to return an error. The module loader currently serializes all such requests, with the barrier in add_unformed_module(). This creates a lot of unnecessary work and delays the boot. It can prevent udev from loading drivers for other devices and might cause timeouts of services waiting on them and subsequently a failed boot. The mentioned serialization was introduced as a side-effect of commit 6e6de3dee51a. The kernel before that merged some of same load requests although it was more by accident and relied on specific timing. The patch brings this behavior back in a more explicit form. The logic is improved as follows: * A check whether a module load matches an already loaded module is moved right after a module name is determined. -EEXIST continues to be returned if the module exists and is live, -EBUSY is returned if a same-name module is going. * A new reference-counted shared_load_info structure is introduced to keep track of duplicate load requests. Two loads are considered equivalent if their module name matches. In case a load duplicates another running insert, the code waits for its completion and then returns -EEXIST or -EBUSY depending on whether it succeeded. Moving the check for same-name module loads earlier has also a positive effect on reducing memory pressure. For instance, David Hildenbrand and Lin Liu reported [1] that when KASAN_INLINE is enabled (resulting in large module size), with plenty of devices that udev wants to probe and with plenty of CPUs that can carry out that probing concurrently, the system can actually run out of module vmap space and trigger vmap allocation errors. This is fixed by the patch too as it avoids duplicate layout_and_allocate() work. [1] https://lore.kernel.org/all/20221013180518.217405-1-david@redhat.com/ Fixes: 6e6de3dee51a ("kernel/module.c: Only return -EEXIST for modules that have finished loading") Reviewed-by: Petr Mladek <pmladek@suse.com> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> --- kernel/module/main.c | 217 ++++++++++++++++++++++++++++++------------- 1 file changed, 155 insertions(+), 62 deletions(-)