kld modules remain loaded if MOD_LOAD handler returns an error

[Attilio Rao]

2010/8/23 Andriy Gapon <avg at 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)
>>> {
>>>     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.

I replied in private, but I really agree with Andriy here.

Thanks,
Attilio


-- 
Peace can only be achieved by understanding - A. Einstein