linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v0 0/3] additional cumulative livepatch doc/samples
@ 2018-02-23 21:33 Joe Lawrence
  2018-02-23 21:33 ` [PATCH v0 1/3] livepatch: add sample cumulative patch Joe Lawrence
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Joe Lawrence @ 2018-02-23 21:33 UTC (permalink / raw)
  To: live-patching, linux-kernel
  Cc: Josh Poimboeuf, Jessica Yu, Jiri Kosina, Miroslav Benes,
	Petr Mladek, Jason Baron, Evgenii Shatokhin

Hi Miroslav,

This is a follow up to my comment on "Re: [PATCH v8 8/8] livepatch:
Atomic replace and cumulative patches documentation".

Here's what I was working on today, I can update for v9 and squash into
a single patch or two if that fits into the patchset better.  (Or Petr,
feel free to grab these and run with them if you prefer.)

I definitely agree that the complexity of the scenarios and cornercases
is starting to get out of hand, at least in keeping it all in my brain
for any period of time :)

I like the idea of transforming the growing sample set into a testsuite
of some kind.  Having regression tests would ease the burden of
reviewing patches and accounting for all these use cases!

Hope these help, let me know if you'd like any modification or other
tests.

Joe Lawrence (3):
  livepatch: add sample cumulative patch
  livepatch: update documentation/samples for callbacks
  livepatch: update documentation for shadow variables

 Documentation/livepatch/callbacks.txt         | 102 ++++++++++++
 Documentation/livepatch/shadow-vars.txt       |  24 +++
 samples/livepatch/Makefile                    |   2 +
 samples/livepatch/livepatch-callbacks-demo2.c | 162 +++++++++++++++++++
 samples/livepatch/livepatch-cumulative.c      | 216 ++++++++++++++++++++++++++
 5 files changed, 506 insertions(+)
 create mode 100644 samples/livepatch/livepatch-callbacks-demo2.c
 create mode 100644 samples/livepatch/livepatch-cumulative.c

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH v0 1/3] livepatch: add sample cumulative patch
  2018-02-23 21:33 [PATCH v0 0/3] additional cumulative livepatch doc/samples Joe Lawrence
@ 2018-02-23 21:33 ` Joe Lawrence
  2018-02-25  1:38   ` Philippe Ombredanne
  2018-02-27 11:37   ` Miroslav Benes
  2018-02-23 21:33 ` [PATCH v0 2/3] livepatch: update documentation/samples for callbacks Joe Lawrence
  2018-02-23 21:33 ` [PATCH v0 3/3] livepatch: update documentation for shadow variables Joe Lawrence
  2 siblings, 2 replies; 16+ messages in thread
From: Joe Lawrence @ 2018-02-23 21:33 UTC (permalink / raw)
  To: live-patching, linux-kernel
  Cc: Josh Poimboeuf, Jessica Yu, Jiri Kosina, Miroslav Benes,
	Petr Mladek, Jason Baron, Evgenii Shatokhin

Add a simple atomic replace / cumulative livepatch example.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 samples/livepatch/Makefile               |   1 +
 samples/livepatch/livepatch-cumulative.c | 216 +++++++++++++++++++++++++++++++
 2 files changed, 217 insertions(+)
 create mode 100644 samples/livepatch/livepatch-cumulative.c

diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 2472ce39a18d..dd0e2a8af1af 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -5,3 +5,4 @@ obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-cumulative.o
diff --git a/samples/livepatch/livepatch-cumulative.c b/samples/livepatch/livepatch-cumulative.c
new file mode 100644
index 000000000000..ab036439e08c
--- /dev/null
+++ b/samples/livepatch/livepatch-cumulative.c
@@ -0,0 +1,216 @@
+/*
+ * Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * livepatch-callbacks-cumulative.c - atomic replace / cumulative livepatch demo
+ *
+ *
+ * Purpose
+ * -------
+ *
+ * Demonstration of atomic replace / cumulative livepatching.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * Step 1 - Load the sample livepatch demo
+ *
+ *   insmod samples/livepatch/livepatch-sample.ko
+ *
+ * Notice that /proc/cmdline was modified by the patch.  For the moment,
+ * /proc/meminfo remains unmodified.
+ *
+ *   head /proc/cmdline /proc/meminfo
+ *   ==> /proc/cmdline <==
+ *   this has been live patched
+ *
+ *   ==> /proc/meminfo <==
+ *   MemTotal:        4041368 kB
+ *   MemFree:         3323504 kB
+ *   MemAvailable:    3619968 kB
+ *   Buffers:            2108 kB
+ *   Cached:           484696 kB
+ *   SwapCached:            0 kB
+ *   Active:           297960 kB
+ *   Inactive:         262964 kB
+ *   Active(anon):      74296 kB
+ *   Inactive(anon):     8300 kB
+ *
+ *
+ * Step 2 - Load a second patch (on top of sample)
+ *
+ *   insmod samples/livepatch/livepatch-cumulative.ko replace=0
+ *
+ * The second livepatch adds a modification to meminfo_proc_show(),
+ * changing the output of /proc/meminfo.  In this case, the second
+ * livepatch *supplements* the features of the first:
+ *
+ *   head /proc/cmdline /proc/meminfo
+ *   ==> /proc/cmdline <==
+ *   this has been live patched
+ *
+ *   ==> /proc/meminfo <==
+ *   this has been live patched
+ *
+ * and module references and livepatch enable counts reflect both
+ * livepatches accordingly:
+ *
+ *   lsmod | grep livepatch
+ *   livepatch_cumulative    16384  1
+ *   livepatch_sample       16384  1
+ *
+ *   head /sys/kernel/livepatch/livepatch_{cumulative,sample}/enabled
+ *   ==> /sys/kernel/livepatch/livepatch_cumulative/enabled <==
+ *   1
+ *
+ *   ==> /sys/kernel/livepatch/livepatch_sample/enabled <==
+ *   1
+ *
+ *
+ * Step 3 - Remove the second patch
+ *
+ *   echo 0 > /sys/kernel/livepatch/livepatch_cumulative/enabled
+ *   rmmod livepatch-cumulative
+ *
+ *
+ * Step 4 - Load a second patch in atomic replace mode
+ *
+ *   insmod samples/livepatch/livepatch-cumulative.ko replace=1
+ *
+ * This time, notice that the second patch has *replaced* the features of
+ * the first place:
+ *
+ *   head /proc/cmdline /proc/meminfo
+ *   ==> /proc/cmdline <==
+ *   BOOT_IMAGE=/vmlinuz-4.16.0-rc2+ root=/dev/mapper/centos-root ro console=tty0 console=ttyS0,115200 rd_NO_PLYMOUTH crashkernel=auto rd.lvm.lv=centos/root rd.lvm.lv=centos/swap rhgb quiet LANG=en_US.UTF-8
+ *
+ *   ==> /proc/meminfo <==
+ *   this has been live patched
+ *
+ * The first patch is automatically disabled:
+ *
+ *   lsmod | grep livepatch
+ *   livepatch_cumulative    16384  1
+ *   livepatch_sample       16384  0
+ *
+ *   head /sys/kernel/livepatch/livepatch_{cumulative,sample}/enabled
+ *   ==> /sys/kernel/livepatch/livepatch_cumulative/enabled <==
+ *   1
+ *
+ *   ==> /sys/kernel/livepatch/livepatch_sample/enabled <==
+ *   0
+ *
+ *
+ * Step 5 - Clean up
+ *
+ * Since the first patch was replaced, it is already disabled and its
+ * module may be removed:
+ *
+ *   rmmod livepatch_sample
+ *   echo 0 > /sys/kernel/livepatch/livepatch_cumulative/enabled
+ *   rmmod livepatch-cumulative
+ */
+
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Joe Lawrence <joe.lawrence@redhat.com>");
+MODULE_DESCRIPTION("Livepatch atomic replace demo");
+
+static int replace;
+module_param(replace, int, 0644);
+MODULE_PARM_DESC(replace, "replace (default=0)");
+
+#if 0
+/* Cumulative patches don't need to re-introduce original functions in
+ * order to "revert" them from previous livepatches.
+ *
+ * - If this module is loaded in atomic replace mode, the ftrace
+ *   handlers (and therefore previous livepatches) will be removed from
+ *   cmdline_proc_show().  The latest cumulative patch contains all
+ *   modified code.
+ *
+ * - Otherwise, by default livepatches supplement each other, and we'd
+ *   need to provide a fresh copy of cmdline_proc_show() to revert its
+ *   behavior.
+ */
+static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
+{
+	seq_printf(m, "%s\n", saved_command_line);
+	return 0;
+}
+#endif
+
+#include <linux/seq_file.h>
+static int livepatch_meminfo_proc_show(struct seq_file *m, void *v)
+{
+	seq_printf(m, "%s\n", "this has been live patched");
+	return 0;
+}
+
+static struct klp_func funcs[] = {
+	{
+		.old_name = "meminfo_proc_show",
+		.new_func = livepatch_meminfo_proc_show,
+	}, { }
+};
+
+static struct klp_object objs[] = {
+	{
+		/* name being NULL means vmlinux */
+		.funcs = funcs,
+	}, { }
+};
+
+static struct klp_patch patch = {
+	.mod = THIS_MODULE,
+	.objs = objs,
+	/* set .replace in the init function below for demo purposes */
+};
+
+static int livepatch_init(void)
+{
+	int ret;
+
+	patch.replace = replace;
+
+	ret = klp_register_patch(&patch);
+	if (ret)
+		return ret;
+	ret = klp_enable_patch(&patch);
+	if (ret) {
+		WARN_ON(klp_unregister_patch(&patch));
+		return ret;
+	}
+	return 0;
+}
+
+static void livepatch_exit(void)
+{
+	WARN_ON(klp_unregister_patch(&patch));
+}
+
+module_init(livepatch_init);
+module_exit(livepatch_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v0 2/3] livepatch: update documentation/samples for callbacks
  2018-02-23 21:33 [PATCH v0 0/3] additional cumulative livepatch doc/samples Joe Lawrence
  2018-02-23 21:33 ` [PATCH v0 1/3] livepatch: add sample cumulative patch Joe Lawrence
@ 2018-02-23 21:33 ` Joe Lawrence
  2018-02-27 12:36   ` Miroslav Benes
  2018-02-23 21:33 ` [PATCH v0 3/3] livepatch: update documentation for shadow variables Joe Lawrence
  2 siblings, 1 reply; 16+ messages in thread
