From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752941AbcHBI7w (ORCPT ); Tue, 2 Aug 2016 04:59:52 -0400 Received: from mx2.suse.de ([195.135.220.15]:43109 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751733AbcHBI72 (ORCPT ); Tue, 2 Aug 2016 04:59:28 -0400 Date: Tue, 2 Aug 2016 10:59:07 +0200 (CEST) From: Miroslav Benes To: Jessica Yu cc: Josh Poimboeuf , Petr Mladek , Jiri Kosina , Chris J Arges , Eugene Shatokhin , live-patching@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/2] livepatch/x86: apply alternatives and paravirt patches after relocations In-Reply-To: <1469078640-26798-3-git-send-email-jeyu@redhat.com> Message-ID: References: <1469078640-26798-1-git-send-email-jeyu@redhat.com> <1469078640-26798-3-git-send-email-jeyu@redhat.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 21 Jul 2016, 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 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(). Would it make sense to update our documentation and include this info? > Signed-off-by: Jessica Yu > --- > arch/x86/kernel/Makefile | 1 + > arch/x86/kernel/livepatch.c | 65 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 66 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..78209f1 > --- /dev/null > +++ b/arch/x86/kernel/livepatch.c > @@ -0,0 +1,65 @@ > +/* > + * 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 . > + */ > + > +#include > +#include > +#include > +#include It seems I don't have such header file here and it causes a build error. Shouldn't it be ? > + > +/* 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"; > + > + /* See livepatch core code for BUILD_BUG_ON() explanation */ > + BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 128); > + > + for (s = info->sechdrs; s < info->sechdrs + info->hdr.e_shnum; s++) { > + /* Apply per-object .klp.arch sections */ > + cnt = sscanf(info->secstrings + s->sh_name, > + ".klp.arch.%55[^.].%127s", > + sec_objname, secname); > + if (cnt != 2) > + continue; > + if (strcmp(sec_objname, objname)) > + continue; > + if (!strcmp("altinstructions", secname)) > + alt = s; > + if (!strcmp("parainstructions", secname)) > + para = s; > + } > + > + if (alt) { > + aseg = (void *) alt->sh_addr; > + apply_alternatives(aseg, aseg + alt->sh_size); > + } > + > + if (para) { > + pseg = (void *) para->sh_addr; > + apply_paravirt(pseg, pseg + para->sh_size); > + } > +} Otherwise it looks good. Regards, Miroslav