[PATCH 0/2] Fix issue with alternatives/paravirt patches

[PATCH 0/2] Fix issue with alternatives/paravirt patches

[Jessica Yu]

Hi,

A few months ago, Chris Arges reported a bug involving alternatives/paravirt
patching that was discussed here [1] and here [2]. To briefly summarize the
bug, patch modules that contained .altinstructions or .parainstructions
sections would break because these alternative/paravirt patches would be
applied first by the module loader (see x86 module_finalize()), then
livepatch would later clobber these patches when applying per-object
relocations. This lead to crashes and unpredictable behavior.

One conclusion we reached from our last discussion was that we will
need to introduce some arch-specific code to address this problem.
This patchset presents a possible fix for the bug by adding a new
arch-specific arch_klp_init_object_loaded() function that by default
does nothing but can be overridden by different arches.

To fix this issue for x86, since we can access a patch module's Elf
sections through mod->klp_info, we can simply delay the calls to
apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
which is called after relocations have been written for an object.
In addition, for patch modules, .parainstructions and .altinstructions are
prefixed by ".klp.arch.${objname}" so that the module loader ignores them
and livepatch can apply them manually.

Currently for kpatch, we don't support including jump table sections in
the patch module, and supporting .smp_locks is currently broken, so we
don't consider those sections (for now).

I did some light testing with some patches to kvm and verified that the
original issue reported in [2] was fixed.

Based on linux-next.

[1] http://thread.gmane.org/gmane.linux.kernel/2185604/
[2] https://github.com/dynup/kpatch/issues/580

Jessica Yu (2):
  livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
  livepatch/x86: apply alternatives and paravirt patches after relocations

 arch/x86/kernel/Makefile    |  1 +
 arch/x86/kernel/livepatch.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/livepatch.h   |  3 +++
 kernel/livepatch/core.c     | 12 +++++++--
 4 files changed, 80 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/kernel/livepatch.c

-- 
2.4.3

[PATCH 1/2] livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks

[Jessica Yu]

Introduce arch_klp_init_object_loaded() to complete any additional
arch-specific tasks during patching. Architecture code may override this
function.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 include/linux/livepatch.h |  3 +++
 kernel/livepatch/core.c   | 12 ++++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index a93a0b2..9072f04 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -116,6 +116,9 @@ int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
 int klp_disable_patch(struct klp_patch *);
 
+void arch_klp_init_object_loaded(struct klp_patch *patch,
+				 struct klp_object *obj);
+
 /* Called from the module loader during module coming/going states */
 int klp_module_coming(struct module *mod);
 void klp_module_going(struct module *mod);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5c2bc10..164eff6 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -274,7 +274,6 @@ static int klp_write_object_relocations(struct module *pmod,
 
 	objname = klp_is_module(obj) ? obj->name : "vmlinux";
 
-	module_disable_ro(pmod);
 	/* For each klp relocation section */
 	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
 		sec = pmod->klp_info->sechdrs + i;
@@ -309,7 +308,6 @@ static int klp_write_object_relocations(struct module *pmod,
 			break;
 	}
 
-	module_enable_ro(pmod);
 	return ret;
 }
 
@@ -763,6 +761,12 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 				    func->old_sympos ? func->old_sympos : 1);
 }
 
+/* Arches may override this to finish any remaining arch-specific tasks */
+void __weak arch_klp_init_object_loaded(struct klp_patch *patch,
+					struct klp_object *obj)
+{
+}
+
 /* parts of the initialization that is done only when the object is loaded */
 static int klp_init_object_loaded(struct klp_patch *patch,
 				  struct klp_object *obj)
@@ -770,10 +774,14 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 	struct klp_func *func;
 	int ret;
 
+	module_disable_ro(patch->mod);
 	ret = klp_write_object_relocations(patch->mod, obj);
 	if (ret)
 		return ret;
 
