Hi All,
Gentle ping on this patch. Is there anything I should change, or
any additional information that would help the review?
Thank You,
Mahanta
On 07/10/26 2:09 pm, Mahanta Jambigi wrote:
> When a module's __init makes it externally reachable (e.g. by registering a
> protocol family or a bus driver) and a later __init step fails,
> do_init_module()
> frees the module while callers that entered through that interface and called
> try_module_get() may still be executing module text.
>
> The normal unload path in try_stop_module() refuses to proceed while
> module_refcount() is non-zero. The init-failure path has no such check and
> races
> to free_module() immediately after synchronize_rcu().
>
> Fix it by waiting for all external references to drain before calling
> free_module(). Add a per-module struct completion refs_gone inside the
> existing
> CONFIG_MODULE_UNLOAD block that is initialised in module_unload_init() and
> completed from module_put() when refcnt drops back to MODULE_REF_BASE and the
> module state is MODULE_STATE_GOING. The fail: path calls wait_for_completion()
> (guarded by the same #ifdef) after dropping the loader's own reference,
> replacing the previous implicit assumption that no external refs exist at that
> point.
>
> Also move wake_up_all(&module_wq) to before the drain so that concurrent
> loaders
> blocked in module_patient_check_exists() are unblocked immediately once
> MODULE_STATE_GOING is visible, rather than after the drain completes.
>
> Fixes: 1fb9341ac348 ("module: put modules in list much earlier.")
> Signed-off-by: Mahanta Jambigi <[email protected]>
> ---
> Link:
> https://lore.kernel.org/all/[email protected]/
> [RFC] module: init-failure path can free a module with live
> try_module_get() users
>
> include/linux/module.h | 2 ++
> kernel/module/main.c | 33 +++++++++++++++++++++++++++-----
> 2 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 96cc98568eea..0dee2ed02a2d 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -562,6 +562,8 @@ struct module {
> void (*exit)(void);
>
> atomic_t refcnt;
> + /* Completed when refcnt drops to MODULE_REF_BASE on init failure. */
> + struct completion refs_gone;
> #endif
>
> #ifdef CONFIG_CONSTRUCTORS
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c1b34dc1e89a..655c693bd8e2 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -655,6 +655,7 @@ static int module_unload_init(struct module *mod)
> * refcnt == 0 means module is going.
> */
> atomic_set(&mod->refcnt, MODULE_REF_BASE);
> + init_completion(&mod->refs_gone);
>
> INIT_LIST_HEAD(&mod->source_list);
> INIT_LIST_HEAD(&mod->target_list);
> @@ -956,6 +957,14 @@ void module_put(struct module *module)
> ret = atomic_dec_if_positive(&module->refcnt);
> WARN_ON(ret < 0); /* Failed to put refcount */
> trace_module_put(module, _RET_IP_);
> + /*
> + * If refcnt just dropped to MODULE_REF_BASE and the module is
> going
> + * away due to an init failure, all external references are
> gone. Wake
> + * any thread waiting in the init-failure drain path.
> + */
> + if (ret == MODULE_REF_BASE &&
> + module->state == MODULE_STATE_GOING)
> + complete(&module->refs_gone);
> }
> }
> EXPORT_SYMBOL(module_put);
> @@ -3206,16 +3214,36 @@ static noinline int do_init_module(struct module *mod)
> fail_free_freeinit:
> kfree(freeinit);
> fail:
> - /* Try to protect us from buggy refcounters. */
> mod->state = MODULE_STATE_GOING;
> synchronize_rcu();
> +
> + /*
> + * Unblock concurrent loaders waiting in module_patient_check_exists()
> on
> + * module_wq. finished_loading() returns true once state is GOING, so
> they
> + * can now proceed. Do this before the drain below so a failed init
> delays
> + * only this task, not every other loader that raced to load the same
> + * module.
> + */
> + wake_up_all(&module_wq);
> +
> + /* Drop the loader's own reference taken in module_unload_init(). */
> module_put(mod);
> +
> +#ifdef CONFIG_MODULE_UNLOAD
> + /*
> + * Wait for all external references taken before MODULE_STATE_GOING
> became
> + * visible to be released. module_put() completes refs_gone when refcnt
> + * drops to MODULE_REF_BASE. The hung-task detector will catch any
> module
> + * whose error path leaks a reference.
> + */
> + wait_for_completion(&mod->refs_gone);
> +#endif
> +
> blocking_notifier_call_chain(&module_notify_list,
> MODULE_STATE_GOING, mod);
> klp_module_going(mod);
> ftrace_release_mod(mod);
> free_module(mod);
> - wake_up_all(&module_wq);
>
> return ret;
> }
