On Wed, Jun 01, 2016 at 10:31:59AM +0200, Miroslav Benes wrote:
> Currently we do not allow patch module to unload since there is no
> method to determine if a task is still running in the patched code.
>
> The consistency model gives us the way because when the unpatching
> finishes we know that all tasks were marked as safe to call an original
> function. Thus every new call to the function calls the original code
> and at the same time no task can be somewhere in the patched code,
> because it had to leave that code to be marked as safe.
>
> We can safely let the patch module go after that.
>
> Completion is used for synchronization between module removal and sysfs
> infrastructure in a similar way to commit 942e443127e9 ("module: Fix
> mod->mkobj.kobj potentially freed too early").
>
> Note that we still do not allow the removal for immediate model, that is
> no consistency model. The module refcount may increase in this case if
> somebody disables and enables the patch several times. This should not
> cause any harm.
>
> With this change a call to try_module_get() is moved to
> __klp_enable_patch from klp_register_patch to make module reference
> counting symmetric (module_put() is in a patch disable path) and to
> allow to take a new reference to a disabled module when being enabled.
>
> Also all kobject_put(&patch->kobj) calls are moved outside of klp_mutex
> lock protection to prevent a deadlock situation when
> klp_unregister_patch is called and sysfs directories are removed. There
> is no need to do the same for other kobject_put() callsites as we
> currently do not have their sysfs counterparts.
>
> Signed-off-by: Miroslav Benes <mbe...@suse.cz>
> ---
> Based on Josh's v2 of the consistency model.
>
> Documentation/livepatch/livepatch.txt | 29 ++++---------
> include/linux/livepatch.h | 3 ++
> kernel/livepatch/core.c | 80
> ++++++++++++++++++++++-------------
> kernel/livepatch/transition.c | 12 +++++-
> samples/livepatch/livepatch-sample.c | 1 -
> 5 files changed, 72 insertions(+), 53 deletions(-)
>
> diff --git a/Documentation/livepatch/livepatch.txt
> b/Documentation/livepatch/livepatch.txt
> index bee86d0fe854..a05124258a73 100644
> --- a/Documentation/livepatch/livepatch.txt
> +++ b/Documentation/livepatch/livepatch.txt
> @@ -272,8 +272,15 @@ section "Livepatch life-cycle" below for more details
> about these
> two operations.
>
> Module removal is only safe when there are no users of the underlying
> -functions. The immediate consistency model is not able to detect this;
> -therefore livepatch modules cannot be removed. See "Limitations" below.
> +functions. The immediate consistency model is not able to detect this. The
> +code just redirects the functions at the very beginning and it does not
> +check if the functions are in use. In other words, it knows when the
> +functions get called but it does not know when the functions return.
> +Therefore it cannot be decided when the livepatch module can be safely
> +removed. This is solved by a hybrid consistency model. When the system is
> +transitioned to a new patch state (patched/unpatched) it is guaranteed that
> +no task sleeps or runs in the old code.
> +
>
> 5. Livepatch life-cycle
> =======================
> @@ -444,24 +451,6 @@ See Documentation/ABI/testing/sysfs-kernel-livepatch for
> more details.
> There is work in progress to remove this limitation.
>
>
> - + Livepatch modules can not be removed.
> -
> - The current implementation just redirects the functions at the very
> - beginning. It does not check if the functions are in use. In other
> - words, it knows when the functions get called but it does not
> - know when the functions return. Therefore it can not decide when
> - the livepatch module can be safely removed.
> -
> - This will get most likely solved once a more complex consistency model
> - is supported. The idea is that a safe state for patching should also
> - mean a safe state for removing the patch.
> -
> - Note that the patch itself might get disabled by writing zero
> - to /sys/kernel/livepatch/<patch>/enabled. It causes that the new
> - code will not longer get called. But it does not guarantee
> - that anyone is not sleeping anywhere in the new code.
> -
> -
> + Livepatch works reliably only when the dynamic ftrace is located at
> the very beginning of the function.
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index cd2dfd95e109..a9651c6e1b52 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -23,6 +23,7 @@
>
> #include <linux/module.h>
> #include <linux/ftrace.h>
> +#include <linux/completion.h>
>
> #if IS_ENABLED(CONFIG_LIVEPATCH)
>
> @@ -114,6 +115,7 @@ struct klp_object {
> * @list: list node for global list of registered patches
> * @kobj: kobject for sysfs resources
> * @enabled: the patch is enabled (but operation may be incomplete)
> + * @finish: for waiting till it is safe to remove the patch module
> */
> struct klp_patch {
> /* external */
> @@ -125,6 +127,7 @@ struct klp_patch {
> struct list_head list;
> struct kobject kobj;
> bool enabled;
> + struct completion finish;
> };
>
> #define klp_for_each_object(patch, obj) \
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index d55701222b49..a649186fb4af 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -29,6 +29,7 @@
> #include <linux/livepatch.h>
> #include <linux/elf.h>
> #include <linux/moduleloader.h>
> +#include <linux/completion.h>
> #include <asm/cacheflush.h>
> #include "patch.h"
> #include "transition.h"
> @@ -371,6 +372,18 @@ static int __klp_enable_patch(struct klp_patch *patch)
> !list_prev_entry(patch, list)->enabled)
> return -EBUSY;
>
> + /*
> + * A reference is taken on the patch module to prevent it from being
> + * unloaded.
> + *
> + * Note: For immediate (no consistency model) patches we don't allow
> + * patch modules to unload since there is no safe/sane method to
> + * determine if a thread is still running in the patched code contained
> + * in the patch module once the ftrace registration is successful.
> + */
> + if (!try_module_get(patch->mod))
> + return -ENODEV;
> +
> pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
> add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
>
> @@ -459,6 +472,15 @@ static ssize_t enabled_store(struct kobject *kobj,
> struct kobj_attribute *attr,
>
> mutex_lock(&klp_mutex);
>
> + if (!klp_is_patch_registered(patch)) {
> + /*
> + * Module with the patch could either disappear meanwhile or is
> + * not properly initialized yet.
> + */
> + ret = -EINVAL;
> + goto err;
> + }
> +
> if (patch->enabled == val) {
> /* already in requested state */
> ret = -EINVAL;
> @@ -515,10 +537,10 @@ static struct attribute *klp_patch_attrs[] = {
>
> static void klp_kobj_release_patch(struct kobject *kobj)
> {
> - /*
> - * Once we have a consistency model we'll need to module_put() the
> - * patch module here. See klp_register_patch() for more details.
> - */
> + struct klp_patch *patch;
> +
> + patch = container_of(kobj, struct klp_patch, kobj);
> + complete(&patch->finish);
> }
>
> static struct kobj_type klp_ktype_patch = {
> @@ -589,7 +611,6 @@ static void klp_free_patch(struct klp_patch *patch)
> klp_free_objects_limited(patch, NULL);
> if (!list_empty(&patch->list))
> list_del(&patch->list);
> - kobject_put(&patch->kobj);
> }
>
> static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> @@ -700,11 +721,14 @@ static int klp_init_patch(struct klp_patch *patch)
> mutex_lock(&klp_mutex);
>
> patch->enabled = false;
> + init_completion(&patch->finish);
>
> ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
> klp_root_kobj, "%s", patch->mod->name);
> - if (ret)
> - goto unlock;
> + if (ret) {
> + mutex_unlock(&klp_mutex);
> + return ret;
> + }
>
> klp_for_each_object(patch, obj) {
> ret = klp_init_object(patch, obj);
> @@ -720,9 +744,12 @@ static int klp_init_patch(struct klp_patch *patch)
>
> free:
> klp_free_objects_limited(patch, obj);
> - kobject_put(&patch->kobj);
> -unlock:
> +
> mutex_unlock(&klp_mutex);
> +
> + kobject_put(&patch->kobj);
> + wait_for_completion(&patch->finish);
> +
> return ret;
> }
>
> @@ -736,23 +763,29 @@ static int klp_init_patch(struct klp_patch *patch)
> */
> int klp_unregister_patch(struct klp_patch *patch)
> {
> - int ret = 0;
> + int ret;
>
> mutex_lock(&klp_mutex);
>
> if (!klp_is_patch_registered(patch)) {
> ret = -EINVAL;
> - goto out;
> + goto err;
> }
>
> if (patch->enabled) {
> ret = -EBUSY;
> - goto out;
> + goto err;
> }
>
> klp_free_patch(patch);
>
> -out:
> + mutex_unlock(&klp_mutex);
> +
> + kobject_put(&patch->kobj);
> + wait_for_completion(&patch->finish);
> +
> + return 0;
> +err:
> mutex_unlock(&klp_mutex);
> return ret;
> }
> @@ -765,12 +798,13 @@ EXPORT_SYMBOL_GPL(klp_unregister_patch);
> * Initializes the data structure associated with the patch and
> * creates the sysfs interface.
> *
> + * There is no need to take the reference on the patch module here. It is
> done
> + * later when the patch is enabled.
> + *
> * Return: 0 on success, otherwise error
> */
> int klp_register_patch(struct klp_patch *patch)
> {
> - int ret;
> -
> if (!patch || !patch->mod)
> return -EINVAL;
>
> @@ -783,21 +817,7 @@ int klp_register_patch(struct klp_patch *patch)
> if (!klp_initialized())
> return -ENODEV;
>
> - /*
> - * A reference is taken on the patch module to prevent it from being
> - * unloaded. Right now, we don't allow patch modules to unload since
> - * there is currently no method to determine if a thread is still
> - * running in the patched code contained in the patch module once
> - * the ftrace registration is successful.
> - */
> - if (!try_module_get(patch->mod))
> - return -ENODEV;
> -
> - ret = klp_init_patch(patch);
> - if (ret)
> - module_put(patch->mod);
> -
> - return ret;
> + return klp_init_patch(patch);
> }
> EXPORT_SYMBOL_GPL(klp_register_patch);
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 92819bb0961b..6cc49d253195 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -197,13 +197,21 @@ void klp_complete_transition(void)
> struct klp_func *func;
> struct task_struct *g, *task;
> unsigned int cpu;
> + bool is_immediate = false;
>
> if (klp_transition_patch->immediate)
> goto done;
>
> - klp_for_each_object(klp_transition_patch, obj)
> - klp_for_each_func(obj, func)
> + klp_for_each_object(klp_transition_patch, obj) {
> + klp_for_each_func(obj, func) {
> func->transition = false;
> + if (func->immediate)
> + is_immediate = true;
Once an immediate function is found you could return from this function since
releasing a reference from the livepatch module is no longer possible.
--chris
> + }
> + }
> +
> + if (klp_target_state == KLP_UNPATCHED && !is_immediate)
> + module_put(klp_transition_patch->mod);
>
> read_lock(&tasklist_lock);
> for_each_process_thread(g, task) {
> diff --git a/samples/livepatch/livepatch-sample.c
> b/samples/livepatch/livepatch-sample.c
> index e34f871e69b1..a84676aa7c62 100644
> --- a/samples/livepatch/livepatch-sample.c
> +++ b/samples/livepatch/livepatch-sample.c
> @@ -82,7 +82,6 @@ static int livepatch_init(void)
>
> static void livepatch_exit(void)
> {
> - WARN_ON(klp_disable_patch(&patch));
> WARN_ON(klp_unregister_patch(&patch));
> }
>
> --
> 2.8.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
