On 11/23/22 16:29, Petr Mladek wrote:
> On Wed 2022-11-23 14:12:26, 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.
>>
>> Since commit 6e6de3dee51a ("kernel/module.c: Only return -EEXIST for
>> modules that have finished loading"), the kernel waits for modules in
>> MODULE_STATE_GOING state to finish unloading before making another
>> attempt to load the same module.
>>
>> This creates unnecessary work in the described scenario and delays the
>> boot. In the worst case, it can prevent udev from loading drivers for
>> other devices and might cause timeouts of services waiting on them and
>> subsequently a failed boot.
>>
>> This patch attempts a different solution for the problem 6e6de3dee51a
>> was trying to solve. Rather than waiting for the unloading to complete,
>> it returns a different error code (-EBUSY) for modules in the GOING
>> state. This should avoid the error situation that was described in
>> 6e6de3dee51a (user space attempting to load a dependent module because
>> the -EEXIST error code would suggest to user space that the first module
>> had been loaded successfully), while avoiding the delay situation too.
>>
>> Fixes: 6e6de3dee51a ("kernel/module.c: Only return -EEXIST for modules that have finished loading")
>> Co-developed-by: Martin Wilck <mwilck@xxxxxxxx>
>> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
>> Signed-off-by: Petr Pavlu <petr.pavlu@xxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>> ---
>>
>> Notes:
>> Sending this alternative patch per the discussion in
>> https://lore.kernel.org/linux-modules/20220919123233.8538-1-petr.pavlu@xxxxxxxx/.
>> The initial version comes internally from Martin, hence the co-developed tag.
>>
>> kernel/module/main.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>> index d02d39c7174e..b7e08d1edc27 100644
>> --- a/kernel/module/main.c
>> +++ b/kernel/module/main.c
>> @@ -2386,7 +2386,8 @@ static bool finished_loading(const char *name)
>> sched_annotate_sleep();
>> mutex_lock(&module_mutex);
>> mod = find_module_all(name, strlen(name), true);
>> - ret = !mod || mod->state == MODULE_STATE_LIVE;
>> + ret = !mod || mod->state == MODULE_STATE_LIVE
>> + || mod->state == MODULE_STATE_GOING;
>> mutex_unlock(&module_mutex);
>>
>> return ret;
>> @@ -2566,7 +2567,8 @@ static int add_unformed_module(struct module *mod)
>> mutex_lock(&module_mutex);
>> old = find_module_all(mod->name, strlen(mod->name), true);
>> if (old != NULL) {
>> - if (old->state != MODULE_STATE_LIVE) {
>> + 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,
>> @@ -2575,7 +2577,7 @@ static int add_unformed_module(struct module *mod)
>> goto out_unlocked;
>> goto again;
>> }
>> - err = -EEXIST;
>> + err = old->state != MODULE_STATE_LIVE ? -EBUSY : -EEXIST;
>
> Hmm, this is not much reliable. It helps only when we manage to read
> the old module state before it is gone.
>
> A better solution would be to always return when there was a parallel
> load. The older patch from Petr Pavlu was more precise because it
> stored result of the exact parallel load. The below code is easier
> and might be good enough.
>
> 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 for the result of the parallel 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 = -EXIST;
> else
> err = -EBUSY;
> goto out;
> }
> mod_update_bounds(mod);
> list_add_rcu(&mod->list, &modules);
> mod_tree_insert(mod);
> err = 0;
>
> out:
> mutex_unlock(&module_mutex);
> out_unlocked:
> return err;
> }
I think this makes sense. The suggested code only needs to have the second
mutex_lock()+find_module_all() pair moved into the preceding if block to work
correctly. I will wait a bit if there is more feedback and post an updated
patch.
Thanks,
Petr
