From owner-freebsd-hackers@FreeBSD.ORG Mon Aug 23 15:10:34 2010 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E70751065697 for ; Mon, 23 Aug 2010 15:10:34 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 9C9D58FC08 for ; Mon, 23 Aug 2010 15:10:34 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id F05F746B60; Mon, 23 Aug 2010 11:10:33 -0400 (EDT) Received: from jhbbsd.localnet (smtp.hudson-trading.com [209.249.190.9]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id ECEE68A03C; Mon, 23 Aug 2010 11:10:32 -0400 (EDT) From: John Baldwin To: Andriy Gapon Date: Mon, 23 Aug 2010 11:10:04 -0400 User-Agent: KMail/1.13.5 (FreeBSD/7.3-CBSD-20100819; KDE/4.4.5; amd64; ; ) References: <201008230810.04710.jhb@freebsd.org> <4C728DF4.2060203@icyb.net.ua> In-Reply-To: <4C728DF4.2060203@icyb.net.ua> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201008231110.04552.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Mon, 23 Aug 2010 11:10:32 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.6 required=4.2 tests=AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: freebsd-hackers@freebsd.org, Ryan Stone Subject: Re: kld modules remain loaded if MOD_LOAD handler returns an error X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Aug 2010 15:10:35 -0000 On Monday, August 23, 2010 11:04:20 am Andriy Gapon wrote: > on 23/08/2010 15:10 John Baldwin said the following: > > On Friday, August 20, 2010 1:13:53 pm Ryan Stone wrote: > >> Consider the following modules: > >> > >> /* first.c */ > >> static int *test; > >> > >> int > >> test_function(void) > >> { > >> return *test; > >> } > >> > >> static int > >> first_modevent(struct module *m, int what, void *arg) > >> { > >> int err = 0; > >> > >> switch (what) { > >> case MOD_LOAD: /* kldload */ > >> test = malloc(sizeof(int), M_TEMP, M_NOWAIT | M_ZERO); > >> if (!test) > >> err = ENOMEM; > >> break; > >> case MOD_UNLOAD: /* kldunload */ > >> break; > >> default: > >> err = EINVAL; > >> break; > >> } > >> return(err); > >> } > >> > >> static moduledata_t first_mod = { > >> "first", > >> first_modevent, > >> NULL > >> }; > >> > >> DECLARE_MODULE(first, first_mod, SI_SUB_KLD, SI_ORDER_ANY); > >> MODULE_VERSION(first, 1); > >> > >> > >> /* second.c */ > >> static int > >> second_modevent(struct module *m, int what, void *arg) > >> { > >> int err = 0; > >> > >> switch (what) { > >> case MOD_LOAD: /* kldload */ > >> test_function(); > >> break; > >> case MOD_UNLOAD: /* kldunload */ > >> break; > >> default: > >> err = EINVAL; > >> break; > >> } > >> return(err); > >> } > >> > >> static moduledata_t second_mod = { > >> "second", > >> second_modevent, > >> NULL > >> }; > >> > >> DECLARE_MODULE(second, second_mod, SI_SUB_KLD, SI_ORDER_ANY); > >> MODULE_DEPEND(second, first, 1, 1, 1); > >> > >> > >> Consider the case where malloc fails in first_modevent. > >> first_modevent will return ENOMEM, but the module will remain loaded. > >> Now when the second module goes and loads, it calls into the first > >> module, which is not initialized properly, and promptly crashes when > >> test_function() dereferences a null pointer. > >> > >> It seems to me that a module should be unloaded if it returns an error > >> from its MOD_LOAD handler. However, that's easier said than done. > >> The MOD_LOAD handler is called from a SYSINIT, and there's no > >> immediately obvious way to pass information about the failure from the > >> SYSINIT to the kernel linker. Anybody have any thoughts on this? > > > > Yeah, it's not easy to fix. Probably we could patch the kernel linker to > > notice if any of the modules for a given linker file had errors during > > initialization and trigger an unload if that occurs. I don't think this would > > be too hard to do. > > John, > > please note that for this testcase we would need to prevent second module's > modevent from being executed at all. > Perhaps a module shouldn't be considered as loaded until modevent caller marks it > as successfully initialized, but I haven't looked at the actual code. Well, if these two event handlers are in the same module, I think that is a bug in the module really. I tend to collapse such things down to a single event handler per kld just so I can really get the ordering correct anyway. :) If they are in two separate .ko files then the other solution would work. We could also hack the module code to mark a linker_file as 'broken' and have the module_helper sysinit not call mod_load if the containing file is 'broken', etc. -- John Baldwin