[RFC PATCH 0/2] livepatch: Avoid possible race when releasing the patch

[RFC PATCH 0/2] livepatch: Avoid possible race when releasing the patch

[Petr Mladek]

There was a long discussion about a possible race with sysfs, kobjects
when removing an unused livepatch, see
https://lkml.kernel.org/g/%3C1462190242-24731-1-git-send-email-mbenes@suse.cz%3E

This patch set tries to implement what looked the most preferred solution
from the discussion. I did my best to keep the patch definition simple.
But I am not super happy with the result.

I send the current state before I spent even more time on different
approaches.

I personally think that we might get better result if we declare
some limited structures, define them statically and then copy all
data into the final structures in a single call. I did not implement
this because it was weird on the first look but I am not sure now.

But even more I would prefer the solution with the completion.
It is already used by the module framework. It does not look
that hacky to me after all.

See the comments in the second patch for more details.


This patch set has been tested against linux-next.


Petr Mladek (2):
  livepatch: Extend the livepatch-sample patch
  livepatch: Use kobjects the right way

 include/linux/livepatch.h            |  70 +++++--
 kernel/livepatch/core.c              | 355 +++++++++++++++++++++++------------
 samples/livepatch/livepatch-sample.c | 159 +++++++++++++---
 3 files changed, 425 insertions(+), 159 deletions(-)

-- 
1.8.5.6

[RFC PATCH 1/2] livepatch: Extend the livepatch-sample patch

[Petr Mladek]

We are going to play with the livepatch API when fixing
the use of kobjects. This change makes the livepatch
more complex and better see the effects of the new API.
But it might be useful on its own.

Anyway, it adds alternative output also for:

   /proc/uptime
   /proc/consoles

and for the sysfs entries created by the module kobject_example:

    /sys/kernel/kobject_example/foo
    /sys/kernel/kobject_example/bar
    /sys/kernel/kobject_example/baz

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 samples/livepatch/livepatch-sample.c | 127 ++++++++++++++++++++++++++++++++++-
 1 file changed, 125 insertions(+), 2 deletions(-)

diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
index e34f871e69b1..b9bec5f8c725 100644
--- a/samples/livepatch/livepatch-sample.c
+++ b/samples/livepatch/livepatch-sample.c
@@ -22,8 +22,18 @@
 #include <linux/livepatch.h>
 
 /*
- * This (dumb) live patch overrides the function that prints the
- * kernel boot cmdline when /proc/cmdline is read.
+ * This (dumb) live patch overrides output from the following files
+ * that provide information about the system:
+ *
+ *	/proc/cmdline
+ *	/proc/uptime
+ *	/proc/consoles
+ *
+ * and also output from sysfs entries created by the module kobject_example:
+ *
+ *	/sys/kernel/kobject_example/foo
+ *	/sys/kernel/kobject_example/bar
+ *	/sys/kernel/kobject_example/baz
  *
  * Example:
  *
@@ -40,23 +50,136 @@
  */
 
 #include <linux/seq_file.h>
+#include <linux/kernel_stat.h>
+#include <linux/cputime.h>
+#include <linux/console.h>
+#include <linux/tty_driver.h>
+
 static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
 {
 	seq_printf(m, "%s\n", "this has been live patched");
 	return 0;
 }
 
+static int livepatch_uptime_proc_show(struct seq_file *m, void *v)
+{
+	struct timespec uptime;
+	struct timespec idle;
+	u64 idletime;
+	u64 nsec;
+	u32 rem;
+	int i;
+
+	idletime = 0;
+	for_each_possible_cpu(i)
+		idletime += (__force u64) kcpustat_cpu(i).cpustat[CPUTIME_IDLE];
+
+	get_monotonic_boottime(&uptime);
+	nsec = cputime64_to_jiffies64(idletime) * TICK_NSEC;
+	idle.tv_sec = div_u64_rem(nsec, NSEC_PER_SEC, &rem);
+	idle.tv_nsec = rem;
+	seq_printf(m, "%s\n", "this has been live patched");
+	seq_printf(m, "%lu.%02lu %lu.%02lu\n",
+			(unsigned long) uptime.tv_sec,
+			(uptime.tv_nsec / (NSEC_PER_SEC / 100)),
+			(unsigned long) idle.tv_sec,
+			(idle.tv_nsec / (NSEC_PER_SEC / 100)));
+	return 0;
+}
+
+static int livepatch_show_console_dev(struct seq_file *m, void *v)
+{
+	static const struct {
+		short flag;
+		char name;
+	} con_flags[] = {
+		{ CON_ENABLED,		'E' },
+		{ CON_CONSDEV,		'C' },
+		{ CON_BOOT,		'B' },
+		{ CON_PRINTBUFFER,	'p' },
+		{ CON_BRL,		'b' },
+		{ CON_ANYTIME,		'a' },
+	};
+	char flags[ARRAY_SIZE(con_flags) + 1];
+	struct console *con = v;
+	unsigned int a;
+	dev_t dev = 0;
+
+	seq_printf(m, "%s\n", "this has been live patched");
+
+	if (con->device) {
+		const struct tty_driver *driver;
+		int index;
+
+		driver = con->device(con, &index);
+		if (driver) {
+			dev = MKDEV(driver->major, driver->minor_start);
+			dev += index;
+		}
+	}
+
+	for (a = 0; a < ARRAY_SIZE(con_flags); a++)
+		flags[a] = (con->flags & con_flags[a].flag) ?
+			con_flags[a].name : ' ';
+	flags[a] = 0;
+
+	seq_setwidth(m, 21 - 1);
+	seq_printf(m, "%s%d", con->name, con->index);
+	seq_pad(m, ' ');
+	seq_printf(m, "%c%c%c (%s)", con->read ? 'R' : '-',
+			con->write ? 'W' : '-', con->unblank ? 'U' : '-',
+			flags);
+	if (dev)
+		seq_printf(m, " %4d:%d", MAJOR(dev), MINOR(dev));
+
+	seq_puts(m, "\n");
+
+	return 0;
+}
+
+static ssize_t livepatch_foo_show(struct kobject *kobj,
+				  struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "foo: this has been livepatched\n");
+}
+
+static ssize_t livepatch_b_show(struct kobject *kobj,
+				struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%s: this has been livepatched\n", attr->attr.name);
+}
+
 static struct klp_func funcs[] = {
 	{
 		.old_name = "cmdline_proc_show",
 		.new_func = livepatch_cmdline_proc_show,
+	}, {
+		.old_name = "uptime_proc_show",
+		.new_func = livepatch_uptime_proc_show,
+	}, {
+		.old_name = "show_console_dev",
+		.new_func = livepatch_show_console_dev,
+	}, { }
+};
+
+static struct klp_func kobject_example_funcs[] = {
+	{
+		.old_name = "foo_show",
+		.new_func = livepatch_foo_show,
+	}, {
+		.old_name = "b_show",
+		.new_func = livepatch_b_show,
 	}, { }
 };
 
