Re: modprobe returns 0 upon -EEXIST from insmod

[Petr Pavlu <petr.pavlu@suse.com>]

On 10/8/25 8:41 AM, Lucas De Marchi wrote:
> On Tue, Aug 19, 2025 at 09:17:50AM -0500, Lucas De Marchi wrote:
>> On Tue, Aug 19, 2025 at 10:52:16AM +0200, Petr Pavlu wrote:
>>> On 8/18/25 11:34 AM, Phil Sutter wrote:
>>>> On Sun, Aug 17, 2025 at 05:54:27PM +0200, Christophe Leroy wrote:
>>>>> Le 17/08/2025 à 01:33, Phil Sutter a écrit :
>>>>>> [Vous ne recevez pas souvent de courriers de phil@nwl.cc. D?couvrez pourquoi ceci est important ? https://aka.ms/LearnAboutSenderIdentification ]
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I admittedly didn't fully analyze the cause, but on my system a call to:
>>>>>>
>>>>>> # insmod /lib/module/$(uname -r)/kernel/net/netfilter/nf_conntrack_ftp.ko
>>>>>>
>>>>>> fails with -EEXIST (due to a previous call to 'nfct add helper ftp inet
>>>>>> tcp'). A call to:
>>>>>>
>>>>>> # modprobe nf_conntrack_ftp
>>>>>>
>>>>>> though returns 0 even though module loading fails. Is there a bug in
>>>>>> modprobe error status handling?
>>>>>>
>>>>>
>>>>> Read the man page : https://linux.die.net/man/8/modprobe
>>>>>
>>>>> In the man page I see:
>>>>>
>>>>>            Normally, modprobe will succeed (and do nothing) if told to
>>>>> insert a module which is already present or to remove a module which
>>>>> isn't present.
>>>>
>>>> This is not a case of already inserted module, it is not loaded before
>>>> the call to modprobe. It is the module_init callback
>>>> nf_conntrack_ftp_init() which returns -EEXIST it received from
>>>> nf_conntrack_helpers_register().
>>
>> is this a real failure condition or something benign like "if it's
>> already registered, there's nothing to do"?
>>
>>>>
>>>> Can't user space distinguish the two causes of -EEXIST? Or in other
>>>> words, is use of -EEXIST in module_init callbacks problematic?
>>>
>>> Unfortunately, error return codes from (f)init_module cannot be reliably
>>> depended upon. For instance, cpufreq drivers have similar behavior of
>>> returning -EEXIST when another cpufreq driver is already registered.
>>> Returning this code unexpectedly can then confuse kmod, as it interprets
>>> -EEXIST to mean "the module is already loaded" [1].
>>
>> well, it's not that it can't be relied on. There's 1 exit code that is
>> treated specially, EEXISTS, because that error is used by the module
>> loading part, before the module_init call, to signify the module is
>> already loaded.
>>
>>>
>>> I have thought about this problem before. We might fix the main
>>> problematic occurrences, but we can't really audit all the code that
>>> module init functions can invoke. I then wonder if it would make sense
>>> for the module loader to warn about any -EEXIST returned by a module's
>>> init function and translate it to -EBUSY.
>>
>> If it's a failure condition then yes, -EBUSY looks appropriate.
> 
> something like this:
> 
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b261849362..e5fb1a4ef3441 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: '%s'->init suspiciously returned %d: Overriding with %d\n",
> +                __func__, mod->name, -EEXIST, -EBUSY);
> +            ret = -EBUSY;
> +        }
>          goto fail_free_freeinit;
>      }
>      if (ret > 0) {

Yes, that's what I had in mind. Could you please send this as a proper
patch to the list?

I only think we should include a hint to explain why this is a problem
and simplify the message somewhat, something like:

pr_warn("%s: init suspiciously returned -EEXIST (reserved for loaded modules), overriding with -EBUSY\n", mod->name);

I realize you based the message on the later warning about the init
function returning a >0 value but I think we should rather update that
message as well. It should follow the usual style of
"<mod-name>: <error-description>". I suggest simplifying it to:

pr_warn("%s: init suspiciously returned %d, it should follow 0/-E convention\n", mod->name, ret);

-- 
Thanks,
Petr