Re: [RFC] module: init-failure path can free a module with live try_module_get() users

[Mahanta Jambigi <mjambigi@linux.ibm.com>]

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?