Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753717AbcDAPq4 (ORCPT ); Fri, 1 Apr 2016 11:46:56 -0400 Received: from mx2.suse.de ([195.135.220.15]:44848 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751299AbcDAPqz (ORCPT ); Fri, 1 Apr 2016 11:46:55 -0400 Date: Fri, 1 Apr 2016 17:46:52 +0200 (CEST) From: Miroslav Benes To: Jiri Kosina cc: Chris J Arges , 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 In-Reply-To: Message-ID: References: <20160329120518.GA21252@canonical.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 Content-Length: 2474 Lines: 70 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