On Tue, Mar 03, 2015 at 04:48:16PM +0100, Petr Mladek wrote: > On Tue 2015-03-03 08:55:17, Josh Poimboeuf wrote: > > Hi Petr, > > > > Good find. Some comments below. > > > > On Tue, Mar 03, 2015 at 12:38:29PM +0100, Petr Mladek wrote: > > > There is a notifier that handles live patches for coming and going > > > modules. > > > It takes klp_mutex lock to avoid races with coming and going patches. > > > > > > Unfortunately, there are some possible races in the current > > > implementation. > > > The problem is that we do not keep the klp_mutex lock all the time when > > > the module is being added or removed. > > > > > > First, the module is visible even before ftrace is ready. If we enable a > > > patch > > > in this time frame, adding ftrace ops will fail and the patch will get > > > rejected > > > just because bad timing. > > > > > > Second, if we are "lucky" and enable the patch for the coming module when > > > the > > > ftrace is ready but before the module notifier has been called. > > > > Based on the notifier priorities, it looks like the ftrace notifier gets > > called last, so I think this particular case can't happen. > > Ftrace handles only going modules using the module notifier. > > Coming modules are handled by ftrace_module_init() that is called directly > from load_module(). The notifiers for the coming modules seems to be > called right after by complete_formation().
Ah, ok. Makes sense. > > > The notifier > > > will try to enable the patch as well. It will detect that it is already > > > patched, > > > return error, and the module will get rejected just because bad timing. > > > The more serious problem is that it will not call the notifier for > > > going module, so that the mess will stay there and we wont be able to load > > > the module later. > > > > > > Third, similar problems are there for going module. If a patch is enabled > > > after > > > the notifier finishes but before the module is removed from the list of > > > modules, > > > the new patch will be applied to the module. The module might disappear at > > > anytime when the patch enabling is in progress, so there might be an > > > access out > > > of memory. Or the whole patch might be applied and some mess will be left, > > > so it will not be possible to load/patch the module again. > > > > > > This patch solves the problem by adding two flags into struct module. > > > They are > > > switched when the notifier is called. Note that we try to solve a race > > > with a > > > coming patch, therefore we do not know which modules will get patched and > > > we > > > need to monitor all modules. This is why I added this to the struct > > > module. > > > > > > The flags are set and checked under the klp_mutex lock. The related > > > operation > > > is finished under the same lock. Therefore they are properly serialized > > > now. > > > > > > Note that the patch solves only the situation when a new patch is > > > registered or > > > enabled. > > > > Did we have a reason for calling klp_find_object_module() in both > > register and enable? I'd think we'd only need it for the register path, > > since the notifier would catch any future loads/unloads. Or am I > > missing something? > > Good question! I guess that it was there before we added the module > notifier and it was supposed to detect modules that were loaded after > the patch registration. > > IMHO, we could replace klp_find_object_module() with > klp_is_object_loaded() in __klp_enable_patch(). It will work because > it will see only modules that were there during registration or > modules that were added by coming handler. It will not see modules > removed by module going handler. Yeah, and __klp_enable_patch() already calls klp_is_object_loaded(), so we can just remove its call to klp_find_object_module(). > I saw this problem and I considered it only an optimization. We need this > patch anyway because the race might get propagated via > klp_register_patch() and followup klp_enable_patch(). I agree, the race exists regardless. Also, you missed my two code comments which are buried below :-) Thanks, Josh > > > There are no such problems when the patch is being removed. it does > > > not matter who disable the patch first, whether the normal > > > disable_patch() or > > > the module notifier. There is nothing to do once the patch is disabled. > > > > > > Signed-off-by: Petr Mladek <[email protected]> > > > --- > > > include/linux/module.h | 5 +++++ > > > kernel/livepatch/core.c | 20 +++++++++++++++++++- > > > kernel/module.c | 6 +++++- > > > 3 files changed, 29 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/linux/module.h b/include/linux/module.h > > > index b653d7c0a05a..7e50d87da510 100644 > > > --- a/include/linux/module.h > > > +++ b/include/linux/module.h > > > @@ -344,6 +344,11 @@ struct module { > > > unsigned long *ftrace_callsites; > > > #endif > > > > > > +#ifdef CONFIG_LIVEPATCH > > > + bool klp_patched; > > > + bool klp_unpatched; > > > +#endif > > > + > > > #ifdef CONFIG_MODULE_UNLOAD > > > /* What modules depend on me? */ > > > struct list_head source_list; > > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > > > index a664e485365f..dee4bbcb60e6 100644 > > > --- a/kernel/livepatch/core.c > > > +++ b/kernel/livepatch/core.c > > > @@ -89,6 +89,8 @@ static bool klp_is_object_loaded(struct klp_object *obj) > > > /* sets obj->mod if object is not vmlinux and module is found */ > > > static void klp_find_object_module(struct klp_object *obj) > > > { > > > + struct module *mod; > > > + > > > if (!klp_is_module(obj)) > > > return; > > > > > > @@ -98,7 +100,14 @@ static void klp_find_object_module(struct klp_object > > > *obj) > > > * the klp_mutex, which is also taken by the module notifier. This > > > * prevents any module from unloading until we release the klp_mutex. > > > */ > > > - obj->mod = find_module(obj->name); > > > + mod = find_module(obj->name); > > > + /* Do not mess work of the module notifier */ > > > + if ((mod->state == MODULE_STATE_COMING && !mod->klp_patched) || > > > + (mod->state == MODULE_STATE_GOING && mod->klp_unpatched)) > > > + obj->mod = NULL; > > > + else > > > + obj->mod = mod; > > > + > > > mutex_unlock(&module_mutex); > > > } > > > > > > > Why do we need two flags for this? The notifer already > > sets/clears obj->mod, so can we rely on the value obj->mod to determine > > if the notifier already ran? > > > > For example: > > > > @@ -89,7 +89,7 @@ static bool klp_is_object_loaded(struct klp_object *obj) > > /* sets obj->mod if object is not vmlinux and module is found */ > > static void klp_find_object_module(struct klp_object *obj) > > { > > - if (!klp_is_module(obj)) > > + if (!klp_is_module(obj) || obj->mod) > > return; > > > > mutex_lock(&module_mutex); > > @@ -98,7 +98,9 @@ static void klp_find_object_module(struct klp_object *obj) > > * the klp_mutex, which is also taken by the module notifier. This > > * prevents any module from unloading until we release the klp_mutex. > > */ > > - obj->mod = find_module(obj->name); > > + mod = find_module(obj->name); > > + if (mod->state == MODULE_STATE_LIVE) > > + obj->mod = mod; > > mutex_unlock(&module_mutex); > > } > > > > > @@ -927,6 +936,15 @@ static int klp_module_notify(struct notifier_block > > > *nb, unsigned long action, > > > > > > mutex_lock(&klp_mutex); > > > > > > + /* > > > + * Each module has to know that the notifier has been called. > > > + * We never know what module will get patched by a new patch. > > > + */ > > > + if (action == MODULE_STATE_COMING) > > > + mod->klp_patched = true; > > > + else > > > + mod->klp_unpatched = true; > > > + > > > list_for_each_entry(patch, &klp_patches, list) { > > > for (obj = patch->objs; obj->funcs; obj++) { > > > if (!klp_is_module(obj) || strcmp(obj->name, mod->name)) > > > diff --git a/kernel/module.c b/kernel/module.c > > > index d856e96a3cce..8357f15b7ed0 100644 > > > --- a/kernel/module.c > > > +++ b/kernel/module.c > > > @@ -852,7 +852,6 @@ SYSCALL_DEFINE2(delete_module, const char __user *, > > > name_user, > > > > > > /* Store the name of the last unloaded module for diagnostic purposes */ > > > strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module)); > > > - > > > free_module(mod); > > > return 0; > > > out: > > > > Gratuitous whitespace change. > > > > > @@ -3271,6 +3270,11 @@ static int load_module(struct load_info *info, > > > const char __user *uargs, > > > } > > > #endif > > > > > > +#ifdef CONFIG_LIVEPATCH > > > + mod->klp_patched = false; > > > + mod->klp_unpatched = false; > > > +#endif > > > + > > > /* To avoid stressing percpu allocator, do this once we're unique. */ > > > err = percpu_modalloc(mod, info); > > > if (err) > > > -- > > > 1.8.5.6 > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
