From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753491AbcFAL3y (ORCPT ); Wed, 1 Jun 2016 07:29:54 -0400 Received: from mail-oi0-f47.google.com ([209.85.218.47]:35261 "EHLO mail-oi0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751875AbcFAL3w (ORCPT ); Wed, 1 Jun 2016 07:29:52 -0400 Date: Wed, 1 Jun 2016 06:29:49 -0500 From: Christopher Arges To: Miroslav Benes Cc: jpoimboe@redhat.com, jeyu@redhat.com, jikos@kernel.org, pmladek@suse.com, jslaby@suse.cz, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, huawei.libin@huawei.com, minfei.huang@yahoo.com Subject: Re: [PATCH v2] livepatch: allow removal of a disabled patch Message-ID: <20160601112949.GA11285@gmail.com> References: <20160601083159.7004-1-mbenes@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160601083159.7004-1-mbenes@suse.cz> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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//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 > #include > +#include > > #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 > #include > #include > +#include > #include > #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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html