+	arch_klp_init_object_loaded(patch, obj);
+	module_enable_ro(patch->mod);
+
 	klp_for_each_func(obj, func) {
 		ret = klp_find_object_symbol(obj->name, func->old_name,
 					     func->old_sympos,
-- 
2.4.3

[PATCH 2/2] livepatch/x86: apply alternatives and paravirt patches after relocations

[Jessica Yu]

Implement arch_klp_init_object_loaded() for x86, which applies
alternatives/paravirt patches. This fixes the order in which relocations
and alternatives/paravirt patches are applied.

Previously, if a patch module had alternatives or paravirt patches,
these were applied first by the module loader before livepatch can apply
per-object relocations. The (buggy) sequence of events was:

(1) Load patch module
(2) Apply alternatives and paravirt patches to patch module
    * Note that these are applied to the new functions in the patch module
(3) Apply per-object relocations to patch module when target module loads.
    * This clobbers what was written in step 2

This lead to crashes and corruption in general, since livepatch would
overwrite or step on previously applied alternative/paravirt patches.
The correct sequence of events should be:

(1) Load patch module
(2) Apply per-object relocations to patch module
(3) Apply per-object alternatives and paravirt patches to patch module

This is fixed by delaying paravirt/alternatives patching until after
relocations are applied. Any .altinstructions or .parainstructions sections in
a patch module are prefixed with ".klp.arch.${objname}" and applied in
arch_klp_init_object_loaded().

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 arch/x86/kernel/Makefile    |  1 +
 arch/x86/kernel/livepatch.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)
 create mode 100644 arch/x86/kernel/livepatch.c

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 0503f5b..4f656fe 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
 obj-y				+= apic/
 obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
 obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
+obj-$(CONFIG_LIVEPATCH)	+= livepatch.o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
 obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
 obj-$(CONFIG_X86_TSC)		+= trace_clock.o
diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
new file mode 100644
index 0000000..f0845b2
--- /dev/null
+++ b/arch/x86/kernel/livepatch.c
@@ -0,0 +1,66 @@
+/*
+ * livepatch.c - x86-specific Kernel Live Patching Core
+ *
+ * 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/>.
+ */
+
+#include <linux/module.h>
+#include <linux/kallsyms.h>
+#include <linux/livepatch.h>
+#include <asm/text-patching.h>
+
+/* Apply per-object alternatives. Based on x86 module_finalize() */
+void arch_klp_init_object_loaded(struct klp_patch *patch,
+				 struct klp_object *obj)
+{
+	int cnt;
+	struct klp_modinfo *info;
+	Elf_Shdr *s, *alt = NULL, *para = NULL;
+	void *aseg, *pseg;
+	const char *objname;
+	char sec_objname[MODULE_NAME_LEN];
+	char secname[KSYM_NAME_LEN];
+
+	info = patch->mod->klp_info;
+	objname = obj->name ? obj->name : "vmlinux";
+
+	for (s = info->sechdrs; s < info->sechdrs + info->hdr.e_shnum; s++) {
+		/*
+		 * Search for .klp.arch sections that apply to this object.
+		 * See BUILD_BUG_ON() in livepatch core code for field width
+		 * explanations.
+		 */
+		cnt = sscanf(info->secstrings + s->sh_name,
+			     ".klp.arch.%55[^.].%127s",
+			     sec_objname, secname);
+		if (cnt != 2)
+			continue;
+		if (strcmp(sec_objname, objname))
+			continue;
+		if (!strcmp("altinstructions", secname))
+			alt = s;
+		if (!strcmp("parainstructions", secname))
+			para = s;
+	}
+
+	if (alt) {
+		aseg = (void *) alt->sh_addr;
+		apply_alternatives(aseg, aseg + alt->sh_size);
+	}
+
+	if (para) {
+		pseg = (void *) para->sh_addr;
+		apply_paravirt(pseg, pseg + para->sh_size);
+	}
+}
-- 
2.4.3

Re: [PATCH 2/2] livepatch/x86: apply alternatives and paravirt patches after relocations

[Josh Poimboeuf]

On Tue, Jul 05, 2016 at 10:35:00PM -0400, Jessica Yu wrote:
> Implement arch_klp_init_object_loaded() for x86, which applies
> alternatives/paravirt patches. This fixes the order in which relocations
> and alternatives/paravirt patches are applied.
> 
> Previously, if a patch module had alternatives or paravirt patches,
> these were applied first by the module loader before livepatch can apply
> per-object relocations. The (buggy) sequence of events was:
> 
> (1) Load patch module
> (2) Apply alternatives and paravirt patches to patch module
>     * Note that these are applied to the new functions in the patch module
> (3) Apply per-object relocations to patch module when target module loads.
>     * This clobbers what was written in step 2
> 
> This lead to crashes and corruption in general, since livepatch would
> overwrite or step on previously applied alternative/paravirt patches.
> The correct sequence of events should be:
> 
> (1) Load patch module
> (2) Apply per-object relocations to patch module
> (3) Apply per-object alternatives and paravirt patches to patch module
> 
> This is fixed by delaying paravirt/alternatives patching until after
> relocations are applied. Any .altinstructions or .parainstructions sections in
> a patch module are prefixed with ".klp.arch.${objname}" and applied in
> arch_klp_init_object_loaded().
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  arch/x86/kernel/Makefile    |  1 +
>  arch/x86/kernel/livepatch.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 67 insertions(+)
>  create mode 100644 arch/x86/kernel/livepatch.c
> 
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 0503f5b..4f656fe 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
>  obj-y				+= apic/
>  obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
>  obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
> +obj-$(CONFIG_LIVEPATCH)	+= livepatch.o
>  obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
>  obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
>  obj-$(CONFIG_X86_TSC)		+= trace_clock.o
> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> new file mode 100644
> index 0000000..f0845b2
> --- /dev/null
> +++ b/arch/x86/kernel/livepatch.c
> @@ -0,0 +1,66 @@
> +/*
> + * livepatch.c - x86-specific Kernel Live Patching Core
> + *
> + * 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/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kallsyms.h>
> +#include <linux/livepatch.h>
> +#include <asm/text-patching.h>
> +
> +/* Apply per-object alternatives. Based on x86 module_finalize() */
> +void arch_klp_init_object_loaded(struct klp_patch *patch,
> +				 struct klp_object *obj)
> +{
> +	int cnt;
> +	struct klp_modinfo *info;
> +	Elf_Shdr *s, *alt = NULL, *para = NULL;
> +	void *aseg, *pseg;
> +	const char *objname;
> +	char sec_objname[MODULE_NAME_LEN];
> +	char secname[KSYM_NAME_LEN];
> +
> +	info = patch->mod->klp_info;
> +	objname = obj->name ? obj->name : "vmlinux";
> +
> +	for (s = info->sechdrs; s < info->sechdrs + info->hdr.e_shnum; s++) {
> +		/*
> +		 * Search for .klp.arch sections that apply to this object.
> +		 * See BUILD_BUG_ON() in livepatch core code for field width
> +		 * explanations.
> +		 */
> +		cnt = sscanf(info->secstrings + s->sh_name,
> +			     ".klp.arch.%55[^.].%127s",
> +			     sec_objname, secname);

I think we should also have a BUILD_BUG_ON() here, otherwise we might
forget this file was also affected if something were to change.

-- 
Josh

Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches

[Petr Mladek]

On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> Hi,
> 
> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> patching that was discussed here [1] and here [2]. To briefly summarize the
> bug, patch modules that contained .altinstructions or .parainstructions
> sections would break because these alternative/paravirt patches would be
> applied first by the module loader (see x86 module_finalize()), then
> livepatch would later clobber these patches when applying per-object
> relocations. This lead to crashes and unpredictable behavior.
> 
> One conclusion we reached from our last discussion was that we will
> need to introduce some arch-specific code to address this problem.
> This patchset presents a possible fix for the bug by adding a new
> arch-specific arch_klp_init_object_loaded() function that by default
> does nothing but can be overridden by different arches.
> 
> To fix this issue for x86, since we can access a patch module's Elf
> sections through mod->klp_info, we can simply delay the calls to
> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> which is called after relocations have been written for an object.
> In addition, for patch modules, .parainstructions and .altinstructions are
> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> and livepatch can apply them manually.

The solution looks correct to me. The fun will be how to generate
the sections. If I get this correctly, it is not enough to rename
the existing ones. Instead, we need to split .parainstructions
and .altinstructions sections into per-object ones.

I wonder if there is a plan for this. Especially I am interested
into the patches created from sources ;-) I wonder if we could add
a tag somewhere and improve the build infrastructure.

Best Regards,
Petr

Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches

[Josh Poimboeuf]

On Thu, Jul 07, 2016 at 05:56:33PM +0200, Petr Mladek wrote:
> On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> > Hi,
> > 
> > A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> > patching that was discussed here [1] and here [2]. To briefly summarize the
> > bug, patch modules that contained .altinstructions or .parainstructions
> > sections would break because these alternative/paravirt patches would be
> > applied first by the module loader (see x86 module_finalize()), then
> > livepatch would later clobber these patches when applying per-object
> > relocations. This lead to crashes and unpredictable behavior.
> > 
> > One conclusion we reached from our last discussion was that we will
> > need to introduce some arch-specific code to address this problem.
> > This patchset presents a possible fix for the bug by adding a new
> > arch-specific arch_klp_init_object_loaded() function that by default
> > does nothing but can be overridden by different arches.
> > 
> > To fix this issue for x86, since we can access a patch module's Elf
> > sections through mod->klp_info, we can simply delay the calls to
> > apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> > which is called after relocations have been written for an object.
> > In addition, for patch modules, .parainstructions and .altinstructions are
> > prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> > and livepatch can apply them manually.
> 
> The solution looks correct to me. The fun will be how to generate
> the sections. If I get this correctly, it is not enough to rename
> the existing ones. Instead, we need to split .parainstructions
> and .altinstructions sections into per-object ones.
> 
> I wonder if there is a plan for this. Especially I am interested
> into the patches created from sources ;-) I wonder if we could add
> a tag somewhere and improve the build infrastructure.

Yeah.  I'd like to reiterate[1] that this would all be a lot easier if
we weren't circumventing module dependencies.

[1] https://lkml.kernel.org/r/20160404161428.3qap2i4vpgda66iw@treble.redhat.com

-- 
Josh

Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches

[Miroslav Benes]

On Thu, 7 Jul 2016, Josh Poimboeuf wrote:

> On Thu, Jul 07, 2016 at 05:56:33PM +0200, Petr Mladek wrote:
> > On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> > > Hi,
> > > 
> > > A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> > > patching that was discussed here [1] and here [2]. To briefly summarize the
> > > bug, patch modules that contained .altinstructions or .parainstructions
> > > sections would break because these alternative/paravirt patches would be
> > > applied first by the module loader (see x86 module_finalize()), then
> > > livepatch would later clobber these patches when applying per-object
> > > relocations. This lead to crashes and unpredictable behavior.
> > > 
> > > One conclusion we reached from our last discussion was that we will
> > > need to introduce some arch-specific code to address this problem.
> > > This patchset presents a possible fix for the bug by adding a new
> > > arch-specific arch_klp_init_object_loaded() function that by default
> > > does nothing but can be overridden by different arches.
> > > 
> > > To fix this issue for x86, since we can access a patch module's Elf
> > > sections through mod->klp_info, we can simply delay the calls to
> > > apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> > > which is called after relocations have been written for an object.
> > > In addition, for patch modules, .parainstructions and .altinstructions are
> > > prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> > > and livepatch can apply them manually.
> > 
> > The solution looks correct to me. The fun will be how to generate
> > the sections. If I get this correctly, it is not enough to rename
> > the existing ones. Instead, we need to split .parainstructions
> > and .altinstructions sections into per-object ones.
> > 
> > I wonder if there is a plan for this. Especially I am interested
> > into the patches created from sources ;-) I wonder if we could add
> > a tag somewhere and improve the build infrastructure.
> 
> Yeah.  I'd like to reiterate[1] that this would all be a lot easier if
> we weren't circumventing module dependencies.
> 
> [1] https://lkml.kernel.org/r/20160404161428.3qap2i4vpgda66iw@treble.redhat.com

