On Thu, Jun 06, 2019 at 10:24:17PM +0000, Nadav Amit wrote: > > +static void static_call_del_module(struct module *mod) > > +{ > > + struct static_call_site *start = mod->static_call_sites; > > + struct static_call_site *stop = mod->static_call_sites + > > + mod->num_static_call_sites; > > + struct static_call_site *site; > > + struct static_call_key *key, *prev_key = NULL; > > + struct static_call_mod *site_mod; > > + > > + for (site = start; site < stop; site++) { > > + key = static_call_key(site); > > + if (key == prev_key) > > + continue; > > + prev_key = key; > > + > > + list_for_each_entry(site_mod, &key->site_mods, list) { > > + if (site_mod->mod == mod) { > > + list_del(&site_mod->list); > > + kfree(site_mod); > > + break; > > + } > > + } > > + } > > I think that for safety, when a module is removed, all the static-calls > should be traversed to check that none of them calls any function in the > removed module. If that happens, perhaps it should be poisoned.
We don't do that for normal indirect calls either.. I suppose we could here, but meh. > > +} > > + > > +static int static_call_module_notify(struct notifier_block *nb, > > + unsigned long val, void *data) > > +{ > > + struct module *mod = data; > > + int ret = 0; > > + > > + cpus_read_lock(); > > + static_call_lock(); > > + > > + switch (val) { > > + case MODULE_STATE_COMING: > > + module_disable_ro(mod); > > + ret = static_call_add_module(mod); > > + module_enable_ro(mod, false); > > Doesn’t it cause some pages to be W+X ? Can it be avoided? I don't know why it does this, jump_labels doesn't seem to need this, and I'm not seeing what static_call needs differently. > > + if (ret) { > > + WARN(1, "Failed to allocate memory for static calls"); > > + static_call_del_module(mod); > > If static_call_add_module() succeeded in changing some of the calls, but not > all, I don’t think that static_call_del_module() will correctly undo > static_call_add_module(). The code transformations, I think, will remain. Hurm, jump_labels has the same problem. I wonder why kernel/module.c:prepare_coming_module() doesn't propagate the error from the notifier call. If it were to do that, I think we'll abort the module load and any modifications get lost anyway.