Re: [PATCH] static_call: Handle module init failure correctly in static_call_del_module()

From: Christophe Leroy
Date: Fri Nov 08 2024 - 03:12:17 EST


Hi Luis,

Le 24/09/2024 à 09:22, Mike Rapoport a écrit :
On Thu, Sep 19, 2024 at 02:53:34AM -0700, Luis Chamberlain wrote:
On Fri, Sep 06, 2024 at 04:24:56PM -0700, Luis Chamberlain wrote:
On Thu, Sep 05, 2024 at 11:44:00AM +0200, Thomas Gleixner wrote:
Now you at least provided the information that the missing cleanup in
the init() function is not the problem. So the obvious place to look is
in the module core code whether there is a failure path _after_
module->init() returned success.

do_init_module()
ret = do_one_initcall(mod->init);
...
ret = module_enable_rodata_ro(mod, true);
if (ret)
goto fail_mutex_unlock;

and that error path does _not_ invoke module->exit(), which is obviously
not correct. Luis?

You're spot on this needs fixing.

Christophe, this is a regression caused by the second hunk of your commit
d1909c0221739 ("module: Don't ignore errors from set_memory_XX()") on v6.9.
Sadly there are a few issues with trying to get to call mod->exit():

- We should try try_stop_module() and that can fail
- source_list may not be empty and that would block removal
- mod->exit may not exist

I'm wondering if instead we should try to do the module_enable_rodata_ro()
before the init, but that requires a bit more careful evaluation...

There is ro_after_init section, we can't really make it RO before ->init()

Surprisingly I never received Luis's email allthough I got this answer from Mike that I overlooked.

So coming back here from https://lore.kernel.org/all/ZyQhbHxDTRXTJgIx@xxxxxxxxxxxxxxxxxxxxxx/

As far as I understand, indeed once init is called it is too late to fail, right ? Especially when the module has no exit() path or when CONFIG_MODULE_UNLOAD is not built in.

So the only thing we can do then is a big fat warning telling set_memory_ro() on ro_after_init memory has failed ?

Maybe we should try and change it to RO then back to RW before calling init, to be on a safer side hopping that if change to RO works once it will work twice ?

Christophe