Re: [RFC PATCH] livepatch/module: Do not patch modules that are not ready

[Josh Poimboeuf <jpoimboe@redhat.com>]

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 <pmladek@suse.cz>
> ---
>  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