Oh, we haven't come to any conclusion. I think it would be a great topic 
for Plumbers conf. It is always better to discuss such things personally. 
What do you think? Any volunteer to propose it? :)

Miroslav

Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches

[Josh Poimboeuf]

On Tue, Jul 12, 2016 at 01:55:54PM +0200, Miroslav Benes wrote:
> On Thu, 7 Jul 2016, Josh Poimboeuf wrote:
> 
> > On Thu, Jul 07, 2016 at 05:56:33PM +0200, Petr Mladek wrote:
> > > On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
> > > > Hi,
> > > > 
> > > > A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> > > > patching that was discussed here [1] and here [2]. To briefly summarize the
> > > > bug, patch modules that contained .altinstructions or .parainstructions
> > > > sections would break because these alternative/paravirt patches would be
> > > > applied first by the module loader (see x86 module_finalize()), then
> > > > livepatch would later clobber these patches when applying per-object
> > > > relocations. This lead to crashes and unpredictable behavior.
> > > > 
> > > > One conclusion we reached from our last discussion was that we will
> > > > need to introduce some arch-specific code to address this problem.
> > > > This patchset presents a possible fix for the bug by adding a new
> > > > arch-specific arch_klp_init_object_loaded() function that by default
> > > > does nothing but can be overridden by different arches.
> > > > 
> > > > To fix this issue for x86, since we can access a patch module's Elf
> > > > sections through mod->klp_info, we can simply delay the calls to
> > > > apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> > > > which is called after relocations have been written for an object.
> > > > In addition, for patch modules, .parainstructions and .altinstructions are
> > > > prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> > > > and livepatch can apply them manually.
> > > 
> > > The solution looks correct to me. The fun will be how to generate
> > > the sections. If I get this correctly, it is not enough to rename
> > > the existing ones. Instead, we need to split .parainstructions
> > > and .altinstructions sections into per-object ones.
> > > 
> > > I wonder if there is a plan for this. Especially I am interested
> > > into the patches created from sources ;-) I wonder if we could add
> > > a tag somewhere and improve the build infrastructure.
> > 
> > Yeah.  I'd like to reiterate[1] that this would all be a lot easier if
> > we weren't circumventing module dependencies.
> > 
> > [1] https://lkml.kernel.org/r/20160404161428.3qap2i4vpgda66iw@treble.redhat.com
> 
> Oh, we haven't come to any conclusion. I think it would be a great topic 
> for Plumbers conf. It is always better to discuss such things personally. 
> What do you think? Any volunteer to propose it? :)

