livepatch/module: Do not patch modules that are not ready

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.

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.

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 <[email protected]>

---
 include/linux/module.h  |  5 +++++
 kernel/livepatch/core.c | 20 +++++++++++++++++++-
 kernel/module.c         |  6 +++++-
 3 files changed, 29 insertions(+), 2 deletions(-)

include/linux/module.h +5 -0

index b653d7c0a05a..7e50d87da510 100644
@@ -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;

kernel/livepatch/core.c +3 -1

index a664e485365f..dee4bbcb60e6 100644
@@ -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);

kernel/livepatch/core.c +9 -0

@@ -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))

kernel/module.c +0 -0

index d856e96a3cce..8357f15b7ed0 100644

kernel/module.c +5 -0

@@ -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)

Signature · 1 line

1.8.5.6

Hi Petr,

Good find. Some comments below.

Quoted 11 lines · Petr Mladek

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.

Quoted 2 lines

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.

Quoted 22 lines

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.

Quoted 2 lines

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?

Quoted 46 lines

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 <[email protected]>

---
 include/linux/module.h  |  5 +++++
 kernel/livepatch/core.c | 20 +++++++++++++++++++-
 kernel/module.c         |  6 +++++-
 3 files changed, 29 insertions(+), 2 deletions(-)

include/linux/module.h +5 -0

index b653d7c0a05a..7e50d87da510 100644
@@ -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;

kernel/livepatch/core.c +3 -1

index a664e485365f..dee4bbcb60e6 100644
@@ -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);

kernel/livepatch/core.c static void klp_find_object_module(struct klp_object *obj) · line 113

Quoted 10 lines

kernel/livepatch/core.c +7 -0

