Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759181AbcDAPBU (ORCPT ); Fri, 1 Apr 2016 11:01:20 -0400 Received: from mx2.suse.de ([195.135.220.15]:39025 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759119AbcDAPBT (ORCPT ); Fri, 1 Apr 2016 11:01:19 -0400 Date: Fri, 1 Apr 2016 17:01:16 +0200 (CEST) From: Jiri Kosina X-X-Sender: jkosina@pobox.suse.cz To: Miroslav Benes 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: 2399 Lines: 70 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; + /* Module is ready to execute: parsing args may do that. */ after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, -32768, 32767, mod, -- Jiri Kosina SUSE Labs