Well, it's somewhat related to my "Livepatch module creation tooling"
proposed talk, because I suspect the tooling could be *much* simpler if
we didn't circumvent module dependencies.  So I'll probably talk about
that aspect of it.

But it would be great if somebody wanted to submit a separate talk to
explore the pros and cons of our current "load patches to modules before
the modules themselves have been loaded" approach and if there are any
viable alternatives.

-- 
Josh

Re: Fix issue with alternatives/paravirt patches

[Jessica Yu]

+++ Josh Poimboeuf [12/07/16 09:01 -0500]:
>On Tue, Jul 12, 2016 at 01:55:54PM +0200, Miroslav Benes wrote:
>> On Thu, 7 Jul 2016, Josh Poimboeuf wrote:
>>
>> > On Thu, Jul 07, 2016 at 05:56:33PM +0200, Petr Mladek wrote:
>> > > On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
>> > > > Hi,
>> > > >
>> > > > A few months ago, Chris Arges reported a bug involving alternatives/paravirt
>> > > > patching that was discussed here [1] and here [2]. To briefly summarize the
>> > > > bug, patch modules that contained .altinstructions or .parainstructions
>> > > > sections would break because these alternative/paravirt patches would be
>> > > > applied first by the module loader (see x86 module_finalize()), then
>> > > > livepatch would later clobber these patches when applying per-object
>> > > > relocations. This lead to crashes and unpredictable behavior.
>> > > >
>> > > > One conclusion we reached from our last discussion was that we will
>> > > > need to introduce some arch-specific code to address this problem.
>> > > > This patchset presents a possible fix for the bug by adding a new
>> > > > arch-specific arch_klp_init_object_loaded() function that by default
>> > > > does nothing but can be overridden by different arches.
>> > > >
>> > > > To fix this issue for x86, since we can access a patch module's Elf
>> > > > sections through mod->klp_info, we can simply delay the calls to
>> > > > apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
>> > > > which is called after relocations have been written for an object.
>> > > > In addition, for patch modules, .parainstructions and .altinstructions are
>> > > > prefixed by ".klp.arch.${objname}" so that the module loader ignores them
>> > > > and livepatch can apply them manually.
>> > >
>> > > The solution looks correct to me. The fun will be how to generate
>> > > the sections. If I get this correctly, it is not enough to rename
>> > > the existing ones. Instead, we need to split .parainstructions
>> > > and .altinstructions sections into per-object ones.
>> > >
>> > > I wonder if there is a plan for this. Especially I am interested
>> > > into the patches created from sources ;-) I wonder if we could add
>> > > a tag somewhere and improve the build infrastructure.
>> >
>> > Yeah.  I'd like to reiterate[1] that this would all be a lot easier if
>> > we weren't circumventing module dependencies.
>> >
>> > [1] https://lkml.kernel.org/r/20160404161428.3qap2i4vpgda66iw@treble.redhat.com
>>
>> Oh, we haven't come to any conclusion. I think it would be a great topic
>> for Plumbers conf. It is always better to discuss such things personally.
>> What do you think? Any volunteer to propose it? :)
>
>Well, it's somewhat related to my "Livepatch module creation tooling"
>proposed talk, because I suspect the tooling could be *much* simpler if
>we didn't circumvent module dependencies.  So I'll probably talk about
>that aspect of it.
>
>But it would be great if somebody wanted to submit a separate talk to
>explore the pros and cons of our current "load patches to modules before
>the modules themselves have been loaded" approach and if there are any
>viable alternatives.

