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. > 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? > 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 > -- Josh -- 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/
