Re: Enabling built-in modules earlier in init

[Paul Goyette <paul%whooppee.com@localhost>]

On Wed, 16 Jun 2010, Antti Kantee wrote:

> I have to admit I haven't been following your work too closely, but
> builtin modules are initialized either when all of them are initialized
> per class or when their initialization is explicitly requested.  So if
> whatever uses PCIVERBOSE requests the load of the PCIVERBOSE module,
> it should be initialized and you should be fine (see module_do_load()).
>
> The only "but" is that explicit loads must be accompanied by
> MODCTL_LOAD_FORCE.  I wrote it that way because of the security use case:
> if you disable a builtin module due to a security hole, you don't want
> it to get autoloaded later.  For file system modules you can always use
> rm, but for builtins you don't have that luxury.  So if that is actually
> what you're chocking on, I suggest adding some flag to determine if the
> module has ever been loaded and ignore the need for -F if it hasn't.

The attached diffs add a new mod_disabled member to the module_t 
structure, and set the value to false in each place that a new entry is 
created.  (Since all of the allocations of module_t structures are done 
with kmem_zalloc() I could probably avoid the explicit setting of the 
value to false.)

The value is set to true whenever a module is removed from active duty 
and returned to the module_builtin list.  (I specifically did NOT mark a 
module disabled if its modcmd(INIT) failed, under the assumption that it 
might succeed in a later retry.)

Loading of an entry from the module_builtin list no longer requires the 
-f flag if the entry has not been disabled.

This approach avoids the need for defining new a new module class, and 
it avoids needing to mess around with the current secmodel_register() 
stuff.




-------------------------------------------------------------------------
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:       |
| Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com    |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
| Kernel Developer |                          | pgoyette at netbsd.org  |
-------------------------------------------------------------------------

Index: sys/module.h
===================================================================
RCS file: /cvsroot/src/sys/sys/module.h,v
retrieving revision 1.23
diff -u -p -r1.23 module.h
--- sys/module.h        24 May 2010 03:50:25 -0000      1.23
+++ sys/module.h        16 Jun 2010 12:42:42 -0000
@@ -90,6 +90,7 @@ typedef struct module {
        time_t                  mod_autotime;
        void                    *mod_ctf;
        u_int                   mod_fbtentries; /* DTrace FBT entrie count */
+       bool                    mod_disabled;
 } module_t;
 
 /*
Index: kern/kern_module.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_module.c,v
retrieving revision 1.69
diff -u -p -r1.69 kern_module.c
--- kern/kern_module.c  26 May 2010 23:53:21 -0000      1.69
+++ kern/kern_module.c  16 Jun 2010 12:42:42 -0000
@@ -195,6 +195,7 @@ module_builtin_add(modinfo_t *const *mip
                modp[i] = kmem_zalloc(sizeof(*modp[i]), KM_SLEEP);
                modp[i]->mod_info = mip[i+mipskip];
                modp[i]->mod_source = MODULE_SOURCE_KERNEL;
+               modp[i]->mod_disabled = false;
        }
        mutex_enter(&module_lock);
 
@@ -416,6 +417,7 @@ module_init_class(modclass_t class)
                         * init.
                         */
                        if (module_do_builtin(mi->mi_name, NULL) != 0) {
+                               mod->mod_disabled = true;
                                TAILQ_REMOVE(&module_builtins, mod, mod_chain);
                                TAILQ_INSERT_TAIL(&bi_fail, mod, mod_chain);
                        }
@@ -794,7 +796,7 @@ module_do_load(const char *name, bool is
                }
        }
        if (mod) {
-               if ((flags & MODCTL_LOAD_FORCE) == 0) {
+               if (mod->mod_disabled && (flags & MODCTL_LOAD_FORCE) == 0) {
                        if (!autoload) {
                                module_error("use -f to reinstate "
                                    "builtin module \"%s\"", name);
@@ -854,6 +856,7 @@ module_do_load(const char *name, bool is
                        return error;
                }
                mod->mod_source = MODULE_SOURCE_FILESYS;
+               mod->mod_disabled = false;
                TAILQ_INSERT_TAIL(&pending, mod, mod_chain);
 
                error = module_fetch_info(mod);
@@ -1086,6 +1089,7 @@ module_do_unload(const char *name)
        }
        if (mod->mod_source == MODULE_SOURCE_KERNEL) {
                mod->mod_nrequired = 0; /* will be re-parsed */
+               mod->mod_disabled = true;
                TAILQ_INSERT_TAIL(&module_builtins, mod, mod_chain);
                module_builtinlist++;
        } else {
@@ -1129,6 +1133,7 @@ module_prime(void *base, size_t size)
        }
 
        TAILQ_INSERT_TAIL(&module_bootlist, mod, mod_chain);
+       mod->mod_disabled = false;
 
        return 0;
 }