[PATCH 0/2] module: Tweak return and warning

[PATCH 0/2] module: Tweak return and warning

[Lucas De Marchi]

Do not let userspace tools and end users confused on the return code and
log messages.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
Lucas De Marchi (2):
      module: Override -EEXISTS module return
      module: Simplify warning on positive returns from module_init()

 kernel/module/main.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

base-commit: e5f0a698b34ed76002dc5cff3804a61c80233a7a
change-id: 20251013-module-warn-ret-59f085298055

Lucas De Marchi

[PATCH 1/2] module: Override -EEXISTS module return

[Lucas De Marchi]

The -EEXIST errno is reserved by the module loading functionality. When
userspace calls [f]init_module(), it expects a -EEXIST to mean that the
module is already loaded in the kernel. If module_init() returns it,
that is not true anymore.

Add a warning and override the return code to workaround modules
currently returning the wrong code. It's expected that they eventually
migrate to a better suited error.

Closes: https://lore.kernel.org/all/aKLzsAX14ybEjHfJ@orbyte.nwl.cc/
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 kernel/module/main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b261849362..74ff87b13c517 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3038,6 +3038,11 @@ static noinline int do_init_module(struct module *mod)
 	if (mod->init != NULL)
 		ret = do_one_initcall(mod->init);
 	if (ret < 0) {
+		if (ret == -EEXIST) {
+			pr_warn("%s: init suspiciously returned -EEXIST: Overriding with -EBUSY\n",
+				mod->name);
+			ret = -EBUSY;
+		}
 		goto fail_free_freeinit;
 	}
 	if (ret > 0) {

-- 
2.51.0

[PATCH 2/2] module: Simplify warning on positive returns from module_init()

[Lucas De Marchi]

It should now be rare to trigger this warning - it doesn't need to be so
verbose. Make it follow the usual style in the module loading code.

For the same reason, drop the dump_stack().

Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 kernel/module/main.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 74ff87b13c517..31c54bf6df4b2 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3045,13 +3045,9 @@ static noinline int do_init_module(struct module *mod)
 		}
 		goto fail_free_freeinit;
 	}
-	if (ret > 0) {
-		pr_warn("%s: '%s'->init suspiciously returned %d, it should "
-			"follow 0/-E convention\n"
-			"%s: loading module anyway...\n",
-			__func__, mod->name, ret, __func__);
-		dump_stack();
-	}
+	if (ret > 0)
+		pr_warn("%s: init suspiciously returned %d, it should follow 0/-E convention\n",
+			mod->name, ret);
 
 	/* Now it's a first class citizen! */
 	mod->state = MODULE_STATE_LIVE;

-- 
2.51.0

Re: [PATCH 1/2] module: Override -EEXISTS module return

[Petr Pavlu]

On 10/13/25 6:26 PM, Lucas De Marchi wrote:
> The -EEXIST errno is reserved by the module loading functionality. When
> userspace calls [f]init_module(), it expects a -EEXIST to mean that the
> module is already loaded in the kernel. If module_init() returns it,
> that is not true anymore.
> 
> Add a warning and override the return code to workaround modules
> currently returning the wrong code. It's expected that they eventually
> migrate to a better suited error.
> 
> Closes: https://lore.kernel.org/all/aKLzsAX14ybEjHfJ@orbyte.nwl.cc/
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  kernel/module/main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b261849362..74ff87b13c517 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3038,6 +3038,11 @@ static noinline int do_init_module(struct module *mod)
>  	if (mod->init != NULL)
>  		ret = do_one_initcall(mod->init);
>  	if (ret < 0) {
> +		if (ret == -EEXIST) {
> +			pr_warn("%s: init suspiciously returned -EEXIST: Overriding with -EBUSY\n",
> +				mod->name);
> +			ret = -EBUSY;
> +		}
>  		goto fail_free_freeinit;
>  	}
>  	if (ret > 0) {
> 

I assume you intentionally omitted the "(reserved for loaded modules)"
part from my previous suggestion [1] to keep the error message short.

Looks ok to me then.

Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>

[1] https://lore.kernel.org/linux-modules/ce7f293c-d9f9-4137-bcad-8cc492d34773@suse.com/

-- 
Thanks,
Petr

Re: [PATCH 2/2] module: Simplify warning on positive returns from module_init()

[Petr Pavlu]

On 10/13/25 6:26 PM, Lucas De Marchi wrote:
> It should now be rare to trigger this warning - it doesn't need to be so
> verbose. Make it follow the usual style in the module loading code.
> 
> For the same reason, drop the dump_stack().
> 
> Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>

-- 
Thanks,
Petr

Re: [PATCH 2/2] module: Simplify warning on positive returns from module_init()

[Aaron Tomlin]

On Mon, Oct 13, 2025 at 09:26:24AM -0700, Lucas De Marchi wrote:
> It should now be rare to trigger this warning - it doesn't need to be so
> verbose. Make it follow the usual style in the module loading code.
> 
> For the same reason, drop the dump_stack().
> 
> Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  kernel/module/main.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 74ff87b13c517..31c54bf6df4b2 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3045,13 +3045,9 @@ static noinline int do_init_module(struct module *mod)
>  		}
>  		goto fail_free_freeinit;
>  	}
> -	if (ret > 0) {
> -		pr_warn("%s: '%s'->init suspiciously returned %d, it should "
> -			"follow 0/-E convention\n"
> -			"%s: loading module anyway...\n",
> -			__func__, mod->name, ret, __func__);
> -		dump_stack();
> -	}
> +	if (ret > 0)
> +		pr_warn("%s: init suspiciously returned %d, it should follow 0/-E convention\n",
> +			mod->name, ret);
>  
>  	/* Now it's a first class citizen! */
>  	mod->state = MODULE_STATE_LIVE;
> 
> -- 
> 2.51.0
> 
> 