+
 static struct klp_object objs[] = {
 	{
 		/* name being NULL means vmlinux */
 		.funcs = funcs,
+	}, {
+		.name = "kobject_example",
+		.funcs = kobject_example_funcs,
 	}, { }
 };
 
-- 
1.8.5.6

[RFC PATCH 2/2] livepatch: Use kobjects the right way

[Petr Mladek]

There is the following note in Documentation/kobject/txt:

  One important point cannot be overstated: every kobject must have a
  release() method, and the kobject must persist (in a consistent state)
  until that method is called. If these constraints are not met, the code
  is flawed.

The release() method is called when the reference count reaches zero. It
typically happens when kobject_put() is called. But it might be delayed
when the related sysfs file is opened for reading or writing. To find
such bugs, the release() method is delayed using a delayed work when
CONFIG_DEBUG_KOBJECT_RELEASE is defined.

Livepatch is on the safe side but only because the patch/module could
not be removed at the moment. A patchset improving the consistency
model is flying around and it would allow to remove unused patches
eventually.

I have simulated the situation when the modules can be removed
and enabled the following config options:

CONFIG_DEBUG_OBJECTS=y
CONFIG_DEBUG_OBJECTS_FREE=y
CONFIG_DEBUG_OBJECTS_TIMERS=y
CONFIG_DEBUG_OBJECTS_WORK=y
CONFIG_DEBUG_OBJECTS_RCU_HEAD=y
CONFIG_DEBUG_OBJECTS_PERCPU_COUNTER=y
CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT=1

CONFIG_DEBUG_KOBJECT=y
CONFIG_DEBUG_KOBJECT_RELEASE=y

Then I got the following crash:

