From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754260AbbKLKFx (ORCPT ); Thu, 12 Nov 2015 05:05:53 -0500 Received: from mx2.suse.de ([195.135.220.15]:45625 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753642AbbKLKFv (ORCPT ); Thu, 12 Nov 2015 05:05:51 -0500 Date: Thu, 12 Nov 2015 11:05:48 +0100 From: Petr Mladek To: Jessica Yu Cc: Rusty Russell , Josh Poimboeuf , Seth Jennings , Jiri Kosina , Vojtech Pavlik , Miroslav Benes , linux-api@vger.kernel.org, live-patching@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: module: save load_info for livepatch modules Message-ID: <20151112100548.GK4431@pathway.suse.cz> References: <1447130755-17383-1-git-send-email-jeyu@redhat.com> <1447130755-17383-3-git-send-email-jeyu@redhat.com> <20151111143152.GG4431@pathway.suse.cz> <20151112044407.GC30025@packer-debian-8-amd64.digitalocean.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151112044407.GC30025@packer-debian-8-amd64.digitalocean.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed 2015-11-11 23:44:08, Jessica Yu wrote: > +++ Petr Mladek [11/11/15 15:31 +0100]: > >On Mon 2015-11-09 23:45:52, Jessica Yu wrote: > >>diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > >>index 6e53441..087a8c7 100644 > >>--- a/kernel/livepatch/core.c > >>+++ b/kernel/livepatch/core.c > >>@@ -1001,6 +1001,23 @@ static struct notifier_block klp_module_nb = { > >> .priority = INT_MIN+1, /* called late but before ftrace notifier */ > >> }; > >> > >>+/* > >>+ * Save necessary information from info in order to be able to > >>+ * patch modules that might be loaded later > >>+ */ > >>+void klp_prepare_patch_module(struct module *mod, struct load_info *info) > >>+{ > >>+ Elf_Shdr *symsect; > >>+ > >>+ symsect = info->sechdrs + info->index.sym; > >>+ /* update sh_addr to point to symtab */ > >>+ symsect->sh_addr = (unsigned long)info->hdr + symsect->sh_offset; > > > >Is livepatch the only user of this value? By other words, is this safe? > > I think it is safe to say yes. klp_prepare_patch_module() is only > called at the very end of load_module(), right before > do_init_module(). Normally, at that point, info->hdr will have already > been freed by free_copy() along with the elf section information > associated with it. But if we have a livepatch module, we don't free. > So we should be the very last user, and there should be nobody > utilizing the memory associated with the load_info struct anymore at > that point. I see. It looks safe at this point. But still I wonder if it would be possible to calculate this later in the livepatch code. It will allow to potentially use the info structure also by other subsystem. BTW: Where is "sh_addr" value used, please? I see it used only in the third patch as info->sechdrs[relindex].sh_addr. But it is an array. I am not sure if it is the same variable. > >>+ mod->info = kzalloc(sizeof(*info), GFP_KERNEL); > >>+ memcpy(mod->info, info, sizeof(*info)); > >>+ > >>+} > > > >It is strange that this funtion is defined in livepatch/core.c > >but declared in module.h. I would move the definition to > >module.c. > > Right, I was trying to keep all the livepatch-related functions > together in livepatch/core.c. but I can move it to module.c if it > makes more sense/Rusty doesn't object to it :-) Sure. I think that we could use some generic name, e.g. copy_module_info(). > >> static int __init klp_init(void) > >> { > >> int ret; > >>diff --git a/kernel/module.c b/kernel/module.c > >>index 8f051a1..8ae3ca5 100644 > >>--- a/kernel/module.c > >>+++ b/kernel/module.c > >>@@ -2137,6 +2123,11 @@ static int simplify_symbols(struct module *mod, const struct load_info *info) > >> (long)sym[i].st_value); > >> break; > >> > >>+#ifdef CONFIG_LIVEPATCH > >>+ case SHN_LIVEPATCH: > >>+ break; > >>+#endif > > > >IMHO, even a kernel compiled without CONFIG_LIVEPATCH should handle livepatch > >modules with grace. It means to reject loading. > > I think even right now, without considering this patchset, we don't > reject modules "gracefully" when we load a livepatch module without > CONFIG_LIVEPATCH. The module loader will complain and reject the > livepatch module, saying something like "Unknown symbol > klp_register_patch." This behavior is the same with or without > this patch series applied. If we want to add a bit more logic to > gracefully reject patch modules, perhaps that should be a different > patch altogether, as I think it is unrelated to the goal of this one :-) Yup, the module load would fail anyway because of the missing symbol. But I think that we should fail on the first error occurence. In each case, IMHO, we should not do the "default:" action for this section even when complied without CONFIG_LIVEPATCH. > >> case SHN_UNDEF: > >> ksym = resolve_symbol_wait(mod, info, name); > >> /* Ok if resolved. */ > >>@@ -2185,6 +2176,11 @@ static int apply_relocations(struct module *mod, const struct load_info *info) > >> if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC)) > >> continue; > >> > >>+#ifdef CONFIG_LIVEPATCH > >>+ if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH) > >>+ continue; > >>+#endif I guess that if we do not trigger the error above, and do not have the check here, we will try to call apply_relocate() below. I guess that it will fail. If we are lucky it will print "Unknown relocation". I think that we could do better. > >>+ > >> if (info->sechdrs[i].sh_type == SHT_REL) > >> err = apply_relocate(info->sechdrs, info->strtab, > >> info->index.sym, i, mod); > >>@@ -3530,8 +3526,20 @@ static int load_module(struct load_info *info, const char __user *uargs, > >> if (err < 0) > >> goto bug_cleanup; > >> > >>+#ifdef CONFIG_LIVEPATCH > >>+ /* > >>+ * Save sechdrs, indices, and other data from info > >>+ * in order to patch to-be-loaded modules. > >>+ * Do not call free_copy() for livepatch modules. > >>+ */ > >>+ if (get_modinfo((struct load_info *)info, "livepatch")) > >>+ klp_prepare_patch_module(mod, info); > >>+ else > >>+ free_copy(info); > >>+#else > > > >I would move this #else one line above and get rid of the > >double free_copy(info); But it is a matter of taste. > > Maybe I'm missing something, but I think we do need the double > free_copy(), because in the CONFIG_LIVEPATCH case, we still want to > call free_copy() for non-livepatch modules. And we want to avoid > calling free_copy() for livepatch modules (hence the extra else). Ah, this was just a cosmetic change. I meant to use something like: #ifdef CONFIG_LIVEPATCH /* * Save sechdrs, indices, and other data from info * in order to patch to-be-loaded modules. * Do not call free_copy() for livepatch modules. */ if (get_modinfo((struct load_info *)info, "livepatch")) klp_prepare_patch_module(mod, info); else #endif /* Get rid of temporary copy. */ free_copy(info); It is a matter of taste. Maybe, your variant was better in the end. Thank you, Petr