From: Joe Lawrence @ 2018-02-23 21:33 UTC (permalink / raw)
  To: live-patching, linux-kernel
  Cc: Josh Poimboeuf, Jessica Yu, Jiri Kosina, Miroslav Benes,
	Petr Mladek, Jason Baron, Evgenii Shatokhin

Update livepatch callback documentation and samples with respect to new
atomic replace / cumulative patch functionality.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 Documentation/livepatch/callbacks.txt         | 102 ++++++++++++++++
 samples/livepatch/Makefile                    |   1 +
 samples/livepatch/livepatch-callbacks-demo2.c | 162 ++++++++++++++++++++++++++
 3 files changed, 265 insertions(+)
 create mode 100644 samples/livepatch/livepatch-callbacks-demo2.c

diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt
index c9776f48e458..b5e67975c5a9 100644
--- a/Documentation/livepatch/callbacks.txt
+++ b/Documentation/livepatch/callbacks.txt
@@ -86,6 +86,13 @@ If the object did successfully patch, but the patch transition never
 started for some reason (e.g., if another object failed to patch),
 only the post-unpatch callback will be called.
 
+If a livepatch is replaced by a cumulative patch, then only the
+callbacks belonging to the cumulative patch will be executed.  This
+simplifies the livepatching core for it is the responsibility of the
+cumulative patch to safely revert whatever needs to be reverted.  See
+Documentation/livepatch/cumulative.txt for more information on such
+patches.
+
 
 Example Use-cases
 =================
@@ -603,3 +610,98 @@ pre-unpatch callbacks are skipped:
   % rmmod samples/livepatch/livepatch-callbacks-busymod.ko
   [  141.279111] livepatch_callbacks_busymod: busymod_work_func exit
   [  141.279760] livepatch_callbacks_busymod: livepatch_callbacks_mod_exit
+
+
+Test 10
+-------
+
+Test loading multiple livepatch modules containing callback routines.
+The livepatching core executes callbacks for all modules.
+
+- load livepatch
+- load second livepatch
+- disable livepatch
+- disable second livepatch
+- unload livepatch
+- unload second livepatch
+
+  % insmod samples/livepatch/livepatch-callbacks-demo.ko
+  [  216.448208] livepatch: enabling patch 'livepatch_callbacks_demo'
+  [  216.448211] livepatch: 'livepatch_callbacks_demo': initializing patching transition
+  [  216.448330] livepatch_callbacks_demo: pre_patch_callback: vmlinux
+  [  216.448341] livepatch: 'livepatch_callbacks_demo': starting patching transition
+  [  218.720099] livepatch: 'livepatch_callbacks_demo': completing patching transition
+  [  218.720179] livepatch_callbacks_demo: post_patch_callback: vmlinux
+  [  218.720180] livepatch: 'livepatch_callbacks_demo': patching complete
+
+  % insmod samples/livepatch/livepatch-callbacks-demo2.ko
+  [  220.126552] livepatch: enabling patch 'livepatch_callbacks_demo2'
+  [  220.126554] livepatch: 'livepatch_callbacks_demo2': initializing patching transition
+  [  220.126592] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
+  [  220.126593] livepatch: 'livepatch_callbacks_demo2': starting patching transition
+  [  221.728091] livepatch: 'livepatch_callbacks_demo2': completing patching transition
+  [  221.728254] livepatch_callbacks_demo2: post_patch_callback: vmlinux
+  [  221.728255] livepatch: 'livepatch_callbacks_demo2': patching complete
+
+  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
+  [  223.434556] livepatch: 'livepatch_callbacks_demo2': initializing unpatching transition
+  [  223.434616] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux
+  [  223.434617] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
+  [  224.736159] livepatch: 'livepatch_callbacks_demo2': completing unpatching transition
+  [  224.736660] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux
+  [  224.736662] livepatch: 'livepatch_callbacks_demo2': unpatching complete
+
+  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+  [  227.284070] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+  [  227.284111] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
+  [  227.284112] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
+  [  228.704142] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
+  [  228.704215] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
+  [  228.704216] livepatch: 'livepatch_callbacks_demo': unpatching complete
+
+  % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
+  % rmmod samples/livepatch/livepatch-callbacks-demo.ko
+
+
+Test 11
+-------
+
+A similar test as the previous one, except this time load the second
+callback demo module as a cumulative (ie, replacement) patch.  The
+livepatching core will only execute klp_object callbacks for the latest
+cumulative patch on the patch stack.
+
+- load livepatch
+- load second livepatch (atomic replace)
+- disable livepatch
+- disable second livepatch
+- unload livepatch
+- unload second livepatch
+
+  % insmod samples/livepatch/livepatch-callbacks-demo.ko
+  [16435.711175] livepatch: enabling patch 'livepatch_callbacks_demo'
+  [16435.711185] livepatch: 'livepatch_callbacks_demo': initializing patching transition
+  [16435.711271] livepatch_callbacks_demo: pre_patch_callback: vmlinux
+  [16435.711297] livepatch: 'livepatch_callbacks_demo': starting patching transition
+  [16436.704092] livepatch: 'livepatch_callbacks_demo': completing patching transition
+  [16436.704363] livepatch_callbacks_demo: post_patch_callback: vmlinux
+  [16436.704364] livepatch: 'livepatch_callbacks_demo': patching complete
+
+  % insmod samples/livepatch/livepatch-callbacks-demo2.ko replace=1
+  [16442.760963] livepatch: enabling patch 'livepatch_callbacks_demo2'
+  [16442.760966] livepatch: 'livepatch_callbacks_demo2': initializing patching transition
+  [16442.761018] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
+  [16442.761018] livepatch: 'livepatch_callbacks_demo2': starting patching transition
+  [16444.704092] livepatch: 'livepatch_callbacks_demo2': completing patching transition
+  [16444.704181] livepatch_callbacks_demo2: post_patch_callback: vmlinux
+  [16444.704181] livepatch: 'livepatch_callbacks_demo2': patching complete
+
+  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
+  [16448.434672] livepatch: 'livepatch_callbacks_demo2': initializing unpatching transition
+  [16448.434712] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
+  [16449.760134] livepatch: 'livepatch_callbacks_demo2': completing unpatching transition
+  [16449.760338] livepatch: 'livepatch_callbacks_demo2': unpatching complete
+  ** TODO ** where are the demo2 unpatch callbacks?
+
+  % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
+  % rmmod samples/livepatch/livepatch-callbacks-demo.ko
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index dd0e2a8af1af..9fb4d9b845bb 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo2.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
 obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-cumulative.o