+	/* 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:

kernel/livepatch/core.c +4 -2

@@ -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);
 }
 

Quoted 21 lines

kernel/livepatch/core.c +9 -0

@@ -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))

kernel/module.c +0 -0

index d856e96a3cce..8357f15b7ed0 100644

kernel/module.c SYSCALL_DEFINE2(delete_module, const char __user *, name_user, · line 857

Quoted 7 lines

kernel/module.c +0 -1

@@ -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.

Quoted 15 lines

kernel/module.c +5 -0

@@ -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)

Signature · 1 line

1.8.5.6

Signature · 1 line

Josh

Quoted 19 lines · Josh Poimboeuf

Hi Petr,

Good find. Some comments below.

Quoted 13 lines · Petr Mladek

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.

Quoted 2 lines

Based on the notifier priorities, it looks like the ftrace notifier gets called last, so I think this particular case can't happen.

Ftrace handles only going modules using the module notifier.

Coming modules are handled by ftrace_module_init() that is called directly from load_module(). The notifiers for the coming modules seems to be called right after by complete_formation().

Quoted 22 lines

Quoted 22 lines

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.

Quoted 7 lines

Quoted 2 lines

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?

Good question! I guess that it was there before we added the module notifier and it was supposed to detect modules that were loaded after the patch registration.

IMHO, we could replace klp_find_object_module() with klp_is_object_loaded() in __klp_enable_patch(). It will work because it will see only modules that were there during registration or modules that were added by coming handler. It will not see modules removed by module going handler.

I saw this problem and I considered it only an optimization. We need this patch anyway because the race might get propagated via klp_register_patch() and followup klp_enable_patch().

Quoted 129 lines

Quoted 46 lines

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 <[email protected]>

---
 include/linux/module.h  |  5 +++++
 kernel/livepatch/core.c | 20 +++++++++++++++++++-
 kernel/module.c         |  6 +++++-
 3 files changed, 29 insertions(+), 2 deletions(-)

include/linux/module.h +5 -0

index b653d7c0a05a..7e50d87da510 100644
@@ -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;

kernel/livepatch/core.c +3 -1

index a664e485365f..dee4bbcb60e6 100644
@@ -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);

kernel/livepatch/core.c static void klp_find_object_module(struct klp_object *obj) · line 113

Quoted 10 lines

kernel/livepatch/core.c +7 -0

+	/* 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:

kernel/livepatch/core.c +4 -2

@@ -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);
 }
 

Quoted 21 lines

kernel/livepatch/core.c +9 -0

@@ -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))

kernel/module.c +0 -0

index d856e96a3cce..8357f15b7ed0 100644

kernel/module.c SYSCALL_DEFINE2(delete_module, const char __user *, name_user, · line 857

Quoted 7 lines

kernel/module.c +0 -1

@@ -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.

Quoted 15 lines

kernel/module.c +5 -0

@@ -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)

Signature · 1 line

1.8.5.6

Hi Petr,

Good find. Some comments below.

Quoted 7 lines · Petr Mladek

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.

Quoted 6 lines

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.

Quoted 19 lines

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.

Quoted 5 lines

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?

Quoted 52 lines

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 <[email protected]>

---
 include/linux/module.h  |  5 +++++
 kernel/livepatch/core.c | 20 +++++++++++++++++++-
 kernel/module.c         |  6 +++++-
 3 files changed, 29 insertions(+), 2 deletions(-)

include/linux/module.h +5 -0

index b653d7c0a05a..7e50d87da510 100644
@@ -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;

kernel/livepatch/core.c +9 -1

index a664e485365f..dee4bbcb60e6 100644
@@ -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;

kernel/livepatch/core.c static void klp_find_object_module(struct klp_object *obj) · line 113

Quoted 4 lines

kernel/livepatch/core.c +1 -0

+
 	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:

kernel/livepatch/core.c +1 -1

@@ -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);

kernel/livepatch/core.c +1 -0

+		obj->mod = mod;
 	mutex_unlock(&module_mutex);
 }
 

Quoted 22 lines

kernel/livepatch/core.c +9 -0

@@ -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))

kernel/module.c +0 -0

index d856e96a3cce..8357f15b7ed0 100644
@@ -852,7 +852,6 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 

kernel/module.c SYSCALL_DEFINE2(delete_module, const char __user *, name_user, · line 857

Quoted 6 lines

kernel/module.c +0 -1

 	/* 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.

Quoted 15 lines

kernel/module.c +5 -0

@@ -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)

Signature · 1 line

1.8.5.6

Signature · 1 line

Josh

Ah, I have missed the comments in the code in the first reply.

Quoted 3 lines

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?

We need this for new patches. There we do not know if the module notifier has already been called or not, see also below.

Of course, we might use more optimal storage for the two flags if requested. For example, two bits in an unsigned char.

Quoted 10 lines

For example:

(quoted diff) +1 -1

@@ -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;
 

Review static void klp_find_object_module(struct klp_object *obj) · line 102

Quoted 8 lines

(quoted diff) +2 -1

 	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)

The check of the MODULE_STATE_LIVE is not enough. We need to init modules when a new patch is registered, the module is still in MODULE_STATE_COMING and the module notify handler has already been called.

kernel/livepatch/core.c +4 -1

@@ -89,16 +89,29 @@ 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))
+	struct module *mod;
+
+	if (!klp_is_module(obj) || obj->mod)
 		return;
 
 	mutex_lock(&module_mutex);
+
 	/*
 	 * We don't need to take a reference on the module here because we have
 	 * the klp_mutex, which is also taken by the module notifier.  This

klp_find_object_module() is called from both the klp register and enable paths. Only the call from the register path is necessary because the module notifier will let us know if the patched module gets loaded or unloaded.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 kernel/livepatch/core.c | 2 --
 1 file changed, 2 deletions(-)

kernel/livepatch/core.c +0 -2

index 01ca088..a74e4e8 100644
@@ -541,8 +541,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	pr_notice("enabling patch '%s'\n", patch->mod->name);
 
 	for (obj = patch->objs; obj->funcs; obj++) {
-		klp_find_object_module(obj);
-
 		if (!klp_is_object_loaded(obj))
 			continue;
 

Signature · 1 line

2.1.0

klp_find_object_module() is called from both the klp register and enable paths. Only the call from the register path is necessary because the module notifier will let us know if the patched module gets loaded or unloaded.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 kernel/livepatch/core.c | 2 --
 1 file changed, 2 deletions(-)

kernel/livepatch/core.c +0 -2

index 01ca088..a74e4e8 100644
@@ -541,8 +541,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	pr_notice("enabling patch '%s'\n", patch->mod->name);
 
 	for (obj = patch->objs; obj->funcs; obj++) {
-		klp_find_object_module(obj);
-
 		if (!klp_is_object_loaded(obj))
 			continue;
 

It's possible for klp_register_patch() to see a module before the COMING notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races. As Pter Mladek reported:

"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. 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."

Fix these races by letting the first one who sees the module do the needed work.

Reported-by Petr Mladek <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

---
 kernel/livepatch/core.c | 53 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 4 deletions(-)

kernel/livepatch/core.c +3 -1

index a74e4e8..19a758c 100644
@@ -89,16 +89,42 @@ 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;
 
 	mutex_lock(&module_mutex);
+
 	/*
 	 * We don't need to take a reference on the module here because we have
 	 * 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);

kernel/livepatch/core.c +8 -2

+	 *   MODULE_STATE_LIVE: The common case.  The module already finished
+	 *   loading.  Just like with MODULE_STATE_COMING, we know the notifier
+	 *   didn't see the object, so we init it here.
+	 */
+	if (mod && (mod->state == MODULE_STATE_COMING ||
+		    mod->state == MODULE_STATE_LIVE))
+		obj->mod = mod;
+
 	mutex_unlock(&module_mutex);
 }
 

@@ -695,8 +721,6 @@ static void klp_free_object_loaded(struct klp_object *obj)
 {
 	struct klp_func *func;
 
-	obj->mod = NULL;
-
 
 	for (func = obj->funcs; func->old_name; func++)
 		func->old_addr = 0;

}

kernel/livepatch/core.c +1 -0

@@ -765,6 +789,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 		return -EINVAL;
 
 	obj->state = KLP_DISABLED;
