Date: Mon, 23 Aug 2010 18:00:56 +0200 From: Attilio Rao <attilio@freebsd.org> To: Andriy Gapon <avg@icyb.net.ua> Cc: freebsd-hackers@freebsd.org, Ryan Stone <rysto32@gmail.com> Subject: Re: kld modules remain loaded if MOD_LOAD handler returns an error Message-ID: <AANLkTinxWAbZsunvQBTVWP5FqMYj=CTdrqYSRSoMBzA4@mail.gmail.com> In-Reply-To: <4C728DF4.2060203@icyb.net.ua> References: <AANLkTi=qJ3WZChWg5axnuH8OBMxzd6JrnmfsmXHqPY2Q@mail.gmail.com> <201008230810.04710.jhb@freebsd.org> <4C728DF4.2060203@icyb.net.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
2010/8/23 Andriy Gapon <avg@icyb.net.ua>:
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTinxWAbZsunvQBTVWP5FqMYj=CTdrqYSRSoMBzA4>