From owner-freebsd-hackers@FreeBSD.ORG Mon Aug 23 15:04:23 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 C176F10656CF; Mon, 23 Aug 2010 15:04:23 +0000 (UTC) (envelope-from avg@icyb.net.ua) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id D0DAE8FC15; Mon, 23 Aug 2010 15:04:22 +0000 (UTC) Received: from odyssey.starpoint.kiev.ua (alpha-e.starpoint.kiev.ua [212.40.38.101]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id SAA18631; Mon, 23 Aug 2010 18:04:21 +0300 (EEST) (envelope-from avg@icyb.net.ua) Message-ID: <4C728DF4.2060203@icyb.net.ua> Date: Mon, 23 Aug 2010 18:04:20 +0300 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; U; FreeBSD amd64; en-US; rv:1.9.2.8) Gecko/20100823 Lightning/1.0b2 Thunderbird/3.1.2 MIME-Version: 1.0 To: John Baldwin References: <201008230810.04710.jhb@freebsd.org> In-Reply-To: <201008230810.04710.jhb@freebsd.org> X-Enigmail-Version: 1.1.2 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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:04:23 -0000 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. -- Andriy Gapon