On Monday, August 23, 2010 11:04:20 am Andriy Gapon wrote: > 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.
Well, if these two event handlers are in the same module, I think that is a bug in the module really. I tend to collapse such things down to a single event handler per kld just so I can really get the ordering correct anyway. :) If they are in two separate .ko files then the other solution would work. We could also hack the module code to mark a linker_file as 'broken' and have the module_helper sysinit not call mod_load if the containing file is 'broken', etc. -- John Baldwin _______________________________________________ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"