In addition to Josh's linked discussion, we also once talked about the
idea of forcing module dependencies (exactly!) one year ago today:

http://www.spinics.net/lists/live-patching/msg00946.html

I've forgotten a lot of what I was blabbering about back then (and
this was way before we talked about the .klp.rela stuff), but I do
remember we talked a bit about forcing module dependencies and
potentially forcing to-be-patched modules to be loaded first before
loading the patch module. I still don't think we should do that but
instead we could potentially implement something more palatable like
enforcing maybe one patch module per object (so maybe depmod could
record dependencies for us) or something like that.

It would be interesting to revisit the problem again, esp. in the
context of recent changes. If I end up collecting enough talking
points, I can submit a proposal.

Jessica

Re: Fix issue with alternatives/paravirt patches

[Jessica Yu]

+++ Petr Mladek [07/07/16 17:56 +0200]:
>On Tue 2016-07-05 22:34:58, Jessica Yu wrote:
>> Hi,
>>
>> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
>> patching that was discussed here [1] and here [2]. To briefly summarize the
>> bug, patch modules that contained .altinstructions or .parainstructions
>> sections would break because these alternative/paravirt patches would be
>> applied first by the module loader (see x86 module_finalize()), then
>> livepatch would later clobber these patches when applying per-object
>> relocations. This lead to crashes and unpredictable behavior.
>>
>> One conclusion we reached from our last discussion was that we will
>> need to introduce some arch-specific code to address this problem.
>> This patchset presents a possible fix for the bug by adding a new
>> arch-specific arch_klp_init_object_loaded() function that by default
>> does nothing but can be overridden by different arches.
>>
>> To fix this issue for x86, since we can access a patch module's Elf
>> sections through mod->klp_info, we can simply delay the calls to
>> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
>> which is called after relocations have been written for an object.
>> In addition, for patch modules, .parainstructions and .altinstructions are
>> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
>> and livepatch can apply them manually.
>
>The solution looks correct to me. The fun will be how to generate
>the sections. If I get this correctly, it is not enough to rename
>the existing ones. Instead, we need to split .parainstructions
>and .altinstructions sections into per-object ones.