dhcp75 login: BUG: unable to handle kernel paging request at ffffffffa0002048
IP: [<ffffffff812aa868>] sysfs_file_ops+0x28/0x60
PGD 1e07067 PUD 1e08063 PMD 139c9a067 PTE 0
Oops: 0000 [#1] SMP
Modules linked in: [last unloaded: livepatch_sample]
CPU: 1 PID: 5667 Comm: bash Tainted: G        W   E K 4.6.0-rc7-next-20160510-4-default+ #231
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: ffff8800ba24c740 ti: ffff880138d04000 task.ti: ffff880138d04000
RIP: 0010:[<ffffffff812aa868>]  [<ffffffff812aa868>] sysfs_file_ops+0x28/0x60
RSP: 0018:ffff880138d07dd8  EFLAGS: 00010202
RAX: 0000000000000001 RBX: ffffffffa0002020 RCX: 0000000000000000
RDX: 0000000000000001 RSI: ffff880035a51430 RDI: 0000000000000246
RBP: ffff880138d07de0 R08: 0000000000000001 R09: 0000000000000000
R10: ffff8800ba24c740 R11: 0000000000000001 R12: 0000000000000002
R13: ffff880139e617c0 R14: ffff880035813a18 R15: ffff880138d07f20
FS:  00007f13a3927700(0000) GS:ffff88013fc80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffa0002048 CR3: 0000000139961000 CR4: 00000000000006e0
Stack:
 ffff880035813a00 ffff880138d07e08 ffffffff812aa8bf ffff880035813a00
 ffff880139e617c0 0000000000000002 ffff880138d07e48 ffffffff812a9e84
 0000000000000001 ffff8800b8a13540 ffff880138d07f20 00007f13a392c000
Call Trace:
 [<ffffffff812aa8bf>] sysfs_kf_write+0x1f/0x60
 [<ffffffff812a9e84>] kernfs_fop_write+0x144/0x1e0
 [<ffffffff81222828>] __vfs_write+0x28/0x120
 [<ffffffff810c61b9>] ? percpu_down_read+0x49/0x80
 [<ffffffff812261c1>] ? __sb_start_write+0xd1/0xf0
 [<ffffffff812261c1>] ? __sb_start_write+0xd1/0xf0
 [<ffffffff81222f22>] vfs_write+0xb2/0x1b0
 [<ffffffff81244176>] ? __fget_light+0x66/0x90
 [<ffffffff81224279>] SyS_write+0x49/0xa0
 [<ffffffff819582fc>] entry_SYSCALL_64_fastpath+0x1f/0xbd
Code: 44 00 00 0f 1f 44 00 00 55 48 89 e5 53 f6 87 89 00 00 00 01 48 8b 47 28 48 8b 98 80 00 00 00 74 0a 8b 05 7c 6d c7 00 85 c0 75 10 <48> 8b 43 28 48 85 c0 74 27 48 8b 40 08 5b 5d c3 48 83 c7 08 e8
RIP  [<ffffffff812aa868>] sysfs_file_ops+0x28/0x60
 RSP <ffff880138d07dd8>
CR2: ffffffffa0002048

The problem was that struct klp_patch was defined statically in
the livepatch module. And the module was removed before the
kobject release() method was called. For a short time, we were
able to open /sys/kernel/livepatch/livepatch_sample/enabled
for writing while the related kobject was already gone.

There are two basic approaches how to get out of this trap.
Either we need to allocate all the structures dynamically
so that they survive the module removal. Or we need to block
the module removal until the sysfs entries are released[*].

This patch is an attempt of the first approach. All three
structures, klp_patch, klp_object, and klp_func, are
dynamically allocated because they all contains kobject
and related sysfs entries.

The question was how to define the user-provided data an easy way.
One approach was to keep the static definition using reduced
structures that would later be copied to dynamically allocated
structures. It is definitely worth consideration[**].

This patch tries another approach. It adds one more level of functions
that are called before the patch is registered. Then patch registration
is a clear boundary that marks the patch as complete and ready for
enabling. New objects or functions could not be added to the patch
from this point on.

The three new functions are klp_create_empty_patch(), klp_add_object(),
and klp_add_func(). They initialize most of the struct members
and contains the related code from the former klp_init_*() functions.

The information about loaded objects is still detected during
the patch registration and from the module callbacks. Also the sysfs
entries are still created during the patch registration. Note that
we should not create sysfs entries before the patch is complete
to make the handling easier. Anyway, these parts of klp_init_*() functions
are moved to klp_registration_*() functions to make it clear
in which phase they happen.

The patch avoids hardcoding the size of the arrays by using linked lists.
Then the standard list_for_each_entry() macro could be used to
iterate the list of objects and functions.

Also there are helper macros that help with error handling and make
the patch definition easier to do and review.

The last thing is a patch removal. It can be done only when the
patch is disabled. And it is done in one step. To make it symmetric,
we need to remove the sysfs entries right after the patch is
removed from the global list. The sysfs entries are handled
via the kobjects. Therefore the kobjects should be removed
at the same time together with the related structures.

All the klp_free*() function are renamed to klp_release_*()
functions to reduce a potential confusion. Note that the
structures are freed by the klp_kobj_release_*() functions
that might be called later.

[*] We might block removing the module using a completion() that
would wait until the top-level kobject release method is called.
This approach is used by the module framework itself, see
mod_kobject_put() and module_kobj_release().

It even seems to be safe. kobject_cleanup() do not longer access
any data from the kobject once the release method is called.
And it is a way how to make sure that the structure has
valid data until the release method is called.

[**] I deemed ugly to define the patch using reduced structures
and just copy them from static defined arrays to a dynamically
defined arrays.

But it would keep the definition reasonable without hardcoded
array size and other tricks. Also it would help to pass all
data in call and avoid the three levels of functions. It will
be possible to do this during the patch registration and
avoid the three levels of functions: create+add/register/enable.
Also it will avoid the asymmetry between the patch creation
and release.

Huh, I send this patch because it is working and to show how
this approach looks like. I would personally would prefer
using the completion. I think that it is not a hack
after all.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/livepatch.h            |  70 +++++--
 kernel/livepatch/core.c              | 355 +++++++++++++++++++++++------------
 samples/livepatch/livepatch-sample.c |  72 +++----
 3 files changed, 320 insertions(+), 177 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index a93a0b23dc8d..2867409eb439 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -40,6 +40,7 @@ enum klp_state {
  * @old_sympos: a hint indicating which symbol position the old function
  *		can be found (optional)
  * @old_addr:	the address of the function being patched
+ * @list:	list node for the list of patched functions in an object
  * @kobj:	kobject for sysfs resources
  * @state:	tracks function-level patch application state
  * @stack_node:	list node for klp_ops func_stack list
@@ -59,15 +60,17 @@ struct klp_func {
 
 	/* internal */
 	unsigned long old_addr;
+	struct list_head list;
+	struct list_head stack_node;
 	struct kobject kobj;
 	enum klp_state state;
-	struct list_head stack_node;
 };
 
 /**
  * struct klp_object - kernel object structure for live patching
  * @name:	module name (or NULL for vmlinux)
  * @funcs:	function entries for functions to be patched in the object
+ * @list:	list node for the list of patched objects
  * @kobj:	kobject for sysfs resources
  * @mod:	kernel module associated with the patched object
  * 		(NULL for vmlinux)
@@ -76,9 +79,10 @@ struct klp_func {
 struct klp_object {
 	/* external */
 	const char *name;
-	struct klp_func *funcs;
 
 	/* internal */
+	struct list_head funcs;
+	struct list_head list;
 	struct kobject kobj;
 	struct module *mod;
 	enum klp_state state;
@@ -95,24 +99,24 @@ struct klp_object {
 struct klp_patch {
 	/* external */
 	struct module *mod;
-	struct klp_object *objs;
 
 	/* internal */
+	struct list_head objs;
 	struct list_head list;
 	struct kobject kobj;
 	enum klp_state state;
 };
 
-#define klp_for_each_object(patch, obj) \
-	for (obj = patch->objs; obj->funcs || obj->name; obj++)
-
-#define klp_for_each_func(obj, func) \
-	for (func = obj->funcs; \
-	     func->old_name || func->new_func || func->old_sympos; \
-	     func++)
+struct klp_patch *klp_create_empty_patch(struct module *mod);
+struct klp_object *klp_add_object(struct klp_patch *patch,
+				  const char *name);
+struct klp_func *klp_add_func(struct klp_object *obj,
+			      const char *old_name,
+			      void *new_func,
+			      unsigned long old_sympos);
 
 int klp_register_patch(struct klp_patch *);
-int klp_unregister_patch(struct klp_patch *);
+int klp_release_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
 int klp_disable_patch(struct klp_patch *);
 
@@ -120,6 +124,50 @@ int klp_disable_patch(struct klp_patch *);
 int klp_module_coming(struct module *mod);
 void klp_module_going(struct module *mod);
 
+#define klp_create_patch_or_die(mod)					\
+({									\
+	struct klp_patch *__patch;					\
+									\
+	__patch = klp_create_empty_patch(mod);				\
+	if (IS_ERR(__patch)) {						\
+		pr_err("livepatch: failed to create empty patch (%ld)\n", \
+		       PTR_ERR(__patch));				\
+		return PTR_ERR(__patch);				\
+	}								\
+	__patch;							\
+})
+
+#define klp_add_object_or_die(patch, obj_name)				\
+({									\
+	struct klp_object *__obj;					\
+									\
+	__obj = klp_add_object(patch, obj_name);			\
+	if (IS_ERR(__obj)) {						\
+		pr_err("livepatch: failed to add the object '%s' for the patch '%s' (%ld)\n", \
+		       obj_name ? obj_name : "vmlinux",			\
+		       patch->mod->name, PTR_ERR(__obj));		\
+		WARN_ON(klp_release_patch(patch));			\
+		return PTR_ERR(__obj);					\
+	}								\
+	__obj;								\
+})
+
+#define klp_add_func_or_die(patch, obj, old_name, new_func, sympos)	\
+({									\
+	struct klp_func *__func;					\
+									\
+	__func = klp_add_func(obj, old_name, new_func, sympos);		\
+	if (IS_ERR(__func)) {						\
+		pr_err("livepatch: failed to add the function '%s' for the object '%s' in the patch '%s' (%ld)\n", \
+		       old_name ? old_name : "NULL",			\
+		       obj->name ? obj->name : "vmlinux",		\
+		       patch->mod->name, PTR_ERR(__func));		\
+		WARN_ON(klp_release_patch(patch));			\
+		return PTR_ERR(__func);					\
+	}								\
+	__func;								\
+})
+
 #else /* !CONFIG_LIVEPATCH */
 
 static inline int klp_module_coming(struct module *mod) { return 0; }
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5c2bc1052691..520be7dcd73a 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -451,7 +451,7 @@ static void klp_disable_object(struct klp_object *obj)
 {
 	struct klp_func *func;
 
-	klp_for_each_func(obj, func)
+	list_for_each_entry(func, &obj->funcs, list)
 		if (func->state == KLP_ENABLED)
 			klp_disable_func(func);
 
@@ -469,7 +469,7 @@ static int klp_enable_object(struct klp_object *obj)
 	if (WARN_ON(!klp_is_object_loaded(obj)))
 		return -EINVAL;
 
-	klp_for_each_func(obj, func) {
+	list_for_each_entry(func, &obj->funcs, list) {
 		ret = klp_enable_func(func);
 		if (ret) {
 			klp_disable_object(obj);
@@ -492,7 +492,7 @@ static int __klp_disable_patch(struct klp_patch *patch)
 
 	pr_notice("disabling patch '%s'\n", patch->mod->name);
 
-	klp_for_each_object(patch, obj) {
+	list_for_each_entry(obj, &patch->objs, list) {
 		if (obj->state == KLP_ENABLED)
 			klp_disable_object(obj);
 	}
@@ -552,7 +552,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
 
 	pr_notice("enabling patch '%s'\n", patch->mod->name);
 
-	klp_for_each_object(patch, obj) {
+	list_for_each_entry(obj, &patch->objs, list) {
 		if (!klp_is_object_loaded(obj))
 			continue;
 
@@ -668,10 +668,13 @@ static struct attribute *klp_patch_attrs[] = {
 
 static void klp_kobj_release_patch(struct kobject *kobj)
 {
+	struct klp_patch *patch = container_of(kobj, struct klp_patch, kobj);
+
 	/*
 	 * Once we have a consistency model we'll need to module_put() the
 	 * patch module here.  See klp_register_patch() for more details.
 	 */
+	kfree(patch);
 }
 
 static struct kobj_type klp_ktype_patch = {
@@ -682,6 +685,10 @@ static struct kobj_type klp_ktype_patch = {
 
 static void klp_kobj_release_object(struct kobject *kobj)
 {
+	struct klp_object *obj = container_of(kobj, struct klp_object, kobj);
+
+	kfree(obj->name);
+	kfree(obj);
 }
 
 static struct kobj_type klp_ktype_object = {
@@ -691,6 +698,10 @@ static struct kobj_type klp_ktype_object = {
 
 static void klp_kobj_release_func(struct kobject *kobj)
 {
+	struct klp_func *func = container_of(kobj, struct klp_func, kobj);
+
+	kfree(func->old_name);
+	kfree(func);
 }
 
 static struct kobj_type klp_ktype_func = {
@@ -699,60 +710,89 @@ static struct kobj_type klp_ktype_func = {
 };
 
 /*
- * Free all functions' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
+ * Free all klp_func structures listed for the given object.  It is called
+ * also when the patch creation or registration fails and some kobjects are
+ * not initialized.  For these, the release function must be called directly.
  */
-static void klp_free_funcs_limited(struct klp_object *obj,
-				   struct klp_func *limit)
+static void klp_release_funcs(struct klp_object *obj)
 {
-	struct klp_func *func;
-
-	for (func = obj->funcs; func->old_name && func != limit; func++)
-		kobject_put(&func->kobj);
+	struct klp_func *func, *tmp;
+
+	list_for_each_entry_safe(func, tmp, &obj->funcs, list) {
+		list_del(&func->list);
+		if (func->kobj.state_initialized)
+			kobject_put(&func->kobj);
+		else
+			klp_kobj_release_func(&func->kobj);
+	}
 }
 
 /* Clean up when a patched object is unloaded */
-static void klp_free_object_loaded(struct klp_object *obj)
+static void klp_unregister_object_loaded(struct klp_object *obj)
 {
 	struct klp_func *func;
 
 	obj->mod = NULL;
 
-	klp_for_each_func(obj, func)
+	list_for_each_entry(func, &obj->funcs, list)
 		func->old_addr = 0;
 }
 
 /*
- * Free all objects' kobjects in the array up to some limit. When limit is
- * NULL, all kobjects are freed.
+ * Free all klp_object structures listed for the given patch.  It is called
+ * also when the patch creation or registration fails and some kobjects are
+ * not initialized.  For these, the release function must be called directly.
  */
-static void klp_free_objects_limited(struct klp_patch *patch,
-				     struct klp_object *limit)
+static void klp_release_objects(struct klp_patch *patch)
 {
-	struct klp_object *obj;
-
-	for (obj = patch->objs; obj->funcs && obj != limit; obj++) {
-		klp_free_funcs_limited(obj, NULL);
-		kobject_put(&obj->kobj);
+	struct klp_object *obj, *tmp;
+
+	list_for_each_entry_safe(obj, tmp, &patch->objs, list) {
+		klp_release_funcs(obj);
+		list_del(&obj->list);
+		if (obj->kobj.state_initialized)
+			kobject_put(&obj->kobj);
+		else
+			klp_kobj_release_object(&obj->kobj);
 	}
 }
 
-static void klp_free_patch(struct klp_patch *patch)
+/**
+ * klp_release_patch() - unregisters a patch and frees all structures
+ * @patch:	Disabled patch to be released
+ *
+ * Removes the patch from the global list, removes the sysfs interface
+ * and frees all the data structures for the patch, objects, and functions.
+ *
+ * Return: 0 on success, otherwise error
+ */
+int klp_release_patch(struct klp_patch *patch)
 {
-	klp_free_objects_limited(patch, NULL);
+	int ret = 0;
+
+	mutex_lock(&klp_mutex);
+
+	if (patch->state == KLP_ENABLED) {
+		ret = -EBUSY;
+		goto err;
+	}
+
+	klp_release_objects(patch);
 	if (!list_empty(&patch->list))
 		list_del(&patch->list);
-	kobject_put(&patch->kobj);
+	if (patch->kobj.state_initialized)
+		kobject_put(&patch->kobj);
+	else
+		klp_kobj_release_patch(&patch->kobj);
+
+err:
+	mutex_unlock(&klp_mutex);
+	return ret;
 }
+EXPORT_SYMBOL_GPL(klp_release_patch);
 
-static int klp_init_func(struct klp_object *obj, struct klp_func *func)
+static int klp_register_func(struct klp_object *obj, struct klp_func *func)
 {
-	if (!func->old_name || !func->new_func)
-		return -EINVAL;
-
-	INIT_LIST_HEAD(&func->stack_node);
-	func->state = KLP_DISABLED;
-
 	/* The format for the sysfs directory is <function,sympos> where sympos
 	 * is the nth occurrence of this symbol in kallsyms for the patched
 	 * object. If the user selects 0 for old_sympos, then 1 will be used
@@ -764,7 +804,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 }
 
 /* parts of the initialization that is done only when the object is loaded */
-static int klp_init_object_loaded(struct klp_patch *patch,
+static int klp_register_object_loaded(struct klp_patch *patch,
 				  struct klp_object *obj)
 {
 	struct klp_func *func;
@@ -774,7 +814,7 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 	if (ret)
 		return ret;
 
-	klp_for_each_func(obj, func) {
+	list_for_each_entry(func, &obj->funcs, list) {
 		ret = klp_find_object_symbol(obj->name, func->old_name,
 					     func->old_sympos,
 					     &func->old_addr);
@@ -785,18 +825,12 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 	return 0;
 }
 
-static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
+static int klp_register_object(struct klp_patch *patch, struct klp_object *obj)
 {
 	struct klp_func *func;
 	int ret;
 	const char *name;
 
-	if (!obj->funcs)
-		return -EINVAL;
-
-	obj->state = KLP_DISABLED;
-	obj->mod = NULL;
-
 	klp_find_object_module(obj);
 
 	name = klp_is_module(obj) ? obj->name : "vmlinux";
@@ -805,137 +839,214 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	if (ret)
 		return ret;
 
-	klp_for_each_func(obj, func) {
-		ret = klp_init_func(obj, func);
-		if (ret)
-			goto free;
-	}
-
-	if (klp_is_object_loaded(obj)) {
-		ret = klp_init_object_loaded(patch, obj);
+	list_for_each_entry(func, &obj->funcs, list) {
+		ret = klp_register_func(obj, func);
 		if (ret)
-			goto free;
+			return ret;
 	}
 
-	return 0;
+	if (klp_is_object_loaded(obj))
+		ret = klp_register_object_loaded(patch, obj);
 
-free:
-	klp_free_funcs_limited(obj, func);
-	kobject_put(&obj->kobj);
 	return ret;
 }
 
-static int klp_init_patch(struct klp_patch *patch)
+/**
+ * klp_register_patch() - registers a patch
+ * @patch:	Patch to be registered
+ *
+ * Creates sysfs interface for the given patch, detects missing
+ * information for loaded objects, links the patch to the global list.
+ *
+ * Never add new objects of functions once the patch gets registered.
+ * These operations are not safe wrt coming or leaving modules and
+ * also wrt enabling or disabling the patch.
+ *
+ * Return: 0 on success, otherwise error
+ */
+int klp_register_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
 	int ret;
 
-	if (!patch->objs)
-		return -EINVAL;
+	if (!klp_initialized())
+		return -ENODEV;
+
+	/*
+	 * A reference is taken on the patch module to prevent it from being
+	 * unloaded.  Right now, we don't allow patch modules to unload since
+	 * there is currently no method to determine if a thread is still
+	 * running in the patched code contained in the patch module once
+	 * the ftrace registration is successful.
+	 */
+	if (!try_module_get(patch->mod))
+		return -ENODEV;
 
 	mutex_lock(&klp_mutex);
 
-	patch->state = KLP_DISABLED;
+	if (klp_is_patch_registered(patch)) {
+		ret = -EINVAL;
+		goto err;
+	}
 
 	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
 				   klp_root_kobj, "%s", patch->mod->name);
 	if (ret)
-		goto unlock;
+		goto err;
 
-	klp_for_each_object(patch, obj) {
-		ret = klp_init_object(patch, obj);
+	list_for_each_entry(obj, &patch->objs, list) {
+		ret = klp_register_object(patch, obj);
 		if (ret)
-			goto free;
+			goto err;
 	}
 
 	list_add_tail(&patch->list, &klp_patches);
 
+err:
 	mutex_unlock(&klp_mutex);
 
-	return 0;
-
-free:
-	klp_free_objects_limited(patch, obj);
-	kobject_put(&patch->kobj);
-unlock:
-	mutex_unlock(&klp_mutex);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(klp_register_patch);
 
 /**
- * klp_unregister_patch() - unregisters a patch
- * @patch:	Disabled patch to be unregistered
+ * klp_add_func() - allocate and initialize struct klp_func, link it into
+ *	the given object structure
+ * @obj:	object structure where the patched function belongs to
+ * @old_name:	name of the function to be patched
+ * @new_func:	pointer to the new function code
+ * @old_sympos: a hint indicating which symbol position the old function
+ *		can be found
  *
- * Frees the data structures and removes the sysfs interface.
+ * Allocates and initializes struct klp_func. Then it links the structure
+ * into the given object structure.
  *
- * Return: 0 on success, otherwise error
+ * The structure must be freed only using klp_release_patch() called for
+ * the related patch structure!
+ *
+ * Never add new functions once the patch is registered! You would risk
+ * an inconsistent state wrt coming or leaving modules and also wrt
+ * enabling or disabling the patch.
+ *
+ * Return: valid pointer on success, ERR_PTR otherwise.
  */
-int klp_unregister_patch(struct klp_patch *patch)
+struct klp_func *klp_add_func(struct klp_object *obj, const char *old_name,
+			      void *new_func, unsigned long old_sympos)
 {
-	int ret = 0;
+	struct klp_func *func;
 
-	mutex_lock(&klp_mutex);
+	if (!obj || !old_name || !new_func || obj->state == KLP_ENABLED)
+		return ERR_PTR(-EINVAL);
 
-	if (!klp_is_patch_registered(patch)) {
-		ret = -EINVAL;
-		goto out;
+	func = kzalloc(sizeof(*func), GFP_KERNEL);
+	if (!func)
+		return ERR_PTR(-ENOMEM);
+
+	func->old_name = kstrdup(old_name, GFP_KERNEL);
+	if (!func->old_name) {
+		kfree(func);
+		return ERR_PTR(-ENOMEM);
 	}
 
-	if (patch->state == KLP_ENABLED) {
-		ret = -EBUSY;
-		goto out;
+	func->new_func = new_func;
+	func->old_sympos = old_sympos;
+	INIT_LIST_HEAD(&func->list);
+	INIT_LIST_HEAD(&func->stack_node);
+	func->state = KLP_DISABLED;
+
+	list_add(&func->list, &obj->funcs);
+
+	return func;
+}
+EXPORT_SYMBOL_GPL(klp_add_func);
+
+/**
+ * klp_add_object() - allocate and initialize struct klp_object, link it into
+ *	to the given patch
+ * &patch	patch structure that will modify the given object
+ * @name:	name of the patched object, it is a name of a kernel module
+ *		or NULL for vmlinux
+ *
+ * Allocates and initializes struct klp_object. Links the structure
+ * into the given patch structure.
+ *
+ * The structure must be freed only using klp_release_patch() called for
+ * the related patch structure!
+ *
+ * Never add new objects once the patch is registered! You would risk
+ * an inconsistent state wrt coming or leaving modules and also wrt
+ * enabling or disabling the patch.
+ *
+ * Return: valid pointer on success, ERR_PTR otherwise.
+ */
+struct klp_object *klp_add_object(struct klp_patch *patch, const char *name)
+{
+	struct klp_object *obj;
+
+	if (!patch || !list_empty(&patch->list))
+		return ERR_PTR(-EINVAL);
+
+	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+	if (!obj)
+		return ERR_PTR(-ENOMEM);
+
+	if (name) {
+		obj->name = kstrdup(name, GFP_KERNEL);
+		if (!obj->name) {
+			kfree(obj);
+			return ERR_PTR(-ENOMEM);
+		}
 	}
 
-	klp_free_patch(patch);
+	INIT_LIST_HEAD(&obj->funcs);
+	INIT_LIST_HEAD(&obj->list);
+	obj->state = KLP_DISABLED;
+	obj->mod = NULL;
 
-out:
-	mutex_unlock(&klp_mutex);
-	return ret;
+	list_add(&obj->list, &patch->objs);
+
+	return obj;
 }
-EXPORT_SYMBOL_GPL(klp_unregister_patch);
+EXPORT_SYMBOL_GPL(klp_add_object);
 
 /**
- * klp_register_patch() - registers a patch
- * @patch:	Patch to be registered
+ * klp_create_empty_patch() - allocate and initialize struct klp_patch
+ * @mod:	kernel module that provides the livepatch
  *
- * Initializes the data structure associated with the patch and
- * creates the sysfs interface.
+ * Allocates and initializes struct klp_patch. The links to the patched
+ * objects and functions can be added using klp_add_object() and
+ * klp_add_func().
  *
- * Return: 0 on success, otherwise error
+ * The structure must be freed only using klp_release_patch()!
+ *
+ * Return: valid pointer on success, ERR_PTR otherwise.
  */
-int klp_register_patch(struct klp_patch *patch)
+struct klp_patch *klp_create_empty_patch(struct module *mod)
 {
-	int ret;
+	struct klp_patch *patch;
 
-	if (!patch || !patch->mod)
-		return -EINVAL;
+	if (!mod)
+		return ERR_PTR(-EINVAL);
 
-	if (!is_livepatch_module(patch->mod)) {
-		pr_err("module %s is not marked as a livepatch module",
-		       patch->mod->name);
-		return -EINVAL;
+	if (!is_livepatch_module(mod)) {
+		pr_err("module '%s' is not marked as a livepatch module\n",
+		       mod->name);
+		return ERR_PTR(-EINVAL);
 	}
 
-	if (!klp_initialized())
-		return -ENODEV;
+	patch = kzalloc(sizeof(*patch), GFP_KERNEL);
+	if (!patch)
+		return ERR_PTR(-ENOMEM);
 
-	/*
-	 * A reference is taken on the patch module to prevent it from being
-	 * unloaded.  Right now, we don't allow patch modules to unload since
-	 * there is currently no method to determine if a thread is still
-	 * running in the patched code contained in the patch module once
-	 * the ftrace registration is successful.
-	 */
-	if (!try_module_get(patch->mod))
-		return -ENODEV;
+	INIT_LIST_HEAD(&patch->objs);
+	INIT_LIST_HEAD(&patch->list);
+	patch->state = KLP_DISABLED;
+	patch->mod = mod;
 
-	ret = klp_init_patch(patch);
-	if (ret)
-		module_put(patch->mod);
+	return patch;
 
-	return ret;
 }
-EXPORT_SYMBOL_GPL(klp_register_patch);
+EXPORT_SYMBOL_GPL(klp_create_empty_patch);
 
 int klp_module_coming(struct module *mod)
 {
@@ -955,13 +1066,13 @@ int klp_module_coming(struct module *mod)
 	mod->klp_alive = true;
 
 	list_for_each_entry(patch, &klp_patches, list) {
-		klp_for_each_object(patch, obj) {
+		list_for_each_entry(obj, &patch->objs, list) {
 			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
 				continue;
 
 			obj->mod = mod;
 
-			ret = klp_init_object_loaded(patch, obj);
+			ret = klp_register_object_loaded(patch, obj);
 			if (ret) {
 				pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n",
 					patch->mod->name, obj->mod->name, ret);
@@ -997,7 +1108,7 @@ err:
 	pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
 		patch->mod->name, obj->mod->name, obj->mod->name);
 	mod->klp_alive = false;
-	klp_free_object_loaded(obj);
+	klp_unregister_object_loaded(obj);
 	mutex_unlock(&klp_mutex);
 
 	return ret;
@@ -1021,7 +1132,7 @@ void klp_module_going(struct module *mod)
 	mod->klp_alive = false;
 
 	list_for_each_entry(patch, &klp_patches, list) {
-		klp_for_each_object(patch, obj) {
+		list_for_each_entry(obj, &patch->objs, list) {
 			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
 				continue;
 
@@ -1031,7 +1142,7 @@ void klp_module_going(struct module *mod)
 				klp_disable_object(obj);
 			}
 
-			klp_free_object_loaded(obj);
+			klp_unregister_object_loaded(obj);
 			break;
 		}
 	}
diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
index b9bec5f8c725..187e4eab6446 100644
--- a/samples/livepatch/livepatch-sample.c
+++ b/samples/livepatch/livepatch-sample.c
@@ -149,55 +149,39 @@ static ssize_t livepatch_b_show(struct kobject *kobj,
 	return sprintf(buf, "%s: this has been livepatched\n", attr->attr.name);
 }
 
-static struct klp_func funcs[] = {
-	{
-		.old_name = "cmdline_proc_show",
-		.new_func = livepatch_cmdline_proc_show,
-	}, {
-		.old_name = "uptime_proc_show",
-		.new_func = livepatch_uptime_proc_show,
-	}, {
-		.old_name = "show_console_dev",
-		.new_func = livepatch_show_console_dev,
-	}, { }
-};
-
-static struct klp_func kobject_example_funcs[] = {
-	{
-		.old_name = "foo_show",
-		.new_func = livepatch_foo_show,
-	}, {
-		.old_name = "b_show",
-		.new_func = livepatch_b_show,
-	}, { }
-};
-
-
-static struct klp_object objs[] = {
-	{
-		/* name being NULL means vmlinux */
-		.funcs = funcs,
-	}, {
-		.name = "kobject_example",
-		.funcs = kobject_example_funcs,
-	}, { }
-};
-
-static struct klp_patch patch = {
-	.mod = THIS_MODULE,
-	.objs = objs,
-};
+static struct klp_patch *patch;
 
 static int livepatch_init(void)
 {
+	struct klp_object *obj;
 	int ret;
 
-	ret = klp_register_patch(&patch);
-	if (ret)
+	/* create empty patch structure */
+	patch = klp_create_patch_or_die(THIS_MODULE);
+
+	/* add info about changes against vmlinux */
+	obj = klp_add_object_or_die(patch, NULL);
+	klp_add_func_or_die(patch, obj, "cmdline_proc_show",
+			    livepatch_cmdline_proc_show, 0);
+	klp_add_func_or_die(patch, obj, "uptime_proc_show",
+			    livepatch_uptime_proc_show, 0);
+	klp_add_func_or_die(patch, obj, "show_console_dev",
+			    livepatch_show_console_dev, 0);
+
+	/* add info about changes against the module kobject_example */
+	obj = klp_add_object_or_die(patch, "kobject_example");
+	klp_add_func_or_die(patch, obj, "foo_show", livepatch_foo_show, 0);
+	klp_add_func_or_die(patch, obj, "b_show", livepatch_b_show, 0);
+
+	ret = klp_register_patch(patch);
+	if (ret) {
+		WARN_ON(klp_release_patch(patch));
 		return ret;
-	ret = klp_enable_patch(&patch);
+	}
+
+	ret = klp_enable_patch(patch);
 	if (ret) {
-		WARN_ON(klp_unregister_patch(&patch));
+		WARN_ON(klp_release_patch(patch));
 		return ret;
 	}
 	return 0;
@@ -205,8 +189,8 @@ static int livepatch_init(void)
 
 static void livepatch_exit(void)
 {
-	WARN_ON(klp_disable_patch(&patch));
-	WARN_ON(klp_unregister_patch(&patch));
+	WARN_ON(klp_disable_patch(patch));
+	WARN_ON(klp_release_patch(patch));
 }
 
 module_init(livepatch_init);
-- 
1.8.5.6

Re: [RFC PATCH 0/2] livepatch: Avoid possible race when releasing the patch

[Josh Poimboeuf]

On Mon, May 23, 2016 at 05:54:06PM +0200, Petr Mladek wrote:
> There was a long discussion about a possible race with sysfs, kobjects
> when removing an unused livepatch, see
> https://lkml.kernel.org/g/%3C1462190242-24731-1-git-send-email-mbenes@suse.cz%3E
> 
> This patch set tries to implement what looked the most preferred solution
> from the discussion. I did my best to keep the patch definition simple.
> But I am not super happy with the result.

Hi Petr,

Thanks a lot for looking at this.  I'm also not crazy about this patch,
but it does help with understanding the proposal.

> I send the current state before I spent even more time on different
> approaches.
> 
> I personally think that we might get better result if we declare
> some limited structures, define them statically and then copy all
> data into the final structures in a single call. I did not implement
> this because it was weird on the first look but I am not sure now.

I agree that would be better than this patch.  In fact, IIRC, that's
what Seth first implemented in the original livepatch patches, but then
the rest of us complained about it ;-)

It would be a clean separation of struct lifetimes, would allow us to
get rid of the "external" and "internal" comments in livepatch.h, and
would reduce the possibility of the user accidentally messing with our
internal state.

But it would be a little unusual to have two sets of structs (private
and public), and it would require some additional boilerplate code to do
the copying.

But even so, I think I would slightly prefer it over anything else.

> But even more I would prefer the solution with the completion.
> It is already used by the module framework. It does not look
> that hacky to me after all.

The completion would be the easiest solution.  In my mind it would be a
hack, but reasonable minds can disagree about that :-)  But anyway, I
could live with it.  I don't have a strong preference either way.

-- 
Josh

Re: livepatch: Avoid possible race when releasing the patch

[Jessica Yu]

+++ Petr Mladek [23/05/16 17:54 +0200]:
>There was a long discussion about a possible race with sysfs, kobjects
>when removing an unused livepatch, see
>https://lkml.kernel.org/g/%3C1462190242-24731-1-git-send-email-mbenes@suse.cz%3E
>
>This patch set tries to implement what looked the most preferred solution
>from the discussion. I did my best to keep the patch definition simple.
>But I am not super happy with the result.
>
>I send the current state before I spent even more time on different
>approaches.
>
>I personally think that we might get better result if we declare
>some limited structures, define them statically and then copy all
>data into the final structures in a single call. I did not implement
>this because it was weird on the first look but I am not sure now.
>
>But even more I would prefer the solution with the completion.
>It is already used by the module framework. It does not look
>that hacky to me after all.

Hi Petr, thanks a lot for the RFC and for exploring this possible
solution. I haven't reviewed the patches thoroughly yet, but at first
glance I admit that I did not think through how much this approach
would complicate the livepatch API, and the new intermediary functions
do seem like overkill in response to the original kobject problem..

I looked at how the module loader used the completion, and in fact
it is used to remedy a nearly identical problem with
DEBUG_KOBJ_RELEASE (see commit 942e443 "Fix mod->mkobj.kobj
potentially freed too early"), and Miroslav's original solution pretty
much took the same approach. We could even mirror that approach and
have something like klp_kobject_put() (much like mod_kobject_put()) to
package up the kobject_put/wait_for_completion calls, but that is
purely a matter of taste.

Anyway, I am just beginning to lean towards the completion solution
again (sorry for jumping back and forth :-/), but we can play with
this patchset a bit more and see if we can come up with something
reasonable.

Thanks,
Jessica

Re: livepatch: Avoid possible race when releasing the patch

[Miroslav Benes]

On Mon, 23 May 2016, Jessica Yu wrote:

> +++ Petr Mladek [23/05/16 17:54 +0200]:
> > There was a long discussion about a possible race with sysfs, kobjects
> > when removing an unused livepatch, see
> > https://lkml.kernel.org/g/%3C1462190242-24731-1-git-send-email-mbenes@suse.cz%3E
> > 
> > This patch set tries to implement what looked the most preferred solution
> > from the discussion. I did my best to keep the patch definition simple.
> > But I am not super happy with the result.
> > 
> > I send the current state before I spent even more time on different
> > approaches.
> > 
> > I personally think that we might get better result if we declare
> > some limited structures, define them statically and then copy all
> > data into the final structures in a single call. I did not implement
> > this because it was weird on the first look but I am not sure now.
> > 
> > But even more I would prefer the solution with the completion.
> > It is already used by the module framework. It does not look
> > that hacky to me after all.
> 
> Hi Petr, thanks a lot for the RFC and for exploring this possible
> solution. I haven't reviewed the patches thoroughly yet, but at first
> glance I admit that I did not think through how much this approach
> would complicate the livepatch API, and the new intermediary functions
> do seem like overkill in response to the original kobject problem..
>
> I looked at how the module loader used the completion, and in fact
> it is used to remedy a nearly identical problem with
> DEBUG_KOBJ_RELEASE (see commit 942e443 "Fix mod->mkobj.kobj
> potentially freed too early"), and Miroslav's original solution pretty
> much took the same approach. We could even mirror that approach and
> have something like klp_kobject_put() (much like mod_kobject_put()) to
> package up the kobject_put/wait_for_completion calls, but that is
> purely a matter of taste.

Hi,

I'm biased here so it is not surprising that I'd go with completion. There 
is even one more thing to be aware of. We have 'struct module *mod' in 
klp_patch and we use it throughout the code. We still need to be careful 
with it even with Petr's approach. The problem stays but it is greatly
diminished to just this one pointer. That is one can call our sysfs 
function which potentially uses mod pointer and the module could go away 
just before that.
 
> Anyway, I am just beginning to lean towards the completion solution
> again (sorry for jumping back and forth :-/), but we can play with
> this patchset a bit more and see if we can come up with something
> reasonable.

Yes. In fact it does not look that bad. Thanks Petr for doing this.

Josh, I agree that we could return to Seth's approach but it still seems a 
bit awkward. Completion is only a small modification and since module 
loader uses it itself it does not look like serious hack to me anymore.

Regards,
Miroslav

Re: livepatch: Avoid possible race when releasing the patch

[Petr Mladek]

On Wed 2016-05-25 10:58:38, Miroslav Benes wrote:
> On Mon, 23 May 2016, Jessica Yu wrote:
> 
> > +++ Petr Mladek [23/05/16 17:54 +0200]:
> > > There was a long discussion about a possible race with sysfs, kobjects
> > > when removing an unused livepatch, see
> > > https://lkml.kernel.org/g/%3C1462190242-24731-1-git-send-email-mbenes@suse.cz%3E
> > > 
> > > This patch set tries to implement what looked the most preferred solution
> > > from the discussion. I did my best to keep the patch definition simple.
> > > But I am not super happy with the result.
> > > 
> > > I send the current state before I spent even more time on different
> > > approaches.
> > > 
> > > I personally think that we might get better result if we declare
> > > some limited structures, define them statically and then copy all
> > > data into the final structures in a single call. I did not implement
> > > this because it was weird on the first look but I am not sure now.
> > > 
> > > But even more I would prefer the solution with the completion.
> > > It is already used by the module framework. It does not look
> > > that hacky to me after all.
> > 
> > Hi Petr, thanks a lot for the RFC and for exploring this possible
> > solution. I haven't reviewed the patches thoroughly yet, but at first
> > glance I admit that I did not think through how much this approach
> > would complicate the livepatch API, and the new intermediary functions
> > do seem like overkill in response to the original kobject problem..
> >
> > I looked at how the module loader used the completion, and in fact
> > it is used to remedy a nearly identical problem with
> > DEBUG_KOBJ_RELEASE (see commit 942e443 "Fix mod->mkobj.kobj
> > potentially freed too early"), and Miroslav's original solution pretty
> > much took the same approach. We could even mirror that approach and
> > have something like klp_kobject_put() (much like mod_kobject_put()) to
> > package up the kobject_put/wait_for_completion calls, but that is
> > purely a matter of taste.
> 
> Hi,
> 
> I'm biased here so it is not surprising that I'd go with completion. There 
> is even one more thing to be aware of. We have 'struct module *mod' in 
> klp_patch and we use it throughout the code. We still need to be careful 
> with it even with Petr's approach. The problem stays but it is greatly
> diminished to just this one pointer.

Yeah, I forgot to mention that we are actually not able to make
patch->mod pointer valid without the completion.

I gave it another week and I have got even more convinced that the
completion would be the right way to go. It is not that hacky after all
and it might safe us some headaches in the future. IMHO,
it is too painful to copy all information from the module just
to make kobjects happy.

Best Regards,
Petr

Re: livepatch: Avoid possible race when releasing the patch

[Josh Poimboeuf]

On Mon, May 30, 2016 at 05:31:03PM +0200, Petr Mladek wrote:
> On Wed 2016-05-25 10:58:38, Miroslav Benes wrote:
> > On Mon, 23 May 2016, Jessica Yu wrote:
> > 
> > > +++ Petr Mladek [23/05/16 17:54 +0200]:
> > > > There was a long discussion about a possible race with sysfs, kobjects
> > > > when removing an unused livepatch, see
> > > > https://lkml.kernel.org/g/%3C1462190242-24731-1-git-send-email-mbenes@suse.cz%3E
> > > > 
> > > > This patch set tries to implement what looked the most preferred solution
> > > > from the discussion. I did my best to keep the patch definition simple.
> > > > But I am not super happy with the result.
> > > > 
> > > > I send the current state before I spent even more time on different
> > > > approaches.
> > > > 
> > > > I personally think that we might get better result if we declare
> > > > some limited structures, define them statically and then copy all
> > > > data into the final structures in a single call. I did not implement
> > > > this because it was weird on the first look but I am not sure now.
> > > > 
> > > > But even more I would prefer the solution with the completion.
> > > > It is already used by the module framework. It does not look
> > > > that hacky to me after all.
> > > 
> > > Hi Petr, thanks a lot for the RFC and for exploring this possible
> > > solution. I haven't reviewed the patches thoroughly yet, but at first
> > > glance I admit that I did not think through how much this approach
> > > would complicate the livepatch API, and the new intermediary functions
> > > do seem like overkill in response to the original kobject problem..
> > >
> > > I looked at how the module loader used the completion, and in fact
> > > it is used to remedy a nearly identical problem with
> > > DEBUG_KOBJ_RELEASE (see commit 942e443 "Fix mod->mkobj.kobj
> > > potentially freed too early"), and Miroslav's original solution pretty
> > > much took the same approach. We could even mirror that approach and
> > > have something like klp_kobject_put() (much like mod_kobject_put()) to
> > > package up the kobject_put/wait_for_completion calls, but that is
> > > purely a matter of taste.
> > 
> > Hi,
> > 
> > I'm biased here so it is not surprising that I'd go with completion. There 
> > is even one more thing to be aware of. We have 'struct module *mod' in 
> > klp_patch and we use it throughout the code. We still need to be careful 
> > with it even with Petr's approach. The problem stays but it is greatly
> > diminished to just this one pointer.
> 
> Yeah, I forgot to mention that we are actually not able to make
> patch->mod pointer valid without the completion.
> 
> I gave it another week and I have got even more convinced that the
> completion would be the right way to go. It is not that hacky after all
> and it might safe us some headaches in the future. IMHO,
> it is too painful to copy all information from the module just
> to make kobjects happy.

Ok, the completion sounds good to me.

-- 
Josh

Re: livepatch: Avoid possible race when releasing the patch

[Jessica Yu]

+++ Petr Mladek [30/05/16 17:31 +0200]:
>On Wed 2016-05-25 10:58:38, Miroslav Benes wrote:
>> On Mon, 23 May 2016, Jessica Yu wrote:
>>
>> > +++ Petr Mladek [23/05/16 17:54 +0200]:
>> > > There was a long discussion about a possible race with sysfs, kobjects
>> > > when removing an unused livepatch, see
>> > > https://lkml.kernel.org/g/%3C1462190242-24731-1-git-send-email-mbenes@suse.cz%3E
>> > >
>> > > This patch set tries to implement what looked the most preferred solution
>> > > from the discussion. I did my best to keep the patch definition simple.
>> > > But I am not super happy with the result.
>> > >
>> > > I send the current state before I spent even more time on different
>> > > approaches.
>> > >
>> > > I personally think that we might get better result if we declare
>> > > some limited structures, define them statically and then copy all
>> > > data into the final structures in a single call. I did not implement
>> > > this because it was weird on the first look but I am not sure now.
>> > >
>> > > But even more I would prefer the solution with the completion.
>> > > It is already used by the module framework. It does not look
>> > > that hacky to me after all.
>> >
>> > Hi Petr, thanks a lot for the RFC and for exploring this possible
>> > solution. I haven't reviewed the patches thoroughly yet, but at first
>> > glance I admit that I did not think through how much this approach
>> > would complicate the livepatch API, and the new intermediary functions
>> > do seem like overkill in response to the original kobject problem..
>> >
>> > I looked at how the module loader used the completion, and in fact
>> > it is used to remedy a nearly identical problem with
>> > DEBUG_KOBJ_RELEASE (see commit 942e443 "Fix mod->mkobj.kobj
>> > potentially freed too early"), and Miroslav's original solution pretty
>> > much took the same approach. We could even mirror that approach and
>> > have something like klp_kobject_put() (much like mod_kobject_put()) to
>> > package up the kobject_put/wait_for_completion calls, but that is
>> > purely a matter of taste.
>>
>> Hi,
>>
>> I'm biased here so it is not surprising that I'd go with completion. There
>> is even one more thing to be aware of. We have 'struct module *mod' in
>> klp_patch and we use it throughout the code. We still need to be careful
>> with it even with Petr's approach. The problem stays but it is greatly
>> diminished to just this one pointer.
>
>Yeah, I forgot to mention that we are actually not able to make
>patch->mod pointer valid without the completion.
>
>I gave it another week and I have got even more convinced that the
>completion would be the right way to go. It is not that hacky after all
>and it might safe us some headaches in the future. IMHO,
>it is too painful to copy all information from the module just
>to make kobjects happy.

Ah yeah, good catch. So even if livepatch internally managed these
structures, the mod pointer in klp_patch still wouldn't be protected,
which the completion trivially fixes. So I'd be OK with the completion
as well. Thanks for the RFC though Petr, in my head it sounded like a
good idea in theory, but in practice turned out to be too cumbersome.

Jessica