| Message ID | 20181023175553.gaobskk26koft6s2@linux-8ccs (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/module: use mod->klp_info section header information | expand |
Hi Jessica,
I love your patch! Perhaps something to improve:
[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on v4.19 next-20181019]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jessica-Yu/arm64-module-use-mod-klp_info-section-header-information/20181024-023709
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: x86_64-randconfig-x002-201842 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
kernel/module.c: In function 'post_relocation':
>> kernel/module.c:3369:30: warning: passing argument 2 of 'copy_module_elf' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
err = copy_module_elf(mod, info);
^~~~
kernel/module.c:2103:12: note: expected 'struct load_info *' but argument is of type 'const struct load_info *'
static int copy_module_elf(struct module *mod, struct load_info *info)
^~~~~~~~~~~~~~~
vim +3369 kernel/module.c
3353
3354 static int post_relocation(struct module *mod, const struct load_info *info)
3355 {
3356 int err;
3357
3358 /* Sort exception table now relocations are done. */
3359 sort_extable(mod->extable, mod->extable + mod->num_exentries);
3360
3361 /* Copy relocated percpu area over. */
3362 percpu_modcopy(mod, (void *)info->sechdrs[info->index.pcpu].sh_addr,
3363 info->sechdrs[info->index.pcpu].sh_size);
3364
3365 /* Setup kallsyms-specific fields. */
3366 add_kallsyms(mod, info);
3367
3368 if (is_livepatch_module(mod)) {
> 3369 err = copy_module_elf(mod, info);
3370 if (err < 0)
3371 return err;
3372 }
3373
3374 /* Arch-specific module finalizing. */
3375 err = module_finalize(info->hdr, info->sechdrs, mod);
3376 if (err < 0)
3377 free_module_elf(mod);
3378
3379 return err;
3380 }
3381
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, 23 Oct 2018, Jessica Yu wrote: > diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c > index f0690c2ca3e0..05067717dfc5 100644 > --- a/arch/arm64/kernel/module-plts.c > +++ b/arch/arm64/kernel/module-plts.c > @@ -210,9 +210,15 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr > *sechdrs, > * entries. Record the symtab address as well. > */ > for (i = 0; i < ehdr->e_shnum; i++) { > - if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) > + if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) { > mod->arch.core.plt = sechdrs + i; > - else if (!strcmp(secstrings + sechdrs[i].sh_name, > ".init.plt")) > + /* > + * Keep the section index for the .plt section for > + * livepatching. Note that .init.plt is irrelevant to > + * livepatch, so only the shndx for .plt is saved. > + */ > + mod->arch.core.plt_shndx = i; > + } else if (!strcmp(secstrings + sechdrs[i].sh_name, > ".init.plt")) Else branches should have braces now too according to the coding style. Otherwise it looks good to me. Thanks, Miroslav
On Tue 2018-10-23 19:55:54, Jessica Yu wrote: > The arm64 module loader keeps a pointer into info->sechdrs to keep track > of section header information for .plt section(s). A pointer to the > relevent section header (struct elf64_shdr) in info->sechdrs is stored > in mod->arch.{init,core}.plt. This pointer may be accessed while > applying relocations in apply_relocate_add() for example. And unlike > normal modules, livepatch modules can call apply_relocate_add() after > module load. But the info struct (and therefore info->sechdrs) gets > freed at the end of load_module() and so mod->arch.{init,core}.plt > becomes an invalid pointer after the module is done loading. > > Luckily, livepatch modules already keep a copy of Elf section header > information in mod->klp_info. So make sure livepatch modules on arm64 > have access to the section headers in klp_info and set > mod->arch.{init,core}.plt to the appropriate section header in > mod->klp_info so that they can call apply_relocate_add() even after > module load. > > diff --git a/kernel/module.c b/kernel/module.c > index f475f30eed8c..f3ac04cc9fc3 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -3367,6 +3367,8 @@ int __weak module_finalize(const Elf_Ehdr *hdr, > > static int post_relocation(struct module *mod, const struct load_info *info) > { > + int err; > + > /* Sort exception table now relocations are done. */ > sort_extable(mod->extable, mod->extable + mod->num_exentries); > > @@ -3377,8 +3379,18 @@ static int post_relocation(struct module *mod, const struct load_info *info) > /* Setup kallsyms-specific fields. */ > add_kallsyms(mod, info); > > + if (is_livepatch_module(mod)) { > + err = copy_module_elf(mod, info); > + if (err < 0) > + return err; > + } > + > /* Arch-specific module finalizing. */ > - return module_finalize(info->hdr, info->sechdrs, mod); > + err = module_finalize(info->hdr, info->sechdrs, mod); > + if (err < 0) if (err < 0 && is_livepatch_module(mod)) > + free_module_elf(mod); > + > + return err; > } Also we need to free the copied stuff in load_module() when anything called after post_relocation() fails. I think that the following would work: --- a/kernel/module.c +++ b/kernel/module.c @@ -3823,6 +3823,8 @@ static int load_module(struct load_info *info, const char __user *uargs, kfree(mod->args); free_arch_cleanup: module_arch_cleanup(mod); + if (is_livepatch_module(mod)) + free_module_elf(mod); free_modinfo: free_modinfo(mod); free_unload: But I suggest to just move copy_module_elf() up and keep calling it from load_module() directly. It would make the error handling more clear. Best Regards, Petr
On Thu, 25 Oct 2018, Petr Mladek wrote: > On Tue 2018-10-23 19:55:54, Jessica Yu wrote: > > The arm64 module loader keeps a pointer into info->sechdrs to keep track > > of section header information for .plt section(s). A pointer to the > > relevent section header (struct elf64_shdr) in info->sechdrs is stored > > in mod->arch.{init,core}.plt. This pointer may be accessed while > > applying relocations in apply_relocate_add() for example. And unlike > > normal modules, livepatch modules can call apply_relocate_add() after > > module load. But the info struct (and therefore info->sechdrs) gets > > freed at the end of load_module() and so mod->arch.{init,core}.plt > > becomes an invalid pointer after the module is done loading. > > > > Luckily, livepatch modules already keep a copy of Elf section header > > information in mod->klp_info. So make sure livepatch modules on arm64 > > have access to the section headers in klp_info and set > > mod->arch.{init,core}.plt to the appropriate section header in > > mod->klp_info so that they can call apply_relocate_add() even after > > module load. > > > > diff --git a/kernel/module.c b/kernel/module.c > > index f475f30eed8c..f3ac04cc9fc3 100644 > > --- a/kernel/module.c > > +++ b/kernel/module.c > > @@ -3367,6 +3367,8 @@ int __weak module_finalize(const Elf_Ehdr *hdr, > > > > static int post_relocation(struct module *mod, const struct load_info *info) > > { > > + int err; > > + > > /* Sort exception table now relocations are done. */ > > sort_extable(mod->extable, mod->extable + mod->num_exentries); > > > > @@ -3377,8 +3379,18 @@ static int post_relocation(struct module *mod, const struct load_info *info) > > /* Setup kallsyms-specific fields. */ > > add_kallsyms(mod, info); > > > > + if (is_livepatch_module(mod)) { > > + err = copy_module_elf(mod, info); > > + if (err < 0) > > + return err; > > + } > > + > > /* Arch-specific module finalizing. */ > > - return module_finalize(info->hdr, info->sechdrs, mod); > > + err = module_finalize(info->hdr, info->sechdrs, mod); > > + if (err < 0) > > if (err < 0 && is_livepatch_module(mod)) Ah, right. > > + free_module_elf(mod); > > + > > + return err; > > } > > Also we need to free the copied stuff in load_module() when > anything called after post_relocation() fails. I think > that the following would work: > > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -3823,6 +3823,8 @@ static int load_module(struct load_info *info, const char __user *uargs, > kfree(mod->args); > free_arch_cleanup: > module_arch_cleanup(mod); > + if (is_livepatch_module(mod)) > + free_module_elf(mod); > free_modinfo: > free_modinfo(mod); > free_unload: Yes, we need to free it somewhere and I missed it. free_arch_cleanup seems to be the correct place. > But I suggest to just move copy_module_elf() up and keep > calling it from load_module() directly. It would make > the error handling more clear. Unfortunately it is not that simple. arm64's module_finalize() uses mod->klp_info with the patch, so copy_module_elf() must be called before. We could move module_finalize() from post_relocation() to load_module() and place copy_module_elf() between those two, but I don't know. That's up to Jessica. Miroslav
+++ Miroslav Benes [25/10/18 11:00 +0200]: >On Thu, 25 Oct 2018, Petr Mladek wrote: > >> On Tue 2018-10-23 19:55:54, Jessica Yu wrote: >> > The arm64 module loader keeps a pointer into info->sechdrs to keep track >> > of section header information for .plt section(s). A pointer to the >> > relevent section header (struct elf64_shdr) in info->sechdrs is stored >> > in mod->arch.{init,core}.plt. This pointer may be accessed while >> > applying relocations in apply_relocate_add() for example. And unlike >> > normal modules, livepatch modules can call apply_relocate_add() after >> > module load. But the info struct (and therefore info->sechdrs) gets >> > freed at the end of load_module() and so mod->arch.{init,core}.plt >> > becomes an invalid pointer after the module is done loading. >> > >> > Luckily, livepatch modules already keep a copy of Elf section header >> > information in mod->klp_info. So make sure livepatch modules on arm64 >> > have access to the section headers in klp_info and set >> > mod->arch.{init,core}.plt to the appropriate section header in >> > mod->klp_info so that they can call apply_relocate_add() even after >> > module load. >> > >> > diff --git a/kernel/module.c b/kernel/module.c >> > index f475f30eed8c..f3ac04cc9fc3 100644 >> > --- a/kernel/module.c >> > +++ b/kernel/module.c >> > @@ -3367,6 +3367,8 @@ int __weak module_finalize(const Elf_Ehdr *hdr, >> > >> > static int post_relocation(struct module *mod, const struct load_info *info) >> > { >> > + int err; >> > + >> > /* Sort exception table now relocations are done. */ >> > sort_extable(mod->extable, mod->extable + mod->num_exentries); >> > >> > @@ -3377,8 +3379,18 @@ static int post_relocation(struct module *mod, const struct load_info *info) >> > /* Setup kallsyms-specific fields. */ >> > add_kallsyms(mod, info); >> > >> > + if (is_livepatch_module(mod)) { >> > + err = copy_module_elf(mod, info); >> > + if (err < 0) >> > + return err; >> > + } >> > + >> > /* Arch-specific module finalizing. */ >> > - return module_finalize(info->hdr, info->sechdrs, mod); >> > + err = module_finalize(info->hdr, info->sechdrs, mod); >> > + if (err < 0) >> >> if (err < 0 && is_livepatch_module(mod)) > >Ah, right. > >> > + free_module_elf(mod); >> > + >> > + return err; >> > } >> >> Also we need to free the copied stuff in load_module() when >> anything called after post_relocation() fails. I think >> that the following would work: >> >> --- a/kernel/module.c >> +++ b/kernel/module.c >> @@ -3823,6 +3823,8 @@ static int load_module(struct load_info *info, const char __user *uargs, >> kfree(mod->args); >> free_arch_cleanup: >> module_arch_cleanup(mod); >> + if (is_livepatch_module(mod)) >> + free_module_elf(mod); >> free_modinfo: >> free_modinfo(mod); >> free_unload: > >Yes, we need to free it somewhere and I missed it. free_arch_cleanup seems >to be the correct place. Good catches, thank you both! >> But I suggest to just move copy_module_elf() up and keep >> calling it from load_module() directly. It would make >> the error handling more clear. > >Unfortunately it is not that simple. arm64's module_finalize() uses >mod->klp_info with the patch, so copy_module_elf() must be called before. >We could move module_finalize() from post_relocation() to load_module() >and place copy_module_elf() between those two, but I don't know. That's up >to Jessica. Yeah, it's a stylistic preference - will shuffle those calls around and see what looks best. v2 to come shortly. Thanks! Jessica
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h index fef773c94e9d..ac9b97f9ae5e 100644 --- a/arch/arm64/include/asm/module.h +++ b/arch/arm64/include/asm/module.h @@ -25,6 +25,7 @@ struct mod_plt_sec { struct elf64_shdr *plt; int plt_num_entries; int plt_max_entries; + int plt_shndx; }; struct mod_arch_specific { diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c index f0690c2ca3e0..05067717dfc5 100644 --- a/arch/arm64/kernel/module-plts.c +++ b/arch/arm64/kernel/module-plts.c @@ -210,9 +210,15 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, * entries. Record the symtab address as well. */ for (i = 0; i < ehdr->e_shnum; i++) { - if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) + if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) { mod->arch.core.plt = sechdrs + i; - else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt")) + /* + * Keep the section index for the .plt section for + * livepatching. Note that .init.plt is irrelevant to + * livepatch, so only the shndx for .plt is saved. + */ + mod->arch.core.plt_shndx = i; + } else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt")) mod->arch.init.plt = sechdrs + i; else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) && !strcmp(secstrings + sechdrs[i].sh_name, diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c index dd23655fda3a..490e56070a7e 100644 --- a/arch/arm64/kernel/module.c +++ b/arch/arm64/kernel/module.c @@ -461,5 +461,15 @@ int module_finalize(const Elf_Ehdr *hdr, #endif } +#ifdef CONFIG_LIVEPATCH + /* + * For livepatching, switch to the saved section header info for .plt + * stored in mod->klp_info. This is needed so that livepatch is able to + * call apply_relocate_add() after patch module load. + */ + if (is_livepatch_module(me)) + me->arch.core.plt = me->klp_info->sechdrs + me->arch.core.plt_shndx; +#endif + return 0; } diff --git a/kernel/module.c b/kernel/module.c index f475f30eed8c..f3ac04cc9fc3 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3367,6 +3367,8 @@ int __weak module_finalize(const Elf_Ehdr *hdr, static int post_relocation(struct module *mod, const struct load_info *info) { + int err; + /* Sort exception table now relocations are done. */ sort_extable(mod->extable, mod->extable + mod->num_exentries); @@ -3377,8 +3379,18 @@ static int post_relocation(struct module *mod, const struct load_info *info) /* Setup kallsyms-specific fields. */ add_kallsyms(mod, info); + if (is_livepatch_module(mod)) { + err = copy_module_elf(mod, info); + if (err < 0) + return err; + } + /* Arch-specific module finalizing. */ - return module_finalize(info->hdr, info->sechdrs, mod); + err = module_finalize(info->hdr, info->sechdrs, mod); + if (err < 0) + free_module_elf(mod); + + return err; } /* Is this module of this name done loading? No locks held. */ @@ -3770,12 +3782,6 @@ static int load_module(struct load_info *info, const char __user *uargs, if (err < 0) goto coming_cleanup; - if (is_livepatch_module(mod)) { - err = copy_module_elf(mod, info); - if (err < 0) - goto sysfs_cleanup; - } - /* Get rid of temporary copy. */ free_copy(info); @@ -3784,8 +3790,6 @@ static int load_module(struct load_info *info, const char __user *uargs, return do_init_module(mod); - sysfs_cleanup: - mod_sysfs_teardown(mod); coming_cleanup: mod->state = MODULE_STATE_GOING; destroy_params(mod->kp, mod->num_kp);
The arm64 module loader keeps a pointer into info->sechdrs to keep track of section header information for .plt section(s). A pointer to the relevent section header (struct elf64_shdr) in info->sechdrs is stored in mod->arch.{init,core}.plt. This pointer may be accessed while applying relocations in apply_relocate_add() for example. And unlike normal modules, livepatch modules can call apply_relocate_add() after module load. But the info struct (and therefore info->sechdrs) gets freed at the end of load_module() and so mod->arch.{init,core}.plt becomes an invalid pointer after the module is done loading. Luckily, livepatch modules already keep a copy of Elf section header information in mod->klp_info. So make sure livepatch modules on arm64 have access to the section headers in klp_info and set mod->arch.{init,core}.plt to the appropriate section header in mod->klp_info so that they can call apply_relocate_add() even after module load. Signed-off-by: Jessica Yu <jeyu@kernel.org> --- Hi! This patch may be applied on top or merged with the 3rd patch. I incoporated Miroslav's suggestions from the discussion. It's needed in order for livepatch modules on arm64 to be able to call apply_relocate_add() post-module-load, otherwise we could end up accessing invalid pointers from apply_relocate_add(). arch/arm64/include/asm/module.h | 1 + arch/arm64/kernel/module-plts.c | 10 ++++++++-- arch/arm64/kernel/module.c | 10 ++++++++++ kernel/module.c | 22 +++++++++++++--------- 4 files changed, 32 insertions(+), 11 deletions(-)