Hi Petr,

That is correct, this means .parainstructions and .altinstructions
will be split per-object and follow a similar naming scheme to the
existing .klp.rela.${objname} sections. Then, we can apply these
sections per-object with apply_alternatives() and apply_paravirt().

>I wonder if there is a plan for this. Especially I am interested
>into the patches created from sources ;-) I wonder if we could add
>a tag somewhere and improve the build infrastructure.

I currently have a working branch of the kpatch-build tools that
generates these sections. I'm hoping to get the code up in some shape
or form soon; I'll post an update when it's up.

Jessica

Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches

[Christopher Arges]

On Tue, Jul 05, 2016 at 10:34:58PM -0400, Jessica Yu wrote:
> Hi,
> 
> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> patching that was discussed here [1] and here [2]. To briefly summarize the
> bug, patch modules that contained .altinstructions or .parainstructions
> sections would break because these alternative/paravirt patches would be
> applied first by the module loader (see x86 module_finalize()), then
> livepatch would later clobber these patches when applying per-object
> relocations. This lead to crashes and unpredictable behavior.
> 
> One conclusion we reached from our last discussion was that we will
> need to introduce some arch-specific code to address this problem.
> This patchset presents a possible fix for the bug by adding a new
> arch-specific arch_klp_init_object_loaded() function that by default
> does nothing but can be overridden by different arches.
> 
> To fix this issue for x86, since we can access a patch module's Elf
> sections through mod->klp_info, we can simply delay the calls to
> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> which is called after relocations have been written for an object.
> In addition, for patch modules, .parainstructions and .altinstructions are
> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> and livepatch can apply them manually.
> 
> Currently for kpatch, we don't support including jump table sections in
> the patch module, and supporting .smp_locks is currently broken, so we
> don't consider those sections (for now).
> 
> I did some light testing with some patches to kvm and verified that the
> original issue reported in [2] was fixed.
> 
> Based on linux-next.
> 

Jessica,

I was able to test these patches on top of linux-next. I took your kpatch
branch and hacked it a bit to get it working and was able to
apply a patch to 'kvm_arch_vm_ioctl' while running a VM workload.

Great job!

Tested-by: Chris J Arges <chris.j.arges@canonical.com>

--chris

> [1] http://thread.gmane.org/gmane.linux.kernel/2185604/
> [2] https://github.com/dynup/kpatch/issues/580
> 
> Jessica Yu (2):
>   livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
>   livepatch/x86: apply alternatives and paravirt patches after relocations
> 
>  arch/x86/kernel/Makefile    |  1 +
>  arch/x86/kernel/livepatch.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/livepatch.h   |  3 +++
>  kernel/livepatch/core.c     | 12 +++++++--
>  4 files changed, 80 insertions(+), 2 deletions(-)
>  create mode 100644 arch/x86/kernel/livepatch.c
> 
> -- 
> 2.4.3
> 

Re: Fix issue with alternatives/paravirt patches

[Jessica Yu]

