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

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

On 08/09/26 5:40 pm, Petr Pavlu wrote:
> 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.

synchronize_rcu() comment — agreed, updated wording:

"synchronize_rcu() waits for any in-progress RCU read-side critical
sections to complete, ensuring all CPUs observe MODULE_STATE_GOING
before we proceed."

>>
>> 	/* 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.

finit_module / idempotent_complete ordering — you are right.
idempotent_complete() is called by idempotent_init_module() after
do_init_module() returns, not before.

> 
>> 	 *
>> 	 * - 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.

Refcount drain — agreed that polling is poor style.  Of the three
approaches you suggested, a per-module completion is the cleanest fit
here.  module_wq is a global queue shared across all module loading
activity; reusing it for the drain would require a condition check on
every wake-up and risk spurious wakes across unrelated modules.  A
separate dedicated wait queue would work but adds more infrastructure
than needed for a single binary event.

A per-module struct completion is the right primitive for "wait for this
one thing to happen exactly once": add refs_gone to struct module inside
CONFIG_MODULE_UNLOAD, initialize it in module_unload_init(), and call
complete() from module_put() when refcnt drops to MODULE_REF_BASE.  The
fail: path then replaces the polling loop with a single
wait_for_completion(&mod->refs_gone).

Rather than continuing the design discussion I will send the patch
directly so it is easier to review concretely.