Fair enough. Looks good to me.

Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>

-- 
Aaron Tomlin

Re: [PATCH 1/2] module: Override -EEXISTS module return

[Aaron Tomlin]

On Mon, Oct 13, 2025 at 09:26:23AM -0700, Lucas De Marchi wrote:
> The -EEXIST errno is reserved by the module loading functionality. When
> userspace calls [f]init_module(), it expects a -EEXIST to mean that the
> module is already loaded in the kernel. If module_init() returns it,
> that is not true anymore.
> 
> Add a warning and override the return code to workaround modules
> currently returning the wrong code. It's expected that they eventually
> migrate to a better suited error.
> 
> Closes: https://lore.kernel.org/all/aKLzsAX14ybEjHfJ@orbyte.nwl.cc/
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  kernel/module/main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b261849362..74ff87b13c517 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3038,6 +3038,11 @@ static noinline int do_init_module(struct module *mod)
>  	if (mod->init != NULL)
>  		ret = do_one_initcall(mod->init);
>  	if (ret < 0) {
> +		if (ret == -EEXIST) {
> +			pr_warn("%s: init suspiciously returned -EEXIST: Overriding with -EBUSY\n",
> +				mod->name);
> +			ret = -EBUSY;
> +		}
>  		goto fail_free_freeinit;
>  	}
>  	if (ret > 0) {
> 
> -- 
> 2.51.0
> 
> 

Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>

-- 
Aaron Tomlin

Re: [PATCH 1/2] module: Override -EEXISTS module return

[Daniel Gomez]

On 13/10/2025 18.26, Lucas De Marchi wrote:
> The -EEXIST errno is reserved by the module loading functionality. When
> userspace calls [f]init_module(), it expects a -EEXIST to mean that the
> module is already loaded in the kernel. If module_init() returns it,
> that is not true anymore.
> 
> Add a warning and override the return code to workaround modules
> currently returning the wrong code. It's expected that they eventually
> migrate to a better suited error.

I've been following the thread (and apologies for the delay) and reviewing the
patches, and I do not believe we should push this workaround. While this "fixes"
the bug reported, it also hides the real problem and drivers will continue
misusing EEXIST at module initialization.

From the bug report thread, I agree with Christophe's suggestion that
nf_conntrack_helpers_register() should return EBUSY instead of EEXIST. This
would fix the root cause for this particular module and will allow others to
change their module behavior, if we also follow up with proper documentation
about EEXIST.

> 
> Closes: https://lore.kernel.org/all/aKLzsAX14ybEjHfJ@orbyte.nwl.cc/
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  kernel/module/main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b261849362..74ff87b13c517 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3038,6 +3038,11 @@ static noinline int do_init_module(struct module *mod)
>  	if (mod->init != NULL)
>  		ret = do_one_initcall(mod->init);
>  	if (ret < 0) {
> +		if (ret == -EEXIST) {
> +			pr_warn("%s: init suspiciously returned -EEXIST: Overriding with -EBUSY\n",
> +				mod->name);
> +			ret = -EBUSY;
> +		}
>  		goto fail_free_freeinit;
>  	}
>  	if (ret > 0) {
> 

Re: [PATCH 1/2] module: Override -EEXISTS module return

[Lucas De Marchi]

On Mon, Nov 10, 2025 at 04:17:47PM +0100, Daniel Gomez wrote:
>On 13/10/2025 18.26, Lucas De Marchi wrote:
>> The -EEXIST errno is reserved by the module loading functionality. When
>> userspace calls [f]init_module(), it expects a -EEXIST to mean that the
>> module is already loaded in the kernel. If module_init() returns it,
>> that is not true anymore.
>>
>> Add a warning and override the return code to workaround modules
>> currently returning the wrong code. It's expected that they eventually
>> migrate to a better suited error.
>
>I've been following the thread (and apologies for the delay) and reviewing the
>patches, and I do not believe we should push this workaround. While this "fixes"
>the bug reported, it also hides the real problem and drivers will continue
>misusing EEXIST at module initialization.
>
>From the bug report thread, I agree with Christophe's suggestion that
>nf_conntrack_helpers_register() should return EBUSY instead of EEXIST. This
>would fix the root cause for this particular module and will allow others to
>change their module behavior, if we also follow up with proper documentation
>about EEXIST.

the fix will always be for the modules to stop returning EEXIST, no
discussion on that. This is the messenger, not the fix. Someone starts
seeing this warning and reports the bug. Then the developers can fix
it. It's much easier than dealing with the outcome of a module returning
EEXIST. Also note that we already have a similar reasoning about adding
a warning for module return codes in this same function.

Even if we had the means to check possible return codes from all
module inits, we'd still have external modules.

See patch 2: after some time we can simplify the warning and maybe even
remove it.

Lucas De Marchi

Re: [PATCH 1/2] module: Override -EEXISTS module return

[Daniel Gomez]

On 10/11/2025 16.31, Lucas De Marchi wrote:
> On Mon, Nov 10, 2025 at 04:17:47PM +0100, Daniel Gomez wrote:
>> On 13/10/2025 18.26, Lucas De Marchi wrote:
>>> The -EEXIST errno is reserved by the module loading functionality. When
>>> userspace calls [f]init_module(), it expects a -EEXIST to mean that the
>>> module is already loaded in the kernel. If module_init() returns it,
>>> that is not true anymore.
>>>
>>> Add a warning and override the return code to workaround modules
>>> currently returning the wrong code. It's expected that they eventually
>>> migrate to a better suited error.
>>
>> I've been following the thread (and apologies for the delay) and reviewing the
>> patches, and I do not believe we should push this workaround. While this "fixes"
>> the bug reported, it also hides the real problem and drivers will continue
>> misusing EEXIST at module initialization.
>>
>> From the bug report thread, I agree with Christophe's suggestion that
>> nf_conntrack_helpers_register() should return EBUSY instead of EEXIST. This
>> would fix the root cause for this particular module and will allow others to
>> change their module behavior, if we also follow up with proper documentation
>> about EEXIST.
> 
> the fix will always be for the modules to stop returning EEXIST, no
> discussion on that. This is the messenger, not the fix. Someone starts
> seeing this warning and rjeports the bug. Then the developers can fix
> it. It's much easier than dealing with the outcome of a module returning
> EEXIST.

I see. Thanks for clarifying.

I'm thinking to merge this at the beginning of the next release cycle. So others
can also have time to process the new warning when testing their modules. I hope
that is okay with you.

FYI, I will follow up this with docs and some drivers fixes I've found.

> Also note that we already have a similar reasoning about adding
> a warning for module return codes in this same function.
> 
> Even if we had the means to check possible return codes from all
> module inits, we'd still have external modules.
> 
> See patch 2: after some time we can simplify the warning and maybe even
> remove it.

That was added almost two decades ago. I hope we don't need to wait that long!
:)

commit e24e2e64c468c8060bb7173abecdf11d00ed5751
Author: Alexey Dobriyan <adobriyan@gmail.com>
Date:   Mon Mar 10 11:43:53 2008 -0700

    modules: warn about suspicious return values from module's ->init() hook

I don't see a way to completely remove any of these, especially with out of 
tree modules... or to capture any possible future/new misses.

> 
> Lucas De Marchi

Re: [PATCH 1/2] module: Override -EEXISTS module return

[Daniel Gomez]

On 13/10/2025 18.26, Lucas De Marchi wrote:
> The -EEXIST errno is reserved by the module loading functionality. When
> userspace calls [f]init_module(), it expects a -EEXIST to mean that the
> module is already loaded in the kernel. If module_init() returns it,
> that is not true anymore.
> 
> Add a warning and override the return code to workaround modules
> currently returning the wrong code. It's expected that they eventually
> migrate to a better suited error.
> 
> Closes: https://lore.kernel.org/all/aKLzsAX14ybEjHfJ@orbyte.nwl.cc/

Technically not true anymore since I delayed this review for quite some time.

FYI, this other patch actually fixes the problem for that driver:

54416fd76770 netfilter: conntrack: helper: Replace -EEXIST by -EBUSY

> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  kernel/module/main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b261849362..74ff87b13c517 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3038,6 +3038,11 @@ static noinline int do_init_module(struct module *mod)
>  	if (mod->init != NULL)
>  		ret = do_one_initcall(mod->init);
>  	if (ret < 0) {
> +		if (ret == -EEXIST) {
> +			pr_warn("%s: init suspiciously returned -EEXIST: Overriding with -EBUSY\n",
> +				mod->name);
> +			ret = -EBUSY;
> +		}
>  		goto fail_free_freeinit;
>  	}
>  	if (ret > 0) {
> 

Re: [PATCH 0/2] module: Tweak return and warning

[Daniel Gomez]

On 13/10/2025 18.26, Lucas De Marchi wrote:
> Do not let userspace tools and end users confused on the return code and
> log messages.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

LGTM,

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

> ---
> Lucas De Marchi (2):
>       module: Override -EEXISTS module return
>       module: Simplify warning on positive returns from module_init()
> 
>  kernel/module/main.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> base-commit: e5f0a698b34ed76002dc5cff3804a61c80233a7a
> change-id: 20251013-module-warn-ret-59f085298055
> 
> Lucas De Marchi
>