[PATCH] module: Don't wait for GOING modules

[PATCH] module: Don't wait for GOING modules

[Petr Pavlu]

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@suse.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
Cc: stable@vger.kernel.org
---

Notes:
    Sending this alternative patch per the discussion in
    https://lore.kernel.org/linux-modules/20220919123233.8538-1-petr.pavlu@suse.com/.
    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;
 		goto out;
 	}
 	mod_update_bounds(mod);
-- 
2.35.3

Re: [PATCH] module: Don't wait for GOING modules

[Petr Mladek]

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@suse.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> Cc: stable@vger.kernel.org
> ---
> 
> Notes:
>     Sending this alternative patch per the discussion in
>     https://lore.kernel.org/linux-modules/20220919123233.8538-1-petr.pavlu@suse.com/.
>     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;
}

Best Regards,
Petr

Re: [PATCH] module: Don't wait for GOING modules

[Petr Pavlu]

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@suse.com>
>> Signed-off-by: Martin Wilck <mwilck@suse.com>
>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>> Cc: stable@vger.kernel.org
>> ---
>>
>> Notes:
>>     Sending this alternative patch per the discussion in
>>     https://lore.kernel.org/linux-modules/20220919123233.8538-1-petr.pavlu@suse.com/.
>>     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

RE: [PATCH] module: Don't wait for GOING modules

[David Laight]

From: Petr Pavlu
> Sent: 26 November 2022 14:43
> 
> 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@suse.com>
> >> Signed-off-by: Martin Wilck <mwilck@suse.com>
> >> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> >> Cc: stable@vger.kernel.org
> >> ---
> >>
> >> Notes:
> >>     Sending this alternative patch per the discussion in
> >>     https://lore.kernel.org/linux-modules/20220919123233.8538-1-petr.pavlu@suse.com/.
> >>     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.

While people have all this code cached in their brains
there is related problem I can easily hit.

If two processes create sctp sockets at the same time and sctp
module has to be loaded then the second process can enter the
module code before is it fully initialised.
This might be because the try_module_get() succeeds before the
module initialisation function returns.

I've avoided the issue by ensuring the socket creates are serialised.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] module: Don't wait for GOING modules

[Petr Mladek]

On Sun 2022-11-27 11:21:45, David Laight wrote:
> From: Petr Pavlu
> > Sent: 26 November 2022 14:43
> > 
> > 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.
> > >>
> 
> While people have all this code cached in their brains
> there is related problem I can easily hit.
> 
> If two processes create sctp sockets at the same time and sctp
> module has to be loaded then the second process can enter the
> module code before is it fully initialised.
> This might be because the try_module_get() succeeds before the
> module initialisation function returns.

Right, the race is there. And it is true that nobody should use
the module until mod->init() succeeds.

Well, I am not sure if there is an easy solution. It might require
reviewing what all try_module_get() callers expect.

We could not easily wait. For example, __sock_create() calls
try_module_get() under rcu_read_lock().

And various callers might want special handing when the module
is coming, going, and when it is not there at all.

I guess that it would require adding some new API and update
the various callers.

> I've avoided the issue by ensuring the socket creates are serialised.

I see. It would be great to have a clean solution, definitely.

Sigh, there are more issues with the module life time. For example,
kobjects might call the release() callback asynchronously and
it might happen when the module/code has gone, see
https://lore.kernel.org/all/20211105063710.4092936-1-ming.lei@redhat.com/

Best Regards,
PEtr