+++ Christopher Arges [08/07/16 00:22 -0500]:
>On Tue, Jul 05, 2016 at 10:34:58PM -0400, Jessica Yu wrote:
>> Hi,
>>
>> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
>> patching that was discussed here [1] and here [2]. To briefly summarize the
>> bug, patch modules that contained .altinstructions or .parainstructions
>> sections would break because these alternative/paravirt patches would be
>> applied first by the module loader (see x86 module_finalize()), then
>> livepatch would later clobber these patches when applying per-object
>> relocations. This lead to crashes and unpredictable behavior.
>>
>> One conclusion we reached from our last discussion was that we will
>> need to introduce some arch-specific code to address this problem.
>> This patchset presents a possible fix for the bug by adding a new
>> arch-specific arch_klp_init_object_loaded() function that by default
>> does nothing but can be overridden by different arches.
>>
>> To fix this issue for x86, since we can access a patch module's Elf
>> sections through mod->klp_info, we can simply delay the calls to
>> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
>> which is called after relocations have been written for an object.
>> In addition, for patch modules, .parainstructions and .altinstructions are
>> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
>> and livepatch can apply them manually.
>>
>> Currently for kpatch, we don't support including jump table sections in
>> the patch module, and supporting .smp_locks is currently broken, so we
>> don't consider those sections (for now).
>>
>> I did some light testing with some patches to kvm and verified that the
>> original issue reported in [2] was fixed.
>>
>> Based on linux-next.
>>
>
>Jessica,
>
>I was able to test these patches on top of linux-next. I took your kpatch
>branch and hacked it a bit to get it working and was able to
>apply a patch to 'kvm_arch_vm_ioctl' while running a VM workload.
>
>Great job!
>
>Tested-by: Chris J Arges <chris.j.arges@canonical.com>

Excellent, thanks for testing it out Chris!

Jessica

Re: [PATCH 0/2] Fix issue with alternatives/paravirt patches

[Miroslav Benes]

On Tue, 5 Jul 2016, Jessica Yu wrote:

> Hi,
> 
> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
> patching that was discussed here [1] and here [2]. To briefly summarize the
> bug, patch modules that contained .altinstructions or .parainstructions
> sections would break because these alternative/paravirt patches would be
> applied first by the module loader (see x86 module_finalize()), then
> livepatch would later clobber these patches when applying per-object
> relocations. This lead to crashes and unpredictable behavior.
> 
> One conclusion we reached from our last discussion was that we will
> need to introduce some arch-specific code to address this problem.
> This patchset presents a possible fix for the bug by adding a new
> arch-specific arch_klp_init_object_loaded() function that by default
> does nothing but can be overridden by different arches.
> 
> To fix this issue for x86, since we can access a patch module's Elf
> sections through mod->klp_info, we can simply delay the calls to
> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
> which is called after relocations have been written for an object.
> In addition, for patch modules, .parainstructions and .altinstructions are
> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
> and livepatch can apply them manually.
> 
> Currently for kpatch, we don't support including jump table sections in
> the patch module, and supporting .smp_locks is currently broken, so we
> don't consider those sections (for now).
> 
> I did some light testing with some patches to kvm and verified that the
> original issue reported in [2] was fixed.
> 
> Based on linux-next.
> 
> [1] http://thread.gmane.org/gmane.linux.kernel/2185604/
> [2] https://github.com/dynup/kpatch/issues/580
> 
> Jessica Yu (2):
>   livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
>   livepatch/x86: apply alternatives and paravirt patches after relocations
>
>  arch/x86/kernel/Makefile    |  1 +
>  arch/x86/kernel/livepatch.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/livepatch.h   |  3 +++
>  kernel/livepatch/core.c     | 12 +++++++--
>  4 files changed, 80 insertions(+), 2 deletions(-)
>  create mode 100644 arch/x86/kernel/livepatch.c

Hi,

thanks Jessica for implementing it. It does not look as bad as I was 
afraid, which is great. Few remarks...

Is there a problem when you need to generate a dynrela for paravirt code? 
I mean one does not know during the build of a patch module if paravirt 
would or would not be applied. If such code needs to be relocated it could 
be a problem for kpatch-build. Is this correct or am I missing something?

Now the other architectures we support. s390 should be ok. There is 
nothing much in module_finalize() there. powerpc is different though. It 
is quite similar to x86_64 case. And also arm64 needs to be handled in 
future.

Thanks,
Miroslav

Re: Fix issue with alternatives/paravirt patches

[Jessica Yu]