diff --git a/samples/livepatch/livepatch-callbacks-demo2.c b/samples/livepatch/livepatch-callbacks-demo2.c
new file mode 100644
index 000000000000..bc07ba7e0568
--- /dev/null
+++ b/samples/livepatch/livepatch-callbacks-demo2.c
@@ -0,0 +1,162 @@
+/*
+ * Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * livepatch-callbacks-demo2.c - (un)patching callbacks livepatch demo
+ *
+ *
+ * Purpose
+ * -------
+ *
+ * Demonstration of registering livepatch (un)patching callbacks and
+ * their behavior in cumulative patches.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * Step 1 - load two livepatch callback demos (default behavior)
+ *
+ *   insmod samples/livepatch/livepatch-callbacks-demo.ko
+ *   insmod samples/livepatch/livepatch-callbacks-demo2.ko replace=0
+ *   echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
+ *   echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ *
+ * Watch dmesg output to see pre and post (un)patch callbacks made for
+ * both livepatch-callbacks-demo and livepatch-callbacks-demo2.
+ *
+ * Remove the modules to prepare for the next step:
+ *
+ *   rmmod samples/livepatch/livepatch-callbacks-demo2.ko
+ *   rmmod samples/livepatch/livepatch-callbacks-demo.ko
+ *
+ * Step 1 - load two livepatch callback demos (cumulative behavior)
+ *
+ *   insmod samples/livepatch/livepatch-callbacks-demo.ko
+ *   insmod samples/livepatch/livepatch-callbacks-demo2.ko replace=1
+ *   echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
+ *   echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ *
+ * Check dmesg output again and notice that when a cumulative patch is
+ * loaded, only its pre and post unpatch callbacks are executed.
+ *
+ * Final cleanup:
+ *
+ *   rmmod samples/livepatch/livepatch-callbacks-demo2.ko
+ *   rmmod samples/livepatch/livepatch-callbacks-demo.ko
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+
+static int replace;
+module_param(replace, int, 0644);
+MODULE_PARM_DESC(replace, "replace (default=0)");
+
+static const char *const module_state[] = {
+	[MODULE_STATE_LIVE]	= "[MODULE_STATE_LIVE] Normal state",
+	[MODULE_STATE_COMING]	= "[MODULE_STATE_COMING] Full formed, running module_init",
+	[MODULE_STATE_GOING]	= "[MODULE_STATE_GOING] Going away",
+	[MODULE_STATE_UNFORMED]	= "[MODULE_STATE_UNFORMED] Still setting it up",
+};
+
+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
+		pr_info("%s: vmlinux\n", callback);
+}
+
+/* Executed on object patching (ie, patch enablement) */
+static int pre_patch_callback(struct klp_object *obj)
+{
+	callback_info(__func__, obj);
+	return 0;
+}
+
+/* Executed on object unpatching (ie, patch disablement) */
+static void post_patch_callback(struct klp_object *obj)
+{
+	callback_info(__func__, obj);
+}
+
+/* Executed on object unpatching (ie, patch disablement) */
+static void pre_unpatch_callback(struct klp_object *obj)
+{
+	callback_info(__func__, obj);
+}
+
+/* Executed on object unpatching (ie, patch disablement) */
+static void post_unpatch_callback(struct klp_object *obj)
+{
+	callback_info(__func__, obj);
+}
+
+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_patch patch = {
+	.mod = THIS_MODULE,
+	.objs = objs,
+};
+
+static int livepatch_callbacks_demo2_init(void)
+{
+	int ret;
+
+	patch.replace = replace;
+
+	ret = klp_register_patch(&patch);
+	if (ret)
+		return ret;
+	ret = klp_enable_patch(&patch);
+	if (ret) {
+		WARN_ON(klp_unregister_patch(&patch));
+		return ret;
+	}
+	return 0;
+}
+
+static void livepatch_callbacks_demo2_exit(void)
+{
+	WARN_ON(klp_unregister_patch(&patch));
+}
+
+module_init(livepatch_callbacks_demo2_init);
+module_exit(livepatch_callbacks_demo2_exit);
+MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v0 3/3] livepatch: update documentation for shadow variables
  2018-02-23 21:33 [PATCH v0 0/3] additional cumulative livepatch doc/samples Joe Lawrence
  2018-02-23 21:33 ` [PATCH v0 1/3] livepatch: add sample cumulative patch Joe Lawrence
  2018-02-23 21:33 ` [PATCH v0 2/3] livepatch: update documentation/samples for callbacks Joe Lawrence
@ 2018-02-23 21:33 ` Joe Lawrence
  2018-03-02 11:58   ` Petr Mladek
  2 siblings, 1 reply; 16+ messages in thread
From: Joe Lawrence @ 2018-02-23 21:33 UTC (permalink / raw)
  To: live-patching, linux-kernel
  Cc: Josh Poimboeuf, Jessica Yu, Jiri Kosina, Miroslav Benes,
	Petr Mladek, Jason Baron, Evgenii Shatokhin

Update livepatch shadow variable documentation with respect to new
atomic replace / cumulative patch functionality.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 Documentation/livepatch/shadow-vars.txt | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
index 89c66634d600..9a2754cf551c 100644
--- a/Documentation/livepatch/shadow-vars.txt
+++ b/Documentation/livepatch/shadow-vars.txt
@@ -179,6 +179,30 @@ doesn't matter what data value the shadow variable holds, its existence
 suggests how to handle the parent object.
 
 
+Use in cumulative patches
+-------------------------
+
+Cumulative livepatches provide a "one-stop" module containing all active
+livepatch code.  A cumulative patch disables and replaces any previously
+loaded livepatch.  Shadow variable lifetimes should be carefully
+considered when loading cumulative livepatches:
+
+- If shadow variables lifetimes are specific to livepatch module
+  versions, it may make sense to free them when the corresponding
+  livepatch module is unloaded.
+
+- If shadow variable instances may be safely handled across cumulative
+  livepatch module versions, then it may make sense to free them from
+  unpatch callbacks.  When a cumulative patch replaces an existing
+  livepatch, only the cumulative patch's callbacks will be executed.
+  This means that new cumulative livepatches may be loaded while
+  deprecated / disabled livepatches may be unloaded without clearing
+  existing shadow variables.
+
+See Documentation/livepatch/callbacks.txt and cumulative.txt for more
+information on these subjects.
+
+
 3. References
 =============
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 1/3] livepatch: add sample cumulative patch
  2018-02-23 21:33 ` [PATCH v0 1/3] livepatch: add sample cumulative patch Joe Lawrence
@ 2018-02-25  1:38   ` Philippe Ombredanne
  2018-02-27 11:54     ` Miroslav Benes
  2018-02-27 11:37   ` Miroslav Benes
  1 sibling, 1 reply; 16+ messages in thread
From: Philippe Ombredanne @ 2018-02-25  1:38 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: live-patching, LKML, Josh Poimboeuf, Jessica Yu, Jiri Kosina,
	Miroslav Benes, Petr Mladek, Jason Baron, Evgenii Shatokhin

Joe,

On Fri, Feb 23, 2018 at 1:33 PM, Joe Lawrence <joe.lawrence@redhat.com> wrote:
> Add a simple atomic replace / cumulative livepatch example.
>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  samples/livepatch/Makefile               |   1 +
>  samples/livepatch/livepatch-cumulative.c | 216 +++++++++++++++++++++++++++++++
>  2 files changed, 217 insertions(+)
>  create mode 100644 samples/livepatch/livepatch-cumulative.c
>
> diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> index 2472ce39a18d..dd0e2a8af1af 100644
> --- a/samples/livepatch/Makefile
> +++ b/samples/livepatch/Makefile
> @@ -5,3 +5,4 @@ obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
>  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
>  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
>  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
> +obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-cumulative.o
> diff --git a/samples/livepatch/livepatch-cumulative.c b/samples/livepatch/livepatch-cumulative.c
> new file mode 100644
> index 000000000000..ab036439e08c
> --- /dev/null
> +++ b/samples/livepatch/livepatch-cumulative.c
> @@ -0,0 +1,216 @@
> +/*
> + * Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */

May be you could use the new SPDX tags instead of this fine but long
legalese? [1]
This would replace ~12 lines of comment by a single line with the same effect.
Thanks!

[1]  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst
--
Philippe

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 1/3] livepatch: add sample cumulative patch
  2018-02-23 21:33 ` [PATCH v0 1/3] livepatch: add sample cumulative patch Joe Lawrence
  2018-02-25  1:38   ` Philippe Ombredanne
@ 2018-02-27 11:37   ` Miroslav Benes
  1 sibling, 0 replies; 16+ messages in thread
From: Miroslav Benes @ 2018-02-27 11:37 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: live-patching, linux-kernel, Josh Poimboeuf, Jessica Yu,
	Jiri Kosina, Petr Mladek, Jason Baron, Evgenii Shatokhin

Hi,

On Fri, 23 Feb 2018, Joe Lawrence wrote:

> Add a simple atomic replace / cumulative livepatch example.

It's not a cumulative patch, so I'd stick with an atomic replace example. 
The same applies to the subject, module name and also the comments.
 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  samples/livepatch/Makefile               |   1 +
>  samples/livepatch/livepatch-cumulative.c | 216 +++++++++++++++++++++++++++++++
>  2 files changed, 217 insertions(+)
>  create mode 100644 samples/livepatch/livepatch-cumulative.c
> 
> diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> index 2472ce39a18d..dd0e2a8af1af 100644
> --- a/samples/livepatch/Makefile
> +++ b/samples/livepatch/Makefile
> @@ -5,3 +5,4 @@ obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
>  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
>  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
>  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
> +obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-cumulative.o
> diff --git a/samples/livepatch/livepatch-cumulative.c b/samples/livepatch/livepatch-cumulative.c
> new file mode 100644
> index 000000000000..ab036439e08c
> --- /dev/null
> +++ b/samples/livepatch/livepatch-cumulative.c
> @@ -0,0 +1,216 @@
> +/*
> + * Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * livepatch-callbacks-cumulative.c - atomic replace / cumulative livepatch demo
> + *
> + *
> + * Purpose
> + * -------
> + *
> + * Demonstration of atomic replace / cumulative livepatching.
> + *
> + *
> + * Usage
> + * -----
> + *
> + * Step 1 - Load the sample livepatch demo
> + *
> + *   insmod samples/livepatch/livepatch-sample.ko
> + *
> + * Notice that /proc/cmdline was modified by the patch.  For the moment,
> + * /proc/meminfo remains unmodified.
> + *
> + *   head /proc/cmdline /proc/meminfo
> + *   ==> /proc/cmdline <==
> + *   this has been live patched

Could you add the module names to the messages in livepatch-sample.c and 
here in the new sample module? It'd be clear what came from where then.

Otherwise it looks good.

Regards,
Miroslav

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 1/3] livepatch: add sample cumulative patch
  2018-02-25  1:38   ` Philippe Ombredanne
@ 2018-02-27 11:54     ` Miroslav Benes
  2018-03-02  1:19       ` Philippe Ombredanne
  0 siblings, 1 reply; 16+ messages in thread
From: Miroslav Benes @ 2018-02-27 11:54 UTC (permalink / raw)
  To: Philippe Ombredanne
  Cc: Joe Lawrence, live-patching, LKML, Josh Poimboeuf, Jessica Yu,
	Jiri Kosina, Petr Mladek, Jason Baron, Evgenii Shatokhin

On Sat, 24 Feb 2018, Philippe Ombredanne wrote:

