Re: [PATCH] module: drain external refs before freeing a failed-init module

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

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 <mjambigi@linux.ibm.com>
> ---
> Link: https://lore.kernel.org/all/5dc1fe2b-289e-4786-b9e9-e181dda8d4e9@linux.ibm.com/
>       [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;
>  }