+++ Miroslav Benes [12/07/16 14:06 +0200]:
>On Tue, 5 Jul 2016, Jessica Yu wrote:
>
>> Hi,
>>
>> A few months ago, Chris Arges reported a bug involving alternatives/paravirt
>> patching that was discussed here [1] and here [2]. To briefly summarize the
>> bug, patch modules that contained .altinstructions or .parainstructions
>> sections would break because these alternative/paravirt patches would be
>> applied first by the module loader (see x86 module_finalize()), then
>> livepatch would later clobber these patches when applying per-object
>> relocations. This lead to crashes and unpredictable behavior.
>>
>> One conclusion we reached from our last discussion was that we will
>> need to introduce some arch-specific code to address this problem.
>> This patchset presents a possible fix for the bug by adding a new
>> arch-specific arch_klp_init_object_loaded() function that by default
>> does nothing but can be overridden by different arches.
>>
>> To fix this issue for x86, since we can access a patch module's Elf
>> sections through mod->klp_info, we can simply delay the calls to
>> apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
>> which is called after relocations have been written for an object.
>> In addition, for patch modules, .parainstructions and .altinstructions are
>> prefixed by ".klp.arch.${objname}" so that the module loader ignores them
>> and livepatch can apply them manually.
>>
>> Currently for kpatch, we don't support including jump table sections in
>> the patch module, and supporting .smp_locks is currently broken, so we
>> don't consider those sections (for now).
>>
>> I did some light testing with some patches to kvm and verified that the
>> original issue reported in [2] was fixed.
>>
>> Based on linux-next.
>>
>> [1] http://thread.gmane.org/gmane.linux.kernel/2185604/
>> [2] https://github.com/dynup/kpatch/issues/580
>>
>> Jessica Yu (2):
>>   livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
>>   livepatch/x86: apply alternatives and paravirt patches after relocations
>>
>>  arch/x86/kernel/Makefile    |  1 +
>>  arch/x86/kernel/livepatch.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/livepatch.h   |  3 +++
>>  kernel/livepatch/core.c     | 12 +++++++--
>>  4 files changed, 80 insertions(+), 2 deletions(-)
>>  create mode 100644 arch/x86/kernel/livepatch.c
>
>Hi,
>
>thanks Jessica for implementing it. It does not look as bad as I was
>afraid, which is great. Few remarks...
>
>Is there a problem when you need to generate a dynrela for paravirt code?
>I mean one does not know during the build of a patch module if paravirt
>would or would not be applied. If such code needs to be relocated it could
>be a problem for kpatch-build. Is this correct or am I missing something?

In kpatch-build, "special" sections like .parainstructions and
.altinstructions are into consideration. These sections are included
in the final patch module if they reference any of the new replacement
code. Like with any other section, kpatch-build will convert any relas
from .rela.parainstructions (for example) to dynrelas if it is
necessary.  And with this patch we apply all "dynrelas" (which are now
actual relas in .klp.rela sections) first before applying any
paravirt/alternatives patches in arch_klp_init_object_loaded(), which
is the correct order.

>Now the other architectures we support. s390 should be ok. There is
>nothing much in module_finalize() there. powerpc is different though. It
>is quite similar to x86_64 case. And also arm64 needs to be handled in
>future.

Yeah, I think we are OK for s390, arm64 looks fairly straightforward
(just a call to apply_alternatives()), powerpc looks slightly more
involved but I haven't thoroughly dug into it yet.

So other arches will need to have arch_klp_init_object_loaded()
implemented eventually (if needed), but I think it is fine for now to
fix this issue on x86 first, since that's where the original bug
cropped up.

Jessica

Re: Fix issue with alternatives/paravirt patches

[Miroslav Benes]

On Thu, 21 Jul 2016, Jessica Yu wrote:

> +++ Miroslav Benes [12/07/16 14:06 +0200]:
> > 
> > Is there a problem when you need to generate a dynrela for paravirt code?
> > I mean one does not know during the build of a patch module if paravirt
> > would or would not be applied. If such code needs to be relocated it could
> > be a problem for kpatch-build. Is this correct or am I missing something?
> 
> In kpatch-build, "special" sections like .parainstructions and
> .altinstructions are into consideration. These sections are included
> in the final patch module if they reference any of the new replacement
> code. Like with any other section, kpatch-build will convert any relas
> from .rela.parainstructions (for example) to dynrelas if it is
> necessary.  And with this patch we apply all "dynrelas" (which are now
> actual relas in .klp.rela sections) first before applying any
> paravirt/alternatives patches in arch_klp_init_object_loaded(), which
> is the correct order.

Ah, right. So it should be ok.

> > Now the other architectures we support. s390 should be ok. There is
> > nothing much in module_finalize() there. powerpc is different though. It
> > is quite similar to x86_64 case. And also arm64 needs to be handled in
> > future.
> 
> Yeah, I think we are OK for s390, arm64 looks fairly straightforward
> (just a call to apply_alternatives()), powerpc looks slightly more
> involved but I haven't thoroughly dug into it yet.
> 
> So other arches will need to have arch_klp_init_object_loaded()
> implemented eventually (if needed), but I think it is fine for now to
> fix this issue on x86 first, since that's where the original bug
> cropped up.

Agreed.

Miroslav