> Joe,
> 
> On Fri, Feb 23, 2018 at 1:33 PM, Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > Add a simple atomic replace / cumulative livepatch example.
> >
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > ---
> >  samples/livepatch/Makefile               |   1 +
> >  samples/livepatch/livepatch-cumulative.c | 216 +++++++++++++++++++++++++++++++
> >  2 files changed, 217 insertions(+)
> >  create mode 100644 samples/livepatch/livepatch-cumulative.c
> >
> > diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> > index 2472ce39a18d..dd0e2a8af1af 100644
> > --- a/samples/livepatch/Makefile
> > +++ b/samples/livepatch/Makefile
> > @@ -5,3 +5,4 @@ obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
> > +obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-cumulative.o
> > diff --git a/samples/livepatch/livepatch-cumulative.c b/samples/livepatch/livepatch-cumulative.c
> > new file mode 100644
> > index 000000000000..ab036439e08c
> > --- /dev/null
> > +++ b/samples/livepatch/livepatch-cumulative.c
> > @@ -0,0 +1,216 @@
> > +/*
> > + * Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> 
> May be you could use the new SPDX tags instead of this fine but long
> legalese? [1]
> This would replace ~12 lines of comment by a single line with the same effect.
> Thanks!

I don't know about that. How come it is perceived as equivalent? I mean, 
we have a well-established way how to say that a particular source 
code/file is distributed with GPL license. Well-established means that 
it's been tested in court AFAIK many times. Even the license itself (found 
in COPYING file) mentions this as way how to attach the license to a file.

Now you want it to be replaced with a tag. Does it say the same? It might. 
It might not. Do we know? Have you got a court ruling which would say that 
this is also a way how to attach a license to a file? I doubt it. It may 
seem trivially clear, but there are no such things in the legal world.

Don't make me wrong. I don't like that copyright thingie much. I don't 
like that you can find even different versions of the text in the kernel 
source code (and not only there).

However I'd prefer to leave at least a note there that the file is still 
distributed under the terms of GPL found in COPYING file. The tag can be 
there too, if it makes someone happy.

Regards,
Miroslav

> [1]  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst
> --
> Philippe
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 2/3] livepatch: update documentation/samples for callbacks
  2018-02-23 21:33 ` [PATCH v0 2/3] livepatch: update documentation/samples for callbacks Joe Lawrence
@ 2018-02-27 12:36   ` Miroslav Benes
  2018-02-27 14:58     ` Joe Lawrence
  0 siblings, 1 reply; 16+ messages in thread
From: Miroslav Benes @ 2018-02-27 12:36 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: live-patching, linux-kernel, Josh Poimboeuf, Jessica Yu,
	Jiri Kosina, Petr Mladek, Jason Baron, Evgenii Shatokhin

On Fri, 23 Feb 2018, Joe Lawrence wrote:

> Update livepatch callback documentation and samples with respect to new
> atomic replace / cumulative patch functionality.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  Documentation/livepatch/callbacks.txt         | 102 ++++++++++++++++
>  samples/livepatch/Makefile                    |   1 +
>  samples/livepatch/livepatch-callbacks-demo2.c | 162 ++++++++++++++++++++++++++
>  3 files changed, 265 insertions(+)
>  create mode 100644 samples/livepatch/livepatch-callbacks-demo2.c
> 
> diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt
> index c9776f48e458..b5e67975c5a9 100644
> --- a/Documentation/livepatch/callbacks.txt
> +++ b/Documentation/livepatch/callbacks.txt
> @@ -86,6 +86,13 @@ If the object did successfully patch, but the patch transition never
>  started for some reason (e.g., if another object failed to patch),
>  only the post-unpatch callback will be called.
>  
> +If a livepatch is replaced by a cumulative patch, then only the
> +callbacks belonging to the cumulative patch will be executed.  This
> +simplifies the livepatching core for it is the responsibility of the
> +cumulative patch to safely revert whatever needs to be reverted.  See
> +Documentation/livepatch/cumulative.txt for more information on such
> +patches.

s/cumulative/atomic replace/ almost everywhere?

'Documentation/livepatch/cumulative.txt' should be 
'Documentation/livepatch/cumulative-patches.txt' and we may rename it 
atomic-replace-patches.txt. I don't know. Cumulative patches forms a 
subset of atomic replace patches in my understanding. The feature itself 
is more general. Even if practically used for cumulative patches only. But 
it is for you and Petr to decide.

>  Example Use-cases
>  =================
> @@ -603,3 +610,98 @@ pre-unpatch callbacks are skipped:
>    % rmmod samples/livepatch/livepatch-callbacks-busymod.ko
>    [  141.279111] livepatch_callbacks_busymod: busymod_work_func exit
>    [  141.279760] livepatch_callbacks_busymod: livepatch_callbacks_mod_exit
> +
> +
> +Test 10
> +-------
> +
> +Test loading multiple livepatch modules containing callback routines.
> +The livepatching core executes callbacks for all modules.
> +
> +- load livepatch
> +- load second livepatch
> +- disable livepatch
> +- disable second livepatch
> +- unload livepatch
> +- unload second livepatch
> +
> +  % insmod samples/livepatch/livepatch-callbacks-demo.ko
> +  [  216.448208] livepatch: enabling patch 'livepatch_callbacks_demo'
> +  [  216.448211] livepatch: 'livepatch_callbacks_demo': initializing patching transition
> +  [  216.448330] livepatch_callbacks_demo: pre_patch_callback: vmlinux
> +  [  216.448341] livepatch: 'livepatch_callbacks_demo': starting patching transition
> +  [  218.720099] livepatch: 'livepatch_callbacks_demo': completing patching transition
> +  [  218.720179] livepatch_callbacks_demo: post_patch_callback: vmlinux
> +  [  218.720180] livepatch: 'livepatch_callbacks_demo': patching complete
> +
> +  % insmod samples/livepatch/livepatch-callbacks-demo2.ko
> +  [  220.126552] livepatch: enabling patch 'livepatch_callbacks_demo2'
> +  [  220.126554] livepatch: 'livepatch_callbacks_demo2': initializing patching transition
> +  [  220.126592] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
> +  [  220.126593] livepatch: 'livepatch_callbacks_demo2': starting patching transition
> +  [  221.728091] livepatch: 'livepatch_callbacks_demo2': completing patching transition
> +  [  221.728254] livepatch_callbacks_demo2: post_patch_callback: vmlinux
> +  [  221.728255] livepatch: 'livepatch_callbacks_demo2': patching complete
> +
> +  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
> +  [  223.434556] livepatch: 'livepatch_callbacks_demo2': initializing unpatching transition
> +  [  223.434616] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux
> +  [  223.434617] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
> +  [  224.736159] livepatch: 'livepatch_callbacks_demo2': completing unpatching transition
> +  [  224.736660] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux
> +  [  224.736662] livepatch: 'livepatch_callbacks_demo2': unpatching complete
> +
> +  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
> +  [  227.284070] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
> +  [  227.284111] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
> +  [  227.284112] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
> +  [  228.704142] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
> +  [  228.704215] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
> +  [  228.704216] livepatch: 'livepatch_callbacks_demo': unpatching complete
> +
> +  % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
> +  % rmmod samples/livepatch/livepatch-callbacks-demo.ko
> +
> +
> +Test 11
> +-------
> +
> +A similar test as the previous one, except this time load the second
> +callback demo module as a cumulative (ie, replacement) patch.  The
> +livepatching core will only execute klp_object callbacks for the latest
> +cumulative patch on the patch stack.
> +
> +- load livepatch
> +- load second livepatch (atomic replace)
> +- disable livepatch

Not needed.

Miroslav

> +- disable second livepatch
> +- unload livepatch
> +- unload second livepatch
> +
> +  % insmod samples/livepatch/livepatch-callbacks-demo.ko
> +  [16435.711175] livepatch: enabling patch 'livepatch_callbacks_demo'
> +  [16435.711185] livepatch: 'livepatch_callbacks_demo': initializing patching transition
> +  [16435.711271] livepatch_callbacks_demo: pre_patch_callback: vmlinux
> +  [16435.711297] livepatch: 'livepatch_callbacks_demo': starting patching transition
> +  [16436.704092] livepatch: 'livepatch_callbacks_demo': completing patching transition
> +  [16436.704363] livepatch_callbacks_demo: post_patch_callback: vmlinux
> +  [16436.704364] livepatch: 'livepatch_callbacks_demo': patching complete
> +
> +  % insmod samples/livepatch/livepatch-callbacks-demo2.ko replace=1
> +  [16442.760963] livepatch: enabling patch 'livepatch_callbacks_demo2'
> +  [16442.760966] livepatch: 'livepatch_callbacks_demo2': initializing patching transition
> +  [16442.761018] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
> +  [16442.761018] livepatch: 'livepatch_callbacks_demo2': starting patching transition
> +  [16444.704092] livepatch: 'livepatch_callbacks_demo2': completing patching transition
> +  [16444.704181] livepatch_callbacks_demo2: post_patch_callback: vmlinux
> +  [16444.704181] livepatch: 'livepatch_callbacks_demo2': patching complete
> +
> +  % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
> +  [16448.434672] livepatch: 'livepatch_callbacks_demo2': initializing unpatching transition
> +  [16448.434712] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
> +  [16449.760134] livepatch: 'livepatch_callbacks_demo2': completing unpatching transition
> +  [16449.760338] livepatch: 'livepatch_callbacks_demo2': unpatching complete
> +  ** TODO ** where are the demo2 unpatch callbacks?
> +
> +  % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
> +  % rmmod samples/livepatch/livepatch-callbacks-demo.ko

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 2/3] livepatch: update documentation/samples for callbacks
  2018-02-27 12:36   ` Miroslav Benes
@ 2018-02-27 14:58     ` Joe Lawrence
  2018-02-28 13:20       ` Miroslav Benes
  2018-03-02 11:11       ` Petr Mladek
  0 siblings, 2 replies; 16+ messages in thread
From: Joe Lawrence @ 2018-02-27 14:58 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: live-patching, linux-kernel, Josh Poimboeuf, Jessica Yu,
	Jiri Kosina, Petr Mladek, Jason Baron, Evgenii Shatokhin

On 02/27/2018 07:36 AM, Miroslav Benes wrote:
> On Fri, 23 Feb 2018, Joe Lawrence wrote:
> 
>> [ ... snip ... ]
>>  
>> +If a livepatch is replaced by a cumulative patch, then only the
>> +callbacks belonging to the cumulative patch will be executed.  This
>> +simplifies the livepatching core for it is the responsibility of the
>> +cumulative patch to safely revert whatever needs to be reverted.  See
>> +Documentation/livepatch/cumulative.txt for more information on such
>> +patches.
> 
> s/cumulative/atomic replace/ almost everywhere?
> 
> 'Documentation/livepatch/cumulative.txt' should be 
> 'Documentation/livepatch/cumulative-patches.txt' and we may rename it 
> atomic-replace-patches.txt. I don't know. Cumulative patches forms a 
> subset of atomic replace patches in my understanding. The feature itself 
> is more general. Even if practically used for cumulative patches only. But 
> it is for you and Petr to decide.

Hi Miroslav,

Thanks for reviewing!

I guess I'm a little confused about the distinction here.

I understood a "cumulative-patch" to mean that it would contain the sum
of all changes.  So instead of this:

  patch 1 = A
+ patch 2 =     B
+ patch 3 =         C
-----------------------
  net     = A + B + C

We can group all of the changes together into a single cumulative-patch
for the same net effect:

  patch 1 = A               -replaced by-
  patch 2 = A + B             -replaced by-
  patch 3 = A + B + C

I assumed this would also mean to include any reverted changes as well.
So in the example above, if change C needed to be reverted, then:

  patch 4 = A + B

and that would still be considered a "cumulative-patch".

In my mind, atomic replace is the mechanism that forces patching to be
cumulative.  Perhaps this is too strict?  Are there other use-cases for
atomic-replace?

>>  Example Use-cases
>>  =================
>>
>> [ ... snip ... ]
>>
>> +Test 11
>> +-------
>> +
>> +A similar test as the previous one, except this time load the second
>> +callback demo module as a cumulative (ie, replacement) patch.  The
>> +livepatching core will only execute klp_object callbacks for the latest
>> +cumulative patch on the patch stack.
>> +
>> +- load livepatch
>> +- load second livepatch (atomic replace)
>> +- disable livepatch
> 
> Not needed.

Good catch.

-- Joe

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 2/3] livepatch: update documentation/samples for callbacks
  2018-02-27 14:58     ` Joe Lawrence
@ 2018-02-28 13:20       ` Miroslav Benes
  2018-03-02 11:11       ` Petr Mladek
  1 sibling, 0 replies; 16+ messages in thread
From: Miroslav Benes @ 2018-02-28 13:20 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: live-patching, linux-kernel, Josh Poimboeuf, Jessica Yu,
	Jiri Kosina, Petr Mladek, Jason Baron, Evgenii Shatokhin

On Tue, 27 Feb 2018, Joe Lawrence wrote:

> On 02/27/2018 07:36 AM, Miroslav Benes wrote:
> > On Fri, 23 Feb 2018, Joe Lawrence wrote:
> > 
> >> [ ... snip ... ]
> >>  
> >> +If a livepatch is replaced by a cumulative patch, then only the
> >> +callbacks belonging to the cumulative patch will be executed.  This
> >> +simplifies the livepatching core for it is the responsibility of the
> >> +cumulative patch to safely revert whatever needs to be reverted.  See
> >> +Documentation/livepatch/cumulative.txt for more information on such
> >> +patches.
> > 
> > s/cumulative/atomic replace/ almost everywhere?
> > 
> > 'Documentation/livepatch/cumulative.txt' should be 
> > 'Documentation/livepatch/cumulative-patches.txt' and we may rename it 
> > atomic-replace-patches.txt. I don't know. Cumulative patches forms a 
> > subset of atomic replace patches in my understanding. The feature itself 
> > is more general. Even if practically used for cumulative patches only. But 
> > it is for you and Petr to decide.
> 
> Hi Miroslav,
> 
> Thanks for reviewing!
> 
> I guess I'm a little confused about the distinction here.
> 
> I understood a "cumulative-patch" to mean that it would contain the sum
> of all changes.  So instead of this:
> 
>   patch 1 = A
> + patch 2 =     B
> + patch 3 =         C
> -----------------------
>   net     = A + B + C
> 
> We can group all of the changes together into a single cumulative-patch
> for the same net effect:
> 
>   patch 1 = A               -replaced by-
>   patch 2 = A + B             -replaced by-
>   patch 3 = A + B + C

Yes.
 
> I assumed this would also mean to include any reverted changes as well.
> So in the example above, if change C needed to be reverted, then:
> 
>   patch 4 = A + B
> 
> and that would still be considered a "cumulative-patch".

Ah, ok. This is where we differ. I didn't consider this to be a cumulative 
patch. But I understand your reasoning. 

Miroslav

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 1/3] livepatch: add sample cumulative patch
  2018-02-27 11:54     ` Miroslav Benes
@ 2018-03-02  1:19       ` Philippe Ombredanne
  2018-03-02  8:31         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 16+ messages in thread
From: Philippe Ombredanne @ 2018-03-02  1:19 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Greg Kroah-Hartman, Joe Lawrence, live-patching, LKML,
	Josh Poimboeuf, Jessica Yu, Jiri Kosina, Petr Mladek,
	Jason Baron, Evgenii Shatokhin

Miroslav,

On Tue, Feb 27, 2018 at 3:54 AM, Miroslav Benes <mbenes@suse.cz> wrote:
> On Sat, 24 Feb 2018, Philippe Ombredanne wrote:
>
>> Joe,
>>
>> On Fri, Feb 23, 2018 at 1:33 PM, Joe Lawrence <joe.lawrence@redhat.com> wrote:
>> > Add a simple atomic replace / cumulative livepatch example.
>> >
>> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
>> > ---
>> >  samples/livepatch/Makefile               |   1 +
>> >  samples/livepatch/livepatch-cumulative.c | 216 +++++++++++++++++++++++++++++++
>> >  2 files changed, 217 insertions(+)
>> >  create mode 100644 samples/livepatch/livepatch-cumulative.c
>> >
>> > diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
>> > index 2472ce39a18d..dd0e2a8af1af 100644
>> > --- a/samples/livepatch/Makefile
>> > +++ b/samples/livepatch/Makefile
>> > @@ -5,3 +5,4 @@ obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
>> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
>> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
>> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
>> > +obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-cumulative.o
>> > diff --git a/samples/livepatch/livepatch-cumulative.c b/samples/livepatch/livepatch-cumulative.c
>> > new file mode 100644
>> > index 000000000000..ab036439e08c
>> > --- /dev/null
>> > +++ b/samples/livepatch/livepatch-cumulative.c
>> > @@ -0,0 +1,216 @@
>> > +/*
>> > + * Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
>> > + *
>> > + * This program is free software; you can redistribute it and/or
>> > + * modify it under the terms of the GNU General Public License
>> > + * as published by the Free Software Foundation; either version 2
>> > + * of the License, or (at your option) any later version.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + * You should have received a copy of the GNU General Public License
>> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>> > + */
>>
>> May be you could use the new SPDX tags instead of this fine but long
>> legalese? [1]
>> This would replace ~12 lines of comment by a single line with the same effect.
>> Thanks!
>
> I don't know about that. How come it is perceived as equivalent? I mean,
> we have a well-established way how to say that a particular source
> code/file is distributed with GPL license. Well-established means that
> it's been tested in court AFAIK many times. Even the license itself (found
> in COPYING file) mentions this as way how to attach the license to a file.
>
> Now you want it to be replaced with a tag. Does it say the same? It might.
> It might not. Do we know? Have you got a court ruling which would say that
> this is also a way how to attach a license to a file? I doubt it. It may
> seem trivially clear, but there are no such things in the legal world.
>
> Don't make me wrong. I don't like that copyright thingie much. I don't
> like that you can find even different versions of the text in the kernel
> source code (and not only there).
>
> However I'd prefer to leave at least a note there that the file is still
> distributed under the terms of GPL found in COPYING file. The tag can be
> there too, if it makes someone happy.
>
> Regards,
> Miroslav
>
>> [1]  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst
>> --
>> Philippe
>>
>

