Re: [PATCH v2] livepatch: allow removal of a disabled patch
On Wed 2016-06-01 10:31:59, 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(>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.
>
> 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
> @@ -459,6 +472,15 @@ static ssize_t enabled_store(struct kobject *kobj,
> struct kobj_attribute *attr,
>
> mutex_lock(_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;
> @@ -700,11 +721,14 @@ static int klp_init_patch(struct klp_patch *patch)
> mutex_lock(_mutex);
>
> patch->enabled = false;
> + init_completion(>finish);
>
> ret = kobject_init_and_add(>kobj, _ktype_patch,
> klp_root_kobj, "%s", patch->mod->name);
> - if (ret)
> - goto unlock;
> + if (ret) {
> + mutex_unlock(_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(>kobj);
> -unlock:
> +
> mutex_unlock(_mutex);
> +
Just for record. The sysfs entry exists but patch->list is not
initialized in this error path. Therefore we could write into
/sys/.../livepatch_foo/enable
and call enabled_store(). It is safe because enabled_store() calls
klp_is_patch_registered() and it does not need patch->list for this
check. Kudos for the klp_is_patch_registered() implementation.
I write this because it is not obvious and it took me some time
to verify that we are on the safe side.
Well, I would feel more comfortable if we initialize patch->list
above.
> + kobject_put(>kobj);
> + wait_for_completion(>finish);
> +
> return ret;
> }
>
> 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());
> WARN_ON(klp_unregister_patch());
Just for record. I was curious if the following race:
CPU0CPU1
rmmod livepatch_foo
livepatch_exit()
echo 1> /sys/.../livepatch_foo/enable
__klp_enable_patch()
lock(_mutex)
...
patch->enabled = true;
...
unlock(_mutex)
klp_unregister_patch()
lock(_mutex);
if (patch->enabled)
ret = -EBUSY;
unlock(_mutex);
Fortunately, this is not possible. livepatch_exit() is called when
the module is in MODULE_STATE_GOING. It means that
Re: [PATCH v2] livepatch: allow removal of a disabled patch
On Wed 2016-06-01 10:31:59, 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(>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.
>
> 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
> @@ -459,6 +472,15 @@ static ssize_t enabled_store(struct kobject *kobj,
> struct kobj_attribute *attr,
>
> mutex_lock(_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;
> @@ -700,11 +721,14 @@ static int klp_init_patch(struct klp_patch *patch)
> mutex_lock(_mutex);
>
> patch->enabled = false;
> + init_completion(>finish);
>
> ret = kobject_init_and_add(>kobj, _ktype_patch,
> klp_root_kobj, "%s", patch->mod->name);
> - if (ret)
> - goto unlock;
> + if (ret) {
> + mutex_unlock(_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(>kobj);
> -unlock:
> +
> mutex_unlock(_mutex);
> +
Just for record. The sysfs entry exists but patch->list is not
initialized in this error path. Therefore we could write into
/sys/.../livepatch_foo/enable
and call enabled_store(). It is safe because enabled_store() calls
klp_is_patch_registered() and it does not need patch->list for this
check. Kudos for the klp_is_patch_registered() implementation.
I write this because it is not obvious and it took me some time
to verify that we are on the safe side.
Well, I would feel more comfortable if we initialize patch->list
above.
> + kobject_put(>kobj);
> + wait_for_completion(>finish);
> +
> return ret;
> }
>
> 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());
> WARN_ON(klp_unregister_patch());
Just for record. I was curious if the following race:
CPU0CPU1
rmmod livepatch_foo
livepatch_exit()
echo 1> /sys/.../livepatch_foo/enable
__klp_enable_patch()
lock(_mutex)
...
patch->enabled = true;
...
unlock(_mutex)
klp_unregister_patch()
lock(_mutex);
if (patch->enabled)
ret = -EBUSY;
unlock(_mutex);
Fortunately, this is not possible. livepatch_exit() is called when
the module is in MODULE_STATE_GOING. It means that try_module_get()
will fail
Re: [PATCH v2] livepatch: allow removal of a disabled patch
On Wed, 1 Jun 2016, Christopher Arges wrote:
> On Wed, Jun 01, 2016 at 10:31:59AM +0200, Miroslav Benes wrote:
> >
> > 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.
That is true, but I think we should tidy up after the patching process
even in this case. That is clear all func->transition, task->patch_state
and task->TIF_PATCH_PENDING flags. These all could be set if
patch->immediate is false and one of the func->immediate is true.
Regards,
Miroslav
Re: [PATCH v2] livepatch: allow removal of a disabled patch
On Wed, 1 Jun 2016, Christopher Arges wrote:
> On Wed, Jun 01, 2016 at 10:31:59AM +0200, Miroslav Benes wrote:
> >
> > 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.
That is true, but I think we should tidy up after the patching process
even in this case. That is clear all func->transition, task->patch_state
and task->TIF_PATCH_PENDING flags. These all could be set if
patch->immediate is false and one of the func->immediate is true.
Regards,
Miroslav
Re: [PATCH v2] livepatch: allow removal of a disabled patch
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(>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
Re: [PATCH v2] livepatch: allow removal of a disabled patch
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(>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 v2] livepatch: allow removal of a disabled patch
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(>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,
[PATCH v2] livepatch: allow removal of a disabled patch
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(>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
