From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B28E1C2D0A3 for ; Thu, 29 Oct 2020 16:40:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4FCBA2151B for ; Thu, 29 Oct 2020 16:40:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1603989618; bh=Cmr4c5D2EfH2hAoCtNb8SeEjBOWCAJ3zobSy3aGD0V8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=Q69HsTOVQTIlDJZtMEe2tr61LZ6DlO1qcCd0Mzir75c7gz7FtlMMwSKcZmb++X1sI GmiHbDH62kMAPLx/y3L6nBqpb4mw3+92hahpyLKROG8vHG3X4jDsXNm3CwdCbd+8xe xI3aIemtOKlhn5vYx/J09FwQmoUV08cgXRRqiKIQ= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727043AbgJ2QkR (ORCPT ); Thu, 29 Oct 2020 12:40:17 -0400 Received: from mail.kernel.org ([198.145.29.99]:50700 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725805AbgJ2QkQ (ORCPT ); Thu, 29 Oct 2020 12:40:16 -0400 Received: from linux-8ccs (p57a236d4.dip0.t-ipconnect.de [87.162.54.212]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 9A39820825; Thu, 29 Oct 2020 16:40:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1603989615; bh=Cmr4c5D2EfH2hAoCtNb8SeEjBOWCAJ3zobSy3aGD0V8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=HJu0V9jDLc4gXY65O9EahzpAoWO7rLf31/fcmr7djwPK8CtBV6p0Ik5uI+tJdA2hu 1eqVIAmR23hfmkgqCPkIbgk2NQCb1LyY/YThjekvcwVuMjmvkEZ2om5tL71nlBu530 2AwEecEsy5LT1J8hWp4kWLZ1HIble3uc9JJorN2s= Date: Thu, 29 Oct 2020 17:40:11 +0100 From: Jessica Yu To: Miroslav Benes Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH] module: set MODULE_STATE_GOING state when a module fails to load Message-ID: <20201029164010.GA5931@linux-8ccs> References: <20201027140336.15409-1-mbenes@suse.cz> <20201028122106.GA6867@linux-8ccs> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: X-OS: Linux linux-8ccs 4.12.14-lp150.12.61-default x86_64 User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +++ Miroslav Benes [29/10/20 13:31 +0100]: >On Wed, 28 Oct 2020, Jessica Yu wrote: > >> +++ 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 >> >--- >> > 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. > >Right. > >> This shouldn't happen if we properly set the module to GOING whenever >> it encounters an error during load_module(). > >That's correct. > >> 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.. > >Hopefully yes. I haven't found anything that would contradict it. > >I think it is even safer to leave UNFORMED there. free_module() explicitly >sets UNFORMED state too while going through the similar process. That's true, and agreed. >ftrace_release_mod() is the only inconsistency there. It is called with >UNFORMED in load_module() if going through ddebug_cleanup label >directly, and with GOING in both do_init_module() before free_module() is >called and delete_module syscall. But it probably does not care. Yes, I guess that inconsistency with ftrace_release_mod() has been there long before this patch :-/ A quick look through ftrace.c doesn't suggest to me there are any direct dependencies on module state there (other than that comment above ftrace_module_init()). In practice there hasn't been any issues.. I have applied this to modules-next. Thanks! Jessica