To the best of my knowledge, this has been debated in person and on
list among maintainers and agreed to.
This has also been reviewed by the LF lawyers. The result of is the
documentation in [1]
You are welcomed not to agree of course, but this would make your
contributions stand out with its legalese boilerplate when we are
trying to get of it.

Greg, anything else to add?

CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
-- 
Cordially
Philippe Ombredanne

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 1/3] livepatch: add sample cumulative patch
  2018-03-02  1:19       ` Philippe Ombredanne
@ 2018-03-02  8:31         ` Greg Kroah-Hartman
  2018-03-02  9:11           ` Miroslav Benes
  0 siblings, 1 reply; 16+ messages in thread
From: Greg Kroah-Hartman @ 2018-03-02  8:31 UTC (permalink / raw)
  To: Philippe Ombredanne
  Cc: Miroslav Benes, Joe Lawrence, live-patching, LKML,
	Josh Poimboeuf, Jessica Yu, Jiri Kosina, Petr Mladek,
	Jason Baron, Evgenii Shatokhin

On Thu, Mar 01, 2018 at 05:19:28PM -0800, Philippe Ombredanne wrote:
> Miroslav,
> 
> On Tue, Feb 27, 2018 at 3:54 AM, Miroslav Benes <mbenes@suse.cz> wrote:
> > On Sat, 24 Feb 2018, Philippe Ombredanne wrote:
> >
> >> Joe,
> >>
> >> On Fri, Feb 23, 2018 at 1:33 PM, Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >> > Add a simple atomic replace / cumulative livepatch example.
> >> >
> >> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> >> > ---
> >> >  samples/livepatch/Makefile               |   1 +
> >> >  samples/livepatch/livepatch-cumulative.c | 216 +++++++++++++++++++++++++++++++
> >> >  2 files changed, 217 insertions(+)
> >> >  create mode 100644 samples/livepatch/livepatch-cumulative.c
> >> >
> >> > diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> >> > index 2472ce39a18d..dd0e2a8af1af 100644
> >> > --- a/samples/livepatch/Makefile
> >> > +++ b/samples/livepatch/Makefile
> >> > @@ -5,3 +5,4 @@ obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
> >> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
> >> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
> >> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
> >> > +obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-cumulative.o
> >> > diff --git a/samples/livepatch/livepatch-cumulative.c b/samples/livepatch/livepatch-cumulative.c
> >> > new file mode 100644
> >> > index 000000000000..ab036439e08c
> >> > --- /dev/null
> >> > +++ b/samples/livepatch/livepatch-cumulative.c
> >> > @@ -0,0 +1,216 @@
> >> > +/*
> >> > + * Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
> >> > + *
> >> > + * This program is free software; you can redistribute it and/or
> >> > + * modify it under the terms of the GNU General Public License
> >> > + * as published by the Free Software Foundation; either version 2
> >> > + * of the License, or (at your option) any later version.
> >> > + *
> >> > + * This program is distributed in the hope that it will be useful,
> >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> > + * GNU General Public License for more details.
> >> > + *
> >> > + * You should have received a copy of the GNU General Public License
> >> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> >> > + */
> >>
> >> May be you could use the new SPDX tags instead of this fine but long
> >> legalese? [1]
> >> This would replace ~12 lines of comment by a single line with the same effect.
> >> Thanks!
> >
> > I don't know about that. How come it is perceived as equivalent? I mean,
> > we have a well-established way how to say that a particular source
> > code/file is distributed with GPL license. Well-established means that
> > it's been tested in court AFAIK many times. Even the license itself (found
> > in COPYING file) mentions this as way how to attach the license to a file.
> >
> > Now you want it to be replaced with a tag. Does it say the same? It might.
> > It might not. Do we know? Have you got a court ruling which would say that
> > this is also a way how to attach a license to a file? I doubt it. It may
> > seem trivially clear, but there are no such things in the legal world.
> >
> > Don't make me wrong. I don't like that copyright thingie much. I don't
> > like that you can find even different versions of the text in the kernel
> > source code (and not only there).
> >
> > However I'd prefer to leave at least a note there that the file is still
> > distributed under the terms of GPL found in COPYING file. The tag can be
> > there too, if it makes someone happy.
> >
> > Regards,
> > Miroslav
> >
> >> [1]  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst
> >> --
> >> Philippe
> >>
> >
> 
> To the best of my knowledge, this has been debated in person and on
> list among maintainers and agreed to.
> This has also been reviewed by the LF lawyers. The result of is the
> documentation in [1]
> You are welcomed not to agree of course, but this would make your
> contributions stand out with its legalese boilerplate when we are
> trying to get of it.
> 
> Greg, anything else to add?

Yes, do not add new "boiler plate" license code in new files, otherwise
you will just have to rip them out again later on.  If you have
questions about this, please contact your company's lawyers, as they
know all about this issue and last I heard, agreed with it.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 1/3] livepatch: add sample cumulative patch
  2018-03-02  8:31         ` Greg Kroah-Hartman
