> > Hi Luis, > > Firstly, thank you for your review and feedback thus far. > > > Please Cc the folks I added in future iterations. > > All right. > > > > If the previously unloaded module is loaded once again it will be removed > > > from the list only if the taints bitmask is the same. > > > > That doesn't seem to be clear. What if say a user loads a module which > > taints the kernel, and then unloads it, and then tries to load a similar > > module with the same name but that it does not taint the kernel? > > > > Would't we loose visibility that at one point the tainting module was > > loaded? OK I see after reviewing the patch that we keep track of each > > module instance unloaded with an attached unsigned long taints. So if > > a module was unloaded with a different taint, we'd see it twice. Is that > > right? > > Indeed - is this acceptable to you? I prefer this approach rather than > remove it from the aforementioned list solely based on the module name. > > > > The number of tracked modules is not fixed and can be modified accordingly. > > > > The commit should mention what happens if the limit is reached. > > I will mention this accordingly. > > > wc -l kernel/*.c| sort -r -n -k 1| head > > 84550 total > > 6143 kernel/workqueue.c > > 4810 kernel/module.c > > 4789 kernel/signal.c > > 3170 kernel/fork.c > > 2997 kernel/auditsc.c > > 2902 kernel/kprobes.c > > 2857 kernel/sysctl.c > > 2760 kernel/sys.c > > 2712 kernel/cpu.c > > > > I think it is time we start splitting module.c out into components, > > and here we might have a good opportunity to do that. There are tons > > of nasty cob webs I'd like to start cleaning up from module.c. So > > how about we start by moving module stuff out to kernel/modules/main.c > > and then you can bring in your taint friend into that directory. > > > > That way we can avoid the #ifdefs, which seem to attract huge spiders. > > Agreed. This makes sense. I'll work on it. Aaron, Luis, I have some ideas and did some work on it. Let me know if we could work together on this. - Allen > > > Maybe live patch stuff go in its own file too? > > At first glance, I believe this is possible too. > > > > > > +static LIST_HEAD(unloaded_tainted_modules); > > > +static int tainted_list_count; > > > +int __read_mostly tainted_list_max_count = 20; > > > > Please read the guidance for __read_mostly on include/linux/cache.h. > > I don't see performance metrics on your commit log to justify this use. > > We don't want people to just be using that for anything they think is > > read often... but not really in the context of what it was originally > > desinged for. > > Understood. > > > Loading and unloading modules... to keep track of *which ones are > > tainted*. I'd find it extremely hard to believe this is such a common > > thing and hot path that we need this. > > > > In any case, since a linked list is used, I'm curious why did you > > decide to bound this to an arbitrary limit of say 20? If this > > feature is enabled why not make this boundless? > > It can be, once set to 0. Indeed, the limit specified above is arbitrary. > Personally, I prefer to have some limit that can be controlled by the user. > In fact, if agreed, I can incorporate the limit [when specified] into the > output generated via print_modules(). > > > > > > +struct mod_unloaded_taint { > > > + struct list_head list; > > > + char name[MODULE_NAME_LEN]; > > > + unsigned long taints; > > > +}; > > > +#endif > > > > > > /* Work queue for freeing init sections in success case */ > > > static void do_free_init(struct work_struct *w); > > > @@ -310,6 +321,47 @@ int unregister_module_notifier(struct notifier_block *nb) > > > } > > > EXPORT_SYMBOL(unregister_module_notifier); > > > > > > +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING > > > + > > > +static int try_add_tainted_module(struct module *mod) > > > +{ > > > + struct mod_unload_taint *mod_taint; > > > + > > > + module_assert_mutex_or_preempt(); > > > + > > > + if (tainted_list_max_count >= 0 && mod->taints) { > > > + if (!tainted_list_max_count && > > > + tainted_list_count >= tainted_list_max_count) { > > > + pr_warn_once("%s: limit reached on the unloaded tainted modules list (count: %d).\n", > > > + mod->name, tainted_list_count); > > > + goto out; > > > + } > > > + > > > + mod_taint = kmalloc(sizeof(*mod_taint), GFP_KERNEL); > > > + if (unlikely(!mod_taint)) > > > + return -ENOMEM; > > > + else { > > > + strlcpy(mod_taint->name, mod->name, > > > + MODULE_NAME_LEN); > > > + mod_taint->taints = mod->taints; > > > + list_add_rcu(&mod_taint->list, > > > + &unloaded_tainted_modules); > > > + tainted_list_count++; > > > + } > > > +out: > > > + } > > > + return 0; > > > +} > > > + > > > +#else /* MODULE_UNLOAD_TAINT_TRACKING */ > > > + > > > +static int try_add_tainted_module(struct module *mod) > > > +{ > > > + return 0; > > > +} > > > + > > > +#endif /* MODULE_UNLOAD_TAINT_TRACKING */ > > > + > > > /* > > > * We require a truly strong try_module_get(): 0 means success. > > > * Otherwise an error is returned due to ongoing or failed > > > @@ -579,6 +631,23 @@ struct module *find_module(const char *name) > > > { > > > return find_module_all(name, strlen(name), false); > > > } > > > +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING > > > +struct mod_unload_taint *find_mod_unload_taint(const char *name, size_t len, > > > + unsigned long taints) > > > +{ > > > + struct mod_unload_taint *mod_taint; > > > + > > > + module_assert_mutex_or_preempt(); > > > + > > > + list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules, list, > > > + lockdep_is_held(&module_mutex)) { > > > + if (strlen(mod_taint->name) == len && !memcmp(mod_taint->name, > > > + name, len) && mod_taint->taints & taints) { > > > + return mod_taint; > > > + } > > > + } > > > + return NULL; > > > +#endif > > > > > > #ifdef CONFIG_SMP > > > > > > @@ -1121,13 +1190,13 @@ static inline int module_unload_init(struct module *mod) > > > } > > > #endif /* CONFIG_MODULE_UNLOAD */ > > > > > > -static size_t module_flags_taint(struct module *mod, char *buf) > > > +static size_t module_flags_taint(unsigned long taints, char *buf) > > > { > > > size_t l = 0; > > > int i; > > > > > > for (i = 0; i < TAINT_FLAGS_COUNT; i++) { > > > - if (taint_flags[i].module && test_bit(i, &mod->taints)) > > > + if (taint_flags[i].module && test_bit(i, &taints)) > > > buf[l++] = taint_flags[i].c_true; > > > } > > > > Please make this its own separate patch. This makes it easier to review > > the other changes. > > No problem, will do. > > > > > > > @@ -1194,7 +1263,7 @@ static ssize_t show_taint(struct module_attribute *mattr, > > > { > > > size_t l; > > > > > > - l = module_flags_taint(mk->mod, buffer); > > > + l = module_flags_taint(mk->mod->taints, buffer); > > > buffer[l++] = '\n'; > > > return l; > > > } > > > @@ -2193,6 +2262,9 @@ static void free_module(struct module *mod) > > > module_bug_cleanup(mod); > > > /* Wait for RCU-sched synchronizing before releasing mod->list and buglist. */ > > > synchronize_rcu(); > > > + if (try_add_tainted_module(mod)) > > > + pr_error("%s: adding tainted module to the unloaded tainted modules list failed.\n", > > > + mod->name); > > > mutex_unlock(&module_mutex); > > > > > > /* Clean up CFI for the module. */ > > > @@ -3670,6 +3742,9 @@ static noinline int do_init_module(struct module *mod) > > > { > > > int ret = 0; > > > struct mod_initfree *freeinit; > > > +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING > > > + struct mod_unload_taint *old; > > > +#endif > > > > > > freeinit = kmalloc(sizeof(*freeinit), GFP_KERNEL); > > > if (!freeinit) { > > > @@ -3703,6 +3778,16 @@ static noinline int do_init_module(struct module *mod) > > > mod->state = MODULE_STATE_LIVE; > > > blocking_notifier_call_chain(&module_notify_list, > > > MODULE_STATE_LIVE, mod); > > > +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING > > > + mutex_lock(&module_mutex); > > > + old = find_mod_unload_taint(mod->name, strlen(mod->name), > > > + mod->taints); > > > + if (old) { > > > + list_del_rcu(&old->list); > > > + synchronize_rcu(); > > > + } > > > + mutex_unlock(&module_mutex); > > > > But here we seem to delete an old instance of the module taint > > history if it is loaded again and has the same taint properties. > > Why? > > At first glance, in this particular case, I believe this makes sense to > avoid duplication i.e. the taint module would be stored in the 'modules' > list thus should be shown once via print_modules(). So, the initial > objective was to only track a "tainted" module when unloaded and once > added/or loaded again [with the same taint(s)] further tracking cease. > > > I mean, if a taint happened once, and our goal is to keep track > > of them, I'd imagine I'd want to know that this had happened > > before, so instead how about just an increment counter for this, > > so know how many times this has happened? Please use u64 for that. > > I have some test environments where module unloaded happens *a lot*. > > If I understand correctly, I do not like this approach but indeed it could > work. Personally, I would like to incorporate the above idea i.e. track > the unload count, into the initial goal. > > > > > > +#endif > > > > > > /* Delay uevent until module has finished its init routine */ > > > kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD); > > > @@ -4511,7 +4596,7 @@ static char *module_flags(struct module *mod, char *buf) > > > mod->state == MODULE_STATE_GOING || > > > mod->state == MODULE_STATE_COMING) { > > > buf[bx++] = '('; > > > - bx += module_flags_taint(mod, buf + bx); > > > + bx += module_flags_taint(mod->taints, buf + bx); > > > > This change can be its own separate patch. > > Will do. > > > > > > /* Show a - for module-is-being-unloaded */ > > > if (mod->state == MODULE_STATE_GOING) > > > buf[bx++] = '-'; > > > @@ -4735,6 +4820,10 @@ void print_modules(void) > > > { > > > struct module *mod; > > > char buf[MODULE_FLAGS_BUF_SIZE]; > > > +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING > > > + struct mod_unload_taint *mod_taint; > > > + size_t l; > > > +#endif > > > > > > printk(KERN_DEFAULT "Modules linked in:"); > > > /* Most callers should already have preempt disabled, but make sure */ > > > @@ -4744,6 +4833,15 @@ void print_modules(void) > > > continue; > > > pr_cont(" %s%s", mod->name, module_flags(mod, buf)); > > > } > > > +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING > > > + printk(KERN_DEFAULT "\nUnloaded tainted modules:"); > > > + list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules, > > > + list) { > > > + l = module_flags_taint(mod_taint->taints, buf); > > > + buf[l++] = '\0'; > > > + pr_cont(" %s(%s)", mod_taint->name, buf); > > > + } > > > +#endif > > > > Ugh yeah no, this has to be in its own file. Reading this file > > is just one huge effort right now. Please make this a helper so we > > don't have to see this eye blinding code. > > Sure, no problem. > > > > > > preempt_enable(); > > > if (last_unloaded_module[0]) > > > pr_cont(" [last unloaded: %s]", last_unloaded_module); > > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > > > index 272f4a272f8c..290ffaa5b553 100644 > > > --- a/kernel/sysctl.c > > > +++ b/kernel/sysctl.c > > > @@ -2078,6 +2078,16 @@ static struct ctl_table kern_table[] = { > > > .extra1 = SYSCTL_ONE, > > > .extra2 = SYSCTL_ONE, > > > }, > > > +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING > > > + { > > > + .procname = "tainted_list_max_count", > > > + .data = &tainted_list_max_count, > > > + .maxlen = sizeof(int), > > > + .mode = 0644, > > > + .proc_handler = proc_dointvec_minmax, > > > + .extra1 = &neg_one, > > > + }, > > > +#endif > > > #endif > > > #ifdef CONFIG_UEVENT_HELPER > > > > Please see kernel/sysctl.c changes on linux-next, we're moving away > > from everyone stuffing their sysclts in kernel/sysctl.c and there > > you can find helpers and examples of how *not* to do this. Its > > on the kernel table so you should be able to just > > register_sysctl_init("kernel", modules_sysctls) and while at it, > > if you spot any sysctls for module under the kern_table, please > > move those over and then your patch would be adding just one new > > entry to that new local modules_sysctls table. > > > > We'll have to coordinate with Andrew given that if your changes > > depend on those changes then we might as well get all your > > changes through Andrew for the next release cycle. > > All right. I will make the required changes. Thanks once again. > > > > Regards, > > -- > Aaron Tomlin > -- - Allen