Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 28 Oct 2020 13:21:06 +0100
From:   Jessica Yu <jeyu@kernel.org>
To:     Miroslav Benes <mbenes@suse.cz>
Cc:     linux-kernel@vger.kernel.org
Subject: Re: [PATCH] module: set MODULE_STATE_GOING state when a module fails
 to load
Message-ID: <20201028122106.GA6867@linux-8ccs>
References: <20201027140336.15409-1-mbenes@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20201027140336.15409-1-mbenes@suse.cz>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

+++ Miroslav Benes [27/10/20 15:03 +0100]:
>If a module fails to load due to an error in prepare_coming_module(),
>the following error handling in load_module() runs with
>MODULE_STATE_COMING in module's state. Fix it by correctly setting
>MODULE_STATE_GOING under "bug_cleanup" label.
>
>Signed-off-by: Miroslav Benes <mbenes@suse.cz>
>---
> kernel/module.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index a4fa44a652a7..b34235082394 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -3991,6 +3991,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> 				     MODULE_STATE_GOING, mod);
> 	klp_module_going(mod);
>  bug_cleanup:
>+	mod->state = MODULE_STATE_GOING;
> 	/* module_bug_cleanup needs module_mutex protection */
> 	mutex_lock(&module_mutex);
> 	module_bug_cleanup(mod);

Thanks for the fix! Hmm, I am wondering if we also need to set the
module to GOING if it happens to fail while it is still UNFORMED.

Currently, when a module is UNFORMED and encounters an error during
load_module(), it stays UNFORMED until it finally goes away. That
sounds fine, but try_module_get() technically permits you to get a
module while it's UNFORMED (but not if it's GOING). Theoretically
someone could increase the refcount of an unformed module that has
encountered an error condition and is in the process of going away.
This shouldn't happen if we properly set the module to GOING whenever
it encounters an error during load_module().

But - I cannot think of a scenario where someone could call
try_module_get() on an unformed module, since find_module() etc. do
not return unformed modules, so they shouldn't be visible outside of
the module loader. So in practice, I think we're probably safe here..