@ 2018-03-02  9:11           ` Miroslav Benes
  0 siblings, 0 replies; 16+ messages in thread
From: Miroslav Benes @ 2018-03-02  9:11 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Philippe Ombredanne, Joe Lawrence, live-patching, LKML,
	Josh Poimboeuf, Jessica Yu, Jiri Kosina, Petr Mladek,
	Jason Baron, Evgenii Shatokhin

On Fri, 2 Mar 2018, Greg Kroah-Hartman wrote:

> On Thu, Mar 01, 2018 at 05:19:28PM -0800, Philippe Ombredanne wrote:
> > Miroslav,
> > 
> > On Tue, Feb 27, 2018 at 3:54 AM, Miroslav Benes <mbenes@suse.cz> wrote:
> > > On Sat, 24 Feb 2018, Philippe Ombredanne wrote:
> > >
> > >> Joe,
> > >>
> > >> On Fri, Feb 23, 2018 at 1:33 PM, Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > >> > Add a simple atomic replace / cumulative livepatch example.
> > >> >
> > >> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > >> > ---
> > >> >  samples/livepatch/Makefile               |   1 +
> > >> >  samples/livepatch/livepatch-cumulative.c | 216 +++++++++++++++++++++++++++++++
> > >> >  2 files changed, 217 insertions(+)
> > >> >  create mode 100644 samples/livepatch/livepatch-cumulative.c
> > >> >
> > >> > diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> > >> > index 2472ce39a18d..dd0e2a8af1af 100644
> > >> > --- a/samples/livepatch/Makefile
> > >> > +++ b/samples/livepatch/Makefile
> > >> > @@ -5,3 +5,4 @@ obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
> > >> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
> > >> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
> > >> >  obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
> > >> > +obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-cumulative.o
> > >> > diff --git a/samples/livepatch/livepatch-cumulative.c b/samples/livepatch/livepatch-cumulative.c
> > >> > new file mode 100644
> > >> > index 000000000000..ab036439e08c
> > >> > --- /dev/null
> > >> > +++ b/samples/livepatch/livepatch-cumulative.c
> > >> > @@ -0,0 +1,216 @@
> > >> > +/*
> > >> > + * Copyright (C) 2018 Joe Lawrence <joe.lawrence@redhat.com>
> > >> > + *
> > >> > + * This program is free software; you can redistribute it and/or
> > >> > + * modify it under the terms of the GNU General Public License
> > >> > + * as published by the Free Software Foundation; either version 2
> > >> > + * of the License, or (at your option) any later version.
> > >> > + *
> > >> > + * This program is distributed in the hope that it will be useful,
> > >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > >> > + * GNU General Public License for more details.
> > >> > + *
> > >> > + * You should have received a copy of the GNU General Public License
> > >> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > >> > + */
> > >>
> > >> May be you could use the new SPDX tags instead of this fine but long
> > >> legalese? [1]
> > >> This would replace ~12 lines of comment by a single line with the same effect.
> > >> Thanks!
> > >
> > > I don't know about that. How come it is perceived as equivalent? I mean,
> > > we have a well-established way how to say that a particular source
> > > code/file is distributed with GPL license. Well-established means that
> > > it's been tested in court AFAIK many times. Even the license itself (found
> > > in COPYING file) mentions this as way how to attach the license to a file.
> > >
> > > Now you want it to be replaced with a tag. Does it say the same? It might.
> > > It might not. Do we know? Have you got a court ruling which would say that
> > > this is also a way how to attach a license to a file? I doubt it. It may
> > > seem trivially clear, but there are no such things in the legal world.
> > >
> > > Don't make me wrong. I don't like that copyright thingie much. I don't
> > > like that you can find even different versions of the text in the kernel
> > > source code (and not only there).
> > >
> > > However I'd prefer to leave at least a note there that the file is still
> > > distributed under the terms of GPL found in COPYING file. The tag can be
> > > there too, if it makes someone happy.
> > >
> > > Regards,
> > > Miroslav
> > >
> > >> [1]  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst
> > >> --
> > >> Philippe
> > >>
> > >
> > 
> > To the best of my knowledge, this has been debated in person and on
> > list among maintainers and agreed to.
> > This has also been reviewed by the LF lawyers. The result of is the
> > documentation in [1]
> > You are welcomed not to agree of course, but this would make your
> > contributions stand out with its legalese boilerplate when we are
> > trying to get of it.
> > 
> > Greg, anything else to add?
> 
> Yes, do not add new "boiler plate" license code in new files, otherwise
> you will just have to rip them out again later on.  If you have
> questions about this, please contact your company's lawyers, as they
> know all about this issue and last I heard, agreed with it.

I certainly will. Thanks.

Miroslav

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 2/3] livepatch: update documentation/samples for callbacks
  2018-02-27 14:58     ` Joe Lawrence
  2018-02-28 13:20       ` Miroslav Benes
@ 2018-03-02 11:11       ` Petr Mladek
  2018-03-02 22:08         ` Joe Lawrence
  1 sibling, 1 reply; 16+ messages in thread
From: Petr Mladek @ 2018-03-02 11:11 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: Miroslav Benes, live-patching, linux-kernel, Josh Poimboeuf,
	Jessica Yu, Jiri Kosina, Jason Baron, Evgenii Shatokhin

On Tue 2018-02-27 09:58:40, Joe Lawrence wrote:
> On 02/27/2018 07:36 AM, Miroslav Benes wrote:
> > On Fri, 23 Feb 2018, Joe Lawrence wrote:
> > 
> >> [ ... snip ... ]
> >>  
> >> +If a livepatch is replaced by a cumulative patch, then only the
> >> +callbacks belonging to the cumulative patch will be executed.  This
> >> +simplifies the livepatching core for it is the responsibility of the
> >> +cumulative patch to safely revert whatever needs to be reverted.  See
> >> +Documentation/livepatch/cumulative.txt for more information on such
> >> +patches.
> > 
> > s/cumulative/atomic replace/ almost everywhere?
> > 
> > 'Documentation/livepatch/cumulative.txt' should be 
> > 'Documentation/livepatch/cumulative-patches.txt' and we may rename it 
> > atomic-replace-patches.txt. I don't know. Cumulative patches forms a 
> > subset of atomic replace patches in my understanding. The feature itself 
> > is more general. Even if practically used for cumulative patches only. But 
> > it is for you and Petr to decide.
> 
> Hi Miroslav,
> 
> Thanks for reviewing!
> 
> I guess I'm a little confused about the distinction here.
> 
> I understood a "cumulative-patch" to mean that it would contain the sum
> of all changes.  So instead of this:
> 
>   patch 1 = A
> + patch 2 =     B
> + patch 3 =         C
> -----------------------
>   net     = A + B + C
> 
> We can group all of the changes together into a single cumulative-patch
> for the same net effect:
> 
>   patch 1 = A               -replaced by-
>   patch 2 = A + B             -replaced by-
>   patch 3 = A + B + C
> 
> I assumed this would also mean to include any reverted changes as well.
> So in the example above, if change C needed to be reverted, then:
> 
>   patch 4 = A + B
> 
> and that would still be considered a "cumulative-patch".

Yes, I would consider this a cumulative patch.


> In my mind, atomic replace is the mechanism that forces patching to be
> cumulative.  Perhaps this is too strict?  Are there other use-cases for
> atomic-replace?

Jason talked about using the atomic replace to get rid of any
existing livepatches and adding another changes instead. The changes
in the old and the new patch might be unrelated. They simply do
not want to mind what was there before. The term "atomic replace"
fits perfectly for this usecase.

My understanding is that cumulative patches do similar thing.
But the old and new patches should be related. In particular,
any new patch should include most changes from the older one.
The only exception is when an old change was wrong and we do
not want it anymore.

Now, your examples are close the the Jason's use case. They
do:

  patch1 = A         -replaced by-
  patch2 =    B
------------------
  result      B

I mean that one change is replaced by an "unrelated" one.
It might confuse people. They might ask why the new patch
is called cumulative when all the older changes are lost.

I would suggest to rename the sample patch to livepatch-test-replace
or so. Also I would try to avoid the world cumulative in the example
to avoid confusion.

I still would prefer to keep the documentation for the feature
called cumulative-patches.txt. From my point of view, the atomic
replace is rather a technical detail. It might be dangerous when
used for non-related patches. On the other, cumulative patches
seem to be a promising way how to keep livepatches maintainable
and safe.

Best Regards,
Petr

PS: I did not added these patches to v9 of the atomic replace
patchset. It was already big enough. And I hope that v9 might
be final. In addition, there are no conflicts on the touched
files side.

In each case, thanks a lot for these nice examples
and for finding the bug.

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 3/3] livepatch: update documentation for shadow variables
  2018-02-23 21:33 ` [PATCH v0 3/3] livepatch: update documentation for shadow variables Joe Lawrence
