On 8/31/26 12:00 PM, Mahanta Jambigi wrote:
> On 28/08/26 5:33 pm, Petr Pavlu wrote:
>> On 8/24/26 8:13 AM, Mahanta Jambigi wrote:
>>> Hi Luis, Petr, Daniel, Sami, Aaron,
>>>
>>> I'm writing to ask about what looks like a generic module-init failure
>>> lifetime problem in the module loader. I ran into it while working on
>>> the SMC networking module (net/smc/), but after several patch
>>> iterations, it seems the root issue may belong in kernel/module/main.c
>>> rather than in SMC itself. I'd appreciate your guidance on whether this
>>> reading is correct, and if so, what fix direction would be preferred.
>>>
>>> THE ISSUE IN do_init_module()
>>> =============================
>>>
>>> include/linux/module.h has a long-standing FIXME in module_is_live():
>>>
>>> /* FIXME: It'd be nice to isolate modules during init, too, so they
>>> aren't used before they (may) fail. But presently too much code
>>> (IDE & SCSI) require entry into the module during init. */
>>> static inline bool module_is_live(struct module *mod)
>>> {
>>> return mod->state != MODULE_STATE_GOING;
>>> }
>>>
>>> Because MODULE_STATE_COMING is not MODULE_STATE_GOING, try_module_get()
>>> can succeed once a module's __init is executing. If __init makes the
>>> module externally reachable partway through and then later fails, the
>>> failure path in do_init_module() appears to do:
>>>
>>> fail:
>>> mod->state = MODULE_STATE_GOING;
>>> synchronize_rcu();
>>> module_put(mod);
>>> ...
>>> free_module(mod);
>>>
>>> synchronize_rcu() waits for RCU readers, but not for threads that
>>> already obtained a module reference via try_module_get() and are still
>>> executing module text.
>>>
>>> By contrast, the normal unload path in try_stop_module() refuses to
>>> proceed while the refcount is non-zero.
>>>
>>> So the asymmetry seems to be that the normal unload path waits for
>>> references to drain, while the init-failure path does not.
>>>
>>> A concrete race would look like:
>>>
>>> 1. Module __init registers an externally reachable interface.
>>> 2. User space enters through that interface and try_module_get()
>>> succeeds while the module is still COMING.
>>> 3. A later __init step fails.
>>> 4. do_init_module() frees the module.
>>> 5. The in-flight caller is still executing module text.
>>>
>>> SMC AS A CONCRETE EXAMPLE
>>> =========================
>>>
>>> In SMC, simply moving registration later does not appear to eliminate
>>> the window, because there are two separate registration points that can
>>> make the module reachable via socket():
>>>
>>> 1. sock_register(&smc_sock_family_ops)
>>> After this, socket(AF_SMC, ...) can succeed and reach
>>> try_module_get() via __sock_create().
>>>
>>> 2. smc_inet_init() -> inet_register_protosw()
>>> After this, socket(AF_INET, SOCK_STREAM, IPPROTO_SMC) can succeed
>>> and again reach try_module_get().
>>>
>>> Either registration point can succeed before a later init step fails.
>>>
>>> This may not be specific to SMC; other protocol modules that become
>>> reachable during init, such as Bluetooth, may have similar exposure and
>>> appear worth auditing as well.
>>>
>>> ON THE FIXME'S IDE/SCSI CONCERN
>>> ===============================
>>>
>>> The FIXME mentions IDE and SCSI as reasons not to isolate modules
>>> during init.
>>>
>>> 1. IDE was removed in Linux 5.14, so that half of the concern no
>>> longer applies.
>>>
>>> 2. SCSI still appears to self-reference during init
>>> (scsi_device_get() -> try_module_get(hostt->module) during
>>> scsi_scan_host()), so a blanket wait-for-refcount-to-drain
>>> approach in the failure path may deadlock there.
>>>
>>> Also, strong_try_module_get() already rejects MODULE_STATE_COMING with
>>> -EBUSY, so the infrastructure for refusing callers during init already
>>> exists in some form.
>>>
>>> QUESTIONS
>>> =========
>>>
>>> First, is my reading of this init-failure refcount/lifetime asymmetry
>>> correct?
>>
>> Your analysis looks correct to me.
>>
>>>
>>> If so, would one of the following directions be acceptable?
>>>
>>> 1. An opt-in mechanism (for example, a module flag) for modules that
>>> are safe to isolate during init and whose init-failure path should
>>> wait for external references to drain.
>>
>> In general, it is preferred if the module loader handles all modules in
>> the same way.
>>
>> I would say that the module loader should wait for external references
>> to drain after an init failure for all modules and that it should be the
>> responsibility of individual modules to ensure that this wait eventually
>> completes. Excluding some modules would mean that the module loader
>> could still free them while they are in use by the kernel.
>>
>> Before such a wait, the module loader should cancel all idempotent
>> module loads. This is especially important during boot when several
>> udevd workers may be trying to insert the same module. In that case,
>> a failed module init function should block only a single udevd task, so
>> that the system can still boot properly.
>>
> Thank you for the clear direction. I agree with both points — uniform
> handling for all modules, and unblocking concurrent loaders before the
> drain wait. Below is the proposed change with the rationale for each
> step. Proposed change to the fail: path in do_init_module().
>
> fail_free_freeinit:
> kfree(freeinit);
> fail:
> /*
> * Mark dying so try_module_get() fails for all new callers.
> * synchronize_rcu() ensures this is visible on all CPUs before
> * we proceed; no new references can be taken after this point.
> */
> mod->state = MODULE_STATE_GOING;
> synchronize_rcu();
The comment is somewhat misleading. The code invokes synchronize_rcu()
to wait for any existing RCU readers to finish before proceeding, making
sure that all new readers now observe the GOING state.
>
> /* Drop the loader's own reference taken in module_unload_init(). */
> module_put(mod);
>
> /*
> * Unblock concurrent loaders before blocking on the drain below,
> * so that a failed init delays only this task, not every udevd
> * worker that raced to load the same module.
> *
> * Two dedup paths exist:
> *
> * - finit_module path: losers of the inode race sleep in
> * idempotent_wait_for_completion(). They are unblocked by
> * idempotent_complete() in idempotent_init_module(), which
> * runs as do_init_module() returns — before we reach here.
> * No action needed.
This looks incorrect. Since this code is added in do_init_module(),
idempotent_complete() has not yet been invoked.
> *
> * - init_module path: callers sleep in module_patient_check_exists()
> * on module_wq waiting for finished_loading(), which returns
> * true once state == MODULE_STATE_GOING. wake_up_all() kicks
> * them loose immediately.
> */
> *wake_up_all*(&module_wq);
>
> /*
> * Drain async workers scheduled during __init (e.g. SCSI async
> * scan). MODULE_STATE_GOING is visible everywhere, so workers
> * that have not yet called try_module_get() will fail cleanly.
> * Workers already holding a reference complete and release it
> * naturally. Must run before free_module() regardless of
> * async_probe_requested.
> */
> *async_synchronize_full*();
>
> /*
> * Wait for references taken before MODULE_STATE_GOING became
> * visible. refcnt is monotonically decreasing from here; the
> * loop terminates provided the module's error path pairs every
> * __module_get() with a module_put(). The hung-task detector
> * catches violations.
> */
> while (*module_refcount*(mod) != 0)
> msleep(10);
I think that instead of repeatedly polling the module refcount, it would
be better for this code to sleep and for module_put() to wake it when
the refcount drops to 0. This could be implemented using either the
existing module_wq, a separate wait queue or a per-module completion.
--
Thanks,
Petr
