--- v3
+++ v5
@@ -25,27 +25,42 @@
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.
+Finally, we need to be very careful about possible races between
+klp_unregister_patch(), kobject_put() functions and operations
+on the related sysfs files.
+
+kobject_put(&patch->kobj) must be called without klp_mutex. Otherwise,
+it might be blocked by enabled_store() that needs the mutex as well.
+In addition, enabled_store() must check if the patch was not
+unregisted in the meantime.
+
+There is no need to do the same for other kobject_put() callsites
+at the moment. Their sysfs operations neither take the lock nor
+they access any data that might be freed in the meantime.
+
+There was an attempt to use kobjects the right way and prevent these
+races by design. But it made the patch definition more complicated
+and opened another can of worms. See
+https://lkml.kernel.org/r/1464018848-4303-1-git-send-email-pmladek@suse.com
+
+[Thanks to Petr Mladek for improving the commit message.]
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
+Reviewed-by: Petr Mladek <pmladek@suse.com>
---
- Documentation/livepatch/livepatch.txt | 29 ++++---------
+ Documentation/livepatch/livepatch.txt | 28 ++++--------
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(-)
+ 5 files changed, 72 insertions(+), 52 deletions(-)
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
-index f87e742..b0eaaf8 100644
+index 4f2aec8..ecdb181 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
-@@ -265,8 +265,15 @@ section "Livepatch life-cycle" below for more details about these
+@@ -316,8 +316,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
@@ -63,8 +78,8 @@
5. Livepatch life-cycle
=======================
-@@ -437,24 +444,6 @@ The current Livepatch implementation has several limitations:
- There is work in progress to remove this limitation.
+@@ -469,23 +476,6 @@ The current Livepatch implementation has several limitations:
+ by "notrace".
- + Livepatch modules can not be removed.
@@ -84,12 +99,11 @@
- 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 8e06fe5..1959e52 100644
+index ed90ad1..194991e 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -23,6 +23,7 @@
@@ -117,7 +131,7 @@
#define klp_for_each_object(patch, obj) \
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
-index 22c0c01..cc44f40 100644
+index 3dc3c90..6844c12 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -29,6 +29,7 @@
@@ -128,7 +142,7 @@
#include <asm/cacheflush.h>
#include "patch.h"
#include "transition.h"
-@@ -377,6 +378,18 @@ static int __klp_enable_patch(struct klp_patch *patch)
+@@ -354,6 +355,18 @@ static int __klp_enable_patch(struct klp_patch *patch)
!list_prev_entry(patch, list)->enabled)
return -EBUSY;
@@ -147,7 +161,7 @@
pr_notice("enabling patch '%s'\n", patch->mod->name);
klp_init_transition(patch, KLP_PATCHED);
-@@ -471,6 +484,15 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
+@@ -442,6 +455,15 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
mutex_lock(&klp_mutex);
@@ -163,7 +177,7 @@
if (patch->enabled == enabled) {
/* already in requested state */
ret = -EINVAL;
-@@ -528,10 +550,10 @@ static struct attribute *klp_patch_attrs[] = {
+@@ -498,10 +520,10 @@ static struct attribute *klp_patch_attrs[] = {
static void klp_kobj_release_patch(struct kobject *kobj)
{
@@ -178,7 +192,7 @@
}
static struct kobj_type klp_ktype_patch = {
-@@ -602,7 +624,6 @@ static void klp_free_patch(struct klp_patch *patch)
+@@ -572,7 +594,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);
@@ -186,7 +200,7 @@
}
static int klp_init_func(struct klp_object *obj, struct klp_func *func)
-@@ -725,11 +746,14 @@ static int klp_init_patch(struct klp_patch *patch)
+@@ -695,11 +716,14 @@ static int klp_init_patch(struct klp_patch *patch)
mutex_lock(&klp_mutex);
patch->enabled = false;
@@ -203,7 +217,7 @@
klp_for_each_object(patch, obj) {
ret = klp_init_object(patch, obj);
-@@ -745,9 +769,12 @@ static int klp_init_patch(struct klp_patch *patch)
+@@ -715,9 +739,12 @@ static int klp_init_patch(struct klp_patch *patch)
free:
klp_free_objects_limited(patch, obj);
@@ -218,7 +232,7 @@
return ret;
}
-@@ -761,23 +788,29 @@ static int klp_init_patch(struct klp_patch *patch)
+@@ -731,23 +758,29 @@ static int klp_init_patch(struct klp_patch *patch)
*/
int klp_unregister_patch(struct klp_patch *patch)
{
@@ -252,7 +266,7 @@
mutex_unlock(&klp_mutex);
return ret;
}
-@@ -790,12 +823,13 @@ EXPORT_SYMBOL_GPL(klp_unregister_patch);
+@@ -760,12 +793,13 @@ EXPORT_SYMBOL_GPL(klp_unregister_patch);
* Initializes the data structure associated with the patch and
* creates the sysfs interface.
*
@@ -268,9 +282,9 @@
if (!patch || !patch->mod)
return -EINVAL;
-@@ -816,21 +850,7 @@ int klp_register_patch(struct klp_patch *patch)
- if (!klp_have_reliable_stack() && !patch->immediate)
+@@ -788,21 +822,7 @@ int klp_register_patch(struct klp_patch *patch)
return -ENOSYS;
+ }
- /*
- * A reference is taken on the patch module to prevent it from being
@@ -292,15 +306,18 @@
EXPORT_SYMBOL_GPL(klp_register_patch);
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
-index 4494fe6..dc950d5 100644
+index 428533e..e96346e 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
-@@ -195,13 +195,21 @@ void klp_complete_transition(void)
+@@ -59,6 +59,7 @@ static void klp_complete_transition(void)
struct klp_func *func;
struct task_struct *g, *task;
unsigned int cpu;
+ bool is_immediate = false;
+ if (klp_target_state == KLP_UNPATCHED) {
+ /*
+@@ -79,9 +80,16 @@ static void klp_complete_transition(void)
if (klp_transition_patch->immediate)
goto done;
@@ -317,13 +334,13 @@
+ if (klp_target_state == KLP_UNPATCHED && !is_immediate)
+ module_put(klp_transition_patch->mod);
- read_lock(&tasklist_lock);
- for_each_process_thread(g, task) {
+ /* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
+ if (klp_target_state == KLP_PATCHED)
diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
-index bb61c65..0625f38 100644
+index 629e0dc..84795223f 100644
--- a/samples/livepatch/livepatch-sample.c
+++ b/samples/livepatch/livepatch-sample.c
-@@ -89,7 +89,6 @@ static int livepatch_init(void)
+@@ -99,7 +99,6 @@ static int livepatch_init(void)
static void livepatch_exit(void)
{