+	obj->mod = NULL;
 
 	klp_find_object_module(obj);
 

kernel/livepatch/core.c +5 -0

+				 */
+				if (!obj->mod)
+					continue;
 
 
 				klp_module_notify_going(patch, obj);
+				obj->mod = NULL;
+			}
 
 			break;
 		}

Signature · 1 line

2.1.0

Quoted 11 lines · Josh Poimboeuf

It's possible for klp_register_patch() to see a module before the COMING notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races. As Pter Mladek reported:

"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.

Ah, this is not true after all. I did not properly check when MODULE_STATE_COMING was set. I though that it was before ftrace was initialized but it was not true.

Quoted 7 lines

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. 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.

Ah, the race is there but the effect is not that serious in the end. It seems that errors from module notifiers are ignored. In fact, we do not propagate the error from klp_module_notify_coming(). It means that WARN() from klp_enable_object() will be printed but the module will be loaded and patched.

I am sorry, I was confused by kGraft where kgr_module_init() was called directly from module_load(). The errors were propagated. It means that kGraft rejects module when the patch cannot be applied.

Note that the current solution is perfectly fine for the simple consistency model.

Quoted 6 lines

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 is true.

Quoted 8 lines

Fix these races by letting the first one who sees the module do the needed work.

Reported-by Petr Mladek <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

---
 kernel/livepatch/core.c | 53 +++++++++++++++++++++++++++++++++++++++++++++----

