Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752218AbcDAQBX (ORCPT ); Fri, 1 Apr 2016 12:01:23 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:48270 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751281AbcDAQBW (ORCPT ); Fri, 1 Apr 2016 12:01:22 -0400 Date: Fri, 1 Apr 2016 11:01:14 -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: <20160401160113.GA7837@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: 2746 Lines: 74 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 I'll test this out and see if it fixes the original issue. --chris