kld modules remain loaded if MOD_LOAD handler returns an error

[John Baldwin]

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