Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1267777
| From | Petr Mladek <pmladek@suse.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: module: save load_info for livepatch modules |
| Date | 2015-11-12 11:10 +0100 |
| Message-ID | <qtWZl-7yp-31@gated-at.bofh.it> (permalink) |
| References | <qt92x-kt-1@gated-at.bofh.it> <qt92y-kt-5@gated-at.bofh.it> <qtEJ3-3Zk-1@gated-at.bofh.it> <qtRZE-4de-3@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[RFC PATCH 0/5] Arch-independent livepatch Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
[RFC PATCH 2/5] module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
Re: [RFC PATCH 2/5] module: save load_info for livepatch modules Minfei Huang <mnfhuang@gmail.com> - 2015-11-11 09:10 +0100
Re: [RFC PATCH 2/5] module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-11 15:20 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-12 06:40 +0100
Re: module: save load_info for livepatch modules Petr Mladek <pmladek@suse.com> - 2015-11-12 11:30 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-12 14:30 +0100
Re: module: save load_info for livepatch modules Petr Mladek <pmladek@suse.com> - 2015-11-12 16:10 +0100
Re: module: save load_info for livepatch modules Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 18:10 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-12 23:20 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-13 13:30 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-13 13:50 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-14 01:40 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-13 14:00 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-14 03:20 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-13 01:30 +0100
Re: [RFC PATCH 2/5] module: save load_info for livepatch modules Petr Mladek <pmladek@suse.com> - 2015-11-11 15:40 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-12 05:50 +0100
Re: module: save load_info for livepatch modules Petr Mladek <pmladek@suse.com> - 2015-11-12 11:10 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-12 15:20 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-13 07:40 +0100
Re: module: save load_info for livepatch modules Miroslav Benes <mbenes@suse.cz> - 2015-11-13 14:10 +0100
Re: module: save load_info for livepatch modules Jessica Yu <jeyu@redhat.com> - 2015-11-13 09:30 +0100
Re: [RFC PATCH 2/5] module: save load_info for livepatch modules Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 18:20 +0100
Re: [RFC PATCH 2/5] module: save load_info for livepatch modules Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 18:30 +0100
[RFC PATCH 1/5] elf: add livepatch-specific elf constants Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
Re: [RFC PATCH 1/5] elf: add livepatch-specific elf constants Petr Mladek <pmladek@suse.com> - 2015-11-11 15:00 +0100
Re: [RFC PATCH 1/5] elf: add livepatch-specific elf constants Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 16:40 +0100
Re: [RFC PATCH 1/5] elf: add livepatch-specific elf constants Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 16:50 +0100
Re: elf: add livepatch-specific elf constants Jessica Yu <jeyu@redhat.com> - 2015-11-13 08:00 +0100
[RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
Re: [RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module Jiri Slaby <jslaby@suse.cz> - 2015-11-10 09:20 +0100
Re: [RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-10 15:00 +0100
Re: samples: livepatch: init reloc list and mark as klp module Jessica Yu <jeyu@redhat.com> - 2015-11-10 19:40 +0100
Re: [RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module Petr Mladek <pmladek@suse.com> - 2015-11-11 16:50 +0100
Re: samples: livepatch: init reloc list and mark as klp module Jessica Yu <jeyu@redhat.com> - 2015-11-12 07:10 +0100
Re: samples: livepatch: init reloc list and mark as klp module Miroslav Benes <mbenes@suse.cz> - 2015-11-12 11:50 +0100
[RFC PATCH 5/5] livepatch: x86: remove unused relocation code Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
Re: [RFC PATCH 5/5] livepatch: x86: remove unused relocation code Petr Mladek <pmladek@suse.com> - 2015-11-11 16:50 +0100
Re: [RFC PATCH 5/5] livepatch: x86: remove unused relocation code Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 19:10 +0100
[RFC PATCH 3/5] livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-10 05:50 +0100
Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations Jiri Slaby <jslaby@suse.cz> - 2015-11-10 09:20 +0100
Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations Miroslav Benes <mbenes@suse.cz> - 2015-11-11 15:40 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-11 21:10 +0100
Re: livepatch: reuse module loader code to write relocations Miroslav Benes <mbenes@suse.cz> - 2015-11-12 16:30 +0100
Re: livepatch: reuse module loader code to write relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 18:50 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-12 21:30 +0100
Re: livepatch: reuse module loader code to write relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 21:40 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-13 08:20 +0100
Re: livepatch: reuse module loader code to write relocations Miroslav Benes <mbenes@suse.cz> - 2015-11-13 15:00 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-12 20:20 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-12 21:40 +0100
Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations Petr Mladek <pmladek@suse.com> - 2015-11-11 16:30 +0100
Re: livepatch: reuse module loader code to write relocations Jessica Yu <jeyu@redhat.com> - 2015-11-11 19:30 +0100
Re: livepatch: reuse module loader code to write relocations Petr Mladek <pmladek@suse.com> - 2015-11-12 10:20 +0100
Re: [RFC PATCH 3/5] livepatch: reuse module loader code to write relocations Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-12 19:00 +0100
Re: [RFC PATCH 0/5] Arch-independent livepatch Miroslav Benes <mbenes@suse.cz> - 2015-11-11 15:10 +0100
Re: [RFC PATCH 0/5] Arch-independent livepatch Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-11 17:30 +0100
csiph-web