Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753133AbcDATHR (ORCPT ); Fri, 1 Apr 2016 15:07:17 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:49924 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750938AbcDATHO (ORCPT ); Fri, 1 Apr 2016 15:07:14 -0400 Date: Fri, 1 Apr 2016 14:07:05 -0500 From: Chris J Arges To: Miroslav Benes Cc: Jiri Kosina , jeyu@redhat.com, jpoimboe@redhat.com, eugene.shatokhin@rosalab.ru, live-patching@vger.kernel.org, Linux Kernel Mailing List , pmladek@suse.cz Subject: Re: Bug with paravirt ops and livepatches Message-ID: <20160401190704.GB7837@canonical.com> References: <20160329120518.GA21252@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6050 Lines: 124 On Fri, Apr 01, 2016 at 05:46:52PM +0200, Miroslav Benes wrote: > On Fri, 1 Apr 2016, Jiri Kosina wrote: > > > On Tue, 29 Mar 2016, Jiri Kosina wrote: > > > > > Agreed; I think we should be safe applying all the alternatives (with > > > paravirt being really just a special case of those) to the coming module > > > at the very last phase; they really are required only during runtime, > > > but nothing else should be depending on them. Right? If anyone is able > > > to come up with and counter-example, please speak up :) > > > > So I have quickly gone through all the architectures that actually do > > overload __weak module_finalize() by their own implementation, and except > > for applying self-modifying code changes and registering unwind tables, > > there doesn't seem to be any relevant heavy-lifting, that'd need to be > > done before relocations have been written. > > > > Is the (completely untested) sort-of-a-patch below a complete rubbish > > (on top of current livepatching.git's for-next)? > > > > > > > > > > diff --git a/kernel/module.c b/kernel/module.c > > index 5f71aa6..c003648 100644 > > --- a/kernel/module.c > > +++ b/kernel/module.c > > @@ -3211,7 +3211,7 @@ int __weak module_finalize(const Elf_Ehdr *hdr, > > return 0; > > } > > > > -static int post_relocation(struct module *mod, const struct load_info *info) > > +static void post_relocation(struct module *mod, const struct load_info *info) > > { > > /* Sort exception table now relocations are done. */ > > sort_extable(mod->extable, mod->extable + mod->num_exentries); > > @@ -3222,9 +3222,6 @@ static int post_relocation(struct module *mod, const struct load_info *info) > > > > /* Setup kallsyms-specific fields. */ > > add_kallsyms(mod, info); > > - > > - /* Arch-specific module finalizing. */ > > - return module_finalize(info->hdr, info->sechdrs, mod); > > } > > > > /* Is this module of this name done loading? No locks held. */ > > @@ -3562,9 +3559,7 @@ static int load_module(struct load_info *info, const char __user *uargs, > > if (err < 0) > > goto free_modinfo; > > > > - err = post_relocation(mod, info); > > - if (err < 0) > > - goto free_modinfo; > > + post_relocation(mod, info); > > > > flush_module_icache(mod); > > > > @@ -3589,6 +3584,11 @@ static int load_module(struct load_info *info, const char __user *uargs, > > if (err) > > goto bug_cleanup; > > > > + /* Arch-specific module finalizing. */ > > + err = module_finalize(info->hdr, info->sechdrs, mod); > > + if (err) > > + goto bug_cleanup; > > goto coming_cleanup; > > Otherwise it looks ok. I'll give it a proper look on Monday though. > > Miroslav > Loading, please wait... starting version 229 [ 1.182869] random: udevadm urandom read with 2 bits of entropy available [ 1.241404] BUG: unable to handle kernel paging request at ffffffffc000f35f [ 1.242760] IP: [] __memcpy+0x17/0x20 [ 1.243870] PGD 1e09067 PUD 1e0b067 PMD 1edb0067 PTE 1ee61161 [ 1.245172] Oops: 0003 [#1] SMP [ 1.245975] Modules linked in: floppy(+) pata_acpi [ 1.247086] CPU: 0 PID: 135 Comm: systemd-udevd Not tainted 4.5.0+ #3 [ 1.248176] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 [ 1.249765] task: ffff88001f360000 ti: ffff88001ed40000 task.ti: ffff88001ed40000 [ 1.251097] RIP: 0010:[] [] __memcpy+0x17/0x20 [ 1.252534] RSP: 0018:ffff88001ed43b78 EFLAGS: 00010002 [ 1.253441] RAX: ffffffffc000f35f RBX: ffffffffc0011fa8 RCX: 0000000000000007 [ 1.254584] RDX: 0000000000000007 RSI: ffff88001ed43ba2 RDI: ffffffffc000f35f [ 1.255736] RBP: ffff88001ed43b90 R08: ffffffff81a046c6 R09: ffffffff81063f77 [ 1.256899] R10: ffffffff81f33f00 R11: ffffffff81f33ee0 R12: 0000000000000246 [ 1.258042] R13: ffffffffc0011fb4 R14: ffffffff81c70005 R15: ffffffff81c6ffff [ 1.259195] FS: 00007fc8b518e8c0(0000) GS:ffff88001fc00000(0000) knlGS:0000000000000000 [ 1.260564] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1.261488] CR2: ffffffffc000f35f CR3: 000000001ed2d000 CR4: 00000000000006f0 [ 1.262587] Stack: [ 1.263039] ffffffff810350f2 ffffffffc0011fa8 ffff88001ed43ba2 ffff88001ed43cc0 [ 1.264565] ffffffff8103584d 401f0f0007c63ec0 ffffffffff57a000 0000000000000000 [ 1.266084] 0000000000000001 ffff88001ed43c9f 0000160000000000 ffff88001ed43bf0 [ 1.267630] Call Trace: [ 1.268139] [] ? text_poke_early+0x22/0x40 [ 1.269065] [] apply_paravirt.part.2+0xad/0x140 [ 1.270046] [] ? set_pte_vaddr_pud+0x3c/0x50 [ 1.270995] [] ? set_pte_vaddr+0x63/0xa0 [ 1.271909] [] ? redo_fd_request+0x122f/0x13ea [floppy] [ 1.272933] [] ? __native_set_fixmap+0x28/0x40 [ 1.273860] [] ? native_set_fixmap+0x3a/0x40 [ 1.274765] [] ? 0xffffffffc0008000 [ 1.285259] [] ? redo_fd_request+0x13ea/0x13ea [floppy] [ 1.286251] [] ? alternatives_smp_module_add+0x59/0x190 [ 1.287244] [] apply_paravirt+0x19/0x20 [ 1.288072] [] module_finalize+0xf4/0x150 [ 1.288923] [] load_module+0x1f87/0x2b00 [ 1.289760] [] ? __vfs_read+0xc8/0x110 [ 1.290577] [] ? ima_post_read_file+0x7d/0xa0 [ 1.291464] [] SYSC_finit_module+0xe6/0x120 [ 1.292326] [] SyS_finit_module+0xe/0x10 [ 1.293162] [] entry_SYSCALL_64_fastpath+0x1e/0xa8 [ 1.294095] Code: ff ff 48 8b 43 60 48 2b 43 50 88 43 4e 5b 5d f3 c3 90 90 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 f3 48 a5 89 d1 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4 c3 0f 1f 80 Tested the patch and noticed a crash on boot once init starts. Does module_finalize in load_module need to be moved before any of the other functions perhaps? --chris