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();
/* 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.
*
* - 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);
blocking_notifier_call_chain(&module_notify_list,
MODULE_STATE_GOING, mod);
klp_module_going(mod);
ftrace_release_mod(mod);
free_module(mod);
return ret;
Does this direction look correct to you?
