From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753357AbbKLEoO (ORCPT ); Wed, 11 Nov 2015 23:44:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50651 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753203AbbKLEoM (ORCPT ); Wed, 11 Nov 2015 23:44:12 -0500 Date: Wed, 11 Nov 2015 23:44:08 -0500 From: Jessica Yu To: Petr Mladek 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: <20151112044407.GC30025@packer-debian-8-amd64.digitalocean.com> References: <1447130755-17383-1-git-send-email-jeyu@redhat.com> <1447130755-17383-3-git-send-email-jeyu@redhat.com> <20151111143152.GG4431@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20151111143152.GG4431@pathway.suse.cz> X-OS: Linux eisen.io 3.16.0-4-amd64 x86_64 User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +++ Petr Mladek [11/11/15 15:31 +0100]: >On Mon 2015-11-09 23:45:52, Jessica Yu wrote: >> In livepatch modules, preserve section, symbol, string information from >> the load_info struct in the module loader. This information is used to >> patch modules that are not loaded in memory yet; specifically it is used >> to resolve remaining symbols and write relocations when the target >> module loads. >> >> Signed-off-by: Jessica Yu >> --- >> include/linux/module.h | 25 +++++++++++++++++++++++++ >> kernel/livepatch/core.c | 17 +++++++++++++++++ >> kernel/module.c | 36 ++++++++++++++++++++++-------------- >> 3 files changed, 64 insertions(+), 14 deletions(-) >> >> diff --git a/include/linux/module.h b/include/linux/module.h >> index 3a19c79..c8680b1 100644 >> --- a/include/linux/module.h >> +++ b/include/linux/module.h >[...] >> @@ -635,6 +651,15 @@ static inline bool module_requested_async_probing(struct module *module) >> return module && module->async_probe_requested; >> } >> >> +#ifdef CONFIG_LIVEPATCH >> +extern void klp_prepare_patch_module(struct module *mod, >> + struct load_info *info); >> +extern int >> +apply_relocate_add(Elf64_Shdr *sechdrs, const char *strtab, >> + unsigned int symindex, unsigned int relsec, >> + struct module *me); >> +#endif > >This function is already declared in moduleloader.h. >It is implemted only when CONFIG_MODULES_USE_ELF_RELA is defined. > >I guess that we want to include moduleloader.h in livepatch. > >> + >> #else /* !CONFIG_MODULES... */ >> >> /* Given an address, look for it in the exception tables. */ >> 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. >> + 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 :-) >> 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 >> @@ -318,20 +318,6 @@ int unregister_module_notifier(struct notifier_block *nb) >> } >> EXPORT_SYMBOL(unregister_module_notifier); >> >> -struct load_info { >> - Elf_Ehdr *hdr; >> - unsigned long len; >> - Elf_Shdr *sechdrs; >> - char *secstrings, *strtab; >> - unsigned long symoffs, stroffs; >> - struct _ddebug *debug; >> - unsigned int num_debug; >> - bool sig_ok; >> - struct { >> - unsigned int sym, str, mod, vers, info, pcpu; >> - } index; >> -}; >> - >> /* We require a truly strong try_module_get(): 0 means failure due to >> ongoing or failed initialization etc. */ >> static inline int strong_try_module_get(struct module *mod) >> @@ -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 :-) >> 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 >> + >> 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). >> /* Get rid of temporary copy. */ >> free_copy(info); >> +#endif > Thanks for the comments, Jessica From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jessica Yu Subject: Re: module: save load_info for livepatch modules Date: Wed, 11 Nov 2015 23:44:08 -0500 Message-ID: <20151112044407.GC30025@packer-debian-8-amd64.digitalocean.com> References: <1447130755-17383-1-git-send-email-jeyu@redhat.com> <1447130755-17383-3-git-send-email-jeyu@redhat.com> <20151111143152.GG4431@pathway.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Return-path: Content-Disposition: inline In-Reply-To: <20151111143152.GG4431-KsEp0d+Q8qECVLCxKZUutA@public.gmane.org> Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Petr Mladek Cc: Rusty Russell , Josh Poimboeuf , Seth Jennings , Jiri Kosina , Vojtech Pavlik , Miroslav Benes , linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, live-patching-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-api@vger.kernel.org +++ Petr Mladek [11/11/15 15:31 +0100]: >On Mon 2015-11-09 23:45:52, Jessica Yu wrote: >> In livepatch modules, preserve section, symbol, string information from >> the load_info struct in the module loader. This information is used to >> patch modules that are not loaded in memory yet; specifically it is used >> to resolve remaining symbols and write relocations when the target >> module loads. >> >> Signed-off-by: Jessica Yu >> --- >> include/linux/module.h | 25 +++++++++++++++++++++++++ >> kernel/livepatch/core.c | 17 +++++++++++++++++ >> kernel/module.c | 36 ++++++++++++++++++++++-------------- >> 3 files changed, 64 insertions(+), 14 deletions(-) >> >> diff --git a/include/linux/module.h b/include/linux/module.h >> index 3a19c79..c8680b1 100644 >> --- a/include/linux/module.h >> +++ b/include/linux/module.h >[...] >> @@ -635,6 +651,15 @@ static inline bool module_requested_async_probing(struct module *module) >> return module && module->async_probe_requested; >> } >> >> +#ifdef CONFIG_LIVEPATCH >> +extern void klp_prepare_patch_module(struct module *mod, >> + struct load_info *info); >> +extern int >> +apply_relocate_add(Elf64_Shdr *sechdrs, const char *strtab, >> + unsigned int symindex, unsigned int relsec, >> + struct module *me); >> +#endif > >This function is already declared in moduleloader.h. >It is implemted only when CONFIG_MODULES_USE_ELF_RELA is defined. > >I guess that we want to include moduleloader.h in livepatch. > >> + >> #else /* !CONFIG_MODULES... */ >> >> /* Given an address, look for it in the exception tables. */ >> 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. >> + 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 :-) >> 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 >> @@ -318,20 +318,6 @@ int unregister_module_notifier(struct notifier_block *nb) >> } >> EXPORT_SYMBOL(unregister_module_notifier); >> >> -struct load_info { >> - Elf_Ehdr *hdr; >> - unsigned long len; >> - Elf_Shdr *sechdrs; >> - char *secstrings, *strtab; >> - unsigned long symoffs, stroffs; >> - struct _ddebug *debug; >> - unsigned int num_debug; >> - bool sig_ok; >> - struct { >> - unsigned int sym, str, mod, vers, info, pcpu; >> - } index; >> -}; >> - >> /* We require a truly strong try_module_get(): 0 means failure due to >> ongoing or failed initialization etc. */ >> static inline int strong_try_module_get(struct module *mod) >> @@ -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 :-) >> 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 >> + >> 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). >> /* Get rid of temporary copy. */ >> free_copy(info); >> +#endif > Thanks for the comments, Jessica