kld modules remain loaded if MOD_LOAD handler returns an error

[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)
>> {
>>     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