@ 2018-03-02 11:58   ` Petr Mladek
  0 siblings, 0 replies; 16+ messages in thread
From: Petr Mladek @ 2018-03-02 11:58 UTC (permalink / raw)
  To: Joe Lawrence
  Cc: live-patching, linux-kernel, Josh Poimboeuf, Jessica Yu,
	Jiri Kosina, Miroslav Benes, Jason Baron, Evgenii Shatokhin

On Fri 2018-02-23 16:33:50, Joe Lawrence wrote:
> Update livepatch shadow variable documentation with respect to new
> atomic replace / cumulative patch functionality.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  Documentation/livepatch/shadow-vars.txt | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
> index 89c66634d600..9a2754cf551c 100644
> --- a/Documentation/livepatch/shadow-vars.txt
> +++ b/Documentation/livepatch/shadow-vars.txt
> @@ -179,6 +179,30 @@ doesn't matter what data value the shadow variable holds, its existence
>  suggests how to handle the parent object.
>  
>  
> +Use in cumulative patches
> +-------------------------
> +
> +Cumulative livepatches provide a "one-stop" module containing all active
> +livepatch code.

I would remove the above sentence. I had troubles to parse and
understand it. I think that the sentence below is better descriptive
and enough :-)

+ A cumulative patch disables and replaces any previously
> +loaded livepatch.  Shadow variable lifetimes should be carefully
> +considered when loading cumulative livepatches:
> +
> +- If shadow variables lifetimes are specific to livepatch module
> +  versions, it may make sense to free them when the corresponding
> +  livepatch module is unloaded.
> +
> +- If shadow variable instances may be safely handled across cumulative
> +  livepatch module versions, then it may make sense to free them from
> +  unpatch callbacks.  When a cumulative patch replaces an existing
> +  livepatch, only the cumulative patch's callbacks will be executed.
> +  This means that new cumulative livepatches may be loaded while
> +  deprecated / disabled livepatches may be unloaded without clearing
> +  existing shadow variables.
> +
> +See Documentation/livepatch/callbacks.txt and cumulative.txt for more

s/cumulative.txt/cumulative-patches.txt/

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v0 2/3] livepatch: update documentation/samples for callbacks
  2018-03-02 11:11       ` Petr Mladek
@ 2018-03-02 22:08         ` Joe Lawrence
  0 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2018-03-02 22:08 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Miroslav Benes, live-patching, linux-kernel, Josh Poimboeuf,
	Jessica Yu, Jiri Kosina, Jason Baron, Evgenii Shatokhin

On 03/02/2018 06:11 AM, Petr Mladek wrote:
> On Tue 2018-02-27 09:58:40, Joe Lawrence wrote:
>> In my mind, atomic replace is the mechanism that forces patching to be
>> cumulative.  Perhaps this is too strict?  Are there other use-cases for
>> atomic-replace?
> 
> Jason talked about using the atomic replace to get rid of any
> existing livepatches and adding another changes instead. The changes
> in the old and the new patch might be unrelated. They simply do
> not want to mind what was there before. The term "atomic replace"
> fits perfectly for this usecase.
> 
> My understanding is that cumulative patches do similar thing.
> But the old and new patches should be related. In particular,
> any new patch should include most changes from the older one.
> The only exception is when an old change was wrong and we do
> not want it anymore.

Yes, I can see the semantic difference between these cases.  In my mind,
I am tainted by an understanding of the implementation... so I lazily
optimized both cases under a common terminology.

That said, you're right about potential confusion, so I'll update the
example and docs to remove references to "cumulative" and just call it
"atomic-replace" :)

> PS: I did not added these patches to v9 of the atomic replace
> patchset. It was already big enough. And I hope that v9 might
> be final. In addition, there are no conflicts on the touched
> files side.

I can continue to update as a separate patchset if that helps the the
other patchset reach a quicker conclusion.

As far as licensing, I don't mind modifying for SPDX tags if that's the
way we want to go.

Thanks,

-- Joe

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-03-02 22:08 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-23 21:33 [PATCH v0 0/3] additional cumulative livepatch doc/samples Joe Lawrence
2018-02-23 21:33 ` [PATCH v0 1/3] livepatch: add sample cumulative patch Joe Lawrence
2018-02-25  1:38   ` Philippe Ombredanne
2018-02-27 11:54     ` Miroslav Benes
2018-03-02  1:19       ` Philippe Ombredanne
2018-03-02  8:31         ` Greg Kroah-Hartman
2018-03-02  9:11           ` Miroslav Benes
2018-02-27 11:37   ` Miroslav Benes
2018-02-23 21:33 ` [PATCH v0 2/3] livepatch: update documentation/samples for callbacks Joe Lawrence
2018-02-27 12:36   ` Miroslav Benes
2018-02-27 14:58     ` Joe Lawrence
2018-02-28 13:20       ` Miroslav Benes
2018-03-02 11:11       ` Petr Mladek
2018-03-02 22:08         ` Joe Lawrence
2018-02-23 21:33 ` [PATCH v0 3/3] livepatch: update documentation for shadow variables Joe Lawrence
2018-03-02 11:58   ` 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).