* [POC 00/23] livepatch: Split livepatch module per-object
@ 2020-01-17 15:03 Petr Mladek
2020-01-17 15:03 ` [POC 01/23] module: Allow to delete module also from inside kernel Petr Mladek
` (22 more replies)
0 siblings, 23 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
Hi,
first, do not get scared by the size of the patchset. There are only
few patches that are really complicated and need attention at this
stage. I just wanted to split it as much as possible to review
and discuss each change separately.
Now to the problem. There are long term complains about maintainability
of the arch-specific code that is needed to livepatch modules that
are loaded after the livepatch itself.
There was always an idea about splitting the livepatch module
per-livepatched object. One interesting approach was drafted
on the last Livepatch microconference at Linux Plubmers 2019.
I played with the idea and came up with this POC. Of course,
there are pros and cons.
On the positive note:
+ The approach seems to work.
+ The same scenarios are supported. It is even newly possible to use
the livepatch-specific relocations and reload the livepatched module.
+ The livepatch-specific relocations are still needed but
they are handled together with other relocations. As
a result, the other code modifications work out of box,
e.g. alternatives, parainstructions.
+ Some problematic code could get removed (last 4 patches):
+ module_disable_ro()
+ arch_klp_init_object_loaded()
+ copy_module_elf()
+ The amount if livepatch-specific hooks in the module loader
is about the same. They are _not_ longer arch-specific. But
they are a bit tricky, see negatives below.
On the negative side:
+ It adds dependency on userspace tool "modprobe" called
via usermodhelper. It brings several new problems:
+ How to distinguish modprobe called by user or by kernel
when resolving races and errors.
+ How to pass the real error code to the usermodhelper caller.
+ Automatic dependencies are generated and handled in
the userspace. Might create unwanted cyclic load.
It requires crazy workarounds from the kernel side,
see the patch 19.
+ There is a new bunch of races that sometimes need a tricky
solution. For example, see the patches 8, 9, 15.
+ It might be slightly more complicated to prepare and use
the livepatches. There are more modules that need to built
and are visibly to the administrators. Also it complicates
sharing some common helper functionality.
From my point of view. The new code is much less arch-dependent
and more self-contained. Therefore it should be easier to maintain
in the long term.
On the other hand, it is more tricky regarding possible races and
infinite loops. They are not always easy to solve because of
"modprobe" called via userspace and because of more switches
between klp_mutex and module_mutex guarded code. Anyway, once
this is solved, it should just work for a long time as is.
All in all, I think that this approach is worth exploring.
I am curious about your opinion.
Best Regards,
Petr
PS: The patchset applies against Linus' master (v5.5-rc6).
Petr Mladek (23):
module: Allow to delete module also from inside kernel
livepatch: Split livepatch modules per livepatched object
livepatch: Better checks of struct klp_object definition
livepatch: Prevent loading livepatch sub-module unintentionally.
livepatch: Initialize and free livepatch submodule
livepatch: Enable the livepatch submodule
livepatch: Remove obsolete functionality from klp_module_coming()
livepatch: Automatically load livepatch module when the patch module
is loaded
livepatch: Handle race when livepatches are reloaded during a module
load
livepatch: Handle modprobe exit code
livepatch: Safely detect forced transition when removing split
livepatch modules
livepatch: Automatically remove livepatch module when the object is
freed
livepatch: Remove livepatch module when the livepatched module is
unloaded
livepatch: Never block livepatch modules when the related module is
being removed
livepatch: Prevent infinite loop when loading livepatch module
livepatch: Add patch into the global list early
livepatch: Load livepatches for modules when loading the main
livepatch
module: Refactor add_unformed_module()
module/livepatch: Allow to use exported symbols from livepatch module
for "vmlinux"
module/livepatch: Relocate local variables in the module loaded when
the livepatch is being loaded
livepatch: Remove obsolete arch_klp_init_object_loaded()
livepatch/module: Remove obsolete copy_module_elf()
module: Remove obsolete module_disable_ro()
Documentation/livepatch/module-elf-format.rst | 15 +-
arch/x86/kernel/Makefile | 1 -
arch/x86/kernel/livepatch.c | 53 --
include/linux/livepatch.h | 36 +-
include/linux/module.h | 10 +-
kernel/livepatch/core.c | 743 ++++++++++++++-------
kernel/livepatch/core.h | 5 -
kernel/livepatch/transition.c | 22 +-
kernel/module.c | 279 ++++----
lib/livepatch/Makefile | 2 +
lib/livepatch/test_klp_atomic_replace.c | 18 +-
lib/livepatch/test_klp_callbacks_demo.c | 90 ++-
lib/livepatch/test_klp_callbacks_demo.h | 11 +
lib/livepatch/test_klp_callbacks_demo2.c | 62 +-
lib/livepatch/test_klp_callbacks_demo2.h | 11 +
...t_klp_callbacks_demo__test_klp_callbacks_busy.c | 50 ++
...st_klp_callbacks_demo__test_klp_callbacks_mod.c | 42 ++
lib/livepatch/test_klp_livepatch.c | 18 +-
lib/livepatch/test_klp_state.c | 53 +-
lib/livepatch/test_klp_state2.c | 53 +-
samples/livepatch/Makefile | 4 +
samples/livepatch/livepatch-callbacks-demo.c | 90 ++-
samples/livepatch/livepatch-callbacks-demo.h | 11 +
...h-callbacks-demo__livepatch-callbacks-busymod.c | 54 ++
...patch-callbacks-demo__livepatch-callbacks-mod.c | 46 ++
samples/livepatch/livepatch-sample.c | 18 +-
samples/livepatch/livepatch-shadow-fix1.c | 120 +---
.../livepatch-shadow-fix1__livepatch-shadow-mod.c | 155 +++++
samples/livepatch/livepatch-shadow-fix2.c | 92 +--
.../livepatch-shadow-fix2__livepatch-shadow-mod.c | 127 ++++
.../testing/selftests/livepatch/test-callbacks.sh | 19 +-
31 files changed, 1424 insertions(+), 886 deletions(-)
delete mode 100644 arch/x86/kernel/livepatch.c
create mode 100644 lib/livepatch/test_klp_callbacks_demo.h
create mode 100644 lib/livepatch/test_klp_callbacks_demo2.h
create mode 100644 lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_busy.c
create mode 100644 lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_mod.c
create mode 100644 samples/livepatch/livepatch-callbacks-demo.h
create mode 100644 samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-busymod.c
create mode 100644 samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-mod.c
create mode 100644 samples/livepatch/livepatch-shadow-fix1__livepatch-shadow-mod.c
create mode 100644 samples/livepatch/livepatch-shadow-fix2__livepatch-shadow-mod.c
--
2.16.4
^ permalink raw reply [flat|nested] 38+ messages in thread
* [POC 01/23] module: Allow to delete module also from inside kernel
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-21 11:11 ` Julien Thierry
2020-01-17 15:03 ` [POC 02/23] livepatch: Split livepatch modules per livepatched object Petr Mladek
` (21 subsequent siblings)
22 siblings, 1 reply; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
Livepatch subsystems will need to automatically load and delete
livepatch module when the livepatched one is being removed
or when the entire livepatch is being removed.
The code stopping the kernel module is moved to separate function
so that it can be reused.
The function always have rights to do the action. Also it does not
need to search for struct module because it is already passed as
a parameter.
On the other hand, it has to make sure that the given struct module
can't be released in parallel. It is achieved by combining module_put()
and module_delete() functionality in a single function.
This patch does not change the existing behavior of delete_module
syscall.
Signed-off-by: Petr Mladek <pmladek@suse.com>
module: Add module_put_and_delete()
---
include/linux/module.h | 5 +++
kernel/module.c | 119 +++++++++++++++++++++++++++++++------------------
2 files changed, 80 insertions(+), 44 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index bd165ba68617..f69f3fd72dd5 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -623,6 +623,7 @@ extern void __module_get(struct module *module);
extern bool try_module_get(struct module *module);
extern void module_put(struct module *module);
+extern int module_put_and_delete(struct module *mod);
#else /*!CONFIG_MODULE_UNLOAD*/
static inline bool try_module_get(struct module *module)
@@ -632,6 +633,10 @@ static inline bool try_module_get(struct module *module)
static inline void module_put(struct module *module)
{
}
+static inline int module_put_and_delete(struct module *mod)
+{
+ return 0;
+}
static inline void __module_get(struct module *module)
{
}
diff --git a/kernel/module.c b/kernel/module.c
index b56f3224b161..b3f11524f8f9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -964,62 +964,36 @@ EXPORT_SYMBOL(module_refcount);
/* This exists whether we can unload or not */
static void free_module(struct module *mod);
-SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
- unsigned int, flags)
+int stop_module(struct module *mod, int flags)
{
- struct module *mod;
- char name[MODULE_NAME_LEN];
- int ret, forced = 0;
-
- if (!capable(CAP_SYS_MODULE) || modules_disabled)
- return -EPERM;
-
- if (strncpy_from_user(name, name_user, MODULE_NAME_LEN-1) < 0)
- return -EFAULT;
- name[MODULE_NAME_LEN-1] = '\0';
+ int forced = 0;
- audit_log_kern_module(name);
-
- if (mutex_lock_interruptible(&module_mutex) != 0)
- return -EINTR;
-
- mod = find_module(name);
- if (!mod) {
- ret = -ENOENT;
- goto out;
- }
-
- if (!list_empty(&mod->source_list)) {
- /* Other modules depend on us: get rid of them first. */
- ret = -EWOULDBLOCK;
- goto out;
- }
+ /* Other modules depend on us: get rid of them first. */
+ if (!list_empty(&mod->source_list))
+ return -EWOULDBLOCK;
/* Doing init or already dying? */
if (mod->state != MODULE_STATE_LIVE) {
/* FIXME: if (force), slam module count damn the torpedoes */
pr_debug("%s already dying\n", mod->name);
- ret = -EBUSY;
- goto out;
+ return -EBUSY;
}
/* If it has an init func, it must have an exit func to unload */
if (mod->init && !mod->exit) {
forced = try_force_unload(flags);
- if (!forced) {
- /* This module can't be removed */
- ret = -EBUSY;
- goto out;
- }
+ /* This module can't be removed */
+ if (!forced)
+ return -EBUSY;
}
/* Stop the machine so refcounts can't move and disable module. */
- ret = try_stop_module(mod, flags, &forced);
- if (ret != 0)
- goto out;
+ return try_stop_module(mod, flags, &forced);
+}
- mutex_unlock(&module_mutex);
- /* Final destruction now no one is using it. */
+/* Final destruction now no one is using it. */
+static void destruct_module(struct module *mod)
+{
if (mod->exit != NULL)
mod->exit();
blocking_notifier_call_chain(&module_notify_list,
@@ -1033,8 +1007,44 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
free_module(mod);
+
/* someone could wait for the module in add_unformed_module() */
wake_up_all(&module_wq);
+}
+
+SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
+ unsigned int, flags)
+{
+ struct module *mod;
+ char name[MODULE_NAME_LEN];
+ int ret;
+
+ if (!capable(CAP_SYS_MODULE) || modules_disabled)
+ return -EPERM;
+
+ if (strncpy_from_user(name, name_user, MODULE_NAME_LEN-1) < 0)
+ return -EFAULT;
+ name[MODULE_NAME_LEN-1] = '\0';
+
+ audit_log_kern_module(name);
+
+ if (mutex_lock_interruptible(&module_mutex) != 0)
+ return -EINTR;
+
+ mod = find_module(name);
+ if (!mod) {
+ ret = -ENOENT;
+ goto out;
+ }
+
+ ret = stop_module(mod, flags);
+ if (ret)
+ goto out;
+
+ mutex_unlock(&module_mutex);
+
+/* Final destruction now no one is using it. */
+ destruct_module(mod);
return 0;
out:
mutex_unlock(&module_mutex);
@@ -1138,20 +1148,41 @@ bool try_module_get(struct module *module)
}
EXPORT_SYMBOL(try_module_get);
-void module_put(struct module *module)
+/* Must be called under module_mutex or with preemtion disabled */
+static void __module_put(struct module* module)
{
int ret;
+ ret = atomic_dec_if_positive(&module->refcnt);
+ WARN_ON(ret < 0); /* Failed to put refcount */
+ trace_module_put(module, _RET_IP_);
+}
+
+void module_put(struct module *module)
+{
if (module) {
preempt_disable();
- ret = atomic_dec_if_positive(&module->refcnt);
- WARN_ON(ret < 0); /* Failed to put refcount */
- trace_module_put(module, _RET_IP_);
+ __module_put(module);
preempt_enable();
}
}
EXPORT_SYMBOL(module_put);
+int module_put_and_delete(struct module *mod)
+{
+ int ret;
+ mutex_lock(&module_mutex);
+ __module_put(mod);
+ ret = stop_module(mod, 0);
+ mutex_unlock(&module_mutex);
+
+ if (ret)
+ return ret;
+
+ destruct_module(mod);
+ return 0;
+}
+
#else /* !CONFIG_MODULE_UNLOAD */
static inline void print_unload_info(struct seq_file *m, struct module *mod)
{
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 02/23] livepatch: Split livepatch modules per livepatched object
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
2020-01-17 15:03 ` [POC 01/23] module: Allow to delete module also from inside kernel Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-21 11:11 ` Julien Thierry
2020-01-17 15:03 ` [POC 03/23] livepatch: Better checks of struct klp_object definition Petr Mladek
` (20 subsequent siblings)
22 siblings, 1 reply; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
One livepatch module allows to fix vmlinux and any number of modules
while providing some guarantees defined by the consistency model.
The patched modules can be loaded at any time: before, during,
or after the livepatch module gets loaded. They can even get
removed and loaded again. This variety of scenarios bring some
troubles. For example, some livepatch symbols could be relocated
only after the related module gets loaded. These changes need
to get cleared when the module gets unloaded so that it can
get eventually loaded again.
As a result some functionality needs to be duplicated by
the livepatching code. Some elf sections need to be preserved
even when they normally can be removed during the module load.
Architecture specific code is involved which makes harder
adding support for new architectures and the maintainace.
The solution is to split the livepatch module per livepatched
object (vmlinux or module). Then both livepatch module and
the livepatched modules could get loaded and removed at the
same time.
This require many changes in the livepatch subsystem, module
loader, sample livepatches and livepatches needed for selftests.
The bad news is that bisection will not work by definition.
The good news is that it allows to do the changes in smaller
steps.
The first step allows to split the existing sample and testing
modules so that they can be later user. It is done by
the following changes:
1. struct klp_patch:
+ Add "patch_name" and "obj_names" to match all the related
livepatch modules.
+ Replace "objs" array with a pointer to a single struct object.
+ move "mod" to struct object.
2. struct klp_object:
+ Add "patch_name" to match all the related livepatch modules.
+ "mod" points to the livepatch module instead of the livepatched
one. The pointer to the livepatched module was used only to
detect whether it was loaded. It will be always loaded
with related livepatch module now.
3. klp_find_object_module() and klp_is_object_loaded() are no longer
needed. Livepatch module is loaded only when the related livepatched
module is loaded.
4. Add klp_add_object() function that will need to initialize
struct object, link it into the related struct klp_patch,
and patch the functions. It will get implemented later.
The livepatches for modules are put into separate source files
that define only struct klp_object() and call the new klp_add_object()
in the init() callback. The name of the module follows the pattern:
<patch_name>__<object_name>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
arch/x86/kernel/livepatch.c | 5 +-
include/linux/livepatch.h | 20 +--
kernel/livepatch/core.c | 139 +++++++-----------
kernel/livepatch/core.h | 5 -
kernel/livepatch/transition.c | 14 +-
lib/livepatch/Makefile | 2 +
lib/livepatch/test_klp_atomic_replace.c | 18 ++-
lib/livepatch/test_klp_callbacks_demo.c | 90 ++++++------
lib/livepatch/test_klp_callbacks_demo.h | 11 ++
lib/livepatch/test_klp_callbacks_demo2.c | 62 ++++++---
lib/livepatch/test_klp_callbacks_demo2.h | 11 ++
...t_klp_callbacks_demo__test_klp_callbacks_busy.c | 50 +++++++
...st_klp_callbacks_demo__test_klp_callbacks_mod.c | 42 ++++++
lib/livepatch/test_klp_livepatch.c | 18 ++-
lib/livepatch/test_klp_state.c | 53 ++++---
lib/livepatch/test_klp_state2.c | 53 ++++---
samples/livepatch/Makefile | 4 +
samples/livepatch/livepatch-callbacks-demo.c | 90 ++++++------
samples/livepatch/livepatch-callbacks-demo.h | 11 ++
...h-callbacks-demo__livepatch-callbacks-busymod.c | 54 +++++++
...patch-callbacks-demo__livepatch-callbacks-mod.c | 46 ++++++
samples/livepatch/livepatch-sample.c | 18 ++-
samples/livepatch/livepatch-shadow-fix1.c | 120 ++--------------
.../livepatch-shadow-fix1__livepatch-shadow-mod.c | 155 +++++++++++++++++++++
samples/livepatch/livepatch-shadow-fix2.c | 92 ++----------
.../livepatch-shadow-fix2__livepatch-shadow-mod.c | 127 +++++++++++++++++
.../testing/selftests/livepatch/test-callbacks.sh | 16 +--
27 files changed, 841 insertions(+), 485 deletions(-)
create mode 100644 lib/livepatch/test_klp_callbacks_demo.h
create mode 100644 lib/livepatch/test_klp_callbacks_demo2.h
create mode 100644 lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_busy.c
create mode 100644 lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_mod.c
create mode 100644 samples/livepatch/livepatch-callbacks-demo.h
create mode 100644 samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-busymod.c
create mode 100644 samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-mod.c
create mode 100644 samples/livepatch/livepatch-shadow-fix1__livepatch-shadow-mod.c
create mode 100644 samples/livepatch/livepatch-shadow-fix2__livepatch-shadow-mod.c
diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
index 6a68e41206e7..728b44eaa168 100644
--- a/arch/x86/kernel/livepatch.c
+++ b/arch/x86/kernel/livepatch.c
@@ -9,8 +9,7 @@
#include <asm/text-patching.h>
/* Apply per-object alternatives. Based on x86 module_finalize() */
-void arch_klp_init_object_loaded(struct klp_patch *patch,
- struct klp_object *obj)
+void arch_klp_init_object_loaded(struct klp_object *obj)
{
int cnt;
struct klp_modinfo *info;
@@ -20,7 +19,7 @@ void arch_klp_init_object_loaded(struct klp_patch *patch,
char sec_objname[MODULE_NAME_LEN];
char secname[KSYM_NAME_LEN];
- info = patch->mod->klp_info;
+ info = obj->mod->klp_info;
objname = obj->name ? obj->name : "vmlinux";
/* See livepatch core code for BUILD_BUG_ON() explanation */
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index e894e74905f3..a4567c17a9f2 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -105,19 +105,21 @@ struct klp_callbacks {
/**
* struct klp_object - kernel object structure for live patching
* @name: module name (or NULL for vmlinux)
+ * @patch_name: module name for vmlinux
+ * @mod: reference to the live patch module for this object
* @funcs: function entries for functions to be patched in the object
* @callbacks: functions to be executed pre/post (un)patching
* @kobj: kobject for sysfs resources
* @func_list: dynamic list of the function entries
* @node: list node for klp_patch obj_list
- * @mod: kernel module associated with the patched object
- * (NULL for vmlinux)
* @dynamic: temporary object for nop functions; dynamically allocated
* @patched: the object's funcs have been added to the klp_ops list
*/
struct klp_object {
/* external */
const char *name;
+ const char *patch_name;
+ struct module *mod;
struct klp_func *funcs;
struct klp_callbacks callbacks;
@@ -125,7 +127,6 @@ struct klp_object {
struct kobject kobj;
struct list_head func_list;
struct list_head node;
- struct module *mod;
bool dynamic;
bool patched;
};
@@ -144,8 +145,9 @@ struct klp_state {
/**
* struct klp_patch - patch structure for live patching
- * @mod: reference to the live patch module
- * @objs: object entries for kernel objects to be patched
+ * @patch_name: livepatch name; same for related livepatch against other objects
+ * @objs: object entry for vmlinux object
+ * @obj_names: names of modules synchronously livepatched with this patch
* @states: system states that can get modified
* @replace: replace all actively used patches
* @list: list node for global list of actively used patches
@@ -158,9 +160,9 @@ struct klp_state {
*/
struct klp_patch {
/* external */
- struct module *mod;
- struct klp_object *objs;
struct klp_state *states;
+ struct klp_object *obj;
+ char **obj_names;
bool replace;
/* internal */
@@ -194,9 +196,9 @@ struct klp_patch {
list_for_each_entry(func, &obj->func_list, node)
int klp_enable_patch(struct klp_patch *);
+int klp_add_object(struct klp_object *);
-void arch_klp_init_object_loaded(struct klp_patch *patch,
- struct klp_object *obj);
+void arch_klp_init_object_loaded(struct klp_object *obj);
/* Called from the module loader during module coming/going states */
int klp_module_coming(struct module *mod);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index c3512e7e0801..bb62c5407b75 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -49,34 +49,6 @@ static bool klp_is_module(struct klp_object *obj)
return obj->name;
}
-/* 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 do not want to block removal of patched modules and therefore
- * we do not take a reference here. The patches are removed by
- * klp_module_going() instead.
- */
- mod = find_module(obj->name);
- /*
- * Do not mess work of klp_module_coming() and klp_module_going().
- * Note that the patch might still be needed before klp_module_going()
- * is called. Module functions can be called even in the GOING state
- * until mod->exit() finishes. This is especially important for
- * patches that modify semantic of the functions.
- */
- if (mod && mod->klp_alive)
- obj->mod = mod;
-
- mutex_unlock(&module_mutex);
-}
-
static bool klp_initialized(void)
{
return !!klp_root_kobj;
@@ -246,18 +218,16 @@ static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
return 0;
}
-static int klp_write_object_relocations(struct module *pmod,
- struct klp_object *obj)
+static int klp_write_object_relocations(struct klp_object *obj)
{
int i, cnt, ret = 0;
const char *objname, *secname;
char sec_objname[MODULE_NAME_LEN];
+ struct module *pmod;
Elf_Shdr *sec;
- if (WARN_ON(!klp_is_object_loaded(obj)))
- return -EINVAL;
-
objname = klp_is_module(obj) ? obj->name : "vmlinux";
+ pmod = obj->mod;
/* For each klp relocation section */
for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
@@ -419,8 +389,8 @@ static void klp_free_object_dynamic(struct klp_object *obj)
static void klp_init_func_early(struct klp_object *obj,
struct klp_func *func);
-static void klp_init_object_early(struct klp_patch *patch,
- struct klp_object *obj);
+static int klp_init_object_early(struct klp_patch *patch,
+ struct klp_object *obj);
static struct klp_object *klp_alloc_object_dynamic(const char *name,
struct klp_patch *patch)
@@ -662,7 +632,7 @@ static void klp_free_patch_finish(struct klp_patch *patch)
/* Put the module after the last access to struct klp_patch. */
if (!patch->forced)
- module_put(patch->mod);
+ module_put(patch->obj->mod);
}
/*
@@ -725,30 +695,28 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
}
/* Arches may override this to finish any remaining arch-specific tasks */
-void __weak arch_klp_init_object_loaded(struct klp_patch *patch,
- struct klp_object *obj)
+void __weak arch_klp_init_object_loaded(struct klp_object *obj)
{
}
/* parts of the initialization that is done only when the object is loaded */
-static int klp_init_object_loaded(struct klp_patch *patch,
- struct klp_object *obj)
+static int klp_init_object_loaded(struct klp_object *obj)
{
struct klp_func *func;
int ret;
mutex_lock(&text_mutex);
- module_disable_ro(patch->mod);
- ret = klp_write_object_relocations(patch->mod, obj);
+ module_disable_ro(obj->mod);
+ ret = klp_write_object_relocations(obj);
if (ret) {
- module_enable_ro(patch->mod, true);
+ module_enable_ro(obj->mod, true);
mutex_unlock(&text_mutex);
return ret;
}
- arch_klp_init_object_loaded(patch, obj);
- module_enable_ro(patch->mod, true);
+ arch_klp_init_object_loaded(obj);
+ module_enable_ro(obj->mod, true);
mutex_unlock(&text_mutex);
@@ -792,11 +760,8 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
return -EINVAL;
obj->patched = false;
- obj->mod = NULL;
- klp_find_object_module(obj);
-
- name = klp_is_module(obj) ? obj->name : "vmlinux";
+ name = obj->name ? obj->name : "vmlinux";
ret = kobject_add(&obj->kobj, &patch->kobj, "%s", name);
if (ret)
return ret;
@@ -807,8 +772,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
return ret;
}
- if (klp_is_object_loaded(obj))
- ret = klp_init_object_loaded(patch, obj);
+ ret = klp_init_object_loaded(obj);
return ret;
}
@@ -820,20 +784,34 @@ static void klp_init_func_early(struct klp_object *obj,
list_add_tail(&func->node, &obj->func_list);
}
-static void klp_init_object_early(struct klp_patch *patch,
+static int klp_init_object_early(struct klp_patch *patch,
struct klp_object *obj)
{
+ struct klp_func *func;
+
+ if (!obj->funcs)
+ return -EINVAL;
+
INIT_LIST_HEAD(&obj->func_list);
kobject_init(&obj->kobj, &klp_ktype_object);
list_add_tail(&obj->node, &patch->obj_list);
+
+ klp_for_each_func_static(obj, func) {
+ klp_init_func_early(obj, func);
+ }
+
+ if (obj->dynamic || try_module_get(obj->mod))
+ return 0;
+
+ return -ENODEV;
}
static int klp_init_patch_early(struct klp_patch *patch)
{
- struct klp_object *obj;
- struct klp_func *func;
+ struct klp_object *obj = patch->obj;
- if (!patch->objs)
+ /* Main patch module is always for vmlinux */
+ if (obj->name)
return -EINVAL;
INIT_LIST_HEAD(&patch->list);
@@ -844,21 +822,7 @@ static int klp_init_patch_early(struct klp_patch *patch)
INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
init_completion(&patch->finish);
- klp_for_each_object_static(patch, obj) {
- if (!obj->funcs)
- return -EINVAL;
-
- klp_init_object_early(patch, obj);
-
- klp_for_each_func_static(obj, func) {
- klp_init_func_early(obj, func);
- }
- }
-
- if (!try_module_get(patch->mod))
- return -ENODEV;
-
- return 0;
+ return klp_init_object_early(patch, obj);
}
static int klp_init_patch(struct klp_patch *patch)
@@ -866,7 +830,7 @@ static int klp_init_patch(struct klp_patch *patch)
struct klp_object *obj;
int ret;
- ret = kobject_add(&patch->kobj, klp_root_kobj, "%s", patch->mod->name);
+ ret = kobject_add(&patch->kobj, klp_root_kobj, "%s", patch->obj->mod->name);
if (ret)
return ret;
@@ -887,6 +851,12 @@ static int klp_init_patch(struct klp_patch *patch)
return 0;
}
+int klp_add_object(struct klp_object *obj)
+{
+ return 0;
+}
+EXPORT_SYMBOL_GPL(klp_add_object);
+
static int __klp_disable_patch(struct klp_patch *patch)
{
struct klp_object *obj;
@@ -930,7 +900,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
if (WARN_ON(patch->enabled))
return -EINVAL;
- pr_notice("enabling patch '%s'\n", patch->mod->name);
+ pr_notice("enabling patch '%s'\n", patch->obj->patch_name);
klp_init_transition(patch, KLP_PATCHED);
@@ -944,9 +914,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
smp_wmb();
klp_for_each_object(patch, obj) {
- if (!klp_is_object_loaded(obj))
- continue;
-
ret = klp_pre_patch_callback(obj);
if (ret) {
pr_warn("pre-patch callback failed for object '%s'\n",
@@ -968,7 +935,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
return 0;
err:
- pr_warn("failed to enable patch '%s'\n", patch->mod->name);
+ pr_warn("failed to enable patch '%s'\n", patch->obj->patch_name);
klp_cancel_transition();
return ret;
@@ -991,12 +958,12 @@ int klp_enable_patch(struct klp_patch *patch)
{
int ret;
- if (!patch || !patch->mod)
+ if (!patch || !patch->obj || !patch->obj->mod)
return -EINVAL;
- if (!is_livepatch_module(patch->mod)) {
+ if (!is_livepatch_module(patch->obj->mod)) {
pr_err("module %s is not marked as a livepatch module\n",
- patch->mod->name);
+ patch->obj->patch_name);
return -EINVAL;
}
@@ -1012,7 +979,7 @@ int klp_enable_patch(struct klp_patch *patch)
if (!klp_is_patch_compatible(patch)) {
pr_err("Livepatch patch (%s) is not compatible with the already installed livepatches.\n",
- patch->mod->name);
+ patch->obj->mod->name);
mutex_unlock(&klp_mutex);
return -EINVAL;
}
@@ -1119,7 +1086,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
klp_pre_unpatch_callback(obj);
pr_notice("reverting patch '%s' on unloading module '%s'\n",
- patch->mod->name, obj->mod->name);
+ patch->obj->patch_name, obj->name);
klp_unpatch_object(obj);
klp_post_unpatch_callback(obj);
@@ -1154,15 +1121,15 @@ int klp_module_coming(struct module *mod)
obj->mod = mod;
- ret = klp_init_object_loaded(patch, obj);
+ ret = klp_init_object_loaded(obj);
if (ret) {
pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
- patch->mod->name, obj->mod->name, ret);
+ patch->obj->patch_name, obj->name, ret);
goto err;
}
pr_notice("applying patch '%s' to loading module '%s'\n",
- patch->mod->name, obj->mod->name);
+ patch->obj->patch_name, obj->name);
ret = klp_pre_patch_callback(obj);
if (ret) {
@@ -1174,7 +1141,7 @@ int klp_module_coming(struct module *mod)
ret = klp_patch_object(obj);
if (ret) {
pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
- patch->mod->name, obj->mod->name, ret);
+ patch->obj->patch_name, obj->name, ret);
klp_post_unpatch_callback(obj);
goto err;
@@ -1197,7 +1164,7 @@ int klp_module_coming(struct module *mod)
* error to the module loader.
*/
pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
- patch->mod->name, obj->mod->name, obj->mod->name);
+ patch->obj->patch_name, obj->name, obj->name);
mod->klp_alive = false;
obj->mod = NULL;
klp_cleanup_module_patches_limited(mod, patch);
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index 38209c7361b6..01980cc0509b 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -18,11 +18,6 @@ void klp_free_replaced_patches_async(struct klp_patch *new_patch);
void klp_unpatch_replaced_patches(struct klp_patch *new_patch);
void klp_discard_nops(struct klp_patch *new_patch);
-static inline bool klp_is_object_loaded(struct klp_object *obj)
-{
- return !obj->name || obj->mod;
-}
-
static inline int klp_pre_patch_callback(struct klp_object *obj)
{
int ret = 0;
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..78e3280560cd 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -74,7 +74,7 @@ static void klp_complete_transition(void)
unsigned int cpu;
pr_debug("'%s': completing %s transition\n",
- klp_transition_patch->mod->name,
+ klp_transition_patch->obj->patch_name,
klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
@@ -120,15 +120,13 @@ static void klp_complete_transition(void)
}
klp_for_each_object(klp_transition_patch, obj) {
- if (!klp_is_object_loaded(obj))
- continue;
if (klp_target_state == KLP_PATCHED)
klp_post_patch_callback(obj);
else if (klp_target_state == KLP_UNPATCHED)
klp_post_unpatch_callback(obj);
}
- pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
+ pr_notice("'%s': %s complete\n", klp_transition_patch->obj->patch_name,
klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
klp_target_state = KLP_UNDEFINED;
@@ -147,7 +145,7 @@ void klp_cancel_transition(void)
return;
pr_debug("'%s': canceling patching transition, going to unpatch\n",
- klp_transition_patch->mod->name);
+ klp_transition_patch->obj->patch_name);
klp_target_state = KLP_UNPATCHED;
klp_complete_transition();
@@ -468,7 +466,7 @@ void klp_start_transition(void)
WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
pr_notice("'%s': starting %s transition\n",
- klp_transition_patch->mod->name,
+ klp_transition_patch->obj->patch_name,
klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
/*
@@ -519,7 +517,7 @@ void klp_init_transition(struct klp_patch *patch, int state)
*/
klp_target_state = state;
- pr_debug("'%s': initializing %s transition\n", patch->mod->name,
+ pr_debug("'%s': initializing %s transition\n", patch->obj->patch_name,
klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
/*
@@ -581,7 +579,7 @@ void klp_reverse_transition(void)
struct task_struct *g, *task;
pr_debug("'%s': reversing transition from %s\n",
- klp_transition_patch->mod->name,
+ klp_transition_patch->obj->patch_name,
klp_target_state == KLP_PATCHED ? "patching to unpatching" :
"unpatching to patching");
diff --git a/lib/livepatch/Makefile b/lib/livepatch/Makefile
index 295b94bff370..812da987da42 100644
--- a/lib/livepatch/Makefile
+++ b/lib/livepatch/Makefile
@@ -4,6 +4,8 @@
obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
test_klp_callbacks_demo.o \
+ test_klp_callbacks_demo__test_klp_callbacks_busy.o \
+ test_klp_callbacks_demo__test_klp_callbacks_mod.o \
test_klp_callbacks_demo2.o \
test_klp_callbacks_busy.o \
test_klp_callbacks_mod.o \
diff --git a/lib/livepatch/test_klp_atomic_replace.c b/lib/livepatch/test_klp_atomic_replace.c
index 5af7093ca00c..bd42f9af9843 100644
--- a/lib/livepatch/test_klp_atomic_replace.c
+++ b/lib/livepatch/test_klp_atomic_replace.c
@@ -26,16 +26,20 @@ static struct klp_func funcs[] = {
}, {}
};
-static struct klp_object objs[] = {
- {
- /* name being NULL means vmlinux */
- .funcs = funcs,
- }, {}
+static struct klp_object obj = {
+ .patch_name = THIS_MODULE->name,
+ .name = NULL, /* vmlinux */
+ .mod = THIS_MODULE,
+ .funcs = funcs,
+};
+
+static char *obj_names[] = {
+ NULL
};
static struct klp_patch patch = {
- .mod = THIS_MODULE,
- .objs = objs,
+ .obj = &obj,
+ .obj_names = obj_names,
/* set .replace in the init function below for demo purposes */
};
diff --git a/lib/livepatch/test_klp_callbacks_demo.c b/lib/livepatch/test_klp_callbacks_demo.c
index 3fd8fe1cd1cc..0e51cbd4b61c 100644
--- a/lib/livepatch/test_klp_callbacks_demo.c
+++ b/lib/livepatch/test_klp_callbacks_demo.c
@@ -6,6 +6,7 @@
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/livepatch.h>
+#include "test_klp_callbacks_demo.h"
static int pre_patch_ret;
module_param(pre_patch_ret, int, 0644);
@@ -20,88 +21,79 @@ static const char *const module_state[] = {
static void callback_info(const char *callback, struct klp_object *obj)
{
- if (obj->mod)
- pr_info("%s: %s -> %s\n", callback, obj->mod->name,
- module_state[obj->mod->state]);
- else
+ struct module *mod;
+
+ if (!obj->name) {
pr_info("%s: vmlinux\n", callback);
+ return;
+ }
+
+ mutex_lock(&module_mutex);
+ mod = find_module(obj->name);
+ if (mod) {
+ pr_info("%s: %s -> %s\n", callback, obj->name,
+ module_state[mod->state]);
+ } else {
+ pr_err("%s: Unable to find module: %s", callback, obj->name);
+ }
+ mutex_unlock(&module_mutex);
}
/* Executed on object patching (ie, patch enablement) */
-static int pre_patch_callback(struct klp_object *obj)
+int pre_patch_callback(struct klp_object *obj)
{
callback_info(__func__, obj);
return pre_patch_ret;
}
+EXPORT_SYMBOL(pre_patch_callback);
/* Executed on object unpatching (ie, patch disablement) */
-static void post_patch_callback(struct klp_object *obj)
+void post_patch_callback(struct klp_object *obj)
{
callback_info(__func__, obj);
}
+EXPORT_SYMBOL(post_patch_callback);
/* Executed on object unpatching (ie, patch disablement) */
-static void pre_unpatch_callback(struct klp_object *obj)
+void pre_unpatch_callback(struct klp_object *obj)
{
callback_info(__func__, obj);
}
+EXPORT_SYMBOL(pre_unpatch_callback);
/* Executed on object unpatching (ie, patch disablement) */
-static void post_unpatch_callback(struct klp_object *obj)
+void post_unpatch_callback(struct klp_object *obj)
{
callback_info(__func__, obj);
}
-
-static void patched_work_func(struct work_struct *work)
-{
- pr_info("%s\n", __func__);
-}
+EXPORT_SYMBOL(post_unpatch_callback);
static struct klp_func no_funcs[] = {
{}
};
-static struct klp_func busymod_funcs[] = {
- {
- .old_name = "busymod_work_func",
- .new_func = patched_work_func,
- }, {}
+static struct klp_object obj = {
+ .patch_name = LIVEPATCH_NAME,
+ .name = NULL, /* vmlinux */
+ .mod = THIS_MODULE,
+ .funcs = no_funcs,
+ .callbacks = {
+ .pre_patch = pre_patch_callback,
+ .post_patch = post_patch_callback,
+ .pre_unpatch = pre_unpatch_callback,
+ .post_unpatch = post_unpatch_callback,
+ },
};
-static struct klp_object objs[] = {
- {
- .name = NULL, /* vmlinux */
- .funcs = no_funcs,
- .callbacks = {
- .pre_patch = pre_patch_callback,
- .post_patch = post_patch_callback,
- .pre_unpatch = pre_unpatch_callback,
- .post_unpatch = post_unpatch_callback,
- },
- }, {
- .name = "test_klp_callbacks_mod",
- .funcs = no_funcs,
- .callbacks = {
- .pre_patch = pre_patch_callback,
- .post_patch = post_patch_callback,
- .pre_unpatch = pre_unpatch_callback,
- .post_unpatch = post_unpatch_callback,
- },
- }, {
- .name = "test_klp_callbacks_busy",
- .funcs = busymod_funcs,
- .callbacks = {
- .pre_patch = pre_patch_callback,
- .post_patch = post_patch_callback,
- .pre_unpatch = pre_unpatch_callback,
- .post_unpatch = post_unpatch_callback,
- },
- }, { }
+static char *obj_names[] = {
+ "test_klp_callbacks_mod",
+ "test_klp_callbacks_busy",
+ NULL
};
static struct klp_patch patch = {
- .mod = THIS_MODULE,
- .objs = objs,
+ .obj = &obj,
+ .obj_names = obj_names,
};
static int test_klp_callbacks_demo_init(void)
diff --git a/lib/livepatch/test_klp_callbacks_demo.h b/lib/livepatch/test_klp_callbacks_demo.h
new file mode 100644
index 000000000000..d34a6c4b6c91
--- /dev/null
+++ b/lib/livepatch/test_klp_callbacks_demo.h
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
+
+#include <linux/livepatch.h>
+
+#define LIVEPATCH_NAME "test_klp_callbacks_demo"
+
+int pre_patch_callback(struct klp_object *obj);
+void post_patch_callback(struct klp_object *obj);
+void pre_unpatch_callback(struct klp_object *obj);
+void post_unpatch_callback(struct klp_object *obj);
diff --git a/lib/livepatch/test_klp_callbacks_demo2.c b/lib/livepatch/test_klp_callbacks_demo2.c
index 5417573e80af..d83711ce52e3 100644
--- a/lib/livepatch/test_klp_callbacks_demo2.c
+++ b/lib/livepatch/test_klp_callbacks_demo2.c
@@ -6,6 +6,7 @@
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/livepatch.h>
+#include "test_klp_callbacks_demo2.h"
static int replace;
module_param(replace, int, 0644);
@@ -20,58 +21,77 @@ static const char *const module_state[] = {
static void callback_info(const char *callback, struct klp_object *obj)
{
- if (obj->mod)
- pr_info("%s: %s -> %s\n", callback, obj->mod->name,
- module_state[obj->mod->state]);
- else
+ struct module *mod;
+
+ if (!obj->name) {
pr_info("%s: vmlinux\n", callback);
+ return;
+ }
+
+ mutex_lock(&module_mutex);
+ mod = find_module(obj->name);
+ if (mod) {
+ pr_info("%s: %s -> %s\n", callback, obj->name,
+ module_state[mod->state]);
+ } else {
+ pr_err("%s: Unable to find module: %s", callback, obj->name);
+ }
+ mutex_unlock(&module_mutex);
}
/* Executed on object patching (ie, patch enablement) */
-static int pre_patch_callback(struct klp_object *obj)
+int pre_patch_callback2(struct klp_object *obj)
{
callback_info(__func__, obj);
return 0;
}
+EXPORT_SYMBOL(pre_patch_callback2);
/* Executed on object unpatching (ie, patch disablement) */
-static void post_patch_callback(struct klp_object *obj)
+void post_patch_callback2(struct klp_object *obj)
{
callback_info(__func__, obj);
}
+EXPORT_SYMBOL(post_patch_callback2);
/* Executed on object unpatching (ie, patch disablement) */
-static void pre_unpatch_callback(struct klp_object *obj)
+void pre_unpatch_callback2(struct klp_object *obj)
{
callback_info(__func__, obj);
}
+EXPORT_SYMBOL(pre_unpatch_callback2);
/* Executed on object unpatching (ie, patch disablement) */
-static void post_unpatch_callback(struct klp_object *obj)
+void post_unpatch_callback2(struct klp_object *obj)
{
callback_info(__func__, obj);
}
+EXPORT_SYMBOL(post_unpatch_callback2);
static struct klp_func no_funcs[] = {
{ }
};
-static struct klp_object objs[] = {
- {
- .name = NULL, /* vmlinux */
- .funcs = no_funcs,
- .callbacks = {
- .pre_patch = pre_patch_callback,
- .post_patch = post_patch_callback,
- .pre_unpatch = pre_unpatch_callback,
- .post_unpatch = post_unpatch_callback,
- },
- }, { }
+static struct klp_object obj = {
+ .patch_name = LIVEPATCH_NAME,
+ .name = NULL, /* vmlinux */
+ .mod = THIS_MODULE,
+ .funcs = no_funcs,
+ .callbacks = {
+ .pre_patch = pre_patch_callback2,
+ .post_patch = post_patch_callback2,
+ .pre_unpatch = pre_unpatch_callback2,
+ .post_unpatch = post_unpatch_callback2,
+ },
+};
+
+static char *obj_names[] = {
+ NULL
};
static struct klp_patch patch = {
- .mod = THIS_MODULE,
- .objs = objs,
+ .obj = &obj,
+ .obj_names = obj_names,
/* set .replace in the init function below for demo purposes */
};
diff --git a/lib/livepatch/test_klp_callbacks_demo2.h b/lib/livepatch/test_klp_callbacks_demo2.h
new file mode 100644
index 000000000000..595da3395628
--- /dev/null
+++ b/lib/livepatch/test_klp_callbacks_demo2.h
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
+
+#include <linux/livepatch.h>
+
+#define LIVEPATCH_NAME "test_klp_callbacks_demo2"
+
+int pre_patch_callback2(struct klp_object *obj);
+void post_patch_callback2(struct klp_object *obj);
+void pre_unpatch_callback2(struct klp_object *obj);
+void post_unpatch_callback2(struct klp_object *obj);
diff --git a/lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_busy.c b/lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_busy.c
new file mode 100644
index 000000000000..5255cfb22d23
--- /dev/null
+++ b/lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_busy.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+#include "test_klp_callbacks_demo.h"
+
+static void patched_work_func(struct work_struct *work)
+{
+ pr_info("%s\n", __func__);
+}
+
+static struct klp_func busymod_funcs[] = {
+ {
+ .old_name = "busymod_work_func",
+ .new_func = patched_work_func,
+ }, {}
+};
+
+static struct klp_object obj = {
+ .patch_name = LIVEPATCH_NAME,
+ .name = "test_klp_callbacks_busy",
+ .mod = THIS_MODULE,
+ .funcs = busymod_funcs,
+ .callbacks = {
+ .pre_patch = pre_patch_callback,
+ .post_patch = post_patch_callback,
+ .pre_unpatch = pre_unpatch_callback,
+ .post_unpatch = post_unpatch_callback,
+ },
+};
+
+static int test_klp_callbacks_demo_init(void)
+{
+ return klp_add_object(&obj);
+}
+
+static void test_klp_callbacks_demo_exit(void)
+{
+}
+
+module_init(test_klp_callbacks_demo_init);
+module_exit(test_klp_callbacks_demo_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
+MODULE_AUTHOR("Joe Lawrence <joe.lawrence@redhat.com>");
+MODULE_DESCRIPTION("Livepatch test: livepatch demo");
diff --git a/lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_mod.c b/lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_mod.c
new file mode 100644
index 000000000000..f2208deecde1
--- /dev/null
+++ b/lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_mod.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+#include "test_klp_callbacks_demo.h"
+
+static struct klp_func no_funcs[] = {
+ {}
+};
+
+static struct klp_object obj = {
+ .patch_name = LIVEPATCH_NAME,
+ .name = "test_klp_callbacks_mod",
+ .mod = THIS_MODULE,
+ .funcs = no_funcs,
+ .callbacks = {
+ .pre_patch = pre_patch_callback,
+ .post_patch = post_patch_callback,
+ .pre_unpatch = pre_unpatch_callback,
+ .post_unpatch = post_unpatch_callback,
+ },
+};
+
+static int test_klp_callbacks_demo_init(void)
+{
+ return klp_add_object(&obj);
+}
+
+static void test_klp_callbacks_demo_exit(void)
+{
+}
+
+module_init(test_klp_callbacks_demo_init);
+module_exit(test_klp_callbacks_demo_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
+MODULE_AUTHOR("Joe Lawrence <joe.lawrence@redhat.com>");
+MODULE_DESCRIPTION("Livepatch test: livepatch demo");
diff --git a/lib/livepatch/test_klp_livepatch.c b/lib/livepatch/test_klp_livepatch.c
index aff08199de71..d7606d7b8367 100644
--- a/lib/livepatch/test_klp_livepatch.c
+++ b/lib/livepatch/test_klp_livepatch.c
@@ -22,16 +22,20 @@ static struct klp_func funcs[] = {
}, { }
};
-static struct klp_object objs[] = {
- {
- /* name being NULL means vmlinux */
- .funcs = funcs,
- }, { }
+struct klp_object obj = {
+ .patch_name = THIS_MODULE->name,
+ .name = NULL, /* vmlinux */
+ .mod = THIS_MODULE,
+ .funcs = funcs,
+};
+
+static char *obj_names[] = {
+ NULL
};
static struct klp_patch patch = {
- .mod = THIS_MODULE,
- .objs = objs,
+ .obj = &obj,
+ .obj_names = obj_names,
};
static int test_klp_livepatch_init(void)
diff --git a/lib/livepatch/test_klp_state.c b/lib/livepatch/test_klp_state.c
index 57a4253acb01..26e8cb86d59d 100644
--- a/lib/livepatch/test_klp_state.c
+++ b/lib/livepatch/test_klp_state.c
@@ -22,11 +22,22 @@ static const char *const module_state[] = {
static void callback_info(const char *callback, struct klp_object *obj)
{
- if (obj->mod)
- pr_info("%s: %s -> %s\n", callback, obj->mod->name,
- module_state[obj->mod->state]);
- else
+ struct module *mod;
+
+ if (!obj->name) {
pr_info("%s: vmlinux\n", callback);
+ return;
+ }
+
+ mutex_lock(&module_mutex);
+ mod = find_module(obj->name);
+ if (mod) {
+ pr_info("%s: %s -> %s\n", callback, obj->name,
+ module_state[mod->state]);
+ } else {
+ pr_err("%s: Unable to find module: %s", callback, obj->name);
+ }
+ mutex_unlock(&module_mutex);
}
static struct klp_patch patch;
@@ -118,19 +129,6 @@ static struct klp_func no_funcs[] = {
{}
};
-static struct klp_object objs[] = {
- {
- .name = NULL, /* vmlinux */
- .funcs = no_funcs,
- .callbacks = {
- .pre_patch = pre_patch_callback,
- .post_patch = post_patch_callback,
- .pre_unpatch = pre_unpatch_callback,
- .post_unpatch = post_unpatch_callback,
- },
- }, { }
-};
-
static struct klp_state states[] = {
{
.id = CONSOLE_LOGLEVEL_STATE,
@@ -138,9 +136,26 @@ static struct klp_state states[] = {
}, { }
};
-static struct klp_patch patch = {
+static struct klp_object obj = {
+ .patch_name = THIS_MODULE->name,
+ .name = NULL, /* vmlinux */
.mod = THIS_MODULE,
- .objs = objs,
+ .funcs = no_funcs,
+ .callbacks = {
+ .pre_patch = pre_patch_callback,
+ .post_patch = post_patch_callback,
+ .pre_unpatch = pre_unpatch_callback,
+ .post_unpatch = post_unpatch_callback,
+ },
+};
+
+static char *obj_names[] = {
+ NULL
+};
+
+static struct klp_patch patch = {
+ .obj = &obj,
+ .obj_names = obj_names,
.states = states,
.replace = true,
};
diff --git a/lib/livepatch/test_klp_state2.c b/lib/livepatch/test_klp_state2.c
index c978ea4d5e67..4942924d5cb2 100644
--- a/lib/livepatch/test_klp_state2.c
+++ b/lib/livepatch/test_klp_state2.c
@@ -22,11 +22,22 @@ static const char *const module_state[] = {
static void callback_info(const char *callback, struct klp_object *obj)
{
- if (obj->mod)
- pr_info("%s: %s -> %s\n", callback, obj->mod->name,
- module_state[obj->mod->state]);
- else
+ struct module *mod;
+
+ if (!obj->name) {
pr_info("%s: vmlinux\n", callback);
+ return;
+ }
+
+ mutex_lock(&module_mutex);
+ mod = find_module(obj->name);
+ if (mod) {
+ pr_info("%s: %s -> %s\n", callback, obj->name,
+ module_state[mod->state]);
+ } else {
+ pr_err("%s: Unable to find module: %s", callback, obj->name);
+ }
+ mutex_unlock(&module_mutex);
}
static struct klp_patch patch;
@@ -147,19 +158,6 @@ static struct klp_func no_funcs[] = {
{}
};
-static struct klp_object objs[] = {
- {
- .name = NULL, /* vmlinux */
- .funcs = no_funcs,
- .callbacks = {
- .pre_patch = pre_patch_callback,
- .post_patch = post_patch_callback,
- .pre_unpatch = pre_unpatch_callback,
- .post_unpatch = post_unpatch_callback,
- },
- }, { }
-};
-
static struct klp_state states[] = {
{
.id = CONSOLE_LOGLEVEL_STATE,
@@ -167,9 +165,26 @@ static struct klp_state states[] = {
}, { }
};
-static struct klp_patch patch = {
+static struct klp_object obj = {
+ .patch_name = THIS_MODULE->name,
+ .name = NULL, /* vmlinux */
.mod = THIS_MODULE,
- .objs = objs,
+ .funcs = no_funcs,
+ .callbacks = {
+ .pre_patch = pre_patch_callback,
+ .post_patch = post_patch_callback,
+ .pre_unpatch = pre_unpatch_callback,
+ .post_unpatch = post_unpatch_callback,
+ },
+};
+
+static char *obj_names[] = {
+ NULL
+};
+
+static struct klp_patch patch = {
+ .obj = &obj,
+ .obj_names = obj_names,
.states = states,
.replace = true,
};
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 9f853eeb6140..922bf3fa335c 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -2,7 +2,11 @@
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1__livepatch-shadow-mod.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2__livepatch-shadow-mod.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo__livepatch-callbacks-mod.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo__livepatch-callbacks-busymod.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
diff --git a/samples/livepatch/livepatch-callbacks-demo.c b/samples/livepatch/livepatch-callbacks-demo.c
index 11c3f4357812..d119f8ba4f3d 100644
--- a/samples/livepatch/livepatch-callbacks-demo.c
+++ b/samples/livepatch/livepatch-callbacks-demo.c
@@ -83,6 +83,7 @@
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/livepatch.h>
+#include "livepatch-callbacks-demo.h"
static int pre_patch_ret;
module_param(pre_patch_ret, int, 0644);
@@ -97,88 +98,79 @@ static const char *const module_state[] = {
static void callback_info(const char *callback, struct klp_object *obj)
{
- if (obj->mod)
- pr_info("%s: %s -> %s\n", callback, obj->mod->name,
- module_state[obj->mod->state]);
- else
+ struct module *mod;
+
+ if (!obj->name) {
pr_info("%s: vmlinux\n", callback);
+ return;
+ }
+
+ mutex_lock(&module_mutex);
+ mod = find_module(obj->name);
+ if (mod) {
+ pr_info("%s: %s -> %s\n", callback, obj->name,
+ module_state[mod->state]);
+ } else {
+ pr_err("%s: Unable to find module: %s", callback, obj->name);
+ }
+ mutex_unlock(&module_mutex);
}
/* Executed on object patching (ie, patch enablement) */
-static int pre_patch_callback(struct klp_object *obj)
+int sample_pre_patch_callback(struct klp_object *obj)
{
callback_info(__func__, obj);
return pre_patch_ret;
}
+EXPORT_SYMBOL(sample_pre_patch_callback);
/* Executed on object unpatching (ie, patch disablement) */
-static void post_patch_callback(struct klp_object *obj)
+void sample_post_patch_callback(struct klp_object *obj)
{
callback_info(__func__, obj);
}
+EXPORT_SYMBOL(sample_post_patch_callback);
/* Executed on object unpatching (ie, patch disablement) */
-static void pre_unpatch_callback(struct klp_object *obj)
+void sample_pre_unpatch_callback(struct klp_object *obj)
{
callback_info(__func__, obj);
}
+EXPORT_SYMBOL(sample_pre_unpatch_callback);
/* Executed on object unpatching (ie, patch disablement) */
-static void post_unpatch_callback(struct klp_object *obj)
+void sample_post_unpatch_callback(struct klp_object *obj)
{
callback_info(__func__, obj);
}
-
-static void patched_work_func(struct work_struct *work)
-{
- pr_info("%s\n", __func__);
-}
+EXPORT_SYMBOL(sample_post_unpatch_callback);
static struct klp_func no_funcs[] = {
{ }
};
-static struct klp_func busymod_funcs[] = {
- {
- .old_name = "busymod_work_func",
- .new_func = patched_work_func,
- }, { }
+static struct klp_object obj = {
+ .patch_name = LIVEPATCH_NAME,
+ .name = NULL, /* vmlinux */
+ .mod = THIS_MODULE,
+ .funcs = no_funcs,
+ .callbacks = {
+ .pre_patch = sample_pre_patch_callback,
+ .post_patch = sample_post_patch_callback,
+ .pre_unpatch = sample_pre_unpatch_callback,
+ .post_unpatch = sample_post_unpatch_callback,
+ },
};
-static struct klp_object objs[] = {
- {
- .name = NULL, /* vmlinux */
- .funcs = no_funcs,
- .callbacks = {
- .pre_patch = pre_patch_callback,
- .post_patch = post_patch_callback,
- .pre_unpatch = pre_unpatch_callback,
- .post_unpatch = post_unpatch_callback,
- },
- }, {
- .name = "livepatch_callbacks_mod",
- .funcs = no_funcs,
- .callbacks = {
- .pre_patch = pre_patch_callback,
- .post_patch = post_patch_callback,
- .pre_unpatch = pre_unpatch_callback,
- .post_unpatch = post_unpatch_callback,
- },
- }, {
- .name = "livepatch_callbacks_busymod",
- .funcs = busymod_funcs,
- .callbacks = {
- .pre_patch = pre_patch_callback,
- .post_patch = post_patch_callback,
- .pre_unpatch = pre_unpatch_callback,
- .post_unpatch = post_unpatch_callback,
- },
- }, { }
+static char *obj_names[] = {
+ "livepatch_callbacks_mod",
+ "livepatch_callbacks_busymod",
+ NULL
};
static struct klp_patch patch = {
- .mod = THIS_MODULE,
- .objs = objs,
+ .obj = &obj,
+ .obj_names = obj_names,
};
static int livepatch_callbacks_demo_init(void)
diff --git a/samples/livepatch/livepatch-callbacks-demo.h b/samples/livepatch/livepatch-callbacks-demo.h
new file mode 100644
index 000000000000..42b0e92f2dda
--- /dev/null
+++ b/samples/livepatch/livepatch-callbacks-demo.h
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
+
+#include <linux/livepatch.h>
+
+#define LIVEPATCH_NAME "livepatch_callbacks_demo"
+
+int sample_pre_patch_callback(struct klp_object *obj);
+void sample_post_patch_callback(struct klp_object *obj);
+void sample_pre_unpatch_callback(struct klp_object *obj);
+void sample_post_unpatch_callback(struct klp_object *obj);
diff --git a/samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-busymod.c b/samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-busymod.c
new file mode 100644
index 000000000000..8eb714ba59d1
--- /dev/null
+++ b/samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-busymod.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2017 Joe Lawrence <joe.lawrence@redhat.com>
+ */
+
+/*
+ * livepatch-callbacks-demo.c - (un)patching callbacks livepatch demo
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+#include "livepatch-callbacks-demo.h"
+
+static void patched_work_func(struct work_struct *work)
+{
+ pr_info("%s\n", __func__);
+}
+
+static struct klp_func busymod_funcs[] = {
+ {
+ .old_name = "busymod_work_func",
+ .new_func = patched_work_func,
+ }, { }
+};
+
+static struct klp_object obj = {
+ .patch_name = LIVEPATCH_NAME,
+ .name = "livepatch_callbacks_busymod",
+ .mod = THIS_MODULE,
+ .funcs = busymod_funcs,
+ .callbacks = {
+ .pre_patch = sample_pre_patch_callback,
+ .post_patch = sample_post_patch_callback,
+ .pre_unpatch = sample_pre_unpatch_callback,
+ .post_unpatch = sample_post_unpatch_callback,
+ },
+};
+
+static int livepatch_callbacks_demo_init(void)
+{
+ return klp_add_object(&obj);
+}
+
+static void livepatch_callbacks_demo_exit(void)
+{
+}
+
+module_init(livepatch_callbacks_demo_init);
+module_exit(livepatch_callbacks_demo_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
diff --git a/samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-mod.c b/samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-mod.c
new file mode 100644
index 000000000000..134fcf4bf69d
--- /dev/null
+++ b/samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-mod.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2017 Joe Lawrence <joe.lawrence@redhat.com>
+ */
+
+/*
+ * livepatch-callbacks-demo.c - (un)patching callbacks livepatch demo
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+#include "livepatch-callbacks-demo.h"
+
+static struct klp_func no_funcs[] = {
+ { }
+};
+
+static struct klp_object obj = {
+ .patch_name = LIVEPATCH_NAME,
+ .name = "livepatch_callbacks_mod",
+ .mod = THIS_MODULE,
+ .funcs = no_funcs,
+ .callbacks = {
+ .pre_patch = sample_pre_patch_callback,
+ .post_patch = sample_post_patch_callback,
+ .pre_unpatch = sample_pre_unpatch_callback,
+ .post_unpatch = sample_post_unpatch_callback,
+ },
+};
+
+static int livepatch_callbacks_demo_init(void)
+{
+ return klp_add_object(&obj);
+}
+
+static void livepatch_callbacks_demo_exit(void)
+{
+}
+
+module_init(livepatch_callbacks_demo_init);
+module_exit(livepatch_callbacks_demo_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
index cd76d7ebe598..94b74c05c4be 100644
--- a/samples/livepatch/livepatch-sample.c
+++ b/samples/livepatch/livepatch-sample.c
@@ -43,16 +43,20 @@ static struct klp_func funcs[] = {
}, { }
};
-static struct klp_object objs[] = {
- {
- /* name being NULL means vmlinux */
- .funcs = funcs,
- }, { }
+static char *obj_names[] = {
+ NULL
};
-static struct klp_patch patch = {
+static struct klp_object obj = {
+ .patch_name = THIS_MODULE->name,
+ .name = NULL, /* vmlinux */
.mod = THIS_MODULE,
- .objs = objs,
+ .funcs = funcs,
+};
+
+static struct klp_patch patch = {
+ .obj = &obj,
+ .obj_names = obj_names,
};
static int livepatch_init(void)
diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index e89ca4546114..62c3f89f26d1 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -27,120 +27,26 @@
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/livepatch.h>
-#include <linux/slab.h>
-/* Shadow variable enums */
-#define SV_LEAK 1
-
-/* Allocate new dummies every second */
-#define ALLOC_PERIOD 1
-/* Check for expired dummies after a few new ones have been allocated */
-#define CLEANUP_PERIOD (3 * ALLOC_PERIOD)
-/* Dummies expire after a few cleanup instances */
-#define EXPIRE_PERIOD (4 * CLEANUP_PERIOD)
-
-struct dummy {
- struct list_head list;
- unsigned long jiffies_expire;
+static struct klp_func no_funcs[] = {
+ {}
};
-/*
- * The constructor makes more sense together with klp_shadow_get_or_alloc().
- * In this example, it would be safe to assign the pointer also to the shadow
- * variable returned by klp_shadow_alloc(). But we wanted to show the more
- * complicated use of the API.
- */
-static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
-{
- void **shadow_leak = shadow_data;
- void *leak = ctor_data;
-
- *shadow_leak = leak;
- return 0;
-}
-
-static struct dummy *livepatch_fix1_dummy_alloc(void)
-{
- struct dummy *d;
- void *leak;
-
- d = kzalloc(sizeof(*d), GFP_KERNEL);
- if (!d)
- return NULL;
-
- d->jiffies_expire = jiffies +
- msecs_to_jiffies(1000 * EXPIRE_PERIOD);
-
- /*
- * Patch: save the extra memory location into a SV_LEAK shadow
- * variable. A patched dummy_free routine can later fetch this
- * pointer to handle resource release.
- */
- leak = kzalloc(sizeof(int), GFP_KERNEL);
- if (!leak) {
- kfree(d);
- return NULL;
- }
-
- klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
- shadow_leak_ctor, leak);
-
- pr_info("%s: dummy @ %p, expires @ %lx\n",
- __func__, d, d->jiffies_expire);
-
- return d;
-}
-
-static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
-{
- void *d = obj;
- void **shadow_leak = shadow_data;
-
- kfree(*shadow_leak);
- pr_info("%s: dummy @ %p, prevented leak @ %p\n",
- __func__, d, *shadow_leak);
-}
-
-static void livepatch_fix1_dummy_free(struct dummy *d)
-{
- void **shadow_leak;
-
- /*
- * Patch: fetch the saved SV_LEAK shadow variable, detach and
- * free it. Note: handle cases where this shadow variable does
- * not exist (ie, dummy structures allocated before this livepatch
- * was loaded.)
- */
- shadow_leak = klp_shadow_get(d, SV_LEAK);
- if (shadow_leak)
- klp_shadow_free(d, SV_LEAK, livepatch_fix1_dummy_leak_dtor);
- else
- pr_info("%s: dummy @ %p leaked!\n", __func__, d);
-
- kfree(d);
-}
-
-static struct klp_func funcs[] = {
- {
- .old_name = "dummy_alloc",
- .new_func = livepatch_fix1_dummy_alloc,
- },
- {
- .old_name = "dummy_free",
- .new_func = livepatch_fix1_dummy_free,
- }, { }
+static struct klp_object obj = {
+ .patch_name = THIS_MODULE->name,
+ .name = NULL, /* vmlinux */
+ .mod = THIS_MODULE,
+ .funcs = no_funcs,
};
-static struct klp_object objs[] = {
- {
- .name = "livepatch_shadow_mod",
- .funcs = funcs,
- }, { }
+static char *obj_names[] = {
+ "livepatch_shadow_mod",
+ NULL
};
static struct klp_patch patch = {
- .mod = THIS_MODULE,
- .objs = objs,
+ .obj = &obj,
+ .obj_names = obj_names,
};
static int livepatch_shadow_fix1_init(void)
@@ -150,8 +56,6 @@ static int livepatch_shadow_fix1_init(void)
static void livepatch_shadow_fix1_exit(void)
{
- /* Cleanup any existing SV_LEAK shadow variables */
- klp_shadow_free_all(SV_LEAK, livepatch_fix1_dummy_leak_dtor);
}
module_init(livepatch_shadow_fix1_init);
diff --git a/samples/livepatch/livepatch-shadow-fix1__livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-fix1__livepatch-shadow-mod.c
new file mode 100644
index 000000000000..904bbc9ccfda
--- /dev/null
+++ b/samples/livepatch/livepatch-shadow-fix1__livepatch-shadow-mod.c
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2017 Joe Lawrence <joe.lawrence@redhat.com>
+ */
+
+/*
+ * livepatch-shadow-fix1.c - Shadow variables, livepatch demo
+ *
+ * Purpose
+ * -------
+ *
+ * Fixes the memory leak introduced in livepatch-shadow-mod through the
+ * use of a shadow variable. This fix demonstrates the "extending" of
+ * short-lived data structures by patching its allocation and release
+ * functions.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * This module is not intended to be standalone. See the "Usage"
+ * section of livepatch-shadow-mod.c.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+#include <linux/slab.h>
+
+/* Shadow variable enums */
+#define SV_LEAK 1
+
+/* Allocate new dummies every second */
+#define ALLOC_PERIOD 1
+/* Check for expired dummies after a few new ones have been allocated */
+#define CLEANUP_PERIOD (3 * ALLOC_PERIOD)
+/* Dummies expire after a few cleanup instances */
+#define EXPIRE_PERIOD (4 * CLEANUP_PERIOD)
+
+struct dummy {
+ struct list_head list;
+ unsigned long jiffies_expire;
+};
+
+/*
+ * The constructor makes more sense together with klp_shadow_get_or_alloc().
+ * In this example, it would be safe to assign the pointer also to the shadow
+ * variable returned by klp_shadow_alloc(). But we wanted to show the more
+ * complicated use of the API.
+ */
+static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
+{
+ void **shadow_leak = shadow_data;
+ void *leak = ctor_data;
+
+ *shadow_leak = leak;
+ return 0;
+}
+
+static struct dummy *livepatch_fix1_dummy_alloc(void)
+{
+ struct dummy *d;
+ void *leak;
+
+ d = kzalloc(sizeof(*d), GFP_KERNEL);
+ if (!d)
+ return NULL;
+
+ d->jiffies_expire = jiffies +
+ msecs_to_jiffies(1000 * EXPIRE_PERIOD);
+
+ /*
+ * Patch: save the extra memory location into a SV_LEAK shadow
+ * variable. A patched dummy_free routine can later fetch this
+ * pointer to handle resource release.
+ */
+ leak = kzalloc(sizeof(int), GFP_KERNEL);
+ if (!leak) {
+ kfree(d);
+ return NULL;
+ }
+
+ klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
+ shadow_leak_ctor, leak);
+
+ pr_info("%s: dummy @ %p, expires @ %lx\n",
+ __func__, d, d->jiffies_expire);
+
+ return d;
+}
+
+static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
+{
+ void *d = obj;
+ void **shadow_leak = shadow_data;
+
+ kfree(*shadow_leak);
+ pr_info("%s: dummy @ %p, prevented leak @ %p\n",
+ __func__, d, *shadow_leak);
+}
+
+static void livepatch_fix1_dummy_free(struct dummy *d)
+{
+ void **shadow_leak;
+
+ /*
+ * Patch: fetch the saved SV_LEAK shadow variable, detach and
+ * free it. Note: handle cases where this shadow variable does
+ * not exist (ie, dummy structures allocated before this livepatch
+ * was loaded.)
+ */
+ shadow_leak = klp_shadow_get(d, SV_LEAK);
+ if (shadow_leak)
+ klp_shadow_free(d, SV_LEAK, livepatch_fix1_dummy_leak_dtor);
+ else
+ pr_info("%s: dummy @ %p leaked!\n", __func__, d);
+
+ kfree(d);
+}
+
+static struct klp_func funcs[] = {
+ {
+ .old_name = "dummy_alloc",
+ .new_func = livepatch_fix1_dummy_alloc,
+ },
+ {
+ .old_name = "dummy_free",
+ .new_func = livepatch_fix1_dummy_free,
+ }, { }
+};
+
+static struct klp_object obj = {
+ .patch_name = "livepatch_shadow_fix1",
+ .name = "livepatch_shadow_mod",
+ .mod = THIS_MODULE,
+ .funcs = funcs,
+};
+
+static int livepatch_shadow_fix1_init(void)
+{
+ return klp_add_object(&obj);
+}
+
+static void livepatch_shadow_fix1_exit(void)
+{
+ /* Cleanup any existing SV_LEAK shadow variables */
+ klp_shadow_free_all(SV_LEAK, livepatch_fix1_dummy_leak_dtor);
+}
+
+module_init(livepatch_shadow_fix1_init);
+module_exit(livepatch_shadow_fix1_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c
index 50d223b82e8b..6f1359c002d5 100644
--- a/samples/livepatch/livepatch-shadow-fix2.c
+++ b/samples/livepatch/livepatch-shadow-fix2.c
@@ -27,92 +27,26 @@
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/livepatch.h>
-#include <linux/slab.h>
-/* Shadow variable enums */
-#define SV_LEAK 1
-#define SV_COUNTER 2
-
-struct dummy {
- struct list_head list;
- unsigned long jiffies_expire;
+static struct klp_func no_funcs[] = {
+ {}
};
-static bool livepatch_fix2_dummy_check(struct dummy *d, unsigned long jiffies)
-{
- int *shadow_count;
-
- /*
- * Patch: handle in-flight dummy structures, if they do not
- * already have a SV_COUNTER shadow variable, then attach a
- * new one.
- */
- shadow_count = klp_shadow_get_or_alloc(d, SV_COUNTER,
- sizeof(*shadow_count), GFP_NOWAIT,
- NULL, NULL);
- if (shadow_count)
- *shadow_count += 1;
-
- return time_after(jiffies, d->jiffies_expire);
-}
-
-static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data)
-{
- void *d = obj;
- void **shadow_leak = shadow_data;
-
- kfree(*shadow_leak);
- pr_info("%s: dummy @ %p, prevented leak @ %p\n",
- __func__, d, *shadow_leak);
-}
-
-static void livepatch_fix2_dummy_free(struct dummy *d)
-{
- void **shadow_leak;
- int *shadow_count;
-
- /* Patch: copy the memory leak patch from the fix1 module. */
- shadow_leak = klp_shadow_get(d, SV_LEAK);
- if (shadow_leak)
- klp_shadow_free(d, SV_LEAK, livepatch_fix2_dummy_leak_dtor);
- else
- pr_info("%s: dummy @ %p leaked!\n", __func__, d);
-
- /*
- * Patch: fetch the SV_COUNTER shadow variable and display
- * the final count. Detach the shadow variable.
- */
- shadow_count = klp_shadow_get(d, SV_COUNTER);
- if (shadow_count) {
- pr_info("%s: dummy @ %p, check counter = %d\n",
- __func__, d, *shadow_count);
- klp_shadow_free(d, SV_COUNTER, NULL);
- }
-
- kfree(d);
-}
-
-static struct klp_func funcs[] = {
- {
- .old_name = "dummy_check",
- .new_func = livepatch_fix2_dummy_check,
- },
- {
- .old_name = "dummy_free",
- .new_func = livepatch_fix2_dummy_free,
- }, { }
+static struct klp_object obj = {
+ .patch_name = THIS_MODULE->name,
+ .name = NULL, /* vmlinux */
+ .mod = THIS_MODULE,
+ .funcs = no_funcs,
};
-static struct klp_object objs[] = {
- {
- .name = "livepatch_shadow_mod",
- .funcs = funcs,
- }, { }
+static char *obj_names[] = {
+ "livepatch_shadow_mod",
+ NULL
};
static struct klp_patch patch = {
- .mod = THIS_MODULE,
- .objs = objs,
+ .obj = &obj,
+ .obj_names = obj_names,
};
static int livepatch_shadow_fix2_init(void)
@@ -122,8 +56,6 @@ static int livepatch_shadow_fix2_init(void)
static void livepatch_shadow_fix2_exit(void)
{
- /* Cleanup any existing SV_COUNTER shadow variables */
- klp_shadow_free_all(SV_COUNTER, NULL);
}
module_init(livepatch_shadow_fix2_init);
diff --git a/samples/livepatch/livepatch-shadow-fix2__livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-fix2__livepatch-shadow-mod.c
new file mode 100644
index 000000000000..f4592b78d667
--- /dev/null
+++ b/samples/livepatch/livepatch-shadow-fix2__livepatch-shadow-mod.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2017 Joe Lawrence <joe.lawrence@redhat.com>
+ */
+
+/*
+ * livepatch-shadow-fix2.c - Shadow variables, livepatch demo
+ *
+ * Purpose
+ * -------
+ *
+ * Adds functionality to livepatch-shadow-mod's in-flight data
+ * structures through a shadow variable. The livepatch patches a
+ * routine that periodically inspects data structures, incrementing a
+ * per-data-structure counter, creating the counter if needed.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * This module is not intended to be standalone. See the "Usage"
+ * section of livepatch-shadow-mod.c.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+#include <linux/slab.h>
+
+/* Shadow variable enums */
+#define SV_LEAK 1
+#define SV_COUNTER 2
+
+struct dummy {
+ struct list_head list;
+ unsigned long jiffies_expire;
+};
+
+static bool livepatch_fix2_dummy_check(struct dummy *d, unsigned long jiffies)
+{
+ int *shadow_count;
+
+ /*
+ * Patch: handle in-flight dummy structures, if they do not
+ * already have a SV_COUNTER shadow variable, then attach a
+ * new one.
+ */
+ shadow_count = klp_shadow_get_or_alloc(d, SV_COUNTER,
+ sizeof(*shadow_count), GFP_NOWAIT,
+ NULL, NULL);
+ if (shadow_count)
+ *shadow_count += 1;
+
+ return time_after(jiffies, d->jiffies_expire);
+}
+
+static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data)
+{
+ void *d = obj;
+ void **shadow_leak = shadow_data;
+
+ kfree(*shadow_leak);
+ pr_info("%s: dummy @ %p, prevented leak @ %p\n",
+ __func__, d, *shadow_leak);
+}
+
+static void livepatch_fix2_dummy_free(struct dummy *d)
+{
+ void **shadow_leak;
+ int *shadow_count;
+
+ /* Patch: copy the memory leak patch from the fix1 module. */
+ shadow_leak = klp_shadow_get(d, SV_LEAK);
+ if (shadow_leak)
+ klp_shadow_free(d, SV_LEAK, livepatch_fix2_dummy_leak_dtor);
+ else
+ pr_info("%s: dummy @ %p leaked!\n", __func__, d);
+
+ /*
+ * Patch: fetch the SV_COUNTER shadow variable and display
+ * the final count. Detach the shadow variable.
+ */
+ shadow_count = klp_shadow_get(d, SV_COUNTER);
+ if (shadow_count) {
+ pr_info("%s: dummy @ %p, check counter = %d\n",
+ __func__, d, *shadow_count);
+ klp_shadow_free(d, SV_COUNTER, NULL);
+ }
+
+ kfree(d);
+}
+
+static struct klp_func funcs[] = {
+ {
+ .old_name = "dummy_check",
+ .new_func = livepatch_fix2_dummy_check,
+ },
+ {
+ .old_name = "dummy_free",
+ .new_func = livepatch_fix2_dummy_free,
+ }, { }
+};
+
+static struct klp_object obj = {
+ .patch_name = "livepatch_shadow_fix2",
+ .name = "livepatch_shadow_mod",
+ .mod = THIS_MODULE,
+ .funcs = funcs,
+};
+
+static int livepatch_shadow_fix2_init(void)
+{
+ return klp_add_object(&obj);
+}
+
+static void livepatch_shadow_fix2_exit(void)
+{
+ /* Cleanup any existing SV_COUNTER shadow variables */
+ klp_shadow_free_all(SV_COUNTER, NULL);
+}
+
+module_init(livepatch_shadow_fix2_init);
+module_exit(livepatch_shadow_fix2_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh
index a35289b13c9c..ccaed35d0901 100755
--- a/tools/testing/selftests/livepatch/test-callbacks.sh
+++ b/tools/testing/selftests/livepatch/test-callbacks.sh
@@ -509,17 +509,17 @@ livepatch: '$MOD_LIVEPATCH': patching complete
% modprobe $MOD_LIVEPATCH2
livepatch: enabling patch '$MOD_LIVEPATCH2'
livepatch: '$MOD_LIVEPATCH2': initializing patching transition
-$MOD_LIVEPATCH2: pre_patch_callback: vmlinux
+$MOD_LIVEPATCH2: pre_patch_callback2: vmlinux
livepatch: '$MOD_LIVEPATCH2': starting patching transition
livepatch: '$MOD_LIVEPATCH2': completing patching transition
-$MOD_LIVEPATCH2: post_patch_callback: vmlinux
+$MOD_LIVEPATCH2: post_patch_callback2: vmlinux
livepatch: '$MOD_LIVEPATCH2': patching complete
% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH2/enabled
livepatch: '$MOD_LIVEPATCH2': initializing unpatching transition
-$MOD_LIVEPATCH2: pre_unpatch_callback: vmlinux
+$MOD_LIVEPATCH2: pre_unpatch_callback2: vmlinux
livepatch: '$MOD_LIVEPATCH2': starting unpatching transition
livepatch: '$MOD_LIVEPATCH2': completing unpatching transition
-$MOD_LIVEPATCH2: post_unpatch_callback: vmlinux
+$MOD_LIVEPATCH2: post_unpatch_callback2: vmlinux
livepatch: '$MOD_LIVEPATCH2': unpatching complete
% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
@@ -568,17 +568,17 @@ livepatch: '$MOD_LIVEPATCH': patching complete
% modprobe $MOD_LIVEPATCH2 replace=1
livepatch: enabling patch '$MOD_LIVEPATCH2'
livepatch: '$MOD_LIVEPATCH2': initializing patching transition
-$MOD_LIVEPATCH2: pre_patch_callback: vmlinux
+$MOD_LIVEPATCH2: pre_patch_callback2: vmlinux
livepatch: '$MOD_LIVEPATCH2': starting patching transition
livepatch: '$MOD_LIVEPATCH2': completing patching transition
-$MOD_LIVEPATCH2: post_patch_callback: vmlinux
+$MOD_LIVEPATCH2: post_patch_callback2: vmlinux
livepatch: '$MOD_LIVEPATCH2': patching complete
% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH2/enabled
livepatch: '$MOD_LIVEPATCH2': initializing unpatching transition
-$MOD_LIVEPATCH2: pre_unpatch_callback: vmlinux
+$MOD_LIVEPATCH2: pre_unpatch_callback2: vmlinux
livepatch: '$MOD_LIVEPATCH2': starting unpatching transition
livepatch: '$MOD_LIVEPATCH2': completing unpatching transition
-$MOD_LIVEPATCH2: post_unpatch_callback: vmlinux
+$MOD_LIVEPATCH2: post_unpatch_callback2: vmlinux
livepatch: '$MOD_LIVEPATCH2': unpatching complete
% rmmod $MOD_LIVEPATCH2
% rmmod $MOD_LIVEPATCH"
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 03/23] livepatch: Better checks of struct klp_object definition
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
2020-01-17 15:03 ` [POC 01/23] module: Allow to delete module also from inside kernel Petr Mladek
2020-01-17 15:03 ` [POC 02/23] livepatch: Split livepatch modules per livepatched object Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-21 11:27 ` Julien Thierry
2020-01-17 15:03 ` [POC 04/23] livepatch: Prevent loading livepatch sub-module unintentionally Petr Mladek
` (19 subsequent siblings)
22 siblings, 1 reply; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
The number of user defined fields increased in struct klp_object after
spliting per-object livepatches. It was sometimes unclear why exactly
the module could not get loded when returned -EINVAL.
Add more checks for the split modules and write useful error
messages on particular errors.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 91 ++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 82 insertions(+), 9 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bb62c5407b75..ec7ffc7db3a7 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -756,9 +756,6 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
int ret;
const char *name;
- if (klp_is_module(obj) && strlen(obj->name) >= MODULE_NAME_LEN)
- return -EINVAL;
-
obj->patched = false;
name = obj->name ? obj->name : "vmlinux";
@@ -851,8 +848,86 @@ static int klp_init_patch(struct klp_patch *patch)
return 0;
}
+static int klp_check_module_name(struct klp_object *obj, bool is_module)
+{
+ char mod_name[MODULE_NAME_LEN];
+ const char *expected_name;
+
+ if (is_module) {
+ snprintf(mod_name, sizeof(mod_name), "%s__%s",
+ obj->patch_name, obj->name);
+ expected_name = mod_name;
+ } else {
+ expected_name = obj->patch_name;
+ }
+
+ if (strcmp(expected_name, obj->mod->name)) {
+ pr_err("The module name %s does not match with obj->patch_name and obj->name. The expected name is: %s\n",
+ obj->mod->name, expected_name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int klp_check_object(struct klp_object *obj, bool is_module)
+{
+
+ if (!obj->mod)
+ return -EINVAL;
+
+ if (!is_livepatch_module(obj->mod)) {
+ pr_err("module %s is not marked as a livepatch module\n",
+ obj->mod->name);
+ return -EINVAL;
+ }
+
+ if (!obj->patch_name) {
+ pr_err("module %s does not have set obj->patch_name\n",
+ obj->mod->name);
+ return -EINVAL;
+ }
+
+ if (strlen(obj->patch_name) >= MODULE_NAME_LEN) {
+ pr_err("module %s has too long obj->patch_name\n",
+ obj->mod->name);
+ return -EINVAL;
+ }
+
+ if (is_module) {
+ if (!obj->name) {
+ pr_err("module %s does not have set obj->name\n",
+ obj->mod->name);
+ return -EINVAL;
+ }
+ if (strlen(obj->name) >= MODULE_NAME_LEN) {
+ pr_err("module %s has too long obj->name\n",
+ obj->mod->name);
+ return -EINVAL;
+ }
+ } else if (obj->name) {
+ pr_err("module %s for vmlinux must not have set obj->name\n",
+ obj->mod->name);
+ return -EINVAL;
+ }
+
+ if (!obj->funcs) {
+ pr_err("module %s does not have set obj->funcs\n",
+ obj->mod->name);
+ return -EINVAL;
+ }
+
+ return klp_check_module_name(obj, is_module);
+}
+
int klp_add_object(struct klp_object *obj)
{
+ int ret;
+
+ ret = klp_check_object(obj, true);
+ if (ret)
+ return ret;
+
return 0;
}
EXPORT_SYMBOL_GPL(klp_add_object);
@@ -958,14 +1033,12 @@ int klp_enable_patch(struct klp_patch *patch)
{
int ret;
- if (!patch || !patch->obj || !patch->obj->mod)
+ if (!patch || !patch->obj)
return -EINVAL;
- if (!is_livepatch_module(patch->obj->mod)) {
- pr_err("module %s is not marked as a livepatch module\n",
- patch->obj->patch_name);
- return -EINVAL;
- }
+ ret = klp_check_object(patch->obj, false);
+ if (ret)
+ return ret;
if (!klp_initialized())
return -ENODEV;
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 04/23] livepatch: Prevent loading livepatch sub-module unintentionally.
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (2 preceding siblings ...)
2020-01-17 15:03 ` [POC 03/23] livepatch: Better checks of struct klp_object definition Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-04-03 17:54 ` Joe Lawrence
2020-01-17 15:03 ` [POC 05/23] livepatch: Initialize and free livepatch submodule Petr Mladek
` (18 subsequent siblings)
22 siblings, 1 reply; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
Livepatch is split into several modules. The main module is for livepatching
vmlinux. The rest is for livepatching other modules.
Only the livepatch module for vmlinux can be loaded by users. Others are
loaded automatically when the related module is or gets loaded.
Users might try to load any livepatch module. It must be allowed
only when the related livepatch module for vmlinux and the livepatched
module are loaded.
Also it is important to check that obj->name is listed in patch->obj_names.
Otherwise this module would not be loaded automatically. And it would
lead into inconsistent behavier. Anyway, the missing name means a mistake
somewhere and must be reported.
klp_add_object() is taking over the job done by klp_module_coming().
The error message is taken from there so that selftests do not need
to get updated.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 77 insertions(+), 1 deletion(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index ec7ffc7db3a7..e2c7dc6c2d5f 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -88,6 +88,18 @@ static struct klp_object *klp_find_object(struct klp_patch *patch,
return NULL;
}
+static struct klp_patch *klp_find_patch(const char *patch_name)
+{
+ struct klp_patch *patch;
+
+ klp_for_each_patch(patch) {
+ if (!strcmp(patch->obj->patch_name, patch_name))
+ return patch;
+ }
+
+ return NULL;
+}
+
struct klp_find_arg {
const char *objname;
const char *name;
@@ -920,15 +932,79 @@ static int klp_check_object(struct klp_object *obj, bool is_module)
return klp_check_module_name(obj, is_module);
}
+static bool klp_is_object_name_supported(struct klp_patch *patch,
+ const char *name)
+{
+ int i;
+
+ for (i = 0; patch->obj_names[i]; i++) {
+ if (!strcmp(name, patch->obj_names[i]))
+ return true;
+ }
+
+ return false;
+}
+
+/* Must be called under klp_mutex */
+static bool klp_is_object_compatible(struct klp_patch *patch,
+ struct klp_object *obj)
+{
+ struct module *patched_mod;
+
+ if (!klp_is_object_name_supported(patch, obj->name)) {
+ pr_err("Livepatch (%s) is not supposed to livepatch the module: %s\n",
+ obj->patch_name, obj->name);
+ return false;
+ }
+
+ mutex_lock(&module_mutex);
+ patched_mod = find_module(obj->name);
+ mutex_unlock(&module_mutex);
+
+ if (!patched_mod) {
+ pr_err("Livepatched module is not loaded: %s\n", obj->name);
+ return false;
+ }
+
+ return true;
+}
+
int klp_add_object(struct klp_object *obj)
{
+ struct klp_patch *patch;
int ret;
ret = klp_check_object(obj, true);
if (ret)
return ret;
+ mutex_lock(&klp_mutex);
+
+ patch = klp_find_patch(obj->patch_name);
+ if (!patch) {
+ pr_err("Can't load livepatch (%s) for module when the livepatch (%s) for vmcore is not loaded\n",
+ obj->mod->name, obj->patch_name);
+ ret = -EINVAL;
+ goto err;
+ }
+
+ if (!klp_is_object_compatible(patch, obj)) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ mutex_unlock(&klp_mutex);
return 0;
+
+err:
+ /*
+ * If a patch is unsuccessfully applied, return
+ * error to the module loader.
+ */
+ pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
+ patch->obj->patch_name, obj->name, obj->name);
+ mutex_unlock(&klp_mutex);
+ return ret;
}
EXPORT_SYMBOL_GPL(klp_add_object);
@@ -1033,7 +1109,7 @@ int klp_enable_patch(struct klp_patch *patch)
{
int ret;
- if (!patch || !patch->obj)
+ if (!patch || !patch->obj || !patch->obj_names)
return -EINVAL;
ret = klp_check_object(patch->obj, false);
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 05/23] livepatch: Initialize and free livepatch submodule
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (3 preceding siblings ...)
2020-01-17 15:03 ` [POC 04/23] livepatch: Prevent loading livepatch sub-module unintentionally Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-21 11:58 ` Julien Thierry
2020-01-17 15:03 ` [POC 06/23] livepatch: Enable the " Petr Mladek
` (17 subsequent siblings)
22 siblings, 1 reply; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
Another step when loading livepatches to livepatch modules is
to initialize the structure, create sysfs entries, do livepatch
specific relocations.
These operation can fail and the objects must be freed that case.
The error message is taken from klp_module_coming() to match
selftests.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 34 +++++++++++++++++++++++++++-------
1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index e2c7dc6c2d5f..6c27b635e5a7 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -583,18 +583,23 @@ static void klp_free_object_loaded(struct klp_object *obj)
}
}
+static void klp_free_object(struct klp_object *obj, bool nops_only)
+{
+ __klp_free_funcs(obj, nops_only);
+
+ if (nops_only && !obj->dynamic)
+ return;
+
+ list_del(&obj->node);
+ kobject_put(&obj->kobj);
+}
+
static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
{
struct klp_object *obj, *tmp_obj;
klp_for_each_object_safe(patch, obj, tmp_obj) {
- __klp_free_funcs(obj, nops_only);
-
- if (nops_only && !obj->dynamic)
- continue;
-
- list_del(&obj->node);
- kobject_put(&obj->kobj);
+ klp_free_object(obj, nops_only);
}
}
@@ -812,6 +817,8 @@ static int klp_init_object_early(struct klp_patch *patch,
if (obj->dynamic || try_module_get(obj->mod))
return 0;
+ /* patch stays when this function fails in klp_add_object() */
+ list_del(&obj->node);
return -ENODEV;
}
@@ -993,9 +1000,22 @@ int klp_add_object(struct klp_object *obj)
goto err;
}
+ ret = klp_init_object_early(patch, obj);
+ if (ret)
+ goto err;
+
+ ret = klp_init_object(patch, obj);
+ if (ret) {
+ pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
+ patch->obj->patch_name, obj->name, ret);
+ goto err_free;
+ }
+
mutex_unlock(&klp_mutex);
return 0;
+err_free:
+ klp_free_object(obj, false);
err:
/*
* If a patch is unsuccessfully applied, return
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 06/23] livepatch: Enable the livepatch submodule
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (4 preceding siblings ...)
2020-01-17 15:03 ` [POC 05/23] livepatch: Initialize and free livepatch submodule Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 07/23] livepatch: Remove obsolete functionality from klp_module_coming() Petr Mladek
` (16 subsequent siblings)
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
The final step when loading livepatch module for livepatching a module
is to actually enable the livepatch.
The steps are exactly the same as in kmp_module_comming().
Note that there is no need to call klp_cleanup_module_patches_limited()
in the error path. klp_add_object() modifies only the single matching
struct klp_patch.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6c27b635e5a7..c21bd9ec2012 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1011,6 +1011,28 @@ int klp_add_object(struct klp_object *obj)
goto err_free;
}
+ pr_notice("applying patch '%s' to loading module '%s'\n",
+ patch->obj->patch_name, obj->name);
+
+ ret = klp_pre_patch_callback(obj);
+ if (ret) {
+ pr_warn("pre-patch callback failed for object '%s'\n",
+ obj->name);
+ goto err_free;
+ }
+
+ ret = klp_patch_object(obj);
+ if (ret) {
+ pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
+ patch->obj->patch_name, obj->name, ret);
+
+ klp_post_unpatch_callback(obj);
+ goto err_free;
+ }
+
+ if (patch != klp_transition_patch)
+ klp_post_patch_callback(obj);
+
mutex_unlock(&klp_mutex);
return 0;
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 07/23] livepatch: Remove obsolete functionality from klp_module_coming()
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (5 preceding siblings ...)
2020-01-17 15:03 ` [POC 06/23] livepatch: Enable the " Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 08/23] livepatch: Automatically load livepatch module when the patch module is loaded Petr Mladek
` (15 subsequent siblings)
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
klp_module_coming() could not operate on the livepatch structures.
Instead it has to load the livepatch module that will call
klp_add_object().
The functionality has been migrated to klp_add_object() in
the previous commits and the code can be finally removed.
The only exception is mod->klp_alive flag. It will still be needed
to decide who is responsible for loading/removing the eventual
livepatch modules.
It is done this way to make the changes manageable and easier
to review.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 59 -------------------------------------------------
1 file changed, 59 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index c21bd9ec2012..bb851f916182 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1290,10 +1290,6 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
int klp_module_coming(struct module *mod)
{
- int ret;
- struct klp_patch *patch;
- struct klp_object *obj;
-
if (WARN_ON(mod->state != MODULE_STATE_COMING))
return -EINVAL;
@@ -1304,64 +1300,9 @@ int klp_module_coming(struct module *mod)
* get patched by a new patch.
*/
mod->klp_alive = true;
-
- klp_for_each_patch(patch) {
- klp_for_each_object(patch, obj) {
- if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
- continue;
-
- obj->mod = mod;
-
- ret = klp_init_object_loaded(obj);
- if (ret) {
- pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
- patch->obj->patch_name, obj->name, ret);
- goto err;
- }
-
- pr_notice("applying patch '%s' to loading module '%s'\n",
- patch->obj->patch_name, obj->name);
-
- ret = klp_pre_patch_callback(obj);
- if (ret) {
- pr_warn("pre-patch callback failed for object '%s'\n",
- obj->name);
- goto err;
- }
-
- ret = klp_patch_object(obj);
- if (ret) {
- pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
- patch->obj->patch_name, obj->name, ret);
-
- klp_post_unpatch_callback(obj);
- goto err;
- }
-
- if (patch != klp_transition_patch)
- klp_post_patch_callback(obj);
-
- break;
- }
- }
-
mutex_unlock(&klp_mutex);
return 0;
-
-err:
- /*
- * If a patch is unsuccessfully applied, return
- * error to the module loader.
- */
- pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
- patch->obj->patch_name, obj->name, obj->name);
- mod->klp_alive = false;
- obj->mod = NULL;
- klp_cleanup_module_patches_limited(mod, patch);
- mutex_unlock(&klp_mutex);
-
- return ret;
}
void klp_module_going(struct module *mod)
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 08/23] livepatch: Automatically load livepatch module when the patch module is loaded
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (6 preceding siblings ...)
2020-01-17 15:03 ` [POC 07/23] livepatch: Remove obsolete functionality from klp_module_coming() Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 09/23] livepatch: Handle race when livepatches are reloaded during a module load Petr Mladek
` (14 subsequent siblings)
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
The klp_module_coming() callback is called from the module loader when
any module is being loaded. It allows to load the related livepatch modules
in MODULE_COMMING state before mod->init() is called. It prevents the module
from loading when the livepatching fails from any reason.
klp_module_coming() originally did several tasks: livepatch-specific
reallocations, registered ftrace handlers, and called object callbacks
when any defined.
All the mentioned tasks were moved into by klp_add_object() that is
called in mod->init() of livepatch modules that are livepatching
other modules.
Instead, klp_module_coming() has to load the needed livepatch module(s).
This functionality is already used in the kernel.
It is solved by two tricks:
1. The list is searched repeatedly from the beginning. The already
loaded objects are skipped. One object is handled in each
iteration.
This solves the problem when a livepatch is removed or added
in the meantime. Especially, it prevents a crash when
the given struct klp_patch disappeared from the list in
the meantime.
The object will get removed automatically when the livepatch
gets removed.
There might be an attempt to load the module twice: via
the coming notifier and via klp_enable_livepatch(). It is
already handled in the module loader code. Both call will
either succeed or fail the same way.
2. There might be a false error when the livepatch gets removed
in the meantime. It is solved by double checking the existence.
It does not solve the situation when the livepatch is removed
and loaded again in the meantime. It will be solved by a separate
patch. Anyway, it is not much realistic scenario.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 81 ++++++++++++++++++++--
.../testing/selftests/livepatch/test-callbacks.sh | 1 +
2 files changed, 77 insertions(+), 5 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bb851f916182..34e3ee2be7ef 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -8,6 +8,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/kmod.h>
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/mutex.h>
@@ -100,6 +101,24 @@ static struct klp_patch *klp_find_patch(const char *patch_name)
return NULL;
}
+/*
+ * Search whether livepatch for a module is loaded.
+ * Do not use for "vmlinux" that is always loaded.
+ * Must be called under klp_mutex.
+ */
+static bool klp_is_object_loaded(struct klp_patch *patch,
+ char *object_name)
+{
+ struct klp_object *obj;
+
+ klp_for_each_object(patch, obj) {
+ if (obj->name && !strcmp(object_name, obj->name))
+ return true;
+ }
+
+ return false;
+}
+
struct klp_find_arg {
const char *objname;
const char *name;
@@ -1082,6 +1101,19 @@ static int __klp_disable_patch(struct klp_patch *patch)
return 0;
}
+static int klp_try_load_object(const char *patch_name, const char *obj_name)
+{
+ int ret;
+
+ ret = request_module("%s__%s", patch_name, obj_name);
+ if (ret) {
+ pr_info("Module load failed: %s__%s\n", patch_name, obj_name);
+ return ret;
+ }
+
+ return 0;
+}
+
static int __klp_enable_patch(struct klp_patch *patch)
{
struct klp_object *obj;
@@ -1290,19 +1322,58 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
int klp_module_coming(struct module *mod)
{
+ char patch_name[MODULE_NAME_LEN];
+ struct klp_patch *patch;
+ int ret = 0;
+
if (WARN_ON(mod->state != MODULE_STATE_COMING))
return -EINVAL;
mutex_lock(&klp_mutex);
+restart:
+ klp_for_each_patch(patch) {
+ if (!klp_is_object_name_supported(patch, mod->name))
+ continue;
+
+ if (klp_is_object_loaded(patch, mod->name))
+ continue;
+
+ strncpy(patch_name, patch->obj->patch_name, sizeof(patch_name));
+ mutex_unlock(&klp_mutex);
+
+ ret = klp_try_load_object(patch_name, mod->name);
+ /*
+ * The load might have failed because the patch has
+ * been removed in the meantime. In this case, the
+ * error might be ignored.
+ *
+ * FIXME: It is not fully proof. The patch might have be
+ * unloaded and loaded again in the mean time.
+ */
+ mutex_lock(&klp_mutex);
+ if (ret) {
+ patch = klp_find_patch(patch_name);
+ if (patch)
+ goto err;
+ ret = 0;
+ }
+
+ /*
+ * The list of patches might have been manipulated
+ * in the meantime.
+ */
+ goto restart;
+ }
+
/*
- * Each module has to know that klp_module_coming()
- * has been called. We never know what module will
- * get patched by a new patch.
+ * All enabled livepatches are loaded now. From this point, any newly
+ * enabled livepatch is responsible for loading the related livepatch
+ * module in klp_enable_patch().
*/
mod->klp_alive = true;
+err:
mutex_unlock(&klp_mutex);
-
- return 0;
+ return ret;
}
void klp_module_going(struct module *mod)
diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh
index ccaed35d0901..39a4f35e5f8e 100755
--- a/tools/testing/selftests/livepatch/test-callbacks.sh
+++ b/tools/testing/selftests/livepatch/test-callbacks.sh
@@ -330,6 +330,7 @@ livepatch: applying patch '$MOD_LIVEPATCH' to loading module '$MOD_TARGET'
$MOD_LIVEPATCH: pre_patch_callback: $MOD_TARGET -> [MODULE_STATE_COMING] Full formed, running module_init
livepatch: pre-patch callback failed for object '$MOD_TARGET'
livepatch: patch '$MOD_LIVEPATCH' failed for module '$MOD_TARGET', refusing to load module '$MOD_TARGET'
+livepatch: Module load failed: ${MOD_LIVEPATCH}__${MOD_TARGET}
modprobe: ERROR: could not insert '$MOD_TARGET': No such device
% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 09/23] livepatch: Handle race when livepatches are reloaded during a module load
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (7 preceding siblings ...)
2020-01-17 15:03 ` [POC 08/23] livepatch: Automatically load livepatch module when the patch module is loaded Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-22 18:51 ` Julien Thierry
2020-01-17 15:03 ` [POC 10/23] livepatch: Handle modprobe exit code Petr Mladek
` (13 subsequent siblings)
22 siblings, 1 reply; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
klp_module_coming() might fail to load a livepatch module when
the related livepatch gets reloaded in the meantime.
Detect this situation by adding a timestamp into struct klp_patch.
local_clock is enough because klp_mutex must be released and taken
several times during this scenario.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
include/linux/livepatch.h | 2 ++
kernel/livepatch/core.c | 9 +++++----
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index a4567c17a9f2..feb33f023f9f 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -155,6 +155,7 @@ struct klp_state {
* @obj_list: dynamic list of the object entries
* @enabled: the patch is enabled (but operation may be incomplete)
* @forced: was involved in a forced transition
+ * ts: timestamp when the livepatch has been loaded
* @free_work: patch cleanup from workqueue-context
* @finish: for waiting till it is safe to remove the patch module
*/
@@ -171,6 +172,7 @@ struct klp_patch {
struct list_head obj_list;
bool enabled;
bool forced;
+ u64 ts;
struct work_struct free_work;
struct completion finish;
};
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 34e3ee2be7ef..8e693c58b736 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -20,6 +20,7 @@
#include <linux/moduleloader.h>
#include <linux/completion.h>
#include <linux/memory.h>
+#include <linux/sched/clock.h>
#include <asm/cacheflush.h>
#include "core.h"
#include "patch.h"
@@ -854,6 +855,7 @@ static int klp_init_patch_early(struct klp_patch *patch)
kobject_init(&patch->kobj, &klp_ktype_patch);
patch->enabled = false;
patch->forced = false;
+ patch->ts = local_clock();
INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
init_completion(&patch->finish);
@@ -1324,6 +1326,7 @@ int klp_module_coming(struct module *mod)
{
char patch_name[MODULE_NAME_LEN];
struct klp_patch *patch;
+ u64 patch_ts;
int ret = 0;
if (WARN_ON(mod->state != MODULE_STATE_COMING))
@@ -1339,6 +1342,7 @@ int klp_module_coming(struct module *mod)
continue;
strncpy(patch_name, patch->obj->patch_name, sizeof(patch_name));
+ patch_ts = patch->ts;
mutex_unlock(&klp_mutex);
ret = klp_try_load_object(patch_name, mod->name);
@@ -1346,14 +1350,11 @@ int klp_module_coming(struct module *mod)
* The load might have failed because the patch has
* been removed in the meantime. In this case, the
* error might be ignored.
- *
- * FIXME: It is not fully proof. The patch might have be
- * unloaded and loaded again in the mean time.
*/
mutex_lock(&klp_mutex);
if (ret) {
patch = klp_find_patch(patch_name);
- if (patch)
+ if (patch && patch->ts == patch_ts)
goto err;
ret = 0;
}
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 10/23] livepatch: Handle modprobe exit code
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (8 preceding siblings ...)
2020-01-17 15:03 ` [POC 09/23] livepatch: Handle race when livepatches are reloaded during a module load Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 11/23] livepatch: Safely detect forced transition when removing split livepatch modules Petr Mladek
` (12 subsequent siblings)
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
request_module() returns classic negative error codes when it was
not even able to call "modprobe" from some reasons. Otherwise,
it returns the exit code multiplied by 256.
modprobe exit code is always 1 in case of error. Use -EINVAL
instead as the least ugly internal error code.
A better solution would be to somehow pass the original error
code from the init_module() syscall or at least the error code
from klp_module_add() functions. But there is no obvious way
how to pass the information.
Global variable is not enough because more livepatch modules
can be loaded simultaneously from klp_enable_patch() and
klp_module_comming().
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 3 +++
tools/testing/selftests/livepatch/test-callbacks.sh | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 8e693c58b736..19ca8baa2f16 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1109,6 +1109,9 @@ static int klp_try_load_object(const char *patch_name, const char *obj_name)
ret = request_module("%s__%s", patch_name, obj_name);
if (ret) {
+ /* modprobe always set exit code 1 on error */
+ if (ret > 0)
+ ret = -EINVAL;
pr_info("Module load failed: %s__%s\n", patch_name, obj_name);
return ret;
}
diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh
index 39a4f35e5f8e..060e5b512367 100755
--- a/tools/testing/selftests/livepatch/test-callbacks.sh
+++ b/tools/testing/selftests/livepatch/test-callbacks.sh
@@ -331,7 +331,7 @@ $MOD_LIVEPATCH: pre_patch_callback: $MOD_TARGET -> [MODULE_STATE_COMING] Full fo
livepatch: pre-patch callback failed for object '$MOD_TARGET'
livepatch: patch '$MOD_LIVEPATCH' failed for module '$MOD_TARGET', refusing to load module '$MOD_TARGET'
livepatch: Module load failed: ${MOD_LIVEPATCH}__${MOD_TARGET}
-modprobe: ERROR: could not insert '$MOD_TARGET': No such device
+modprobe: ERROR: could not insert '$MOD_TARGET': Invalid argument
% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
$MOD_LIVEPATCH: pre_unpatch_callback: vmlinux
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 11/23] livepatch: Safely detect forced transition when removing split livepatch modules
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (9 preceding siblings ...)
2020-01-17 15:03 ` [POC 10/23] livepatch: Handle modprobe exit code Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-22 10:15 ` Julien Thierry
2020-01-17 15:03 ` [POC 12/23] livepatch: Automatically remove livepatch module when the object is freed Petr Mladek
` (11 subsequent siblings)
22 siblings, 1 reply; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
The information about forced livepatch transition is currently stored
in struct klp_patch. But there is not any obvious safe way how to
access it when the split livepatch modules are removed.
module_put() for the livepatch module must be called only when neither
the code nor the data are accessed. The best location is
klp_kobj_release_object(). It can be called asynchronously when
the kobject hierarchy is already destroyed and struct klp_patch might
already be gone.
Solve this by moving the "force" flag into struct klp_object.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
include/linux/livepatch.h | 4 ++--
kernel/livepatch/core.c | 11 ++++++++---
kernel/livepatch/transition.c | 8 ++++++--
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index feb33f023f9f..e021e512b207 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -114,6 +114,7 @@ struct klp_callbacks {
* @node: list node for klp_patch obj_list
* @dynamic: temporary object for nop functions; dynamically allocated
* @patched: the object's funcs have been added to the klp_ops list
+ * @forced: was involved in a forced transition
*/
struct klp_object {
/* external */
@@ -129,6 +130,7 @@ struct klp_object {
struct list_head node;
bool dynamic;
bool patched;
+ bool forced;
};
/**
@@ -154,7 +156,6 @@ struct klp_state {
* @kobj: kobject for sysfs resources
* @obj_list: dynamic list of the object entries
* @enabled: the patch is enabled (but operation may be incomplete)
- * @forced: was involved in a forced transition
* ts: timestamp when the livepatch has been loaded
* @free_work: patch cleanup from workqueue-context
* @finish: for waiting till it is safe to remove the patch module
@@ -171,7 +172,6 @@ struct klp_patch {
struct kobject kobj;
struct list_head obj_list;
bool enabled;
- bool forced;
u64 ts;
struct work_struct free_work;
struct completion finish;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 19ca8baa2f16..2f15ff360676 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -551,8 +551,13 @@ static void klp_kobj_release_object(struct kobject *kobj)
obj = container_of(kobj, struct klp_object, kobj);
- if (obj->dynamic)
+ if (obj->dynamic) {
klp_free_object_dynamic(obj);
+ return;
+ }
+
+ if (klp_is_module(obj) && !obj->forced)
+ module_put(obj->mod);
}
static struct kobj_type klp_ktype_object = {
@@ -668,7 +673,7 @@ static void klp_free_patch_finish(struct klp_patch *patch)
wait_for_completion(&patch->finish);
/* Put the module after the last access to struct klp_patch. */
- if (!patch->forced)
+ if (!patch->obj->forced)
module_put(patch->obj->mod);
}
@@ -829,6 +834,7 @@ static int klp_init_object_early(struct klp_patch *patch,
INIT_LIST_HEAD(&obj->func_list);
kobject_init(&obj->kobj, &klp_ktype_object);
list_add_tail(&obj->node, &patch->obj_list);
+ obj->forced = false;
klp_for_each_func_static(obj, func) {
klp_init_func_early(obj, func);
@@ -854,7 +860,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
INIT_LIST_HEAD(&patch->obj_list);
kobject_init(&patch->kobj, &klp_ktype_patch);
patch->enabled = false;
- patch->forced = false;
patch->ts = local_clock();
INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
init_completion(&patch->finish);
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 78e3280560cd..e99c27a5ddbf 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -626,6 +626,7 @@ void klp_copy_process(struct task_struct *child)
void klp_force_transition(void)
{
struct klp_patch *patch;
+ struct klp_object *obj;
struct task_struct *g, *task;
unsigned int cpu;
@@ -639,6 +640,9 @@ void klp_force_transition(void)
for_each_possible_cpu(cpu)
klp_update_patch_state(idle_task(cpu));
- klp_for_each_patch(patch)
- patch->forced = true;
+ klp_for_each_patch(patch) {
+ klp_for_each_object(patch, obj) {
+ obj->forced = true;
+ }
+ }
}
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 12/23] livepatch: Automatically remove livepatch module when the object is freed
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (10 preceding siblings ...)
2020-01-17 15:03 ` [POC 11/23] livepatch: Safely detect forced transition when removing split livepatch modules Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 13/23] livepatch: Remove livepatch module when the livepatched module is unloaded Petr Mladek
` (10 subsequent siblings)
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
Make it easy to deal with the split livepatch modules. Remove the livepatch
modules that livepatched modules when they are no longer used.
It must be done from workqueue context to avoid deadlocks.
It must not be done when klp_add_module() fails because is called from
the livepatch module init callback. The module will not load at
all in this case.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
include/linux/livepatch.h | 4 ++++
kernel/livepatch/core.c | 19 ++++++++++++++++++-
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index e021e512b207..4afb7f3a5a36 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -115,6 +115,8 @@ struct klp_callbacks {
* @dynamic: temporary object for nop functions; dynamically allocated
* @patched: the object's funcs have been added to the klp_ops list
* @forced: was involved in a forced transition
+ * @add_err: failed to add the object when loading the livepatch module
+ * @remove_work: remove module from workqueue-context
*/
struct klp_object {
/* external */
@@ -131,6 +133,8 @@ struct klp_object {
bool dynamic;
bool patched;
bool forced;
+ bool add_err;
+ struct work_struct remove_work;
};
/**
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 2f15ff360676..6c51b194da57 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -556,8 +556,14 @@ static void klp_kobj_release_object(struct kobject *kobj)
return;
}
- if (klp_is_module(obj) && !obj->forced)
+ if (obj->forced || !klp_is_module(obj))
+ return;
+
+ /* Must not explicitely remove module when adding failed. */
+ if (obj->add_err)
module_put(obj->mod);
+ else
+ schedule_work(&obj->remove_work);
}
static struct kobj_type klp_ktype_object = {
@@ -677,6 +683,14 @@ static void klp_free_patch_finish(struct klp_patch *patch)
module_put(patch->obj->mod);
}
+static void klp_remove_module_work_fn(struct work_struct *work)
+{
+ struct klp_object *obj =
+ container_of(work, struct klp_object, remove_work);
+
+ module_put_and_delete(obj->mod);
+}
+
/*
* The livepatch might be freed from sysfs interface created by the patch.
* This work allows to wait until the interface is destroyed in a separate
@@ -835,6 +849,8 @@ static int klp_init_object_early(struct klp_patch *patch,
kobject_init(&obj->kobj, &klp_ktype_object);
list_add_tail(&obj->node, &patch->obj_list);
obj->forced = false;
+ obj->add_err = false;
+ INIT_WORK(&obj->remove_work, klp_remove_module_work_fn);
klp_for_each_func_static(obj, func) {
klp_init_func_early(obj, func);
@@ -1063,6 +1079,7 @@ int klp_add_object(struct klp_object *obj)
return 0;
err_free:
+ obj->add_err = true;
klp_free_object(obj, false);
err:
/*
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 13/23] livepatch: Remove livepatch module when the livepatched module is unloaded
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (11 preceding siblings ...)
2020-01-17 15:03 ` [POC 12/23] livepatch: Automatically remove livepatch module when the object is freed Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 14/23] livepatch: Never block livepatch modules when the related module is being removed Petr Mladek
` (9 subsequent siblings)
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
klp_module_going() does very similar things as before splitting
the livepatches. Especially, it unpatches the related code, calls
the callbacks, unlinks the structures from the linked lists.
It does not have to clean up the structure because the entire livepatch
module is going to be freed and removed.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 84 ++++++++++++++++++-------------------------------
1 file changed, 31 insertions(+), 53 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6c51b194da57..73462b66f63f 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -599,21 +599,6 @@ static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
}
}
-/* Clean up when a patched object is unloaded */
-static void klp_free_object_loaded(struct klp_object *obj)
-{
- struct klp_func *func;
-
- obj->mod = NULL;
-
- klp_for_each_func(obj, func) {
- func->old_func = NULL;
-
- if (func->nop)
- func->new_func = NULL;
- }
-}
-
static void klp_free_object(struct klp_object *obj, bool nops_only)
{
__klp_free_funcs(obj, nops_only);
@@ -909,6 +894,24 @@ static int klp_init_patch(struct klp_patch *patch)
return 0;
}
+static void klp_remove_object(struct klp_object *obj)
+{
+ struct klp_patch *patch =
+ container_of(obj->kobj.parent, struct klp_patch, kobj);
+
+ if (patch != klp_transition_patch)
+ klp_pre_unpatch_callback(obj);
+
+ pr_notice("reverting patch '%s' on unloading module '%s'\n",
+ patch->obj->patch_name, obj->name);
+
+ klp_unpatch_object(obj);
+
+ klp_post_unpatch_callback(obj);
+
+ klp_free_object(obj, false);
+}
+
static int klp_check_module_name(struct klp_object *obj, bool is_module)
{
char mod_name[MODULE_NAME_LEN];
@@ -1313,40 +1316,6 @@ void klp_discard_nops(struct klp_patch *new_patch)
klp_free_objects_dynamic(klp_transition_patch);
}
-/*
- * Remove parts of patches that touch a given kernel module. The list of
- * patches processed might be limited. When limit is NULL, all patches
- * will be handled.
- */
-static void klp_cleanup_module_patches_limited(struct module *mod,
- struct klp_patch *limit)
-{
- struct klp_patch *patch;
- struct klp_object *obj;
-
- klp_for_each_patch(patch) {
- if (patch == limit)
- break;
-
- klp_for_each_object(patch, obj) {
- if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
- continue;
-
- if (patch != klp_transition_patch)
- klp_pre_unpatch_callback(obj);
-
- pr_notice("reverting patch '%s' on unloading module '%s'\n",
- patch->obj->patch_name, obj->name);
- klp_unpatch_object(obj);
-
- klp_post_unpatch_callback(obj);
-
- klp_free_object_loaded(obj);
- break;
- }
- }
-}
-
int klp_module_coming(struct module *mod)
{
char patch_name[MODULE_NAME_LEN];
@@ -1404,19 +1373,28 @@ int klp_module_coming(struct module *mod)
void klp_module_going(struct module *mod)
{
+ struct klp_patch *patch;
+ struct klp_object *obj, *tmp_obj;
+
if (WARN_ON(mod->state != MODULE_STATE_GOING &&
mod->state != MODULE_STATE_COMING))
return;
mutex_lock(&klp_mutex);
/*
- * Each module has to know that klp_module_going()
- * has been called. We never know what module will
- * get patched by a new patch.
+ * All already enabled livepatches for this module as going to be
+ * removed now. From this point, klp_enable_patch() must not load
+ * any new livepatch modules for this module.
*/
mod->klp_alive = false;
- klp_cleanup_module_patches_limited(mod, NULL);
+ klp_for_each_patch(patch) {
+ klp_for_each_object_safe(patch, obj, tmp_obj) {
+ if (obj->name && !strcmp(obj->name, mod->name)) {
+ klp_remove_object(obj);
+ }
+ }
+ }
mutex_unlock(&klp_mutex);
}
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 14/23] livepatch: Never block livepatch modules when the related module is being removed
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (12 preceding siblings ...)
2020-01-17 15:03 ` [POC 13/23] livepatch: Remove livepatch module when the livepatched module is unloaded Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 15/23] livepatch: Prevent infinite loop when loading livepatch module Petr Mladek
` (8 subsequent siblings)
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
Originally, it was not possible to remove the single livepatch module when
the code was used during a forced transition. There were no guarantees that
the code was no longer used.
Even the split livepatch modules have to stay when the entire livepatch
gets disabled/replaced and the livepatch modules were used during
a forced transition.
On the other hand, klp_module_going() callback is called when the patched
module is about to be removed. It's code should no longer be used.
The same should be true also for the related livepatch module. Therefore
the livepatch module could always get removed as well.
It allows to load the patched module again in the future. Otherwise,
it would get blocked because the related livepatch module could not
get loaded more times.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 73462b66f63f..4b55d805f3ec 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1391,6 +1391,13 @@ void klp_module_going(struct module *mod)
klp_for_each_patch(patch) {
klp_for_each_object_safe(patch, obj, tmp_obj) {
if (obj->name && !strcmp(obj->name, mod->name)) {
+ /*
+ * The livepatched module is about to be
+ * destroyed. It's code is no longer used.
+ * Same is true for the livepatch even when
+ * it was part of forced transition.
+ */
+ obj->forced = false;
klp_remove_object(obj);
}
}
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 15/23] livepatch: Prevent infinite loop when loading livepatch module
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (13 preceding siblings ...)
2020-01-17 15:03 ` [POC 14/23] livepatch: Never block livepatch modules when the related module is being removed Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 16/23] livepatch: Add patch into the global list early Petr Mladek
` (7 subsequent siblings)
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
Livepatch modules should get automatically removed when the patched
module gets removed. But the problem with forced flag has shown that
there might be situations where it did not work as expected.
The problem with forced flag is solved now. But there might be other
situations that are not known at the moment.
Be on the safe side and add a paranoid check for preventing an infinite
loop in klp_module_coming() callback. It might happen when it tries to load
a livepatch module that has already been loaded in the past and that
was not removed because of a bug somewhere.
POC NOTE: The main purpose of this patch is to show potential problems
with the split livepatches. It is hard to say if the check is
worth it or whether even more checks are needed.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 4b55d805f3ec..688ad81def14 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1316,6 +1316,25 @@ void klp_discard_nops(struct klp_patch *new_patch)
klp_free_objects_dynamic(klp_transition_patch);
}
+static bool klp_is_object_module_alive(const char *patch_name,
+ const char *obj_name)
+{
+ static char mod_name[MODULE_NAME_LEN];
+ struct module *mod;
+ bool alive = false;
+
+ mutex_lock(&module_mutex);
+
+ snprintf(mod_name, sizeof(mod_name), "%s__%s", patch_name, obj_name);
+ mod = find_module(mod_name);
+ if (mod && mod->state & MODULE_STATE_LIVE)
+ alive = true;
+
+ mutex_unlock(&module_mutex);
+
+ return alive;
+}
+
int klp_module_coming(struct module *mod)
{
char patch_name[MODULE_NAME_LEN];
@@ -1335,6 +1354,19 @@ int klp_module_coming(struct module *mod)
if (klp_is_object_loaded(patch, mod->name))
continue;
+ /*
+ * Paranoid check. Prevent infinite loop when a livepatch
+ * module is alive and klp_is_object_loaded() does not
+ * see it. It might happen when the object is removed
+ * and module_put() is not called. This should never happen
+ * when everything works as expected.
+ */
+ if (klp_is_object_module_alive(patch->obj->mod->name,
+ mod->name)) {
+ ret = -EBUSY;
+ goto err;
+ }
+
strncpy(patch_name, patch->obj->patch_name, sizeof(patch_name));
patch_ts = patch->ts;
mutex_unlock(&klp_mutex);
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 16/23] livepatch: Add patch into the global list early
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (14 preceding siblings ...)
2020-01-17 15:03 ` [POC 15/23] livepatch: Prevent infinite loop when loading livepatch module Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 17/23] livepatch: Load livepatches for modules when loading the main livepatch Petr Mladek
` (6 subsequent siblings)
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
The objects for livepatched modules need to be loaded before the livepatch
gets enabled. klp_add_module() has to find the patch. The code is easier
when the being-enabled patch can be found in the global list.
Fortunately, there is no problem to add patch into the list already
in the early init. klp_free_patch_start() will remove it in case
of any later error.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 688ad81def14..06676ec63ba7 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -852,6 +852,7 @@ static int klp_init_object_early(struct klp_patch *patch,
static int klp_init_patch_early(struct klp_patch *patch)
{
struct klp_object *obj = patch->obj;
+ int ret;
/* Main patch module is always for vmlinux */
if (obj->name)
@@ -865,7 +866,11 @@ static int klp_init_patch_early(struct klp_patch *patch)
INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
init_completion(&patch->finish);
- return klp_init_object_early(patch, obj);
+ ret = klp_init_object_early(patch, obj);
+ if (!ret)
+ list_add_tail(&patch->list, &klp_patches);
+
+ return ret;
}
static int klp_init_patch(struct klp_patch *patch)
@@ -889,8 +894,6 @@ static int klp_init_patch(struct klp_patch *patch)
return ret;
}
- list_add_tail(&patch->list, &klp_patches);
-
return 0;
}
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 17/23] livepatch: Load livepatches for modules when loading the main livepatch
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (15 preceding siblings ...)
2020-01-17 15:03 ` [POC 16/23] livepatch: Add patch into the global list early Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 18/23] module: Refactor add_unformed_module() Petr Mladek
` (5 subsequent siblings)
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
The livepatch modules have been split per-object. The livepatch module for
"vmlinux" must always exist. It includes struct klp_patch and defines
the sysfs interface. It must be loaded first and removed last.
The related livepatch modules that are livepatching already loaded modules
must be loaded before the main livepatch gets enabled.
It might be possible to check if a livepatch module exists for each
loaded module by calling "modprobe -qR". But it would be a nightmare
to avoid races.
Instead, names of all livepatched modules are statically defined
in patch->obj_names array. klp_loaded_objects_livepatches()
iterates over this list and loads the needed modules one
by one. It must be called with klp_mutex released. It is
safe becase:
1. The livepatch itself could not be removed until it returns
from mod->init(). Note that it is not visible in sysfs
interface at the moment.
2. Any operation with other livepatches is prevented using
klp_loading_livepatch variable. The affected operations
are klp_enable_livepatch() and enabled_store().
3. Modules loaded and removed in parallel are properly handled
by klp_module_coming() and klp_module_going() callbacks.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/livepatch/core.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 79 insertions(+), 1 deletion(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 06676ec63ba7..6d4ec7642908 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -45,6 +45,7 @@ DEFINE_MUTEX(klp_mutex);
LIST_HEAD(klp_patches);
static struct kobject *klp_root_kobj;
+static struct klp_patch *klp_loading_patch;
static bool klp_is_module(struct klp_object *obj)
{
@@ -332,6 +333,16 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
goto out;
}
+ /*
+ * Do not manipulate other livepatches when a new one is being
+ * loaded to make our live easier. The patch that is being londed
+ * is not visible in sysfs at this point.
+ */
+ if (klp_loading_patch) {
+ ret = -EBUSY;
+ goto out;
+ }
+
/*
* Allow to reverse a pending transition in both ways. It might be
* necessary to complete the transition without forcing and breaking
@@ -1052,6 +1063,9 @@ int klp_add_object(struct klp_object *obj)
if (ret)
goto err;
+ if (!patch->enabled)
+ goto out;
+
ret = klp_init_object(patch, obj);
if (ret) {
pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
@@ -1081,6 +1095,7 @@ int klp_add_object(struct klp_object *obj)
if (patch != klp_transition_patch)
klp_post_patch_callback(obj);
+out:
mutex_unlock(&klp_mutex);
return 0;
@@ -1147,12 +1162,43 @@ static int klp_try_load_object(const char *patch_name, const char *obj_name)
return 0;
}
+/*
+ * This function is guarded by klp_loading_patch instead of klp_mutex to avoid
+ * deadlocks. It loads other livepatch modules via modprobe called in userspace.
+ * The other modules need to take klp-mutex as well.
+ */
+static int klp_load_objects_livepatches(struct klp_patch *patch)
+{
+ int i, ret;
+
+ for (i = 0; patch->obj_names[i]; i++) {
+ char *obj_name = patch->obj_names[i];
+ struct module *patched_mod;
+ bool obj_alive;
+
+ mutex_lock(&module_mutex);
+ patched_mod = find_module(obj_name);
+ obj_alive = patched_mod ? patched_mod->klp_alive : false;
+ mutex_unlock(&module_mutex);
+
+ /* Load livepatch module only for loaded objects. */
+ if (!obj_alive)
+ continue;
+
+ ret = klp_try_load_object(patch->obj->patch_name, obj_name);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
static int __klp_enable_patch(struct klp_patch *patch)
{
struct klp_object *obj;
int ret;
- if (klp_transition_patch)
+ if (klp_transition_patch || klp_loading_patch)
return -EBUSY;
if (WARN_ON(patch->enabled))
@@ -1199,6 +1245,21 @@ static int __klp_enable_patch(struct klp_patch *patch)
return ret;
}
+static int klp_check_patches_in_progress(void)
+{
+ struct klp_patch *patch;
+
+ if (klp_loading_patch)
+ patch = klp_loading_patch;
+ else if (klp_transition_patch)
+ patch = klp_transition_patch;
+ else
+ return 0;
+
+ pr_err("Already enabling livepatch: %s\n", patch->obj->mod->name);
+ return -EBUSY;
+}
+
/**
* klp_enable_patch() - enable the livepatch
* @patch: patch to be enabled
@@ -1233,6 +1294,12 @@ int klp_enable_patch(struct klp_patch *patch)
mutex_lock(&klp_mutex);
+ ret = klp_check_patches_in_progress();
+ if (ret) {
+ mutex_unlock(&klp_mutex);
+ return ret;
+ }
+
if (!klp_is_patch_compatible(patch)) {
pr_err("Livepatch patch (%s) is not compatible with the already installed livepatches.\n",
patch->obj->mod->name);
@@ -1246,6 +1313,17 @@ int klp_enable_patch(struct klp_patch *patch)
return ret;
}
+ klp_loading_patch = patch;
+ mutex_unlock(&klp_mutex);
+
+ ret = klp_load_objects_livepatches(patch);
+
+ mutex_lock(&klp_mutex);
+ klp_loading_patch = NULL;
+
+ if (ret)
+ goto err;
+
ret = klp_init_patch(patch);
if (ret)
goto err;
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 18/23] module: Refactor add_unformed_module()
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (16 preceding siblings ...)
2020-01-17 15:03 ` [POC 17/23] livepatch: Load livepatches for modules when loading the main livepatch Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 19/23] module/livepatch: Allow to use exported symbols from livepatch module for "vmlinux" Petr Mladek
` (4 subsequent siblings)
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
The livepatching code will need to add another condition to avoid
waiting. Let's make the code slightly less hairy.
Of course, it is a matter of taste. The main ideas of refactoring:
+ Make it clear what happens when old->state == MODULE_STATE_LIVE.
+ Make it more clear when we are leaving the function immediately
and when we are doing some extra actions.
+ Be able to add another check that has to be done outside
module_mutex and can result into an immediate return.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/module.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index b3f11524f8f9..ac45d465ff23 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3721,28 +3721,30 @@ static int add_unformed_module(struct module *mod)
again:
mutex_lock(&module_mutex);
old = find_module_all(mod->name, strlen(mod->name), true);
+
if (old != NULL) {
- if (old->state != MODULE_STATE_LIVE) {
- /* Wait in case it fails to load. */
+ if (old->state == MODULE_STATE_LIVE) {
mutex_unlock(&module_mutex);
- err = wait_event_interruptible(module_wq,
- finished_loading(mod->name));
- if (err)
- goto out_unlocked;
- goto again;
+ return -EEXIST;
}
- err = -EEXIST;
- goto out;
+
+ /* Wait in case it fails to load. */
+ mutex_unlock(&module_mutex);
+ err = wait_event_interruptible(module_wq,
+ finished_loading(mod->name));
+ if (err)
+ return err;
+
+ /* Did the load succeded or failed? */
+ goto again;
}
+
mod_update_bounds(mod);
list_add_rcu(&mod->list, &modules);
mod_tree_insert(mod);
- err = 0;
-out:
mutex_unlock(&module_mutex);
-out_unlocked:
- return err;
+ return 0;
}
static int complete_formation(struct module *mod, struct load_info *info)
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 19/23] module/livepatch: Allow to use exported symbols from livepatch module for "vmlinux"
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (17 preceding siblings ...)
2020-01-17 15:03 ` [POC 18/23] module: Refactor add_unformed_module() Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-04-06 18:48 ` Joe Lawrence
2020-01-17 15:03 ` [POC 20/23] module/livepatch: Relocate local variables in the module loaded when the livepatch is being loaded Petr Mladek
` (3 subsequent siblings)
22 siblings, 1 reply; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
HINT: Get some coffee before reading this commit message.
Stop reading when it gets too complicated. It is possible that we
will need to resolve symbols from livepatch modules another way.
Livepatches need to access also non-exported symbols anyway.
Or just ask me to explain the problem a better way. I have
ended in many cycles when thinking about it. And it might
be much easier from another point of view.
The split per-object livepatches brings even more types of module
dependencies. Let's split them into few categories:
A. Livepatch module using an exported symbol from "vmlinux".
It is quite common and works by definition. Livepatch module is just
a module from this point of view.
B. Livepatch module using an exported symbol from the patched module.
It should be avoided even with the non-split livepatch module. The module
loader automatically takes reference to make sure the modules are
unloaded in the right order. This would basically prevent the livepatched
module from unloading.
Note that it would be perfectly safe to remove this automatic
dependency. The livepatch framework makes sure that the livepatch
module is loaded only when the patched one is loaded. But it cannot
be implemented easily, see below.
C. Livepatch module using an exported symbol from the main livepatch module
for "vmlinux".
It is the 2nd most realistic variant. It even exists in current
selftests. Namely, test_klp_callback_demo* modules share
the implementation of callbacks. It avoids code duplication.
And it is actually needed to make module parameters working.
Note that the current implementation allows to pass module parameters
only to the main livepatch module for "vmlinux". It should not be a real
life problem. The parameters are used in selftests. But they are
not used in practice.
D. Livepatch modules might depend on each other. Note that dependency on
the main livepatch module for "vmlinux" has got a separate category 'C'.
The dependencies between modules are quite rare. But they exist.
One can assume that this might be useful also on the livepatching
level.
To keep it sane, the livepatch modules should just follow
the dependencies of the related patched modules. By other words,
the livepatch modules might or should have the same dependencies
as the patched counter parts but nothing more.
Do these dependencies need some special handling?
Yes, the livepatch module load might get blocked in the following
situations:
1. Recursion caused by dependency of type B:
If a livepatch module uses a symbol from the patched module and
the livepatch module is loaded by klp_module_coming().
The module loader will then automatically try to load the patched module
once again while it is blocked in the klp_module_coming() callback
and STATE_COMING.
2. Recursion caused by dependency of type C:
Livepatch for "vmlinux" loads other livepatch modules from
klp_enabled_patch() in mod->init(). If any of these other
modules use symbol from the main module for "vmlinux" the module
loaded will automatically try to load the main module once
again. The first instance is waiting in klp_enable_patch()
and STATE_COMING.
3. Deadlock caused by dependency of type D:
Random dependencies between livepatch and patched modules might
create a cycle. It need not be obvious and detected by the existing
code because some dependencies are created by livepatching code.
It loads the livepatches automatically for patched modules.
What can be done?
First, put aside the problems with random dependencies of type D:
They are not much realistic. Livepatches follow upstream changes.
As a result livepatch modules should have the same of less
dependencies than the related patched modules.
Note that the existing code is able to handle non-cyclic dependencies
between livepatch modules. The module loader is able to load more
modules in parallel. Also klp_add_object() can be called more times
in parallel.
Second, let's look more at the recursion problems. They might cause
deadlock on two locations in the module loader:
i. The recursive instance waits in add_unformed_module() until the first
instance reaches STATE_LIVE. But this never happens because the first
instance waits until the recursive instance finishes.
ii. resolve_symbol() wants to get reference of the module where
the symbol comes from. But strong_try_module_get() refuses
to get reference when the module is still in STATE_COMING.
It does not want to block removing the module when the load
eventually fails later.
Is it possible to avoid the recursion?
1. It might be possible to avoid calling exported symbols directly
from the livepatch. For example, using kallsyms lookup. Or some
special relocation. This would require some extra action when
preparing the livepatchi.
2. Hide these dependencies to depmod and the module loader. This
would require changes in the user space tools.
And what about breaking the cycle?
It seems acceptable to return immediately from the nested load
if we are able to guarantee:
+ The recursion is clearly caused by one the of above described
livepatch dependency type.
+ The code from affected modules will be used only after the modules
are successfully loaded in the end.
+ The modules will get unloaded when there are other errors
on the way.
+ The original caller will get the right error code. By other
words, only the automatically triggered loads will be finished
prematurely.
OK, this patch provides the solution for the dependency of type C. It is when
other livepatch modules use symbols from the livepatch module for vmlinux.
The recursive load returns -EEXIST immediately when the recursive module
tries to load the same livepatch that is already being loaded. It is
safe because:
+ Only one livepatch can be enabled at a time. Any attempt to enable
another livepatch would return -EBUSY.
+ The livepatch module for "vmlinux" is never loaded automatically
because of any dependency. Other livepatch modules for other objects
might depend on it but the module for "vmlinux" must always be loaded
explicitly by the system administrator.
By other words, the recursion of the livepatch module for "vmlinux" that
is just being enabled is always caused by dependency of type C.
The recursive instance can and actually must return immediately. It will
allow to proceed. The entire operation will succeed only when the outer
load succeeds.
But there is still the problem that strong_try_module_get() could
not get reference. Isn't it?
Yes, it is solved by not taking references from livepatch modules.
The reference is needed to make sure that the module will not get
removed prematurely. But this is guaranteed by the livepatch code:
+ Livepatch module for "vmlinux" is always loaded first.
+ Livepatch modules for other objects can't be loaded before
the module for "vmlinux" is loaded, see the checks
in klp_add_object().
+ Livepatch module is always loaded and removed automatically
together with the patched module. The dependencies between
the patched modules are enough as long as the dependencies
of livepatch modules are symmetric.
Finally. dependency of type B will still cause deadlock. It happens
when a livepatch module uses a symbols from the patched module.
It cannot be solved easily because:
+ The patched module is loaded recursively (not the livepatch one).
There is no clear way how to make sure that the recursive
module load is the one triggered by "modprobe" called from
klp_module_coming().
As a result, -EEXIST might be returned also to modprobe
called by the user. It would force the user to double
check that the patched module has really been loaded.
+ If the above problem is not fixed then the result from
modprobe is not reliable. The livepatch code might need
to double check that request_module() really added the
klp_object into the related klp_patch structure.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
include/linux/livepatch.h | 2 ++
kernel/livepatch/core.c | 16 ++++++++++++++++
kernel/module.c | 16 ++++++++++++++++
3 files changed, 34 insertions(+)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 4afb7f3a5a36..4776deb7418c 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -207,6 +207,7 @@ int klp_add_object(struct klp_object *);
void arch_klp_init_object_loaded(struct klp_object *obj);
/* Called from the module loader during module coming/going states */
+bool klp_break_recursion(struct module *mod);
int klp_module_coming(struct module *mod);
void klp_module_going(struct module *mod);
@@ -244,6 +245,7 @@ struct klp_state *klp_get_prev_state(unsigned long id);
#else /* !CONFIG_LIVEPATCH */
+static inline bool klp_break_recursion(struct module *mod) { return false; }
static inline int klp_module_coming(struct module *mod) { return 0; }
static inline void klp_module_going(struct module *mod) {}
static inline bool klp_patch_pending(struct task_struct *task) { return false; }
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6d4ec7642908..9e00871fbc06 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1416,6 +1416,22 @@ static bool klp_is_object_module_alive(const char *patch_name,
return alive;
}
+bool klp_break_recursion(struct module *mod)
+{
+ bool ret = false;
+
+ if (!mod->klp)
+ return false;
+
+ mutex_lock(&klp_mutex);
+ if (klp_loading_patch &&
+ !strcmp(klp_loading_patch->obj->mod->name, mod->name))
+ ret = true;
+ mutex_unlock(&klp_mutex);
+
+ return ret;
+}
+
int klp_module_coming(struct module *mod)
{
char patch_name[MODULE_NAME_LEN];
diff --git a/kernel/module.c b/kernel/module.c
index ac45d465ff23..bd92854b42c2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -867,6 +867,13 @@ int ref_module(struct module *a, struct module *b)
{
int err;
+ /*
+ * Livepatch modules have their own dependency handling.
+ * Implicit dependencies might cause cycles.
+ */
+ if (is_livepatch_module(a))
+ return 0;
+
if (b == NULL || already_uses(a, b))
return 0;
@@ -3730,6 +3737,15 @@ static int add_unformed_module(struct module *mod)
/* Wait in case it fails to load. */
mutex_unlock(&module_mutex);
+
+ /*
+ * Livepatch modules might use exported symbols from vmlinux.
+ * It creates automatic dependencies and recursive module load.
+ * Livepatch core handles the consistency on its own.
+ */
+ if (klp_break_recursion(mod))
+ return -EEXIST;
+
err = wait_event_interruptible(module_wq,
finished_loading(mod->name));
if (err)
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 20/23] module/livepatch: Relocate local variables in the module loaded when the livepatch is being loaded
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (18 preceding siblings ...)
2020-01-17 15:03 ` [POC 19/23] module/livepatch: Allow to use exported symbols from livepatch module for "vmlinux" Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-18 10:29 ` kbuild test robot
2020-04-03 18:00 ` Joe Lawrence
2020-01-17 15:03 ` [POC 21/23] livepatch: Remove obsolete arch_klp_init_object_loaded() Petr Mladek
` (2 subsequent siblings)
22 siblings, 2 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
The special SHF_RELA_LIVEPATCH section is still needed to find static
(non-exported) symbols. But it can be done together with the other
relocations when the livepatch module is being loaded.
There is no longer needed to copy the info section. The related
code in the module loaded will get removed in separate patch.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
include/linux/livepatch.h | 4 +++
kernel/livepatch/core.c | 62 +++--------------------------------------------
kernel/module.c | 16 +++++++-----
3 files changed, 18 insertions(+), 64 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 4776deb7418c..d721e79ac691 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -211,6 +211,10 @@ bool klp_break_recursion(struct module *mod);
int klp_module_coming(struct module *mod);
void klp_module_going(struct module *mod);
+int klp_resolve_symbols(Elf_Shdr *sechdrs,
+ unsigned int relsec,
+ struct module *pmod);
+
void klp_copy_process(struct task_struct *child);
void klp_update_patch_state(struct task_struct *task);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 9e00871fbc06..4da6bbd687d0 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -196,12 +196,15 @@ static int klp_find_object_symbol(const char *objname, const char *name,
return -EINVAL;
}
-static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
+int klp_resolve_symbols(Elf_Shdr *sechdrs,
+ unsigned int relsec,
+ struct module *pmod)
{
int i, cnt, vmlinux, ret;
char objname[MODULE_NAME_LEN];
char symname[KSYM_NAME_LEN];
char *strtab = pmod->core_kallsyms.strtab;
+ Elf_Shdr *relasec = sechdrs + relsec;
Elf_Rela *relas;
Elf_Sym *sym;
unsigned long sympos, addr;
@@ -251,54 +254,6 @@ static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
return 0;
}
-static int klp_write_object_relocations(struct klp_object *obj)
-{
- int i, cnt, ret = 0;
- const char *objname, *secname;
- char sec_objname[MODULE_NAME_LEN];
- struct module *pmod;
- Elf_Shdr *sec;
-
- objname = klp_is_module(obj) ? obj->name : "vmlinux";
- pmod = obj->mod;
-
- /* For each klp relocation section */
- for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
- sec = pmod->klp_info->sechdrs + i;
- secname = pmod->klp_info->secstrings + sec->sh_name;
- if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
- continue;
-
- /*
- * Format: .klp.rela.sec_objname.section_name
- * See comment in klp_resolve_symbols() for an explanation
- * of the selected field width value.
- */
- cnt = sscanf(secname, ".klp.rela.%55[^.]", sec_objname);
- if (cnt != 1) {
- pr_err("section %s has an incorrectly formatted name\n",
- secname);
- ret = -EINVAL;
- break;
- }
-
- if (strcmp(objname, sec_objname))
- continue;
-
- ret = klp_resolve_symbols(sec, pmod);
- if (ret)
- break;
-
- ret = apply_relocate_add(pmod->klp_info->sechdrs,
- pmod->core_kallsyms.strtab,
- pmod->klp_info->symndx, i, pmod);
- if (ret)
- break;
- }
-
- return ret;
-}
-
/*
* Sysfs Interface
*
@@ -758,18 +713,9 @@ static int klp_init_object_loaded(struct klp_object *obj)
int ret;
mutex_lock(&text_mutex);
-
module_disable_ro(obj->mod);
- ret = klp_write_object_relocations(obj);
- if (ret) {
- module_enable_ro(obj->mod, true);
- mutex_unlock(&text_mutex);
- return ret;
- }
-
arch_klp_init_object_loaded(obj);
module_enable_ro(obj->mod, true);
-
mutex_unlock(&text_mutex);
klp_for_each_func(obj, func) {
diff --git a/kernel/module.c b/kernel/module.c
index bd92854b42c2..c14b5135db27 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2410,16 +2410,20 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
continue;
- /* Livepatch relocation sections are applied by livepatch */
- if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
- continue;
-
- if (info->sechdrs[i].sh_type == SHT_REL)
+ /* Livepatch need to resolve static symbols. */
+ if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH) {
+ err = klp_resolve_symbols(info->sechdrs, i, mod);
+ if (err < 0)
+ break;
+ err = apply_relocate_add(info->sechdrs, info->strtab,
+ info->index.sym, i, mod);
+ } else if (info->sechdrs[i].sh_type == SHT_REL) {
err = apply_relocate(info->sechdrs, info->strtab,
info->index.sym, i, mod);
- else if (info->sechdrs[i].sh_type == SHT_RELA)
+ } else if (info->sechdrs[i].sh_type == SHT_RELA) {
err = apply_relocate_add(info->sechdrs, info->strtab,
info->index.sym, i, mod);
+ }
if (err < 0)
break;
}
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 21/23] livepatch: Remove obsolete arch_klp_init_object_loaded()
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (19 preceding siblings ...)
2020-01-17 15:03 ` [POC 20/23] module/livepatch: Relocate local variables in the module loaded when the livepatch is being loaded Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-01-17 15:03 ` [POC 22/23] livepatch/module: Remove obsolete copy_module_elf() Petr Mladek
2020-01-17 15:03 ` [POC 23/23] module: Remove obsolete module_disable_ro() Petr Mladek
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
The livepatch specific-relocation is finally done together with the normal
module relocations. As a result, alternatives, paraintructions, and other
architecture-specific modifications are done correctly by the module
loader out of box. There is no longer need to do them explicitely by
arch_klp_init_object_loaded().
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
Documentation/livepatch/module-elf-format.rst | 29 ---------------
arch/x86/kernel/Makefile | 1 -
arch/x86/kernel/livepatch.c | 52 ---------------------------
include/linux/livepatch.h | 2 --
kernel/livepatch/core.c | 11 ------
5 files changed, 95 deletions(-)
delete mode 100644 arch/x86/kernel/livepatch.c
diff --git a/Documentation/livepatch/module-elf-format.rst b/Documentation/livepatch/module-elf-format.rst
index 2a591e6f8e6c..9f0c997d4940 100644
--- a/Documentation/livepatch/module-elf-format.rst
+++ b/Documentation/livepatch/module-elf-format.rst
@@ -14,8 +14,6 @@ This document outlines the Elf format requirements that livepatch modules must f
4. Livepatch symbols
4.1 A livepatch module's symbol table
4.2 Livepatch symbol format
- 5. Architecture-specific sections
- 6. Symbol table and Elf section access
1. Background and motivation
============================
@@ -297,30 +295,3 @@ See include/uapi/linux/elf.h for the actual definitions.
[*]
Note that the 'Ndx' (Section index) for these symbols is SHN_LIVEPATCH (0xff20).
"OS" means OS-specific.
-
-5. Architecture-specific sections
-=================================
-Architectures may override arch_klp_init_object_loaded() to perform
-additional arch-specific tasks when a target module loads, such as applying
-arch-specific sections. On x86 for example, we must apply per-object
-.altinstructions and .parainstructions sections when a target module loads.
-These sections must be prefixed with ".klp.arch.$objname." so that they can
-be easily identified when iterating through a patch module's Elf sections
-(See arch/x86/kernel/livepatch.c for a complete example).
-
-6. Symbol table and Elf section access
-======================================
-A livepatch module's symbol table is accessible through module->symtab.
-
-Since apply_relocate_add() requires access to a module's section headers,
-symbol table, and relocation section indices, Elf information is preserved for
-livepatch modules and is made accessible by the module loader through
-module->klp_info, which is a klp_modinfo struct. When a livepatch module loads,
-this struct is filled in by the module loader. Its fields are documented below::
-
- struct klp_modinfo {
- Elf_Ehdr hdr; /* Elf header */
- Elf_Shdr *sechdrs; /* Section header table */
- char *secstrings; /* String table for the section headers */
- unsigned int symndx; /* The symbol table section index */
- };
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 6175e370ee4a..21540cac67b9 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -89,7 +89,6 @@ obj-$(CONFIG_X86_MPPARSE) += mpparse.o
obj-y += apic/
obj-$(CONFIG_X86_REBOOTFIXUPS) += reboot_fixups_32.o
obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
-obj-$(CONFIG_LIVEPATCH) += livepatch.o
obj-$(CONFIG_FUNCTION_TRACER) += ftrace_$(BITS).o
obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o
diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
deleted file mode 100644
index 728b44eaa168..000000000000
--- a/arch/x86/kernel/livepatch.c
+++ /dev/null
@@ -1,52 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * livepatch.c - x86-specific Kernel Live Patching Core
- */
-
-#include <linux/module.h>
-#include <linux/kallsyms.h>
-#include <linux/livepatch.h>
-#include <asm/text-patching.h>
-
-/* Apply per-object alternatives. Based on x86 module_finalize() */
-void arch_klp_init_object_loaded(struct klp_object *obj)
-{
- int cnt;
- struct klp_modinfo *info;
- Elf_Shdr *s, *alt = NULL, *para = NULL;
- void *aseg, *pseg;
- const char *objname;
- char sec_objname[MODULE_NAME_LEN];
- char secname[KSYM_NAME_LEN];
-
- info = obj->mod->klp_info;
- objname = obj->name ? obj->name : "vmlinux";
-
- /* See livepatch core code for BUILD_BUG_ON() explanation */
- BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 128);
-
- for (s = info->sechdrs; s < info->sechdrs + info->hdr.e_shnum; s++) {
- /* Apply per-object .klp.arch sections */
- cnt = sscanf(info->secstrings + s->sh_name,
- ".klp.arch.%55[^.].%127s",
- sec_objname, secname);
- if (cnt != 2)
- continue;
- if (strcmp(sec_objname, objname))
- continue;
- if (!strcmp(".altinstructions", secname))
- alt = s;
- if (!strcmp(".parainstructions", secname))
- para = s;
- }
-
- if (alt) {
- aseg = (void *) alt->sh_addr;
- apply_alternatives(aseg, aseg + alt->sh_size);
- }
-
- if (para) {
- pseg = (void *) para->sh_addr;
- apply_paravirt(pseg, pseg + para->sh_size);
- }
-}
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index d721e79ac691..3b27ef1a7291 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -204,8 +204,6 @@ struct klp_patch {
int klp_enable_patch(struct klp_patch *);
int klp_add_object(struct klp_object *);
-void arch_klp_init_object_loaded(struct klp_object *obj);
-
/* Called from the module loader during module coming/going states */
bool klp_break_recursion(struct module *mod);
int klp_module_coming(struct module *mod);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 4da6bbd687d0..cc0ac93fe8cd 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -701,23 +701,12 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
func->old_sympos ? func->old_sympos : 1);
}
-/* Arches may override this to finish any remaining arch-specific tasks */
-void __weak arch_klp_init_object_loaded(struct klp_object *obj)
-{
-}
-
/* parts of the initialization that is done only when the object is loaded */
static int klp_init_object_loaded(struct klp_object *obj)
{
struct klp_func *func;
int ret;
- mutex_lock(&text_mutex);
- module_disable_ro(obj->mod);
- arch_klp_init_object_loaded(obj);
- module_enable_ro(obj->mod, true);
- mutex_unlock(&text_mutex);
-
klp_for_each_func(obj, func) {
ret = klp_find_object_symbol(obj->name, func->old_name,
func->old_sympos,
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 22/23] livepatch/module: Remove obsolete copy_module_elf()
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (20 preceding siblings ...)
2020-01-17 15:03 ` [POC 21/23] livepatch: Remove obsolete arch_klp_init_object_loaded() Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
2020-04-03 18:03 ` Joe Lawrence
2020-01-17 15:03 ` [POC 23/23] module: Remove obsolete module_disable_ro() Petr Mladek
22 siblings, 1 reply; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
The split livepatch modules can be relocated immedidately when they
are loaded. There is no longer needed to preserve the elf sections.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
Documentation/livepatch/module-elf-format.rst | 18 ++++++
include/linux/module.h | 3 -
kernel/module.c | 87 ---------------------------
3 files changed, 18 insertions(+), 90 deletions(-)
diff --git a/Documentation/livepatch/module-elf-format.rst b/Documentation/livepatch/module-elf-format.rst
index 9f0c997d4940..8c6b894c4661 100644
--- a/Documentation/livepatch/module-elf-format.rst
+++ b/Documentation/livepatch/module-elf-format.rst
@@ -14,6 +14,7 @@ This document outlines the Elf format requirements that livepatch modules must f
4. Livepatch symbols
4.1 A livepatch module's symbol table
4.2 Livepatch symbol format
+ 5. Symbol table and Elf section access
1. Background and motivation
============================
@@ -295,3 +296,20 @@ See include/uapi/linux/elf.h for the actual definitions.
[*]
Note that the 'Ndx' (Section index) for these symbols is SHN_LIVEPATCH (0xff20).
"OS" means OS-specific.
+
+5. Symbol table and Elf section access
+======================================
+A livepatch module's symbol table is accessible through module->symtab.
+
+Since apply_relocate_add() requires access to a module's section headers,
+symbol table, and relocation section indices, Elf information is preserved for
+livepatch modules and is made accessible by the module loader through
+module->klp_info, which is a klp_modinfo struct. When a livepatch module loads,
+this struct is filled in by the module loader. Its fields are documented below::
+
+ struct klp_modinfo {
+ Elf_Ehdr hdr; /* Elf header */
+ Elf_Shdr *sechdrs; /* Section header table */
+ char *secstrings; /* String table for the section headers */
+ unsigned int symndx; /* The symbol table section index */
+ };
diff --git a/include/linux/module.h b/include/linux/module.h
index f69f3fd72dd5..8545f3087274 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -483,9 +483,6 @@ struct module {
#ifdef CONFIG_LIVEPATCH
bool klp; /* Is this a livepatch module? */
bool klp_alive;
-
- /* Elf information */
- struct klp_modinfo *klp_info;
#endif
#ifdef CONFIG_MODULE_UNLOAD
diff --git a/kernel/module.c b/kernel/module.c
index c14b5135db27..442926fc5f34 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2125,82 +2125,6 @@ static void module_enable_nx(const struct module *mod) { }
static void module_enable_x(const struct module *mod) { }
#endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */
-
-#ifdef CONFIG_LIVEPATCH
-/*
- * Persist Elf information about a module. Copy the Elf header,
- * section header table, section string table, and symtab section
- * index from info to mod->klp_info.
- */
-static int copy_module_elf(struct module *mod, struct load_info *info)
-{
- unsigned int size, symndx;
- int ret;
-
- size = sizeof(*mod->klp_info);
- mod->klp_info = kmalloc(size, GFP_KERNEL);
- if (mod->klp_info == NULL)
- return -ENOMEM;
-
- /* Elf header */
- size = sizeof(mod->klp_info->hdr);
- memcpy(&mod->klp_info->hdr, info->hdr, size);
-
- /* Elf section header table */
- size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
- mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
- if (mod->klp_info->sechdrs == NULL) {
- ret = -ENOMEM;
- goto free_info;
- }
-
- /* Elf section name string table */
- size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
- mod->klp_info->secstrings = kmemdup(info->secstrings, size, GFP_KERNEL);
- if (mod->klp_info->secstrings == NULL) {
- ret = -ENOMEM;
- goto free_sechdrs;
- }
-
- /* Elf symbol section index */
- symndx = info->index.sym;
- mod->klp_info->symndx = symndx;
-
- /*
- * For livepatch modules, core_kallsyms.symtab is a complete
- * copy of the original symbol table. Adjust sh_addr to point
- * to core_kallsyms.symtab since the copy of the symtab in module
- * init memory is freed at the end of do_init_module().
- */
- mod->klp_info->sechdrs[symndx].sh_addr = \
- (unsigned long) mod->core_kallsyms.symtab;
-
- return 0;
-
-free_sechdrs:
- kfree(mod->klp_info->sechdrs);
-free_info:
- kfree(mod->klp_info);
- return ret;
-}
-
-static void free_module_elf(struct module *mod)
-{
- kfree(mod->klp_info->sechdrs);
- kfree(mod->klp_info->secstrings);
- kfree(mod->klp_info);
-}
-#else /* !CONFIG_LIVEPATCH */
-static int copy_module_elf(struct module *mod, struct load_info *info)
-{
- return 0;
-}
-
-static void free_module_elf(struct module *mod)
-{
-}
-#endif /* CONFIG_LIVEPATCH */
-
void __weak module_memfree(void *module_region)
{
/*
@@ -2244,9 +2168,6 @@ static void free_module(struct module *mod)
/* Free any allocated parameters. */
destroy_params(mod->kp, mod->num_kp);
- if (is_livepatch_module(mod))
- free_module_elf(mod);
-
/* Now we can delete it from the lists */
mutex_lock(&module_mutex);
/* Unlink carefully: kallsyms could be walking list. */
@@ -3967,12 +3888,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
if (err < 0)
goto coming_cleanup;
- if (is_livepatch_module(mod)) {
- err = copy_module_elf(mod, info);
- if (err < 0)
- goto sysfs_cleanup;
- }
-
/* Get rid of temporary copy. */
free_copy(info);
@@ -3981,8 +3896,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
return do_init_module(mod);
- sysfs_cleanup:
- mod_sysfs_teardown(mod);
coming_cleanup:
mod->state = MODULE_STATE_GOING;
destroy_params(mod->kp, mod->num_kp);
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [POC 23/23] module: Remove obsolete module_disable_ro()
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
` (21 preceding siblings ...)
2020-01-17 15:03 ` [POC 22/23] livepatch/module: Remove obsolete copy_module_elf() Petr Mladek
@ 2020-01-17 15:03 ` Petr Mladek
22 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-17 15:03 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
The split livepatch modules are relocated immediately during the module
load. There is no longer needed to disable the RO protection. The function
can be finally remove because livepatching was the only user (sinner).
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
include/linux/module.h | 2 --
kernel/module.c | 13 -------------
2 files changed, 15 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 8545f3087274..5c9e661ac3bc 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -854,12 +854,10 @@ extern int module_sysfs_initialized;
extern void set_all_modules_text_rw(void);
extern void set_all_modules_text_ro(void);
extern void module_enable_ro(const struct module *mod, bool after_init);
-extern void module_disable_ro(const struct module *mod);
#else
static inline void set_all_modules_text_rw(void) { }
static inline void set_all_modules_text_ro(void) { }
static inline void module_enable_ro(const struct module *mod, bool after_init) { }
-static inline void module_disable_ro(const struct module *mod) { }
#endif
#ifdef CONFIG_GENERIC_BUG
diff --git a/kernel/module.c b/kernel/module.c
index 442926fc5f34..d435bad80d7d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2030,19 +2030,6 @@ static void frob_writable_data(const struct module_layout *layout,
(layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
}
-/* livepatching wants to disable read-only so it can frob module. */
-void module_disable_ro(const struct module *mod)
-{
- if (!rodata_enabled)
- return;
-
- frob_text(&mod->core_layout, set_memory_rw);
- frob_rodata(&mod->core_layout, set_memory_rw);
- frob_ro_after_init(&mod->core_layout, set_memory_rw);
- frob_text(&mod->init_layout, set_memory_rw);
- frob_rodata(&mod->init_layout, set_memory_rw);
-}
-
void module_enable_ro(const struct module *mod, bool after_init)
{
if (!rodata_enabled)
--
2.16.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [POC 20/23] module/livepatch: Relocate local variables in the module loaded when the livepatch is being loaded
2020-01-17 15:03 ` [POC 20/23] module/livepatch: Relocate local variables in the module loaded when the livepatch is being loaded Petr Mladek
@ 2020-01-18 10:29 ` kbuild test robot
2020-04-03 18:00 ` Joe Lawrence
1 sibling, 0 replies; 38+ messages in thread
From: kbuild test robot @ 2020-01-18 10:29 UTC (permalink / raw)
To: Petr Mladek
Cc: kbuild-all, Jiri Kosina, Josh Poimboeuf, Miroslav Benes,
Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel, Petr Mladek
[-- Attachment #1: Type: text/plain, Size: 3002 bytes --]
Hi Petr,
I love your patch! Yet something to improve:
[auto build test ERROR on jeyu/modules-next]
[also build test ERROR on kselftest/next tip/x86/core linus/master v5.5-rc6 next-20200117]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Petr-Mladek/livepatch-Split-livepatch-module-per-object/20200118-090135
base: https://git.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git modules-next
config: mips-32r2_defconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
kernel/module.c: In function 'apply_relocations':
>> kernel/module.c:2418:10: error: implicit declaration of function 'klp_resolve_symbols'; did you mean 'resolve_symbol'? [-Werror=implicit-function-declaration]
err = klp_resolve_symbols(info->sechdrs, i, mod);
^~~~~~~~~~~~~~~~~~~
resolve_symbol
cc1: some warnings being treated as errors
vim +2418 kernel/module.c
2398
2399 static int apply_relocations(struct module *mod, const struct load_info *info)
2400 {
2401 unsigned int i;
2402 int err = 0;
2403
2404 /* Now do relocations. */
2405 for (i = 1; i < info->hdr->e_shnum; i++) {
2406 unsigned int infosec = info->sechdrs[i].sh_info;
2407
2408 /* Not a valid relocation section? */
2409 if (infosec >= info->hdr->e_shnum)
2410 continue;
2411
2412 /* Don't bother with non-allocated sections */
2413 if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
2414 continue;
2415
2416 /* Livepatch need to resolve static symbols. */
2417 if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH) {
> 2418 err = klp_resolve_symbols(info->sechdrs, i, mod);
2419 if (err < 0)
2420 break;
2421 err = apply_relocate_add(info->sechdrs, info->strtab,
2422 info->index.sym, i, mod);
2423 } else if (info->sechdrs[i].sh_type == SHT_REL) {
2424 err = apply_relocate(info->sechdrs, info->strtab,
2425 info->index.sym, i, mod);
2426 } else if (info->sechdrs[i].sh_type == SHT_RELA) {
2427 err = apply_relocate_add(info->sechdrs, info->strtab,
2428 info->index.sym, i, mod);
2429 }
2430 if (err < 0)
2431 break;
2432 }
2433 return err;
2434 }
2435
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 22346 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 01/23] module: Allow to delete module also from inside kernel
2020-01-17 15:03 ` [POC 01/23] module: Allow to delete module also from inside kernel Petr Mladek
@ 2020-01-21 11:11 ` Julien Thierry
0 siblings, 0 replies; 38+ messages in thread
From: Julien Thierry @ 2020-01-21 11:11 UTC (permalink / raw)
To: Petr Mladek, Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel
Hi Petr,
On 1/17/20 3:03 PM, Petr Mladek wrote:
> Livepatch subsystems will need to automatically load and delete
> livepatch module when the livepatched one is being removed
> or when the entire livepatch is being removed.
>
> The code stopping the kernel module is moved to separate function
> so that it can be reused.
>
> The function always have rights to do the action. Also it does not
> need to search for struct module because it is already passed as
> a parameter.
>
> On the other hand, it has to make sure that the given struct module
> can't be released in parallel. It is achieved by combining module_put()
> and module_delete() functionality in a single function.
>
> This patch does not change the existing behavior of delete_module
> syscall.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
>
> module: Add module_put_and_delete()
> ---
> include/linux/module.h | 5 +++
> kernel/module.c | 119 +++++++++++++++++++++++++++++++------------------
> 2 files changed, 80 insertions(+), 44 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index bd165ba68617..f69f3fd72dd5 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -623,6 +623,7 @@ extern void __module_get(struct module *module);
> extern bool try_module_get(struct module *module);
>
> extern void module_put(struct module *module);
> +extern int module_put_and_delete(struct module *mod);
>
> #else /*!CONFIG_MODULE_UNLOAD*/
> static inline bool try_module_get(struct module *module)
> @@ -632,6 +633,10 @@ static inline bool try_module_get(struct module *module)
> static inline void module_put(struct module *module)
> {
> }
> +static inline int module_put_and_delete(struct module *mod)
> +{
> + return 0;
> +}
> static inline void __module_get(struct module *module)
> {
> }
> diff --git a/kernel/module.c b/kernel/module.c
> index b56f3224b161..b3f11524f8f9 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -964,62 +964,36 @@ EXPORT_SYMBOL(module_refcount);
> /* This exists whether we can unload or not */
> static void free_module(struct module *mod);
>
> -SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> - unsigned int, flags)
> +int stop_module(struct module *mod, int flags)
> {
> - struct module *mod;
> - char name[MODULE_NAME_LEN];
> - int ret, forced = 0;
> -
> - if (!capable(CAP_SYS_MODULE) || modules_disabled)
> - return -EPERM;
> -
> - if (strncpy_from_user(name, name_user, MODULE_NAME_LEN-1) < 0)
> - return -EFAULT;
> - name[MODULE_NAME_LEN-1] = '\0';
> + int forced = 0;
>
> - audit_log_kern_module(name);
> -
> - if (mutex_lock_interruptible(&module_mutex) != 0)
> - return -EINTR;
> -
> - mod = find_module(name);
> - if (!mod) {
> - ret = -ENOENT;
> - goto out;
> - }
> -
> - if (!list_empty(&mod->source_list)) {
> - /* Other modules depend on us: get rid of them first. */
> - ret = -EWOULDBLOCK;
> - goto out;
> - }
> + /* Other modules depend on us: get rid of them first. */
> + if (!list_empty(&mod->source_list))
> + return -EWOULDBLOCK;
>
> /* Doing init or already dying? */
> if (mod->state != MODULE_STATE_LIVE) {
> /* FIXME: if (force), slam module count damn the torpedoes */
> pr_debug("%s already dying\n", mod->name);
> - ret = -EBUSY;
> - goto out;
> + return -EBUSY;
> }
>
> /* If it has an init func, it must have an exit func to unload */
> if (mod->init && !mod->exit) {
> forced = try_force_unload(flags);
> - if (!forced) {
> - /* This module can't be removed */
> - ret = -EBUSY;
> - goto out;
> - }
> + /* This module can't be removed */
> + if (!forced)
> + return -EBUSY;
> }
>
> /* Stop the machine so refcounts can't move and disable module. */
> - ret = try_stop_module(mod, flags, &forced);
> - if (ret != 0)
> - goto out;
> + return try_stop_module(mod, flags, &forced);
> +}
>
> - mutex_unlock(&module_mutex);
> - /* Final destruction now no one is using it. */
> +/* Final destruction now no one is using it. */
Nit: Looks like some copy/paste mixup
> +static void destruct_module(struct module *mod)
> +{
> if (mod->exit != NULL)
> mod->exit();
> blocking_notifier_call_chain(&module_notify_list,
> @@ -1033,8 +1007,44 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
>
> free_module(mod);
> +
> /* someone could wait for the module in add_unformed_module() */
> wake_up_all(&module_wq);
> +}
> +
> +SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> + unsigned int, flags)
> +{
> + struct module *mod;
> + char name[MODULE_NAME_LEN];
> + int ret;
> +
> + if (!capable(CAP_SYS_MODULE) || modules_disabled)
> + return -EPERM;
> +
> + if (strncpy_from_user(name, name_user, MODULE_NAME_LEN-1) < 0)
> + return -EFAULT;
> + name[MODULE_NAME_LEN-1] = '\0';
> +
> + audit_log_kern_module(name);
> +
> + if (mutex_lock_interruptible(&module_mutex) != 0)
> + return -EINTR;
> +
> + mod = find_module(name);
> + if (!mod) {
> + ret = -ENOENT;
> + goto out;
> + }
> +
> + ret = stop_module(mod, flags);
> + if (ret)
> + goto out;
> +
> + mutex_unlock(&module_mutex);
> +
> +/* Final destruction now no one is using it. */
> + destruct_module(mod);
> return 0;
> out:
> mutex_unlock(&module_mutex);
> @@ -1138,20 +1148,41 @@ bool try_module_get(struct module *module)
> }
> EXPORT_SYMBOL(try_module_get);
>
> -void module_put(struct module *module)
> +/* Must be called under module_mutex or with preemtion disabled */
Might be worth adding some asserts to check for that.
> +static void __module_put(struct module* module)
> {
> int ret;
>
> + ret = atomic_dec_if_positive(&module->refcnt);
> + WARN_ON(ret < 0); /* Failed to put refcount */
> + trace_module_put(module, _RET_IP_);
> +}
> +
> +void module_put(struct module *module)
> +{
> if (module) {
> preempt_disable();
> - ret = atomic_dec_if_positive(&module->refcnt);
> - WARN_ON(ret < 0); /* Failed to put refcount */
> - trace_module_put(module, _RET_IP_);
> + __module_put(module);
> preempt_enable();
> }
> }
> EXPORT_SYMBOL(module_put);
>
> +int module_put_and_delete(struct module *mod)
> +{
> + int ret;
> + mutex_lock(&module_mutex);
> + __module_put(mod);
> + ret = stop_module(mod, 0);
> + mutex_unlock(&module_mutex);
> +
> + if (ret)
> + return ret;
> +
> + destruct_module(mod);
> + return 0;
> +}
> +
> #else /* !CONFIG_MODULE_UNLOAD */
> static inline void print_unload_info(struct seq_file *m, struct module *mod)
> {
>
Thanks,
--
Julien Thierry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 02/23] livepatch: Split livepatch modules per livepatched object
2020-01-17 15:03 ` [POC 02/23] livepatch: Split livepatch modules per livepatched object Petr Mladek
@ 2020-01-21 11:11 ` Julien Thierry
2020-01-28 12:16 ` Petr Mladek
0 siblings, 1 reply; 38+ messages in thread
From: Julien Thierry @ 2020-01-21 11:11 UTC (permalink / raw)
To: Petr Mladek, Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel
Hi Petr,
On 1/17/20 3:03 PM, Petr Mladek wrote:
> One livepatch module allows to fix vmlinux and any number of modules
> while providing some guarantees defined by the consistency model.
>
> The patched modules can be loaded at any time: before, during,
> or after the livepatch module gets loaded. They can even get
> removed and loaded again. This variety of scenarios bring some
> troubles. For example, some livepatch symbols could be relocated
> only after the related module gets loaded. These changes need
> to get cleared when the module gets unloaded so that it can
> get eventually loaded again.
>
> As a result some functionality needs to be duplicated by
> the livepatching code. Some elf sections need to be preserved
> even when they normally can be removed during the module load.
> Architecture specific code is involved which makes harder
> adding support for new architectures and the maintainace.
>
> The solution is to split the livepatch module per livepatched
> object (vmlinux or module). Then both livepatch module and
> the livepatched modules could get loaded and removed at the
> same time.
>
> This require many changes in the livepatch subsystem, module
> loader, sample livepatches and livepatches needed for selftests.
>
> The bad news is that bisection will not work by definition.
> The good news is that it allows to do the changes in smaller
> steps.
>
> The first step allows to split the existing sample and testing
> modules so that they can be later user. It is done by
> the following changes:
>
> 1. struct klp_patch:
>
> + Add "patch_name" and "obj_names" to match all the related
> livepatch modules.
>
> + Replace "objs" array with a pointer to a single struct object.
>
> + move "mod" to struct object.
>
> 2. struct klp_object:
>
> + Add "patch_name" to match all the related livepatch modules.
>
> + "mod" points to the livepatch module instead of the livepatched
> one. The pointer to the livepatched module was used only to
> detect whether it was loaded. It will be always loaded
> with related livepatch module now.
>
> 3. klp_find_object_module() and klp_is_object_loaded() are no longer
> needed. Livepatch module is loaded only when the related livepatched
> module is loaded.
>
> 4. Add klp_add_object() function that will need to initialize
> struct object, link it into the related struct klp_patch,
> and patch the functions. It will get implemented later.
>
> The livepatches for modules are put into separate source files
> that define only struct klp_object() and call the new klp_add_object()
> in the init() callback. The name of the module follows the pattern:
>
> <patch_name>__<object_name>
>
Is that a requirement? Or is it just the convention followed for the
current tests?
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> arch/x86/kernel/livepatch.c | 5 +-
> include/linux/livepatch.h | 20 +--
> kernel/livepatch/core.c | 139 +++++++-----------
> kernel/livepatch/core.h | 5 -
> kernel/livepatch/transition.c | 14 +-
> lib/livepatch/Makefile | 2 +
> lib/livepatch/test_klp_atomic_replace.c | 18 ++-
> lib/livepatch/test_klp_callbacks_demo.c | 90 ++++++------
> lib/livepatch/test_klp_callbacks_demo.h | 11 ++
> lib/livepatch/test_klp_callbacks_demo2.c | 62 ++++++---
> lib/livepatch/test_klp_callbacks_demo2.h | 11 ++
> ...t_klp_callbacks_demo__test_klp_callbacks_busy.c | 50 +++++++
> ...st_klp_callbacks_demo__test_klp_callbacks_mod.c | 42 ++++++
> lib/livepatch/test_klp_livepatch.c | 18 ++-
> lib/livepatch/test_klp_state.c | 53 ++++---
> lib/livepatch/test_klp_state2.c | 53 ++++---
> samples/livepatch/Makefile | 4 +
> samples/livepatch/livepatch-callbacks-demo.c | 90 ++++++------
> samples/livepatch/livepatch-callbacks-demo.h | 11 ++
> ...h-callbacks-demo__livepatch-callbacks-busymod.c | 54 +++++++
> ...patch-callbacks-demo__livepatch-callbacks-mod.c | 46 ++++++
> samples/livepatch/livepatch-sample.c | 18 ++-
> samples/livepatch/livepatch-shadow-fix1.c | 120 ++--------------
> .../livepatch-shadow-fix1__livepatch-shadow-mod.c | 155 +++++++++++++++++++++
> samples/livepatch/livepatch-shadow-fix2.c | 92 ++----------
> .../livepatch-shadow-fix2__livepatch-shadow-mod.c | 127 +++++++++++++++++
> .../testing/selftests/livepatch/test-callbacks.sh | 16 +--
> 27 files changed, 841 insertions(+), 485 deletions(-)
> create mode 100644 lib/livepatch/test_klp_callbacks_demo.h
> create mode 100644 lib/livepatch/test_klp_callbacks_demo2.h
> create mode 100644 lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_busy.c
> create mode 100644 lib/livepatch/test_klp_callbacks_demo__test_klp_callbacks_mod.c
> create mode 100644 samples/livepatch/livepatch-callbacks-demo.h
> create mode 100644 samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-busymod.c
> create mode 100644 samples/livepatch/livepatch-callbacks-demo__livepatch-callbacks-mod.c
> create mode 100644 samples/livepatch/livepatch-shadow-fix1__livepatch-shadow-mod.c
> create mode 100644 samples/livepatch/livepatch-shadow-fix2__livepatch-shadow-mod.c
>
> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> index 6a68e41206e7..728b44eaa168 100644
> --- a/arch/x86/kernel/livepatch.c
> +++ b/arch/x86/kernel/livepatch.c
> @@ -9,8 +9,7 @@
> #include <asm/text-patching.h>
>
> /* Apply per-object alternatives. Based on x86 module_finalize() */
> -void arch_klp_init_object_loaded(struct klp_patch *patch,
> - struct klp_object *obj)
> +void arch_klp_init_object_loaded(struct klp_object *obj)
> {
> int cnt;
> struct klp_modinfo *info;
> @@ -20,7 +19,7 @@ void arch_klp_init_object_loaded(struct klp_patch *patch,
> char sec_objname[MODULE_NAME_LEN];
> char secname[KSYM_NAME_LEN];
>
> - info = patch->mod->klp_info;
> + info = obj->mod->klp_info;
> objname = obj->name ? obj->name : "vmlinux";
>
> /* See livepatch core code for BUILD_BUG_ON() explanation */
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index e894e74905f3..a4567c17a9f2 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -105,19 +105,21 @@ struct klp_callbacks {
> /**
> * struct klp_object - kernel object structure for live patching
> * @name: module name (or NULL for vmlinux)
> + * @patch_name: module name for vmlinux
> + * @mod: reference to the live patch module for this object
> * @funcs: function entries for functions to be patched in the object
> * @callbacks: functions to be executed pre/post (un)patching
> * @kobj: kobject for sysfs resources
> * @func_list: dynamic list of the function entries
> * @node: list node for klp_patch obj_list
> - * @mod: kernel module associated with the patched object
> - * (NULL for vmlinux)
> * @dynamic: temporary object for nop functions; dynamically allocated
> * @patched: the object's funcs have been added to the klp_ops list
> */
> struct klp_object {
> /* external */
> const char *name;
> + const char *patch_name;
> + struct module *mod;
> struct klp_func *funcs;
> struct klp_callbacks callbacks;
>
> @@ -125,7 +127,6 @@ struct klp_object {
> struct kobject kobj;
> struct list_head func_list;
> struct list_head node;
> - struct module *mod;
> bool dynamic;
> bool patched;
> };
> @@ -144,8 +145,9 @@ struct klp_state {
>
> /**
> * struct klp_patch - patch structure for live patching
> - * @mod: reference to the live patch module
> - * @objs: object entries for kernel objects to be patched
> + * @patch_name: livepatch name; same for related livepatch against other objects
You forgot to add that to the structure.
> + * @objs: object entry for vmlinux object
Nit: s/objs/obj/
> + * @obj_names: names of modules synchronously livepatched with this patch
Not sure I understand the purpose of this. Is it to check that the
klp_object that will get linked to this patch are part of a
pre-established set?
> * @states: system states that can get modified
> * @replace: replace all actively used patches
> * @list: list node for global list of actively used patches
> @@ -158,9 +160,9 @@ struct klp_state {
> */
> struct klp_patch {
> /* external */
> - struct module *mod;
> - struct klp_object *objs;
> struct klp_state *states;
> + struct klp_object *obj;
> + char **obj_names;
> bool replace;
>
> /* internal */
> @@ -194,9 +196,9 @@ struct klp_patch {
> list_for_each_entry(func, &obj->func_list, node)
>
> int klp_enable_patch(struct klp_patch *);
> +int klp_add_object(struct klp_object *);
>
> -void arch_klp_init_object_loaded(struct klp_patch *patch,
> - struct klp_object *obj);
> +void arch_klp_init_object_loaded(struct klp_object *obj);
>
> /* Called from the module loader during module coming/going states */
> int klp_module_coming(struct module *mod);
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index c3512e7e0801..bb62c5407b75 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -49,34 +49,6 @@ static bool klp_is_module(struct klp_object *obj)
> return obj->name;
> }
>
> -/* 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 do not want to block removal of patched modules and therefore
> - * we do not take a reference here. The patches are removed by
> - * klp_module_going() instead.
> - */
> - mod = find_module(obj->name);
> - /*
> - * Do not mess work of klp_module_coming() and klp_module_going().
> - * Note that the patch might still be needed before klp_module_going()
> - * is called. Module functions can be called even in the GOING state
> - * until mod->exit() finishes. This is especially important for
> - * patches that modify semantic of the functions.
> - */
> - if (mod && mod->klp_alive)
> - obj->mod = mod;
> -
> - mutex_unlock(&module_mutex);
> -}
> -
> static bool klp_initialized(void)
> {
> return !!klp_root_kobj;
> @@ -246,18 +218,16 @@ static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
> return 0;
> }
>
> -static int klp_write_object_relocations(struct module *pmod,
> - struct klp_object *obj)
> +static int klp_write_object_relocations(struct klp_object *obj)
> {
> int i, cnt, ret = 0;
> const char *objname, *secname;
> char sec_objname[MODULE_NAME_LEN];
> + struct module *pmod;
> Elf_Shdr *sec;
>
> - if (WARN_ON(!klp_is_object_loaded(obj)))
> - return -EINVAL;
> -
> objname = klp_is_module(obj) ? obj->name : "vmlinux";
> + pmod = obj->mod;
>
> /* For each klp relocation section */
> for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> @@ -419,8 +389,8 @@ static void klp_free_object_dynamic(struct klp_object *obj)
>
> static void klp_init_func_early(struct klp_object *obj,
> struct klp_func *func);
> -static void klp_init_object_early(struct klp_patch *patch,
> - struct klp_object *obj);
> +static int klp_init_object_early(struct klp_patch *patch,
> + struct klp_object *obj);
>
> static struct klp_object *klp_alloc_object_dynamic(const char *name,
> struct klp_patch *patch)
> @@ -662,7 +632,7 @@ static void klp_free_patch_finish(struct klp_patch *patch)
>
> /* Put the module after the last access to struct klp_patch. */
> if (!patch->forced)
> - module_put(patch->mod);
> + module_put(patch->obj->mod);
> }
>
> /*
> @@ -725,30 +695,28 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> }
>
> /* Arches may override this to finish any remaining arch-specific tasks */
> -void __weak arch_klp_init_object_loaded(struct klp_patch *patch,
> - struct klp_object *obj)
> +void __weak arch_klp_init_object_loaded(struct klp_object *obj)
> {
> }
>
> /* parts of the initialization that is done only when the object is loaded */
> -static int klp_init_object_loaded(struct klp_patch *patch,
> - struct klp_object *obj)
> +static int klp_init_object_loaded(struct klp_object *obj)
> {
> struct klp_func *func;
> int ret;
>
> mutex_lock(&text_mutex);
>
> - module_disable_ro(patch->mod);
> - ret = klp_write_object_relocations(patch->mod, obj);
> + module_disable_ro(obj->mod);
> + ret = klp_write_object_relocations(obj);
> if (ret) {
> - module_enable_ro(patch->mod, true);
> + module_enable_ro(obj->mod, true);
> mutex_unlock(&text_mutex);
> return ret;
> }
>
> - arch_klp_init_object_loaded(patch, obj);
> - module_enable_ro(patch->mod, true);
> + arch_klp_init_object_loaded(obj);
> + module_enable_ro(obj->mod, true);
>
> mutex_unlock(&text_mutex);
>
> @@ -792,11 +760,8 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> return -EINVAL;
>
> obj->patched = false;
> - obj->mod = NULL;
>
> - klp_find_object_module(obj);
> -
> - name = klp_is_module(obj) ? obj->name : "vmlinux";
> + name = obj->name ? obj->name : "vmlinux";
> ret = kobject_add(&obj->kobj, &patch->kobj, "%s", name);
> if (ret)
> return ret;
> @@ -807,8 +772,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> return ret;
> }
>
> - if (klp_is_object_loaded(obj))
> - ret = klp_init_object_loaded(patch, obj);
> + ret = klp_init_object_loaded(obj);
>
> return ret;
> }
> @@ -820,20 +784,34 @@ static void klp_init_func_early(struct klp_object *obj,
> list_add_tail(&func->node, &obj->func_list);
> }
>
> -static void klp_init_object_early(struct klp_patch *patch,
> +static int klp_init_object_early(struct klp_patch *patch,
> struct klp_object *obj)
> {
> + struct klp_func *func;
> +
> + if (!obj->funcs)
> + return -EINVAL;
> +
> INIT_LIST_HEAD(&obj->func_list);
> kobject_init(&obj->kobj, &klp_ktype_object);
> list_add_tail(&obj->node, &patch->obj_list);
> +
> + klp_for_each_func_static(obj, func) {
> + klp_init_func_early(obj, func);
> + }
> +
> + if (obj->dynamic || try_module_get(obj->mod))
> + return 0;
> +
> + return -ENODEV;
> }
>
> static int klp_init_patch_early(struct klp_patch *patch)
> {
> - struct klp_object *obj;
> - struct klp_func *func;
> + struct klp_object *obj = patch->obj;
>
> - if (!patch->objs)
> + /* Main patch module is always for vmlinux */
> + if (obj->name)
> return -EINVAL;
>
> INIT_LIST_HEAD(&patch->list);
> @@ -844,21 +822,7 @@ static int klp_init_patch_early(struct klp_patch *patch)
> INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
> init_completion(&patch->finish);
>
> - klp_for_each_object_static(patch, obj) {
I think we can get rid of klp_for_each_object_static(), no? Now the
klp_patch is only associated to a single klp_object, so everything will
be dynamic. Is this correct?
> - if (!obj->funcs)
> - return -EINVAL;
> -
> - klp_init_object_early(patch, obj);
> -
> - klp_for_each_func_static(obj, func) {
> - klp_init_func_early(obj, func);
> - }
> - }
> -
> - if (!try_module_get(patch->mod))
> - return -ENODEV;
> -
> - return 0;
> + return klp_init_object_early(patch, obj);
> }
>
> static int klp_init_patch(struct klp_patch *patch)
> @@ -866,7 +830,7 @@ static int klp_init_patch(struct klp_patch *patch)
> struct klp_object *obj;
> int ret;
>
> - ret = kobject_add(&patch->kobj, klp_root_kobj, "%s", patch->mod->name);
> + ret = kobject_add(&patch->kobj, klp_root_kobj, "%s", patch->obj->mod->name);
> if (ret)
> return ret;
>
> @@ -887,6 +851,12 @@ static int klp_init_patch(struct klp_patch *patch)
> return 0;
> }
>
> +int klp_add_object(struct klp_object *obj)
> +{
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(klp_add_object);
> +
> static int __klp_disable_patch(struct klp_patch *patch)
> {
> struct klp_object *obj;
> @@ -930,7 +900,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
> if (WARN_ON(patch->enabled))
> return -EINVAL;
>
> - pr_notice("enabling patch '%s'\n", patch->mod->name);
> + pr_notice("enabling patch '%s'\n", patch->obj->patch_name);
>
> klp_init_transition(patch, KLP_PATCHED);
>
> @@ -944,9 +914,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
> smp_wmb();
>
> klp_for_each_object(patch, obj) {
> - if (!klp_is_object_loaded(obj))
> - continue;
> -
> ret = klp_pre_patch_callback(obj);
> if (ret) {
> pr_warn("pre-patch callback failed for object '%s'\n",
> @@ -968,7 +935,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
>
> return 0;
> err:
> - pr_warn("failed to enable patch '%s'\n", patch->mod->name);
> + pr_warn("failed to enable patch '%s'\n", patch->obj->patch_name);
>
> klp_cancel_transition();
> return ret;
> @@ -991,12 +958,12 @@ int klp_enable_patch(struct klp_patch *patch)
> {
> int ret;
>
> - if (!patch || !patch->mod)
> + if (!patch || !patch->obj || !patch->obj->mod)
> return -EINVAL;
>
> - if (!is_livepatch_module(patch->mod)) {
> + if (!is_livepatch_module(patch->obj->mod)) {
> pr_err("module %s is not marked as a livepatch module\n",
> - patch->mod->name);
> + patch->obj->patch_name);
Shouldn't that be "patch->obj->mod->name" ?
> return -EINVAL;
> }
>
> @@ -1012,7 +979,7 @@ int klp_enable_patch(struct klp_patch *patch)
>
> if (!klp_is_patch_compatible(patch)) {
> pr_err("Livepatch patch (%s) is not compatible with the already installed livepatches.\n",
> - patch->mod->name);
> + patch->obj->mod->name);
> mutex_unlock(&klp_mutex);
> return -EINVAL;
> }
> @@ -1119,7 +1086,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
> klp_pre_unpatch_callback(obj);
>
> pr_notice("reverting patch '%s' on unloading module '%s'\n",
> - patch->mod->name, obj->mod->name);
> + patch->obj->patch_name, obj->name);
> klp_unpatch_object(obj);
>
> klp_post_unpatch_callback(obj);
> @@ -1154,15 +1121,15 @@ int klp_module_coming(struct module *mod)
>
> obj->mod = mod;
>
> - ret = klp_init_object_loaded(patch, obj);
> + ret = klp_init_object_loaded(obj);
> if (ret) {
> pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> - patch->mod->name, obj->mod->name, ret);
> + patch->obj->patch_name, obj->name, ret);
> goto err;
> }
>
> pr_notice("applying patch '%s' to loading module '%s'\n",
> - patch->mod->name, obj->mod->name);
> + patch->obj->patch_name, obj->name);
>
> ret = klp_pre_patch_callback(obj);
> if (ret) {
> @@ -1174,7 +1141,7 @@ int klp_module_coming(struct module *mod)
> ret = klp_patch_object(obj);
> if (ret) {
> pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> - patch->mod->name, obj->mod->name, ret);
> + patch->obj->patch_name, obj->name, ret);
>
> klp_post_unpatch_callback(obj);
> goto err;
> @@ -1197,7 +1164,7 @@ int klp_module_coming(struct module *mod)
> * error to the module loader.
> */
> pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
> - patch->mod->name, obj->mod->name, obj->mod->name);
> + patch->obj->patch_name, obj->name, obj->name);
> mod->klp_alive = false;
> obj->mod = NULL;
> klp_cleanup_module_patches_limited(mod, patch);
> diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
> index 38209c7361b6..01980cc0509b 100644
> --- a/kernel/livepatch/core.h
> +++ b/kernel/livepatch/core.h
> @@ -18,11 +18,6 @@ void klp_free_replaced_patches_async(struct klp_patch *new_patch);
> void klp_unpatch_replaced_patches(struct klp_patch *new_patch);
> void klp_discard_nops(struct klp_patch *new_patch);
>
> -static inline bool klp_is_object_loaded(struct klp_object *obj)
> -{
> - return !obj->name || obj->mod;
> -}
> -
> static inline int klp_pre_patch_callback(struct klp_object *obj)
> {
> int ret = 0;
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index f6310f848f34..78e3280560cd 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -74,7 +74,7 @@ static void klp_complete_transition(void)
> unsigned int cpu;
>
> pr_debug("'%s': completing %s transition\n",
> - klp_transition_patch->mod->name,
> + klp_transition_patch->obj->patch_name,
> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
> @@ -120,15 +120,13 @@ static void klp_complete_transition(void)
> }
>
> klp_for_each_object(klp_transition_patch, obj) {
> - if (!klp_is_object_loaded(obj))
> - continue;
> if (klp_target_state == KLP_PATCHED)
> klp_post_patch_callback(obj);
> else if (klp_target_state == KLP_UNPATCHED)
> klp_post_unpatch_callback(obj);
> }
>
> - pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
> + pr_notice("'%s': %s complete\n", klp_transition_patch->obj->patch_name,
> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> klp_target_state = KLP_UNDEFINED;
> @@ -147,7 +145,7 @@ void klp_cancel_transition(void)
> return;
>
> pr_debug("'%s': canceling patching transition, going to unpatch\n",
> - klp_transition_patch->mod->name);
> + klp_transition_patch->obj->patch_name);
>
> klp_target_state = KLP_UNPATCHED;
> klp_complete_transition();
> @@ -468,7 +466,7 @@ void klp_start_transition(void)
> WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
>
> pr_notice("'%s': starting %s transition\n",
> - klp_transition_patch->mod->name,
> + klp_transition_patch->obj->patch_name,
Isn't the transition per livepatched module rather than per-patch now?
If so, would it make more sense to display also the name of the module
being patched/unpatched?
> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> /*
> @@ -519,7 +517,7 @@ void klp_init_transition(struct klp_patch *patch, int state)
> */
> klp_target_state = state;
>
> - pr_debug("'%s': initializing %s transition\n", patch->mod->name,
> + pr_debug("'%s': initializing %s transition\n", patch->obj->patch_name,
Ditto.
> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> /*
> @@ -581,7 +579,7 @@ void klp_reverse_transition(void)
> struct task_struct *g, *task;
>
> pr_debug("'%s': reversing transition from %s\n",
> - klp_transition_patch->mod->name,
> + klp_transition_patch->obj->patch_name,
Ditto.
> klp_target_state == KLP_PATCHED ? "patching to unpatching" :
> "unpatching to patching");
>
[...]
Cheers,
--
Julien Thierry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 03/23] livepatch: Better checks of struct klp_object definition
2020-01-17 15:03 ` [POC 03/23] livepatch: Better checks of struct klp_object definition Petr Mladek
@ 2020-01-21 11:27 ` Julien Thierry
0 siblings, 0 replies; 38+ messages in thread
From: Julien Thierry @ 2020-01-21 11:27 UTC (permalink / raw)
To: Petr Mladek, Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel
Hi Petr,
On 1/17/20 3:03 PM, Petr Mladek wrote:
> The number of user defined fields increased in struct klp_object after
> spliting per-object livepatches. It was sometimes unclear why exactly
> the module could not get loded when returned -EINVAL.
>
> Add more checks for the split modules and write useful error
> messages on particular errors.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> kernel/livepatch/core.c | 91 ++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 82 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index bb62c5407b75..ec7ffc7db3a7 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -756,9 +756,6 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> int ret;
> const char *name;
>
> - if (klp_is_module(obj) && strlen(obj->name) >= MODULE_NAME_LEN)
> - return -EINVAL;
> -
> obj->patched = false;
>
> name = obj->name ? obj->name : "vmlinux";
> @@ -851,8 +848,86 @@ static int klp_init_patch(struct klp_patch *patch)
> return 0;
> }
>
> +static int klp_check_module_name(struct klp_object *obj, bool is_module)
> +{
> + char mod_name[MODULE_NAME_LEN];
> + const char *expected_name;
> +
> + if (is_module) {
> + snprintf(mod_name, sizeof(mod_name), "%s__%s",
> + obj->patch_name, obj->name);
> + expected_name = mod_name;
> + } else {
> + expected_name = obj->patch_name;
> + }
> +
> + if (strcmp(expected_name, obj->mod->name)) {
I'm not sure I understand the point of enforcing this.
> + pr_err("The module name %s does not match with obj->patch_name and obj->name. The expected name is: %s\n",
> + obj->mod->name, expected_name);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int klp_check_object(struct klp_object *obj, bool is_module)
> +{
> +
> + if (!obj->mod)
> + return -EINVAL;
> +
> + if (!is_livepatch_module(obj->mod)) {
> + pr_err("module %s is not marked as a livepatch module\n",
> + obj->mod->name);
> + return -EINVAL;
> + }
> +
> + if (!obj->patch_name) {
> + pr_err("module %s does not have set obj->patch_name\n",
> + obj->mod->name);
> + return -EINVAL;
> + }
> +
> + if (strlen(obj->patch_name) >= MODULE_NAME_LEN) {
> + pr_err("module %s has too long obj->patch_name\n",
> + obj->mod->name);
> + return -EINVAL;
> + }
> +
> + if (is_module) {
> + if (!obj->name) {
> + pr_err("module %s does not have set obj->name\n",
> + obj->mod->name);
> + return -EINVAL;
> + }
> + if (strlen(obj->name) >= MODULE_NAME_LEN) {
> + pr_err("module %s has too long obj->name\n",
> + obj->mod->name);
> + return -EINVAL;
> + }
> + } else if (obj->name) {
> + pr_err("module %s for vmlinux must not have set obj->name\n",
> + obj->mod->name);
> + return -EINVAL;
> + }
> +
> + if (!obj->funcs) {
> + pr_err("module %s does not have set obj->funcs\n",
> + obj->mod->name);
> + return -EINVAL;
> + }
> +
> + return klp_check_module_name(obj, is_module);
> +}
> +
> int klp_add_object(struct klp_object *obj)
> {
> + int ret;
> +
> + ret = klp_check_object(obj, true);
> + if (ret)
> + return ret;
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(klp_add_object);
> @@ -958,14 +1033,12 @@ int klp_enable_patch(struct klp_patch *patch)
> {
> int ret;
>
> - if (!patch || !patch->obj || !patch->obj->mod)
> + if (!patch || !patch->obj)
> return -EINVAL;
>
> - if (!is_livepatch_module(patch->obj->mod)) {
> - pr_err("module %s is not marked as a livepatch module\n",
> - patch->obj->patch_name);
> - return -EINVAL;
> - }
> + ret = klp_check_object(patch->obj, false);
> + if (ret)
> + return ret;
>
> if (!klp_initialized())
> return -ENODEV;
>
Otherwise this looks good to me.
Cheers,
--
Julien Thierry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 05/23] livepatch: Initialize and free livepatch submodule
2020-01-17 15:03 ` [POC 05/23] livepatch: Initialize and free livepatch submodule Petr Mladek
@ 2020-01-21 11:58 ` Julien Thierry
0 siblings, 0 replies; 38+ messages in thread
From: Julien Thierry @ 2020-01-21 11:58 UTC (permalink / raw)
To: Petr Mladek, Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel
Hi Petr,
On 1/17/20 3:03 PM, Petr Mladek wrote:
> Another step when loading livepatches to livepatch modules is
> to initialize the structure, create sysfs entries, do livepatch
> specific relocations.
>
> These operation can fail and the objects must be freed that case.
> The error message is taken from klp_module_coming() to match
> selftests.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> kernel/livepatch/core.c | 34 +++++++++++++++++++++++++++-------
> 1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index e2c7dc6c2d5f..6c27b635e5a7 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -583,18 +583,23 @@ static void klp_free_object_loaded(struct klp_object *obj)
> }
> }
>
> +static void klp_free_object(struct klp_object *obj, bool nops_only)
> +{
> + __klp_free_funcs(obj, nops_only);
> +
> + if (nops_only && !obj->dynamic)
> + return;
> +
> + list_del(&obj->node);
> + kobject_put(&obj->kobj);
> +}
> +
> static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
> {
> struct klp_object *obj, *tmp_obj;
>
> klp_for_each_object_safe(patch, obj, tmp_obj) {
> - __klp_free_funcs(obj, nops_only);
> -
> - if (nops_only && !obj->dynamic)
> - continue;
> -
> - list_del(&obj->node);
> - kobject_put(&obj->kobj);
> + klp_free_object(obj, nops_only);
> }
> }
>
> @@ -812,6 +817,8 @@ static int klp_init_object_early(struct klp_patch *patch,
> if (obj->dynamic || try_module_get(obj->mod))
> return 0;
>
> + /* patch stays when this function fails in klp_add_object() */
> + list_del(&obj->node);
> return -ENODEV;
> }
>
> @@ -993,9 +1000,22 @@ int klp_add_object(struct klp_object *obj)
> goto err;
> }
>
> + ret = klp_init_object_early(patch, obj);
klp_init_object_early() can fail after adding obj to patch->obj_list.
This wouldn't get cleaned up by the "err" path.
It probably would keep things simple to only add obj to patch->obj_list
if early initialization is successful in patch 2 (ofc I'm talking about
the actual patch of this patch series ;) ).
> + if (ret)
> + goto err;
> +
> + ret = klp_init_object(patch, obj);
> + if (ret) {
> + pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
> + patch->obj->patch_name, obj->name, ret);
> + goto err_free;
> + }
> +
> mutex_unlock(&klp_mutex);
> return 0;
>
> +err_free:
> + klp_free_object(obj, false);
> err:
> /*
> * If a patch is unsuccessfully applied, return
>
Cheers,
--
Julien Thierry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 11/23] livepatch: Safely detect forced transition when removing split livepatch modules
2020-01-17 15:03 ` [POC 11/23] livepatch: Safely detect forced transition when removing split livepatch modules Petr Mladek
@ 2020-01-22 10:15 ` Julien Thierry
0 siblings, 0 replies; 38+ messages in thread
From: Julien Thierry @ 2020-01-22 10:15 UTC (permalink / raw)
To: Petr Mladek, Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel
Hi Petr,
On 1/17/20 3:03 PM, Petr Mladek wrote:
> The information about forced livepatch transition is currently stored
> in struct klp_patch. But there is not any obvious safe way how to
> access it when the split livepatch modules are removed.
>
If that's the only motivation to do this, klp_objects could have a
reference to the klp_patch they are part of. This could easily be set
when adding the klp_object to the klp_patch->obj_list in
klp_init_object_early() .
Having this reference could also prove useful in future scenarios.
Cheers,
--
Julien Thierry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 09/23] livepatch: Handle race when livepatches are reloaded during a module load
2020-01-17 15:03 ` [POC 09/23] livepatch: Handle race when livepatches are reloaded during a module load Petr Mladek
@ 2020-01-22 18:51 ` Julien Thierry
0 siblings, 0 replies; 38+ messages in thread
From: Julien Thierry @ 2020-01-22 18:51 UTC (permalink / raw)
To: Petr Mladek, Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, live-patching,
linux-kernel
Hi Petr,
On 1/17/20 3:03 PM, Petr Mladek wrote:
> klp_module_coming() might fail to load a livepatch module when
> the related livepatch gets reloaded in the meantime.
>
> Detect this situation by adding a timestamp into struct klp_patch.
> local_clock is enough because klp_mutex must be released and taken
> several times during this scenario.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> include/linux/livepatch.h | 2 ++
> kernel/livepatch/core.c | 9 +++++----
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index a4567c17a9f2..feb33f023f9f 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -155,6 +155,7 @@ struct klp_state {
> * @obj_list: dynamic list of the object entries
> * @enabled: the patch is enabled (but operation may be incomplete)
> * @forced: was involved in a forced transition
> + * ts: timestamp when the livepatch has been loaded
Nit: Missing '@'.
> * @free_work: patch cleanup from workqueue-context
> * @finish: for waiting till it is safe to remove the patch module
> */
> @@ -171,6 +172,7 @@ struct klp_patch {
> struct list_head obj_list;
> bool enabled;
> bool forced;
> + u64 ts;
> struct work_struct free_work;
> struct completion finish;
> };
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 34e3ee2be7ef..8e693c58b736 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -20,6 +20,7 @@
> #include <linux/moduleloader.h>
> #include <linux/completion.h>
> #include <linux/memory.h>
> +#include <linux/sched/clock.h>
> #include <asm/cacheflush.h>
> #include "core.h"
> #include "patch.h"
> @@ -854,6 +855,7 @@ static int klp_init_patch_early(struct klp_patch *patch)
> kobject_init(&patch->kobj, &klp_ktype_patch);
> patch->enabled = false;
> patch->forced = false;
> + patch->ts = local_clock();
> INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
> init_completion(&patch->finish);
>
> @@ -1324,6 +1326,7 @@ int klp_module_coming(struct module *mod)
> {
> char patch_name[MODULE_NAME_LEN];
> struct klp_patch *patch;
> + u64 patch_ts;
> int ret = 0;
>
> if (WARN_ON(mod->state != MODULE_STATE_COMING))
> @@ -1339,6 +1342,7 @@ int klp_module_coming(struct module *mod)
> continue;
>
> strncpy(patch_name, patch->obj->patch_name, sizeof(patch_name));
> + patch_ts = patch->ts;
> mutex_unlock(&klp_mutex);
>
> ret = klp_try_load_object(patch_name, mod->name);
> @@ -1346,14 +1350,11 @@ int klp_module_coming(struct module *mod)
> * The load might have failed because the patch has
> * been removed in the meantime. In this case, the
> * error might be ignored.
> - *
> - * FIXME: It is not fully proof. The patch might have be
> - * unloaded and loaded again in the mean time.
> */
> mutex_lock(&klp_mutex);
> if (ret) {
> patch = klp_find_patch(patch_name);
> - if (patch)
> + if (patch && patch->ts == patch_ts)
> goto err;
If the timestamps differ, we have found the klp_patch, feels a bit of a
waste to go through the list again without trying to load it right away.
Admittedly this is to solve a race condition which should not even
happen very often...
> ret = 0;
> }
>
Cheers,
--
Julien Thierry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 02/23] livepatch: Split livepatch modules per livepatched object
2020-01-21 11:11 ` Julien Thierry
@ 2020-01-28 12:16 ` Petr Mladek
0 siblings, 0 replies; 38+ messages in thread
From: Petr Mladek @ 2020-01-28 12:16 UTC (permalink / raw)
To: Julien Thierry
Cc: Jiri Kosina, Josh Poimboeuf, Miroslav Benes, Joe Lawrence,
Kamalesh Babulal, Nicolai Stange, live-patching, linux-kernel
On Tue 2020-01-21 11:11:45, Julien Thierry wrote:
> Hi Petr,
>
> On 1/17/20 3:03 PM, Petr Mladek wrote:
> > One livepatch module allows to fix vmlinux and any number of modules
> > while providing some guarantees defined by the consistency model.
> >
> > The solution is to split the livepatch module per livepatched
> > object (vmlinux or module). Then both livepatch module and
> > the livepatched modules could get loaded and removed at the
> > same time.
> >
> > The livepatches for modules are put into separate source files
> > that define only struct klp_object() and call the new klp_add_object()
> > in the init() callback. The name of the module follows the pattern:
> >
> > <patch_name>__<object_name>
> >
>
> Is that a requirement? Or is it just the convention followed for the current
> tests?
This naming pattern is enforced by the code. The reason is to
distinguish the purpose of each livepatch module.
+ Livepatch module for "vmlinux" and the related livepatch modules
for other objects.
+ Different livepatches (versions) that might be installed at the
same time. This happens even with cumulative livepatches.
It is important for the functionality:
+ Consistency checks that all and right livepatch modules are
loaded.
+ Automatic loading of livepatch modules for modules when the patched
module is being loaded.
But it should be "clear" even for humans because the livepatch modules are
listed by lsmod, ...
Of course, we could talk about other naming scheme, another approach.
> > @@ -844,21 +822,7 @@ static int klp_init_patch_early(struct klp_patch *patch)
> > INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
> > init_completion(&patch->finish);
> > - klp_for_each_object_static(patch, obj) {
>
> I think we can get rid of klp_for_each_object_static(), no? Now the
> klp_patch is only associated to a single klp_object, so everything will be
> dynamic. Is this correct?
Yes, the macro klp_for_each_object_static() is not longer needed.
Just to be sure. It would be better to say that all klp_object
structures will be in the linked lists only.
Most structures are still defined statically. The name "dynamic" is
used for the dynamically allocated structures. They are used for
"nop" functions that might be needed when doing atomic replace
of cumulative patches and functions that are not longer patched.
See obj->dynamic and func->nop.
> > @@ -991,12 +958,12 @@ int klp_enable_patch(struct klp_patch *patch)
> > {
> > int ret;
> > - if (!patch || !patch->mod)
> > + if (!patch || !patch->obj || !patch->obj->mod)
> > return -EINVAL;
> > - if (!is_livepatch_module(patch->mod)) {
> > + if (!is_livepatch_module(patch->obj->mod)) {
> > pr_err("module %s is not marked as a livepatch module\n",
> > - patch->mod->name);
> > + patch->obj->patch_name);
>
> Shouldn't that be "patch->obj->mod->name" ?
They are actually the same. Note that it is redundant only in
struct klp_object that is in the livepatch module for vmlinux.
Hmm, it might be possible to get rid of it after I added the array
patch->obj_names. But I would prefer to keep it as a consistency
check.
One big drawback of the split modules approach is that there are
suddenly many more livepatch modules. The kernel code has to make
sure always the right ones are loaded. It is great to have some
cross-checks.
> > return -EINVAL;
> > }
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index f6310f848f34..78e3280560cd 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -147,7 +145,7 @@ void klp_cancel_transition(void)
> > return;
> > pr_debug("'%s': canceling patching transition, going to unpatch\n",
> > - klp_transition_patch->mod->name);
> > + klp_transition_patch->obj->patch_name);
> > klp_target_state = KLP_UNPATCHED;
> > klp_complete_transition();
> > @@ -468,7 +466,7 @@ void klp_start_transition(void)
> > WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
> > pr_notice("'%s': starting %s transition\n",
> > - klp_transition_patch->mod->name,
> > + klp_transition_patch->obj->patch_name,
>
> Isn't the transition per livepatched module rather than per-patch now?
> If so, would it make more sense to display also the name of the module being
> patched/unpatched?
The transition still happens for the entire livepatch defined by
struct klp_patch. All needed livepatch modules for the other objects
are loaded before the transition starts, see the patch 17/24
("livepatch: Load livepatches for modules when loading the main
livepatch").
> > klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
> > /*
Best Regards,
Petr
PS: Julien,
first, thanks a lot for looking at the patchset.
I am going to answer questions and comments that are related to
the overall design. The most important question is if the split
livepatch modules are the way to go. I hope that this patchset
shows possible wins and catches so that we could decide if it
is worth the effort.
Anyway, feel free to comment even details when you notice
a mistake. There might be some catches that I missed, ...
Best Regards,
Petr
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 04/23] livepatch: Prevent loading livepatch sub-module unintentionally.
2020-01-17 15:03 ` [POC 04/23] livepatch: Prevent loading livepatch sub-module unintentionally Petr Mladek
@ 2020-04-03 17:54 ` Joe Lawrence
0 siblings, 0 replies; 38+ messages in thread
From: Joe Lawrence @ 2020-04-03 17:54 UTC (permalink / raw)
To: Petr Mladek
Cc: Jiri Kosina, Josh Poimboeuf, Miroslav Benes, Kamalesh Babulal,
Nicolai Stange, live-patching, linux-kernel
On Fri, Jan 17, 2020 at 04:03:04PM +0100, Petr Mladek wrote:
> Livepatch is split into several modules. The main module is for livepatching
> vmlinux. The rest is for livepatching other modules.
>
> Only the livepatch module for vmlinux can be loaded by users. Others are
> loaded automatically when the related module is or gets loaded.
>
> Users might try to load any livepatch module. It must be allowed
> only when the related livepatch module for vmlinux and the livepatched
> module are loaded.
>
> Also it is important to check that obj->name is listed in patch->obj_names.
> Otherwise this module would not be loaded automatically. And it would
> lead into inconsistent behavier. Anyway, the missing name means a mistake
> somewhere and must be reported.
>
> klp_add_object() is taking over the job done by klp_module_coming().
> The error message is taken from there so that selftests do not need
> to get updated.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> kernel/livepatch/core.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index ec7ffc7db3a7..e2c7dc6c2d5f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
>
> [ ... snip ... ]
>
> int klp_add_object(struct klp_object *obj)
> {
> + struct klp_patch *patch;
> int ret;
>
> ret = klp_check_object(obj, true);
> if (ret)
> return ret;
>
> + mutex_lock(&klp_mutex);
> +
> + patch = klp_find_patch(obj->patch_name);
> + if (!patch) {
> + pr_err("Can't load livepatch (%s) for module when the livepatch (%s) for vmcore is not loaded\n",
> + obj->mod->name, obj->patch_name);
nit: s/vmcore/vmlinux in the error message?
> + ret = -EINVAL;
> + goto err;
Minor code snafu: !patch for this exit path means ...
> + }
> +
> + if (!klp_is_object_compatible(patch, obj)) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + mutex_unlock(&klp_mutex);
> return 0;
> +
> +err:
> + /*
> + * If a patch is unsuccessfully applied, return
> + * error to the module loader.
> + */
> + pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
> + patch->obj->patch_name, obj->name, obj->name);
... we can't access patch->obj->patch_name here.
-- Joe
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 20/23] module/livepatch: Relocate local variables in the module loaded when the livepatch is being loaded
2020-01-17 15:03 ` [POC 20/23] module/livepatch: Relocate local variables in the module loaded when the livepatch is being loaded Petr Mladek
2020-01-18 10:29 ` kbuild test robot
@ 2020-04-03 18:00 ` Joe Lawrence
1 sibling, 0 replies; 38+ messages in thread
From: Joe Lawrence @ 2020-04-03 18:00 UTC (permalink / raw)
To: Petr Mladek
Cc: Jiri Kosina, Josh Poimboeuf, Miroslav Benes, Kamalesh Babulal,
Nicolai Stange, live-patching, linux-kernel
On Fri, Jan 17, 2020 at 04:03:20PM +0100, Petr Mladek wrote:
> The special SHF_RELA_LIVEPATCH section is still needed to find static
> (non-exported) symbols. But it can be done together with the other
> relocations when the livepatch module is being loaded.
>
> There is no longer needed to copy the info section. The related
> code in the module loaded will get removed in separate patch.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> include/linux/livepatch.h | 4 +++
> kernel/livepatch/core.c | 62 +++--------------------------------------------
> kernel/module.c | 16 +++++++-----
> 3 files changed, 18 insertions(+), 64 deletions(-)
>
>
> [ ... snip ... ]
>
> diff --git a/kernel/module.c b/kernel/module.c
> index bd92854b42c2..c14b5135db27 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2410,16 +2410,20 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
> if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
> continue;
>
> - /* Livepatch relocation sections are applied by livepatch */
> - if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> - continue;
> -
> - if (info->sechdrs[i].sh_type == SHT_REL)
> + /* Livepatch need to resolve static symbols. */
> + if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH) {
> + err = klp_resolve_symbols(info->sechdrs, i, mod);
> + if (err < 0)
> + break;
> + err = apply_relocate_add(info->sechdrs, info->strtab,
> + info->index.sym, i, mod);
> + } else if (info->sechdrs[i].sh_type == SHT_REL) {
> err = apply_relocate(info->sechdrs, info->strtab,
> info->index.sym, i, mod);
> - else if (info->sechdrs[i].sh_type == SHT_RELA)
> + } else if (info->sechdrs[i].sh_type == SHT_RELA) {
> err = apply_relocate_add(info->sechdrs, info->strtab,
> info->index.sym, i, mod);
> + }
> if (err < 0)
> break;
> }
Hi Petr,
At first I thought there was a simple order of operations problem here
with respect to klp_resolve_symbols() accessing core_kallsyms before
they were setup by add_kallsyms():
load_module
apply_relocations
/* Livepatch need to resolve static symbols. */
if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH) {
err = klp_resolve_symbols(info->sechdrs, i, mod);
klp_resolve_symbols
sym = pmod->core_kallsyms.symtab + ELF_R_SYM(relas[i].r_info);
^^^^^^^^^^^^^^^^^^^^
used before init (below)
...
post_relocation
add_kallsyms
/*
* Now populate the cut down core kallsyms for after init
* and set types up while we still have access to sections.
*/
mod->core_kallsyms.symtab = dst = mod->core_layout.base + info->symoffs;
mod->core_kallsyms.strtab = s = mod->core_layout.base + info->stroffs;
mod->core_kallsyms.typetab = mod->core_layout.base + info->core_typeoffs;
^^^^^^^^^^^^^^^^^^^^^
core_kallsyms initialized here
But after tinkering with the patchset, a larger problem is that
klp_resolve_symbols() writes st_values to the core_kallsyms copies, but
then apply_relocate_add() references the originals in the load_info
structure.
I assume that klp_resolve_symbols() originally looked at the
core_kallsyms copies for handling the late module patching case. If we
no longer need to support that, then how about this slight modification
to klp_resolve_symbols() to make it look more the like
apply_relocate{_add,} calls?
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 3b27ef1a7291..54d5a4045e5a 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -210,6 +210,8 @@ int klp_module_coming(struct module *mod);
void klp_module_going(struct module *mod);
int klp_resolve_symbols(Elf_Shdr *sechdrs,
+ const char *strtab,
+ unsigned int symindex,
unsigned int relsec,
struct module *pmod);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index cc0ac93fe8cd..02638e3b09b0 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -197,13 +197,14 @@ static int klp_find_object_symbol(const char *objname, const char *name,
}
int klp_resolve_symbols(Elf_Shdr *sechdrs,
+ const char *strtab,
+ unsigned int symindex,
unsigned int relsec,
struct module *pmod)
{
int i, cnt, vmlinux, ret;
char objname[MODULE_NAME_LEN];
char symname[KSYM_NAME_LEN];
- char *strtab = pmod->core_kallsyms.strtab;
Elf_Shdr *relasec = sechdrs + relsec;
Elf_Rela *relas;
Elf_Sym *sym;
@@ -224,7 +225,8 @@ int klp_resolve_symbols(Elf_Shdr *sechdrs,
relas = (Elf_Rela *) relasec->sh_addr;
/* For each rela in this klp relocation section */
for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
- sym = pmod->core_kallsyms.symtab + ELF_R_SYM(relas[i].r_info);
+ sym = (Elf64_Sym *)sechdrs[symindex].sh_addr +
+ ELF_R_SYM(relas[i].r_info);
if (sym->st_shndx != SHN_LIVEPATCH) {
pr_err("symbol %s is not marked as a livepatch symbol\n",
strtab + sym->st_name);
diff --git a/kernel/module.c b/kernel/module.c
index d435bad80d7d..a65f089f19c9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2320,7 +2320,8 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
/* Livepatch need to resolve static symbols. */
if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH) {
- err = klp_resolve_symbols(info->sechdrs, i, mod);
+ err = klp_resolve_symbols(info->sechdrs, info->strtab,
+ info->index.sym, i, mod);
if (err < 0)
break;
err = apply_relocate_add(info->sechdrs, info->strtab,
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
-- Joe
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [POC 22/23] livepatch/module: Remove obsolete copy_module_elf()
2020-01-17 15:03 ` [POC 22/23] livepatch/module: Remove obsolete copy_module_elf() Petr Mladek
@ 2020-04-03 18:03 ` Joe Lawrence
0 siblings, 0 replies; 38+ messages in thread
From: Joe Lawrence @ 2020-04-03 18:03 UTC (permalink / raw)
To: Petr Mladek
Cc: Jiri Kosina, Josh Poimboeuf, Miroslav Benes, Kamalesh Babulal,
Nicolai Stange, live-patching, linux-kernel
On Fri, Jan 17, 2020 at 04:03:22PM +0100, Petr Mladek wrote:
> The split livepatch modules can be relocated immedidately when they
> are loaded. There is no longer needed to preserve the elf sections.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> Documentation/livepatch/module-elf-format.rst | 18 ++++++
> include/linux/module.h | 3 -
> kernel/module.c | 87 ---------------------------
> 3 files changed, 18 insertions(+), 90 deletions(-)
>
> diff --git a/Documentation/livepatch/module-elf-format.rst b/Documentation/livepatch/module-elf-format.rst
> index 9f0c997d4940..8c6b894c4661 100644
> --- a/Documentation/livepatch/module-elf-format.rst
> +++ b/Documentation/livepatch/module-elf-format.rst
> @@ -14,6 +14,7 @@ This document outlines the Elf format requirements that livepatch modules must f
> 4. Livepatch symbols
> 4.1 A livepatch module's symbol table
> 4.2 Livepatch symbol format
> + 5. Symbol table and Elf section access
>
> 1. Background and motivation
> ============================
> @@ -295,3 +296,20 @@ See include/uapi/linux/elf.h for the actual definitions.
> [*]
> Note that the 'Ndx' (Section index) for these symbols is SHN_LIVEPATCH (0xff20).
> "OS" means OS-specific.
> +
> +5. Symbol table and Elf section access
> +======================================
> +A livepatch module's symbol table is accessible through module->symtab.
> +
> +Since apply_relocate_add() requires access to a module's section headers,
> +symbol table, and relocation section indices, Elf information is preserved for
> +livepatch modules and is made accessible by the module loader through
> +module->klp_info, which is a klp_modinfo struct. When a livepatch module loads,
> +this struct is filled in by the module loader. Its fields are documented below::
> +
> + struct klp_modinfo {
> + Elf_Ehdr hdr; /* Elf header */
> + Elf_Shdr *sechdrs; /* Section header table */
> + char *secstrings; /* String table for the section headers */
> + unsigned int symndx; /* The symbol table section index */
> + };
I think this file was inadvertently reverted, or at least the Symbol
table and Elf section access section was supposed to stay gone, right?
-- Joe
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 19/23] module/livepatch: Allow to use exported symbols from livepatch module for "vmlinux"
2020-01-17 15:03 ` [POC 19/23] module/livepatch: Allow to use exported symbols from livepatch module for "vmlinux" Petr Mladek
@ 2020-04-06 18:48 ` Joe Lawrence
2020-04-07 7:33 ` Miroslav Benes
0 siblings, 1 reply; 38+ messages in thread
From: Joe Lawrence @ 2020-04-06 18:48 UTC (permalink / raw)
To: Petr Mladek
Cc: Jiri Kosina, Josh Poimboeuf, Miroslav Benes, Kamalesh Babulal,
Nicolai Stange, live-patching, linux-kernel
On Fri, Jan 17, 2020 at 04:03:19PM +0100, Petr Mladek wrote:
> HINT: Get some coffee before reading this commit message.
>
> Stop reading when it gets too complicated. It is possible that we
> will need to resolve symbols from livepatch modules another way.
> Livepatches need to access also non-exported symbols anyway.
>
> Or just ask me to explain the problem a better way. I have
> ended in many cycles when thinking about it. And it might
> be much easier from another point of view.
>
> The split per-object livepatches brings even more types of module
> dependencies. Let's split them into few categories:
>
> A. Livepatch module using an exported symbol from "vmlinux".
>
> It is quite common and works by definition. Livepatch module is just
> a module from this point of view.
>
Hi Petr,
If only all the cases were so easy :)
> B. Livepatch module using an exported symbol from the patched module.
>
> It should be avoided even with the non-split livepatch module. The module
> loader automatically takes reference to make sure the modules are
> unloaded in the right order. This would basically prevent the livepatched
> module from unloading.
>
> Note that it would be perfectly safe to remove this automatic
> dependency. The livepatch framework makes sure that the livepatch
> module is loaded only when the patched one is loaded. But it cannot
> be implemented easily, see below.
Do you envision klp-convert providing this functionality?
For reference, kpatch-build will take this example input patch:
diff -U 15 -Nupr linux-3.10.0-1133.el7.x86_64.old/drivers/cdrom/cdrom.c linux-3.10.0-1133.el7.x86_64/drivers/cdrom/cdrom.c
--- linux-3.10.0-1133.el7.x86_64.old/drivers/cdrom/cdrom.c 2020-04-06 11:58:34.470969120 -0400
+++ linux-3.10.0-1133.el7.x86_64/drivers/cdrom/cdrom.c 2020-04-06 12:01:07.882719611 -0400
@@ -1429,30 +1429,32 @@ unsigned int cdrom_check_events(struct c
EXPORT_SYMBOL(cdrom_check_events);
/* We want to make media_changed accessible to the user through an
* ioctl. The main problem now is that we must double-buffer the
* low-level implementation, to assure that the VFS and the user both
* see a medium change once.
*/
static
int media_changed(struct cdrom_device_info *cdi, int queue)
{
unsigned int mask = (1 << (queue & 1));
int ret = !!(cdi->mc_flags & mask);
bool changed;
+pr_info("%lx\n", (unsigned long) cdrom_check_events);
+
if (!CDROM_CAN(CDC_MEDIA_CHANGED))
return ret;
/* changed since last call? */
if (cdi->ops->check_events) {
BUG_ON(!queue); /* shouldn't be called from VFS path */
cdrom_update_events(cdi, DISK_EVENT_MEDIA_CHANGE);
changed = cdi->ioctl_events & DISK_EVENT_MEDIA_CHANGE;
cdi->ioctl_events = 0;
} else
changed = cdi->ops->media_changed(cdi, CDSL_CURRENT);
if (changed) {
cdi->mc_flags = 0x3; /* set bit on both queues */
ret |= 1;
and you'll have the original cdrom.ko, owner of exported
cdrom_check_events, and a new livepatch-cdrom.ko that references it.
kpatch-build converts the symbol/rela combination like so:
% readelf --wide --symbols livepatch-test.ko | grep cdrom_check_events
79: 0000000000000000 0 FUNC GLOBAL DEFAULT OS [0xff20] .klp.sym.cdrom.cdrom_check_events,0
% readelf --wide --relocs livepatch-test.ko | awk '/cdrom_check_events/' RS="\n\n" ORS="\n\n"
Relocation section '.klp.rela.cdrom..text.media_changed' at offset 0x97fc0 contains 1 entries:
Offset Info Type Symbol's Value Symbol's Name + Addend
0000000000000016 0000004f0000000b R_X86_64_32S 0000000000000000 .klp.sym.cdrom.cdrom_check_events,0 + 0
That dodges the implicit module reference on cdrom.ko, but would it
still be safe in this klp-split POC?
FWIW, I have been working an updated klp-convert for v5.6 that includes
a bunch of fixes and such... modifying it to convert module-export
references like this was quite easy.
> C. Livepatch module using an exported symbol from the main livepatch module
> for "vmlinux".
>
> It is the 2nd most realistic variant. It even exists in current
> selftests. Namely, test_klp_callback_demo* modules share
> the implementation of callbacks. It avoids code duplication.
> And it is actually needed to make module parameters working.
I had to double check this and the exports were introduced by this POC
just to avoid code duplication, right? If so, would it be worth the
extra code to avoid providing bad example usage? Or at least do so for
sample/livepatch/ and not selftests.
> Note that the current implementation allows to pass module parameters
> only to the main livepatch module for "vmlinux". It should not be a real
> life problem. The parameters are used in selftests. But they are
> not used in practice.
Yeah, we don't use module parameters for kpatch either, AFAIK they are
only useful as a quick and easy method to poke those selftests.
On a related note, one possible work around for case C is to use shadow
variables created by the main livepatch module to store symbol
locations or the values themselves. This might get tedious for real
kernel API, but has been reasonable for livepatch bookkeeping states,
counts, etc.
> D. Livepatch modules might depend on each other. Note that dependency on
> the main livepatch module for "vmlinux" has got a separate category 'C'.
>
> The dependencies between modules are quite rare. But they exist.
> One can assume that this might be useful also on the livepatching
> level.
>
> To keep it sane, the livepatch modules should just follow
> the dependencies of the related patched modules. By other words,
> the livepatch modules might or should have the same dependencies
> as the patched counter parts but nothing more.
>
I agree, I think this is the only sane way to approach case D.
> Do these dependencies need some special handling?
>
> [ ... snip ... ]
>
This is the multiple-coffee cup section that I'm still trying to wrap my
brain around.
In the meantime, could we simplify or avoid any of these by enforcing
things on the build side? I don't know if these are even detectable,
but perhaps we could prevent them from even occuring. /shrugs
-- Joe
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 19/23] module/livepatch: Allow to use exported symbols from livepatch module for "vmlinux"
2020-04-06 18:48 ` Joe Lawrence
@ 2020-04-07 7:33 ` Miroslav Benes
2020-04-07 20:57 ` Joe Lawrence
0 siblings, 1 reply; 38+ messages in thread
From: Miroslav Benes @ 2020-04-07 7:33 UTC (permalink / raw)
To: Joe Lawrence
Cc: Petr Mladek, Jiri Kosina, Josh Poimboeuf, Kamalesh Babulal,
Nicolai Stange, live-patching, linux-kernel
On Mon, 6 Apr 2020, Joe Lawrence wrote:
> On Fri, Jan 17, 2020 at 04:03:19PM +0100, Petr Mladek wrote:
> > HINT: Get some coffee before reading this commit message.
> >
> > Stop reading when it gets too complicated. It is possible that we
> > will need to resolve symbols from livepatch modules another way.
> > Livepatches need to access also non-exported symbols anyway.
> >
> > Or just ask me to explain the problem a better way. I have
> > ended in many cycles when thinking about it. And it might
> > be much easier from another point of view.
> >
> > The split per-object livepatches brings even more types of module
> > dependencies. Let's split them into few categories:
> >
> > A. Livepatch module using an exported symbol from "vmlinux".
> >
> > It is quite common and works by definition. Livepatch module is just
> > a module from this point of view.
> >
>
> Hi Petr,
>
> If only all the cases were so easy :)
>
> > B. Livepatch module using an exported symbol from the patched module.
> >
> > It should be avoided even with the non-split livepatch module. The module
> > loader automatically takes reference to make sure the modules are
> > unloaded in the right order. This would basically prevent the livepatched
> > module from unloading.
> >
> > Note that it would be perfectly safe to remove this automatic
> > dependency. The livepatch framework makes sure that the livepatch
> > module is loaded only when the patched one is loaded. But it cannot
> > be implemented easily, see below.
>
> Do you envision klp-convert providing this functionality?
>
> For reference, kpatch-build will take this example input patch:
>
> diff -U 15 -Nupr linux-3.10.0-1133.el7.x86_64.old/drivers/cdrom/cdrom.c linux-3.10.0-1133.el7.x86_64/drivers/cdrom/cdrom.c
> --- linux-3.10.0-1133.el7.x86_64.old/drivers/cdrom/cdrom.c 2020-04-06 11:58:34.470969120 -0400
> +++ linux-3.10.0-1133.el7.x86_64/drivers/cdrom/cdrom.c 2020-04-06 12:01:07.882719611 -0400
> @@ -1429,30 +1429,32 @@ unsigned int cdrom_check_events(struct c
> EXPORT_SYMBOL(cdrom_check_events);
>
> /* We want to make media_changed accessible to the user through an
> * ioctl. The main problem now is that we must double-buffer the
> * low-level implementation, to assure that the VFS and the user both
> * see a medium change once.
> */
>
> static
> int media_changed(struct cdrom_device_info *cdi, int queue)
> {
> unsigned int mask = (1 << (queue & 1));
> int ret = !!(cdi->mc_flags & mask);
> bool changed;
>
> +pr_info("%lx\n", (unsigned long) cdrom_check_events);
> +
> if (!CDROM_CAN(CDC_MEDIA_CHANGED))
> return ret;
>
> /* changed since last call? */
> if (cdi->ops->check_events) {
> BUG_ON(!queue); /* shouldn't be called from VFS path */
> cdrom_update_events(cdi, DISK_EVENT_MEDIA_CHANGE);
> changed = cdi->ioctl_events & DISK_EVENT_MEDIA_CHANGE;
> cdi->ioctl_events = 0;
> } else
> changed = cdi->ops->media_changed(cdi, CDSL_CURRENT);
>
> if (changed) {
> cdi->mc_flags = 0x3; /* set bit on both queues */
> ret |= 1;
>
> and you'll have the original cdrom.ko, owner of exported
> cdrom_check_events, and a new livepatch-cdrom.ko that references it.
> kpatch-build converts the symbol/rela combination like so:
>
> % readelf --wide --symbols livepatch-test.ko | grep cdrom_check_events
> 79: 0000000000000000 0 FUNC GLOBAL DEFAULT OS [0xff20] .klp.sym.cdrom.cdrom_check_events,0
>
> % readelf --wide --relocs livepatch-test.ko | awk '/cdrom_check_events/' RS="\n\n" ORS="\n\n"
> Relocation section '.klp.rela.cdrom..text.media_changed' at offset 0x97fc0 contains 1 entries:
> Offset Info Type Symbol's Value Symbol's Name + Addend
> 0000000000000016 0000004f0000000b R_X86_64_32S 0000000000000000 .klp.sym.cdrom.cdrom_check_events,0 + 0
>
> That dodges the implicit module reference on cdrom.ko, but would it
> still be safe in this klp-split POC?
That is one way to get around the dependency problem. And I think it
should work even with the PoC. It should (and I don't remember all details
now unfortunately) guarantee that the patched module is always available
for the livepatch and since there is no explicit dependency, the recursion
issue is gone.
However, I think the goal was to follow the most natural road and leverage
the existing dependency system. Meaning, since the presence of a patched
module in the system before its patching is guaranteed now, there is no
reason not to use its exported symbols directly like anywhere else. But it
introduces the recursion problem, so we may drop it.
> FWIW, I have been working an updated klp-convert for v5.6 that includes
> a bunch of fixes and such... modifying it to convert module-export
> references like this was quite easy.
*THUMBS UP* :)
> > C. Livepatch module using an exported symbol from the main livepatch module
> > for "vmlinux".
> >
> > It is the 2nd most realistic variant. It even exists in current
> > selftests. Namely, test_klp_callback_demo* modules share
> > the implementation of callbacks. It avoids code duplication.
> > And it is actually needed to make module parameters working.
>
> I had to double check this and the exports were introduced by this POC
> just to avoid code duplication, right? If so, would it be worth the
> extra code to avoid providing bad example usage? Or at least do so for
> sample/livepatch/ and not selftests.
I agree.
This case sounds a bit quirky to me.
> > Note that the current implementation allows to pass module parameters
> > only to the main livepatch module for "vmlinux". It should not be a real
> > life problem. The parameters are used in selftests. But they are
> > not used in practice.
>
> Yeah, we don't use module parameters for kpatch either, AFAIK they are
> only useful as a quick and easy method to poke those selftests.
>
> On a related note, one possible work around for case C is to use shadow
> variables created by the main livepatch module to store symbol
> locations or the values themselves. This might get tedious for real
> kernel API, but has been reasonable for livepatch bookkeeping states,
> counts, etc.
>
> > D. Livepatch modules might depend on each other. Note that dependency on
> > the main livepatch module for "vmlinux" has got a separate category 'C'.
> >
> > The dependencies between modules are quite rare. But they exist.
> > One can assume that this might be useful also on the livepatching
> > level.
> >
> > To keep it sane, the livepatch modules should just follow
> > the dependencies of the related patched modules. By other words,
> > the livepatch modules might or should have the same dependencies
> > as the patched counter parts but nothing more.
> >
>
> I agree, I think this is the only sane way to approach case D.
>
> > Do these dependencies need some special handling?
> >
> > [ ... snip ... ]
> >
>
> This is the multiple-coffee cup section that I'm still trying to wrap my
> brain around.
Yes, I really need to go through the patch set once again and try to
digest it.
Miroslav
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [POC 19/23] module/livepatch: Allow to use exported symbols from livepatch module for "vmlinux"
2020-04-07 7:33 ` Miroslav Benes
@ 2020-04-07 20:57 ` Joe Lawrence
0 siblings, 0 replies; 38+ messages in thread
From: Joe Lawrence @ 2020-04-07 20:57 UTC (permalink / raw)
To: Miroslav Benes
Cc: Petr Mladek, Jiri Kosina, Josh Poimboeuf, Kamalesh Babulal,
Nicolai Stange, live-patching, linux-kernel
On Tue, Apr 07, 2020 at 09:33:08AM +0200, Miroslav Benes wrote:
> On Mon, 6 Apr 2020, Joe Lawrence wrote:
>
> > On Fri, Jan 17, 2020 at 04:03:19PM +0100, Petr Mladek wrote:
> > > HINT: Get some coffee before reading this commit message.
> > >
> > > [ ... snip ... ]
> > >
> > > B. Livepatch module using an exported symbol from the patched module.
> > >
> > > It should be avoided even with the non-split livepatch module. The module
> > > loader automatically takes reference to make sure the modules are
> > > unloaded in the right order. This would basically prevent the livepatched
> > > module from unloading.
> > >
> > > Note that it would be perfectly safe to remove this automatic
> > > dependency. The livepatch framework makes sure that the livepatch
> > > module is loaded only when the patched one is loaded. But it cannot
> > > be implemented easily, see below.
> >
> > Do you envision klp-convert providing this functionality?
> >
> > [ ... snip ... ]
>
> That is one way to get around the dependency problem. And I think it
> should work even with the PoC. It should (and I don't remember all details
> now unfortunately) guarantee that the patched module is always available
> for the livepatch and since there is no explicit dependency, the recursion
> issue is gone.
>
> However, I think the goal was to follow the most natural road and leverage
> the existing dependency system. Meaning, since the presence of a patched
> module in the system before its patching is guaranteed now, there is no
> reason not to use its exported symbols directly like anywhere else. But it
> introduces the recursion problem, so we may drop it.
>
> > FWIW, I have been working an updated klp-convert for v5.6 that includes
> > a bunch of fixes and such... modifying it to convert module-export
> > references like this was quite easy.
>
> *THUMBS UP* :)
Hi Miroslav,
Perhaps the following bug report is premature, but it was far easier to
start playing with the PoC code than audit all the individual race
conditions :) This came out of the mentioned klp-convert rebase that I
later put on-top of Petr's PoC, and then started writing up some more
selftests to see how the new livepatching world might look.
Forgive me if I'm violating some obvious assumption that the PoC makes,
but I think the experiment may still be useful in pointing out a
problematic use-case / potential pitfall a livepatch author might
encounter.
tl;dr: prepare_coming_module() calls jump_label_add_module() *after* it
calls klp_module_coming(). In the PoC, the latter function now
searches for any livepatches that may apply to the loading
module and tries to load them before proceeding. If one of
those livepatch modules references any static key defined by the
originally loading module, the static key entry structures may
not be setup correctly.
The test case:
- A kernel module defines a static key called test_klp_true_key and on
__init, calls static_branch_disable(). I don't think the __init part
is required for this case, however it was just the easiest way to
write the test that I was working on at the time. AFAIK we could
change the key any where else with the same problem.
- A livepatch module that references test_klp_true_key. The overarching
test case was for the key's module owner (target) and
livepatch (livepatch__target) to toggle the key and for both target
and livepatch__target modules to be patched accordingly.
- klp-convert was enhanced to modify relocations to test_klp_true_key in
both .text and __jump_table sections. It previously could not handle
this scenario.
Pseudocode
==========
target.c
--------
static DEFINE_STATIC_KEY_TRUE(test_klp_true_key);
...
__init function() {
static_branch_disable(&test_klp_true_key);
}
livepatch__target.c
-------------------
extern struct static_key_true test_klp_true_key;
...
pr_info("static_branch_likely(&test_klp_true_key) is %s\n",
static_branch_likely(&test_klp_true_key) ? "true" : "false");
Test
====
% modprobe livepatch # livepatch__target only loads when target loads
% modprobe target
(crash in jump_label_update)
Callchain and notes
===================
(livepatch is already loaded)
target
------
load_module
apply_relocations
post_relocations
module_finalize
jump_label_apply_nops
complete_formation
mod->state = MODULE_STATE_COMING
prepare_coming_module
klp_module_coming
klp_try_load_object
patch_name = livepatch, obj_name = target
livepatch__target
-----------------
load_module
apply_relocations
post_relocations
module_finalize
jump_label_apply_nops
complete_formation
mod->state = MODULE_STATE_COMING
prepare_coming_module
blocking_notifier_call_chain(MODULE_STATE_COMING)
jump_label_module_notify (MODULE_STATE_COMING)
jump_label_add_module
first time processing test_klp_true_key, within_module()
returns false (the key is defined in the other module),
and we treat it as !static_key_linked() so the code goes
ahead and makes it linked
static_key_set_linked
key->type |= JUMP_TYPE_LINKED
do_init_module
mod->state = MODULE_STATE_LIVE;
blocking_notifier_call_chain(MODULE_STATE_LIVE)
jump_label_module_notify (MODULE_STATE_LIVE)
target
------
(prepare_coming_module cont...)
blocking_notifier_call_chain(MODULE_STATE_COMING)
jump_label_module_notify(MODULE_STATE_COMING)
jump_label_add_module
second time processing test_klp_true_key, within_module()
returns true this time and we go straight to
static_key_set_entries(), which is careful enough to verify the
that the entries aren't already linked, but not the key itself
static_key_set_entries
WARN_ON_ONCE((unsigned long)entries & JUMP_TYPE_MASK)
do_init_module
mod->state = MODULE_STATE_LIVE;
blocking_notifier_call_chain(MODULE_STATE_LIVE)
jump_label_module_notify (MODULE_STATE_LIVE)
Ok, so it seems that jump_label_add_module() assumes that any given key
that isn't present in said module, is assumed to have already been
initialized and therefore it enters that convoluted JUMP_TYPE_LINKED
code.
When we later try call jump_label_add_module() for the target module,
the same function assumes that the key is not linked and we're left with
a weird static_key_mod entry that later crashes the box.
tl;dr2: Could we safely reorder klp_module_{coming,going}() with respect
to the module_notifier callbacks? i.e.
blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod);
klp_module_coming(mod);
... and ...
klp_module_going(mod);
blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod);
This fixes the above test case, but I wasn't sure if it now violates
some other part of the PoC or consistency model.
-- Joe
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2020-04-07 20:57 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-17 15:03 [POC 00/23] livepatch: Split livepatch module per-object Petr Mladek
2020-01-17 15:03 ` [POC 01/23] module: Allow to delete module also from inside kernel Petr Mladek
2020-01-21 11:11 ` Julien Thierry
2020-01-17 15:03 ` [POC 02/23] livepatch: Split livepatch modules per livepatched object Petr Mladek
2020-01-21 11:11 ` Julien Thierry
2020-01-28 12:16 ` Petr Mladek
2020-01-17 15:03 ` [POC 03/23] livepatch: Better checks of struct klp_object definition Petr Mladek
2020-01-21 11:27 ` Julien Thierry
2020-01-17 15:03 ` [POC 04/23] livepatch: Prevent loading livepatch sub-module unintentionally Petr Mladek
2020-04-03 17:54 ` Joe Lawrence
2020-01-17 15:03 ` [POC 05/23] livepatch: Initialize and free livepatch submodule Petr Mladek
2020-01-21 11:58 ` Julien Thierry
2020-01-17 15:03 ` [POC 06/23] livepatch: Enable the " Petr Mladek
2020-01-17 15:03 ` [POC 07/23] livepatch: Remove obsolete functionality from klp_module_coming() Petr Mladek
2020-01-17 15:03 ` [POC 08/23] livepatch: Automatically load livepatch module when the patch module is loaded Petr Mladek
2020-01-17 15:03 ` [POC 09/23] livepatch: Handle race when livepatches are reloaded during a module load Petr Mladek
2020-01-22 18:51 ` Julien Thierry
2020-01-17 15:03 ` [POC 10/23] livepatch: Handle modprobe exit code Petr Mladek
2020-01-17 15:03 ` [POC 11/23] livepatch: Safely detect forced transition when removing split livepatch modules Petr Mladek
2020-01-22 10:15 ` Julien Thierry
2020-01-17 15:03 ` [POC 12/23] livepatch: Automatically remove livepatch module when the object is freed Petr Mladek
2020-01-17 15:03 ` [POC 13/23] livepatch: Remove livepatch module when the livepatched module is unloaded Petr Mladek
2020-01-17 15:03 ` [POC 14/23] livepatch: Never block livepatch modules when the related module is being removed Petr Mladek
2020-01-17 15:03 ` [POC 15/23] livepatch: Prevent infinite loop when loading livepatch module Petr Mladek
2020-01-17 15:03 ` [POC 16/23] livepatch: Add patch into the global list early Petr Mladek
2020-01-17 15:03 ` [POC 17/23] livepatch: Load livepatches for modules when loading the main livepatch Petr Mladek
2020-01-17 15:03 ` [POC 18/23] module: Refactor add_unformed_module() Petr Mladek
2020-01-17 15:03 ` [POC 19/23] module/livepatch: Allow to use exported symbols from livepatch module for "vmlinux" Petr Mladek
2020-04-06 18:48 ` Joe Lawrence
2020-04-07 7:33 ` Miroslav Benes
2020-04-07 20:57 ` Joe Lawrence
2020-01-17 15:03 ` [POC 20/23] module/livepatch: Relocate local variables in the module loaded when the livepatch is being loaded Petr Mladek
2020-01-18 10:29 ` kbuild test robot
2020-04-03 18:00 ` Joe Lawrence
2020-01-17 15:03 ` [POC 21/23] livepatch: Remove obsolete arch_klp_init_object_loaded() Petr Mladek
2020-01-17 15:03 ` [POC 22/23] livepatch/module: Remove obsolete copy_module_elf() Petr Mladek
2020-04-03 18:03 ` Joe Lawrence
2020-01-17 15:03 ` [POC 23/23] module: Remove obsolete module_disable_ro() Petr Mladek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).