kernel/livepatch/core.c static int klp_module_notify(struct notifier_block *nb, unsigned long action, · line 997

Quoted 8 lines

kernel/livepatch/core.c +5 -0

@@ -965,10 +990,30 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
 				continue;
 
 			if (action == MODULE_STATE_COMING) {
+
+				/*
+				 * Check for a small window where the register
+				 * path already initialized the object.
+				 */

s/path/patch/

Review static int klp_module_notify(struct notifier_block *nb, unsigned long action, · line 999

Quoted 2 lines

(quoted diff) +2 -0

+				if (obj->mod)
+					continue;

This might break stacking. The recently registered patch might become the last on the stack and thus unused.

Quoted 8 lines · Josh Poimboeuf

It's possible for klp_register_patch() to see a module before the COMING notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races. As Pter Mladek reported:

"The problem is that we do not keep the klp_mutex lock all the time when the module is being added or removed.

Quoted 3 lines

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.

Ah, this is not true after all. I did not properly check when MODULE_STATE_COMING was set. I though that it was before ftrace was initialized but it was not true.

Quoted 7 lines

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. 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.

Ah, the race is there but the effect is not that serious in the end. It seems that errors from module notifiers are ignored. In fact, we do not propagate the error from klp_module_notify_coming(). It means that WARN() from klp_enable_object() will be printed but the module will be loaded and patched.

I am sorry, I was confused by kGraft where kgr_module_init() was called directly from module_load(). The errors were propagated. It means that kGraft rejects module when the patch cannot be applied.

Note that the current solution is perfectly fine for the simple consistency model.

Quoted 6 lines

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 is true.

Quoted 36 lines

Fix these races by letting the first one who sees the module do the needed work.

Reported-by Petr Mladek <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

---
 kernel/livepatch/core.c | 53 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 4 deletions(-)

kernel/livepatch/core.c +9 -1

index a74e4e8..19a758c 100644
@@ -89,16 +89,42 @@ 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;
 
 	mutex_lock(&module_mutex);
+
 	/*
 	 * We don't need to take a reference on the module here because we have
 	 * 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);
+
+	/*
+	 * Be careful here to prevent races with the notifier:
+	 *
+	 * - MODULE_STATE_COMING: This may be before or after the notifier gets

kernel/livepatch/core.c static bool klp_is_object_loaded(struct klp_object *obj) · line 113

Quoted 4 lines

kernel/livepatch/core.c +4 -0

+	 *   called.  If after, the notifier didn't see the object anyway
+	 *   because it hadn't been added to the patch list yet.  Either way,
+	 *   ftrace is already initialized, so it's safe to just go ahead and
+	 *   initialize the object here.

Well, we need to be careful. This will handle only the newly registered patch. If there are other patches against the module on the stack, it might produce wrong order at func_stack, see below.

kernel/livepatch/core.c static bool klp_is_object_loaded(struct klp_object *obj) · line 118

Quoted 5 lines

kernel/livepatch/core.c +5 -0

+	 *
+	 * - MODULE_STATE_GOING: Similar to MODULE_STATE_COMING, this may be
+	 *   before or after the notifier gets called.  If after, the notifer
+	 *   didn't see the object anyway.  Either way it's safe to just
+	 *   pretend that it's already gone and leave obj->mod at NULL.

This is true for the current simple consistency model.

Note that there will be a small race window if we allow dependency between patched functions and introduce more a complex consistency model with lazy patching. The problem is the following sequence:

CPU0 CPU1

delete_module() #SYSCALL

   try_stop_module()
     mod->state = MODULE_STATE_GOING;

   mutex_unlock(&module_mutex);

klp_register_patch() klp_enable_patch()

#save place to switch universe

b() # from module that is going
a() # from core (patched)

   mod->exit();

Note that the function b() can be called until we call mod->exit().

If we do not apply patch against b() because it is in MODULE_STATE_GOING. It will call patched a() with modified semantic and things might get wrong.

Well, the above described race is rather theoretical. It will happen only when a patched module is being removed and a module with a patch is added at the same time. Also it means that some other CPU will manage to register, enable the patch, switch the universe, and call function from the patched module during the small race window.

I am not sure if we need to handle such a type of race if the solution adds too big complexity.

Quoted 32 lines

kernel/livepatch/core.c +8 -2

+	 *   MODULE_STATE_LIVE: The common case.  The module already finished
+	 *   loading.  Just like with MODULE_STATE_COMING, we know the notifier
+	 *   didn't see the object, so we init it here.
+	 */
+	if (mod && (mod->state == MODULE_STATE_COMING ||
+		    mod->state == MODULE_STATE_LIVE))
+		obj->mod = mod;
+
 	mutex_unlock(&module_mutex);
 }
 

@@ -695,8 +721,6 @@ static void klp_free_object_loaded(struct klp_object *obj)
 {
 	struct klp_func *func;
 
-	obj->mod = NULL;
-
 
 	for (func = obj->funcs; func->old_name; func++)
 		func->old_addr = 0;

}

kernel/livepatch/core.c +1 -0

@@ -765,6 +789,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 		return -EINVAL;
 
 	obj->state = KLP_DISABLED;
+	obj->mod = NULL;
 
 	klp_find_object_module(obj);
 

@@ -965,10 +990,30 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
 				continue;
 

kernel/livepatch/core.c static int klp_module_notify(struct notifier_block *nb, unsigned long action, · line 997

Quoted 6 lines

kernel/livepatch/core.c +5 -0

 			if (action == MODULE_STATE_COMING) {
+
+				/*
+				 * Check for a small window where the register
+				 * path already initialized the object.
+				 */

s/path/patch/

kernel/livepatch/core.c static int klp_module_notify(struct notifier_block *nb, unsigned long action, · line 999

Quoted 5 lines

kernel/livepatch/core.c +2 -0

+				if (obj->mod)
+					continue;

This might break stacking. The recently registered patch might become the last on the stack and thus unused.

Quoted 4 lines

kernel/livepatch/core.c +1 -1

 				obj->mod = mod;
 				klp_module_notify_coming(patch, obj);
-			} else /* MODULE_STATE_GOING */
+			} else {

kernel/livepatch/core.c static int klp_module_notify(struct notifier_block *nb, unsigned long action, · line 1009

Quoted 6 lines

kernel/livepatch/core.c +6 -0

+				/* MODULE_STATE_GOING */
+
+				/*
+				 * Check for a small window where the register
+				 * path already saw the GOING state and thus
+				 * didn't set obj->mod.

Same typo here.

Also I would write that it did not initialize the object. It is not only about setting obj->mod.

Quoted 33 lines

CPU0 CPU1

delete_module() #SYSCALL

   try_stop_module()
     mod->state = MODULE_STATE_GOING;

   mutex_unlock(&module_mutex);

klp_register_patch() klp_enable_patch()

#save place to switch universe

b() # from module that is going
a() # from core (patched)

   mod->exit();

Note that the function b() can be called until we call mod->exit().

If we do not apply patch against b() because it is in MODULE_STATE_GOING. It will call patched a() with modified semantic and things might get wrong.

Well, the above described race is rather theoretical. It will happen only when a patched module is being removed and a module with a patch is added at the same time. Also it means that some other CPU will manage to register, enable the patch, switch the universe, and call function from the patched module during the small race window.

Quoted 2 lines

I am not sure if we need to handle such a type of race if the solution adds too big complexity.

But b() can't be called after the module is in MODULE_STATE_GOING, right? try_stop_module() makes sure it's not being used before changing its state.

Quoted 36 lines · Josh Poimboeuf

Quoted 35 lines

CPU0 CPU1

delete_module() #SYSCALL

   try_stop_module()
     mod->state = MODULE_STATE_GOING;

   mutex_unlock(&module_mutex);

klp_register_patch() klp_enable_patch()

#save place to switch universe

b() # from module that is going
a() # from core (patched)

   mod->exit();

Note that the function b() can be called until we call mod->exit().

If we do not apply patch against b() because it is in MODULE_STATE_GOING. It will call patched a() with modified semantic and things might get wrong.

Well, the above described race is rather theoretical. It will happen only when a patched module is being removed and a module with a patch is added at the same time. Also it means that some other CPU will manage to register, enable the patch, switch the universe, and call function from the patched module during the small race window.

I am not sure if we need to handle such a type of race if the solution adds too big complexity.

Quoted 3 lines

But b() can't be called after the module is in MODULE_STATE_GOING, right? try_stop_module() makes sure it's not being used before changing its state.

If b() is called from __exit() of the particular module, then you end up in exactly the situation described above, don't you?

Quoted 23 lines · Petr Mladek

Quoted 8 lines · Josh Poimboeuf

It's possible for klp_register_patch() to see a module before the COMING notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races. As Pter Mladek reported:

"The problem is that we do not keep the klp_mutex lock all the time when the module is being added or removed.

Quoted 7 lines

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. 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.

Ah, the race is there but the effect is not that serious in the end. It seems that errors from module notifiers are ignored. In fact, we do not propagate the error from klp_module_notify_coming(). It means that WARN() from klp_enable_object() will be printed but the module will be loaded and patched.

Quoted 3 lines

I am sorry, I was confused by kGraft where kgr_module_init() was called directly from module_load(). The errors were propagated. It means that kGraft rejects module when the patch cannot be applied.

True, but we should still eliminate the race. Initializing the object twice could cause some sneaky bugs, if not now then in the future.

Quoted 50 lines

Note that the current solution is perfectly fine for the simple consistency model.

Quoted 6 lines

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 is true.

Quoted 36 lines

Fix these races by letting the first one who sees the module do the needed work.

Reported-by Petr Mladek <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

---
 kernel/livepatch/core.c | 53 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 4 deletions(-)

kernel/livepatch/core.c +9 -1

index a74e4e8..19a758c 100644
@@ -89,16 +89,42 @@ 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;
 
 	mutex_lock(&module_mutex);
+
 	/*
 	 * We don't need to take a reference on the module here because we have
 	 * 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);
+
+	/*
+	 * Be careful here to prevent races with the notifier:
+	 *
+	 * - MODULE_STATE_COMING: This may be before or after the notifier gets

Quoted 8 lines

kernel/livepatch/core.c static bool klp_is_object_loaded(struct klp_object *obj) · line 113

Quoted 4 lines

kernel/livepatch/core.c +4 -0

+	 *   called.  If after, the notifier didn't see the object anyway
+	 *   because it hadn't been added to the patch list yet.  Either way,
+	 *   ftrace is already initialized, so it's safe to just go ahead and
+	 *   initialize the object here.

Well, we need to be careful. This will handle only the newly registered patch. If there are other patches against the module on the stack, it might produce wrong order at func_stack, see below.

Sorry, but I don't follow. Can you give an example?

Quoted 47 lines

kernel/livepatch/core.c static bool klp_is_object_loaded(struct klp_object *obj) · line 118

Quoted 5 lines

kernel/livepatch/core.c +5 -0

+	 *
+	 * - MODULE_STATE_GOING: Similar to MODULE_STATE_COMING, this may be
+	 *   before or after the notifier gets called.  If after, the notifer
+	 *   didn't see the object anyway.  Either way it's safe to just
+	 *   pretend that it's already gone and leave obj->mod at NULL.

This is true for the current simple consistency model.

Note that there will be a small race window if we allow dependency between patched functions and introduce more a complex consistency model with lazy patching. The problem is the following sequence:

CPU0 CPU1

delete_module() #SYSCALL

   try_stop_module()
     mod->state = MODULE_STATE_GOING;

   mutex_unlock(&module_mutex);

klp_register_patch() klp_enable_patch()

#save place to switch universe

b() # from module that is going
a() # from core (patched)

   mod->exit();

Note that the function b() can be called until we call mod->exit().

If we do not apply patch against b() because it is in MODULE_STATE_GOING. It will call patched a() with modified semantic and things might get wrong.

Well, the above described race is rather theoretical. It will happen only when a patched module is being removed and a module with a patch is added at the same time. Also it means that some other CPU will manage to register, enable the patch, switch the universe, and call function from the patched module during the small race window.

Quoted 2 lines

I am not sure if we need to handle such a type of race if the solution adds too big complexity.

But b() can't be called after the module is in MODULE_STATE_GOING, right? try_stop_module() makes sure it's not being used before changing its state.

Quoted 32 lines

Quoted 32 lines

kernel/livepatch/core.c +8 -2

+	 *   MODULE_STATE_LIVE: The common case.  The module already finished
+	 *   loading.  Just like with MODULE_STATE_COMING, we know the notifier
+	 *   didn't see the object, so we init it here.
+	 */
+	if (mod && (mod->state == MODULE_STATE_COMING ||
+		    mod->state == MODULE_STATE_LIVE))
+		obj->mod = mod;
+
 	mutex_unlock(&module_mutex);
 }
 

@@ -695,8 +721,6 @@ static void klp_free_object_loaded(struct klp_object *obj)
 {
 	struct klp_func *func;
 
-	obj->mod = NULL;
-
 
 	for (func = obj->funcs; func->old_name; func++)
 		func->old_addr = 0;

}

kernel/livepatch/core.c +1 -0

@@ -765,6 +789,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 		return -EINVAL;
 
 	obj->state = KLP_DISABLED;
+	obj->mod = NULL;
 
 	klp_find_object_module(obj);
 

@@ -965,10 +990,30 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
 				continue;
 

Quoted 7 lines

kernel/livepatch/core.c static int klp_module_notify(struct notifier_block *nb, unsigned long action, · line 997

Quoted 6 lines

kernel/livepatch/core.c +5 -0

 			if (action == MODULE_STATE_COMING) {
+
+				/*
+				 * Check for a small window where the register
+				 * path already initialized the object.
+				 */

s/path/patch/

Not a typo, I was referring to the register code path. Is there a clearer way to say that?

Quoted 8 lines

kernel/livepatch/core.c static int klp_module_notify(struct notifier_block *nb, unsigned long action, · line 999

Quoted 5 lines

kernel/livepatch/core.c +2 -0

+				if (obj->mod)
+					continue;

This might break stacking. The recently registered patch might become the last on the stack and thus unused.

Example please :-)

Quoted 4 lines

Quoted 4 lines

kernel/livepatch/core.c +1 -1

 				obj->mod = mod;
 				klp_module_notify_coming(patch, obj);
-			} else /* MODULE_STATE_GOING */
+			} else {

Quoted 8 lines

kernel/livepatch/core.c static int klp_module_notify(struct notifier_block *nb, unsigned long action, · line 1009

Quoted 6 lines

kernel/livepatch/core.c +6 -0

+				/* MODULE_STATE_GOING */
+
+				/*
+				 * Check for a small window where the register
+				 * path already saw the GOING state and thus
+				 * didn't set obj->mod.

Same typo here.

Same not actually a typo ;-)

Quoted 2 lines

Also I would write that it did not initialize the object. It is not only about setting obj->mod.

Ok.

Signature · 1 line

Josh

For example, let's have three patches (P1, P2, P3) for the functions a() and b() where a() is from vmcore and b() is from a module M. Something like:

	a()	b()

P1 a1() b1()
P2 a2() b2()
P3 a3() b3(3)

If you load the module M after all patches are registered and enabled. The ftrace ops for function a() and b() has listed the functions in this order

	ops_a->func_stack -> list(a3,a2,a1)
	ops_b->func_stack -> list(b3,b2,b1)

, so the pointer to b3() is the first and will be used.

Then you might have the following scenario. Let's start with state when patches P1 and P2 are registered and enabled but the module M is not loaded. Then ftrace ops for b() does not exist. Then we get into the following race:

CPU0 CPU1

load_module(M)

complete_formation()

  mod->state = MODULE_STATE_COMING;
  mutex_unlock(&module_mutex);

					klp_register_patch(P3);
					klp_enable_patch(P3);

					# STATE 1

  klp_module_notify(M)
    klp_module_notify_coming(P1);
    klp_module_notify_coming(P2);
    klp_module_notify_coming(P3);

# STATE 2

The ftrace ops for a() and b() then looks:

STATE1:

	ops_a->func_stack -> list(a3,a2,a1);
	ops_b->func_stack -> list(b3);

STATE2:

	ops_a->func_stack -> list(a3,a2,a1);
	ops_b->func_stack -> list(b2,b1,b3);

therefore, b2() is used for the module but a3() is used for vmcore because they were the last added.

My plan is to fix this problem by calling klp_module_init() directly in load_module() just after ftrace_module_init(). It will solve this problem because it will be called in MODULE_STATE_UNFORMED.

It will have another big advantage. It will allow to pass the error code and refuse loading modules that could not get patched. This will be needed for the more complex patches anyway. We have to prevent running module code that is inconsistent with the patched system.

I am still in doubts how to best solve the problem for going modules. Your suggested solution is fine for now. But we will need a better fix after adding the more complex consistency model.

Well, we could just get a reference on all patched modules to prevent them from being unloaded.

Quoted 2 lines · Josh Poimboeuf

Well, we could just get a reference on all patched modules to prevent them from being unloaded.

Is that really a solution for cases where you are unloading a module (it has "just" been switched to MODULE_STATE_GOING) and enable a patch which patches one of its functions directly after it got switched to MODULE_STATE_GOING?

Quoted 11 lines · Josh Poimboeuf

It's possible for klp_register_patch() to see a module before the COMING notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races. As Pter Mladek reported:

"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.

Ah, this is not true after all. I did not properly check when MODULE_STATE_COMING was set. I though that it was before ftrace was initialized but it was not true.

Quoted 7 lines

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. 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.

Ah, the race is there but the effect is not that serious in the end. It seems that errors from module notifiers are ignored. In fact, we do not propagate the error from klp_module_notify_coming(). It means that WARN() from klp_enable_object() will be printed but the module will be loaded and patched.

I am sorry, I was confused by kGraft where kgr_module_init() was called directly from module_load(). The errors were propagated. It means that kGraft rejects module when the patch cannot be applied.

Note that the current solution is perfectly fine for the simple consistency model.

Quoted 6 lines

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 is true.

Quoted 47 lines · Petr Mladek

Quoted 11 lines · Josh Poimboeuf

It's possible for klp_register_patch() to see a module before the COMING notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races. As Pter Mladek reported:

"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.

Ah, this is not true after all. I did not properly check when MODULE_STATE_COMING was set. I though that it was before ftrace was initialized but it was not true.

Quoted 7 lines

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. 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.

Ah, the race is there but the effect is not that serious in the end. It seems that errors from module notifiers are ignored. In fact, we do not propagate the error from klp_module_notify_coming(). It means that WARN() from klp_enable_object() will be printed but the module will be loaded and patched.

I am sorry, I was confused by kGraft where kgr_module_init() was called directly from module_load(). The errors were propagated. It means that kGraft rejects module when the patch cannot be applied.

Note that the current solution is perfectly fine for the simple consistency model.

Quoted 6 lines

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."

Quoted 1 line

This is true.

No, that's not true if you try_get_module() before patching. After the module state goes GOING (more correctly say, after try_release_module_ref() succeeded), all try_get_module() must fail :)
So, please make sure to get module when applying patches.

Quoted 48 lines · Masami Hiramatsu

Quoted 47 lines · Petr Mladek

Quoted 11 lines · Josh Poimboeuf

It's possible for klp_register_patch() to see a module before the COMING notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races. As Pter Mladek reported:

"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.

Ah, this is not true after all. I did not properly check when MODULE_STATE_COMING was set. I though that it was before ftrace was initialized but it was not true.

Quoted 7 lines

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. 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.

Ah, the race is there but the effect is not that serious in the end. It seems that errors from module notifiers are ignored. In fact, we do not propagate the error from klp_module_notify_coming(). It means that WARN() from klp_enable_object() will be printed but the module will be loaded and patched.

I am sorry, I was confused by kGraft where kgr_module_init() was called directly from module_load(). The errors were propagated. It means that kGraft rejects module when the patch cannot be applied.

Note that the current solution is perfectly fine for the simple consistency model.

Quoted 6 lines

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."

Quoted 6 lines

Quoted 1 line

This is true.

No, that's not true if you try_get_module() before patching. After the module state goes GOING (more correctly say, after try_release_module_ref() succeeded), all try_get_module() must fail :)
So, please make sure to get module when applying patches.

Hi Masami,

As Jikos pointed out elsewhere, try_get_module() won't solve all the GOING races.

The module can be in GOING before mod->exit() is called. If we apply a patch between GOING getting set and mod->exit(), try_module_get() will fail and the module won't be patched. But module code can still run before or during mod->exit(), so the unpatched module code might interact badly with new patched code elsewhere.

Quoted 61 lines · Josh Poimboeuf

Quoted 48 lines · Masami Hiramatsu

Quoted 47 lines · Petr Mladek

Quoted 11 lines · Josh Poimboeuf

It's possible for klp_register_patch() to see a module before the COMING notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races. As Pter Mladek reported:

"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.

Ah, this is not true after all. I did not properly check when MODULE_STATE_COMING was set. I though that it was before ftrace was initialized but it was not true.

Quoted 7 lines

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. 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.

Ah, the race is there but the effect is not that serious in the end. It seems that errors from module notifiers are ignored. In fact, we do not propagate the error from klp_module_notify_coming(). It means that WARN() from klp_enable_object() will be printed but the module will be loaded and patched.

I am sorry, I was confused by kGraft where kgr_module_init() was called directly from module_load(). The errors were propagated. It means that kGraft rejects module when the patch cannot be applied.

Note that the current solution is perfectly fine for the simple consistency model.

Quoted 6 lines

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."

Quoted 6 lines

Quoted 1 line

This is true.

No, that's not true if you try_get_module() before patching. After the module state goes GOING (more correctly say, after try_release_module_ref() succeeded), all try_get_module() must fail :)
So, please make sure to get module when applying patches.

Hi Masami,

As Jikos pointed out elsewhere, try_get_module() won't solve all the GOING races.

Quoted 5 lines

The module can be in GOING before mod->exit() is called. If we apply a patch between GOING getting set and mod->exit(), try_module_get() will fail and the module won't be patched. But module code can still run before or during mod->exit(), so the unpatched module code might interact badly with new patched code elsewhere.

Hmm, in that case, we'd better have new GONE state for the module. At least kprobe needs it.

Quoted 68 lines · Masami Hiramatsu

Quoted 66 lines · Josh Poimboeuf

Quoted 48 lines · Masami Hiramatsu

Quoted 47 lines · Petr Mladek

Quoted 11 lines · Josh Poimboeuf

It's possible for klp_register_patch() to see a module before the COMING notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races. As Pter Mladek reported:

"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.

Ah, this is not true after all. I did not properly check when MODULE_STATE_COMING was set. I though that it was before ftrace was initialized but it was not true.

Quoted 7 lines

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. 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.

Ah, the race is there but the effect is not that serious in the end. It seems that errors from module notifiers are ignored. In fact, we do not propagate the error from klp_module_notify_coming(). It means that WARN() from klp_enable_object() will be printed but the module will be loaded and patched.

I am sorry, I was confused by kGraft where kgr_module_init() was called directly from module_load(). The errors were propagated. It means that kGraft rejects module when the patch cannot be applied.

Note that the current solution is perfectly fine for the simple consistency model.

Quoted 6 lines

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."

Quoted 6 lines

Quoted 1 line

This is true.

No, that's not true if you try_get_module() before patching. After the module state goes GOING (more correctly say, after try_release_module_ref() succeeded), all try_get_module() must fail :)
So, please make sure to get module when applying patches.

Hi Masami,

As Jikos pointed out elsewhere, try_get_module() won't solve all the GOING races.

The module can be in GOING before mod->exit() is called. If we apply a patch between GOING getting set and mod->exit(), try_module_get() will fail and the module won't be patched. But module code can still run before or during mod->exit(), so the unpatched module code might interact badly with new patched code elsewhere.

Quoted 2 lines

Hmm, in that case, we'd better have new GONE state for the module. At least kprobe needs it.

What is the exact problem with kprobes, please?

Note that the notifiers for MODULE_STATE_GOING are called after mod->exit(). Therefore it is safe to kill kprobes the fast way when using the notifier.

Quoted 69 lines · Petr Mladek

Quoted 68 lines · Masami Hiramatsu

Quoted 66 lines · Josh Poimboeuf

Quoted 48 lines · Masami Hiramatsu

Quoted 47 lines · Petr Mladek

Quoted 11 lines · Josh Poimboeuf

It's possible for klp_register_patch() to see a module before the COMING notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races. As Pter Mladek reported:

"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.

Ah, this is not true after all. I did not properly check when MODULE_STATE_COMING was set. I though that it was before ftrace was initialized but it was not true.

Quoted 7 lines

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. 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.

Ah, the race is there but the effect is not that serious in the end. It seems that errors from module notifiers are ignored. In fact, we do not propagate the error from klp_module_notify_coming(). It means that WARN() from klp_enable_object() will be printed but the module will be loaded and patched.

I am sorry, I was confused by kGraft where kgr_module_init() was called directly from module_load(). The errors were propagated. It means that kGraft rejects module when the patch cannot be applied.

Note that the current solution is perfectly fine for the simple consistency model.

Quoted 6 lines

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."

Quoted 6 lines

Quoted 1 line

This is true.

No, that's not true if you try_get_module() before patching. After the module state goes GOING (more correctly say, after try_release_module_ref() succeeded), all try_get_module() must fail :)
So, please make sure to get module when applying patches.

Hi Masami,

As Jikos pointed out elsewhere, try_get_module() won't solve all the GOING races.

The module can be in GOING before mod->exit() is called. If we apply a patch between GOING getting set and mod->exit(), try_module_get() will fail and the module won't be patched. But module code can still run before or during mod->exit(), so the unpatched module code might interact badly with new patched code elsewhere.

Quoted 4 lines

Quoted 2 lines

Hmm, in that case, we'd better have new GONE state for the module. At least kprobe needs it.

What is the exact problem with kprobes, please?

Ah, sorry, I miss understood the issue.

Quoted 3 lines

Note that the notifiers for MODULE_STATE_GOING are called after mod->exit(). Therefore it is safe to kill kprobes the fast way when using the notifier.

Right.

        /* Stop the machine so refcounts can't move and disable module. */
        ret = try_stop_module(mod, flags, &forced);
        if (ret != 0)
                goto out;

        mutex_unlock(&module_mutex);
        /* Final destruction now no one is using it. */
        if (mod->exit != NULL)
                mod->exit();
        blocking_notifier_call_chain(&module_notify_list,
                                     MODULE_STATE_GOING, mod);
        async_synchronize_full();

And, kprobes also can try to put a probe between mutex_unlock() and mod->exit() on mod->exit() function.
Since kprobe tries to get module when registering, it will fail but mod->exit() is called after that fail.

So, there is also a race.

However, I'm not sure that is actual serious issue. Actually, we can suppose this module unloading context is not changing universe. thus it is expected behavior, isn't it?

Actually, we can suppose this module unloading context is not changing universe. thus it is expected behavior, isn't it?

Quoted 47 lines · Petr Mladek

Quoted 11 lines · Josh Poimboeuf

It's possible for klp_register_patch() to see a module before the COMING notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races. As Pter Mladek reported:

"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.

Ah, this is not true after all. I did not properly check when MODULE_STATE_COMING was set. I though that it was before ftrace was initialized but it was not true.

Quoted 7 lines

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. 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.

Ah, the race is there but the effect is not that serious in the end. It seems that errors from module notifiers are ignored. In fact, we do not propagate the error from klp_module_notify_coming(). It means that WARN() from klp_enable_object() will be printed but the module will be loaded and patched.

I am sorry, I was confused by kGraft where kgr_module_init() was called directly from module_load(). The errors were propagated. It means that kGraft rejects module when the patch cannot be applied.

Note that the current solution is perfectly fine for the simple consistency model.

Quoted 6 lines

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."

Quoted 1 line

This is true.

No, that's not true if you try_get_module() before patching. After the module state goes GOING (more correctly say, after try_release_module_ref() succeeded), all try_get_module() must fail :)
So, please make sure to get module when applying patches.