From owner-freebsd-hackers@FreeBSD.ORG Mon Aug 23 16:00:58 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 896AE1065673; Mon, 23 Aug 2010 16:00:58 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-qw0-f54.google.com (mail-qw0-f54.google.com [209.85.216.54]) by mx1.freebsd.org (Postfix) with ESMTP id 1FDC28FC1F; Mon, 23 Aug 2010 16:00:57 +0000 (UTC) Received: by qwg5 with SMTP id 5so5827103qwg.13 for ; Mon, 23 Aug 2010 09:00:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:sender:received :in-reply-to:references:date:x-google-sender-auth:message-id:subject :from:to:cc:content-type:content-transfer-encoding; bh=BrMxt6cSoI2/ThnmbC+c1XdcS1gOviq6kFkuCk3CRSE=; b=Zz8s/Am0DDSo4tFSLY4X0QLbSH6AjdPEJVsNTJYoQhWJeMIC297fNMQtMZSnIoUqez R/7cz7cEFDejf8R2xbzFzYiiRWVL0dT53I6wwnyFYNfUyupcW1kpHQy3tkxDu5fyTsyU EhcGg8e6NHLFXfmjhoaCIa856Vi0L/MidCZ8c= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=AUnN07PqQi72s5EQm2zkc1i9YRTWu6B7TjDMWRFkBZJDMzwHsZoZTe9pCILgz1+WOy 4ZMsQAX+BhT/N2BqmaDtzEFXpsX10F0za9LfTjR1CDQNIqrjbr6z0KBIDf8liXWJyijp 5pm90GYfpjJ8qAt15eF6BihYY818m6CDYKdwo= MIME-Version: 1.0 Received: by 10.229.10.217 with SMTP id q25mr2578983qcq.274.1282579257079; Mon, 23 Aug 2010 09:00:57 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.229.222.209 with HTTP; Mon, 23 Aug 2010 09:00:56 -0700 (PDT) In-Reply-To: <4C728DF4.2060203@icyb.net.ua> References: <201008230810.04710.jhb@freebsd.org> <4C728DF4.2060203@icyb.net.ua> Date: Mon, 23 Aug 2010 18:00:56 +0200 X-Google-Sender-Auth: c5miOgGa3mOA_12lXKfp8vvOKdI Message-ID: From: Attilio Rao To: Andriy Gapon Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 16:00:58 -0000 2010/8/23 Andriy Gapon : > 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) >>> { >>> =C2=A0 =C2=A0 return *test; >>> } >>> >>> static int >>> first_modevent(struct module *m, int what, void *arg) >>> { >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 int err =3D 0; >>> >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 switch (what) { >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 case MOD_LOAD: =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0/* kldload */ >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 test =3D malloc= (sizeof(int), M_TEMP, M_NOWAIT | M_ZERO); >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!test) >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 err =3D ENOMEM; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 case MOD_UNLOAD: =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0/* kldunload */ >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 default: >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D EINVAL; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 } >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return(err); >>> } >>> >>> static moduledata_t first_mod =3D { >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "first", >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 first_modevent, >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 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) >>> { >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 int err =3D 0; >>> >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 switch (what) { >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 case MOD_LOAD: =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0/* kldload */ >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 test_function()= ; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 case MOD_UNLOAD: =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0/* kldunload */ >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 default: >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D EINVAL; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 } >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return(err); >>> } >>> >>> static moduledata_t second_mod =3D { >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 "second", >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 second_modevent, >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 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. =C2=A0However, 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. =C2=A0Anybody have any thoughts on this? >> >> Yeah, it's not easy to fix. =C2=A0Probably we could patch the kernel lin= ker to >> notice if any of the modules for a given linker file had errors during >> initialization and trigger an unload if that occurs. =C2=A0I 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. I replied in private, but I really agree with Andriy here. Thanks, Attilio --=20 Peace can only be achieved by understanding - A. Einstein