Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1266222 > unrolled thread

[RFC PATCH 0/5] Arch-independent livepatch

Back to article view | Back to linux.kernel

Contents

  [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

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1268649 — Re: module: save load_info for livepatch modules

+++ Miroslav Benes [12/11/15 15:19 +0100]:
>On Thu, 12 Nov 2015, Petr Mladek wrote:
>
>> 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.
>
>Jessica, why do we need to update sh_addr for symtab? It is not clear to
>me.

Ah, I definitely need to make that comment a lot more informative.
Will make sure to add that in v2. 

So, the sh_addr field tells us where a certain section is in memory.
Here, we need to update the symbol table section's sh_addr because if
we don't, it will eventually point to freed module init memory, which
is freed in do_init_module(). Let me explain what happens.

At the beginning of load_module(), the sh_addr fields of each section
initially point to the vmalloc'd memory within info->hdr (which is
allocated in copy_module_from_{fd,user}() in module.c). The sh_addr's
are first assigned in rewrite_section_headers(), called from
setup_load_info(). These sh_addr's initially just point to an offset
within info->hdr depending on each section's sh_offset.

However, in move_module(), where we layout and allocate the memory
where the module will finally reside, these sh_addr's will get
reassigned. For the symtab section's sh_addr, it gets reassigned to
module init memory. (In layout_symtab(), you'll see that the symtab
section gets marked with INIT_OFFSET_MASK, which indicates that it
will get an address in init memory when the sh_addr's get reassigned
in move_module()). Thus the symbol table that simplify_symbols() uses
is actually in init memory, and will be freed later in
do_init_module().

info->hdr is just a temporary holding place for module elf section
data in memory. Normally, we would get rid of info->hdr and free the
memory associated with it at the end of the module loading process
(via free_copy()). But in this patchset, we save all the original elf
section information because we need it (along with the original
symtab) in order to make the call to apply_relocate_add(). If you look
at apply_relocate_add() for x86, s390, etc you'll see that it expects
a symbol table at the symbol section's sh_addr field (basically, an
array of Elf_Sym's). This is why we fix up the sh_addr of the symtab
section to point back to the memory associated with info->hdr (and not
module init memory). I hope that makes sense.

Thanks,
Jessica
--
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/

[toc] | [prev] | [next] | [standalone]

#1268936 — Re: module: save load_info for livepatch modules

On Fri, 13 Nov 2015, Jessica Yu wrote:

> +++ Miroslav Benes [12/11/15 15:19 +0100]:
> > On Thu, 12 Nov 2015, Petr Mladek wrote:
> > 
> > > 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.
> > 
> > Jessica, why do we need to update sh_addr for symtab? It is not clear to
> > me.
> 
> Ah, I definitely need to make that comment a lot more informative.
> Will make sure to add that in v2. 
> So, the sh_addr field tells us where a certain section is in memory.
> Here, we need to update the symbol table section's sh_addr because if
> we don't, it will eventually point to freed module init memory, which
> is freed in do_init_module(). Let me explain what happens.
> 
> At the beginning of load_module(), the sh_addr fields of each section
> initially point to the vmalloc'd memory within info->hdr (which is
> allocated in copy_module_from_{fd,user}() in module.c). The sh_addr's
> are first assigned in rewrite_section_headers(), called from
> setup_load_info(). These sh_addr's initially just point to an offset
> within info->hdr depending on each section's sh_offset.
> 
> However, in move_module(), where we layout and allocate the memory
> where the module will finally reside, these sh_addr's will get
> reassigned. For the symtab section's sh_addr, it gets reassigned to
> module init memory. (In layout_symtab(), you'll see that the symtab
> section gets marked with INIT_OFFSET_MASK, which indicates that it
> will get an address in init memory when the sh_addr's get reassigned
> in move_module()). Thus the symbol table that simplify_symbols() uses
> is actually in init memory, and will be freed later in
> do_init_module().
> 
> info->hdr is just a temporary holding place for module elf section
> data in memory. Normally, we would get rid of info->hdr and free the
> memory associated with it at the end of the module loading process
> (via free_copy()). But in this patchset, we save all the original elf
> section information because we need it (along with the original
> symtab) in order to make the call to apply_relocate_add(). If you look
> at apply_relocate_add() for x86, s390, etc you'll see that it expects
> a symbol table at the symbol section's sh_addr field (basically, an
> array of Elf_Sym's). This is why we fix up the sh_addr of the symtab
> section to point back to the memory associated with info->hdr (and not
> module init memory). I hope that makes sense.

Great explanation. Thanks.

Only info->hdr makes me worried. See my other mail.

Miroslav
--
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/

[toc] | [prev] | [next] | [standalone]

#1268683 — Re: module: save load_info for livepatch modules

+++ Petr Mladek [12/11/15 11:05 +0100]:
>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.

We can technically reassign sh_addr later in livepatch somewhere, yes,
I'd have to think more about where it'd make the most sense to do
this. Maybe in patch_init? It just seemed at the time a bit clearer to
do it in klp_prepare_patch_module() (soon to be called
copy_module_info() probably).

>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.

I will add a more informative comment in the code, see my reply to
Miroslav.

>
>> >>+	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.

See comment below -- 

>
>> >> 		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.

For the loading of livepatch modules in !CONFIG_LIVEPATCH kernels, we
should probably gracefully reject it in the beginning of load_module()
(so that MODULE_INFO flag might come in handy here after all). If it's
a livepatch module && !CONFIG_LIVEPATCH, reject it. Then we wouldn't
even call apply_relocations() here, we wouldn't run into the
possibility of this check falling through, nor would
simplify_symbols() be even called.

>> >>+
>> >> 		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);
>

Oh OK, so that's what you meant. :-)

Thanks,
Jessica
--
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/

[toc] | [prev] | [next] | [standalone]

#1268117 — Re: [RFC PATCH 2/5] module: save load_info for livepatch modules

On Mon, Nov 09, 2015 at 11:45:52PM -0500, 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 <jeyu@redhat.com>
> ---
>  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
> @@ -36,6 +36,20 @@ struct modversion_info {
>  	char name[MODULE_NAME_LEN];
>  };
>  
> +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;
> +};
> +
>  struct module;
>  
>  struct module_kobject {
> @@ -462,6 +476,8 @@ struct module {
>  
>  #ifdef CONFIG_LIVEPATCH
>  	bool klp_alive;
> +	/* save info to patch to-be-loaded modules */
> +	struct load_info *info;
>  #endif
>  
>  #ifdef CONFIG_MODULE_UNLOAD
> @@ -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
> +
>  #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;
> +
> +	mod->info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	memcpy(mod->info, info, sizeof(*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
> @@ -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
> +

The kernel coding style generally frowns on ifdefs inside functions.
IMO it's fine to just remove the ifdef completely here.

>  		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
> +

Same here, I think the ifdef can be removed.

>  		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
>  	/* Get rid of temporary copy. */
>  	free_copy(info);
> +#endif

Same here, IMO.  (And as Miroslav suggested, klp_prepare_patch_module()
can be renamed to something else (save_info()?) which exists even in the
!CONFIG_LIVEPATCH case.

-- 
Josh
--
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/

[toc] | [prev] | [next] | [standalone]

#1268122 — Re: [RFC PATCH 2/5] module: save load_info for livepatch modules

On Mon, Nov 09, 2015 at 11:45:52PM -0500, 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 <jeyu@redhat.com>
> ---
>  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
> @@ -36,6 +36,20 @@ struct modversion_info {
>  	char name[MODULE_NAME_LEN];
>  };
>  
> +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;
> +};
> +
>  struct module;
>  
>  struct module_kobject {
> @@ -462,6 +476,8 @@ struct module {
>  
>  #ifdef CONFIG_LIVEPATCH
>  	bool klp_alive;
> +	/* save info to patch to-be-loaded modules */
> +	struct load_info *info;
>  #endif
>  
>  #ifdef CONFIG_MODULE_UNLOAD
> @@ -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
> +
>  #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;
> +
> +	mod->info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	memcpy(mod->info, info, sizeof(*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
> @@ -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
> +

It would be good to add a short comment here stating that livepatch
symbols are resolved later by the klp code.

>  		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
> +

Same here, a short comment stating that livepatch relocations are
applied later by klp code.

>  		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
>  	/* Get rid of temporary copy. */
>  	free_copy(info);
> +#endif
>  
>  	/* Done! */
>  	trace_module_load(mod);
> -- 
> 2.4.3
> 

-- 
Josh
--
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/

[toc] | [prev] | [next] | [standalone]

#1266227 — [RFC PATCH 1/5] elf: add livepatch-specific elf constants

Add livepatch elf reloc section flag, livepatch symbol bind
and section index

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 include/uapi/linux/elf.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index 71e1d0e..967ce1b 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -118,6 +118,7 @@ typedef __s64	Elf64_Sxword;
 #define STB_LOCAL  0
 #define STB_GLOBAL 1
 #define STB_WEAK   2
+#define STB_LIVEPATCH_EXT 11
 
 #define STT_NOTYPE  0
 #define STT_OBJECT  1
@@ -286,6 +287,7 @@ typedef struct elf64_phdr {
 #define SHF_ALLOC	0x2
 #define SHF_EXECINSTR	0x4
 #define SHF_MASKPROC	0xf0000000
+#define SHF_RELA_LIVEPATCH  0x4000000
 
 /* special section indexes */
 #define SHN_UNDEF	0
@@ -295,6 +297,7 @@ typedef struct elf64_phdr {
 #define SHN_ABS		0xfff1
 #define SHN_COMMON	0xfff2
 #define SHN_HIRESERVE	0xffff
+#define SHN_LIVEPATCH 0xff21
  
 typedef struct elf32_shdr {
   Elf32_Word	sh_name;
-- 
2.4.3

--
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/

[toc] | [prev] | [next] | [standalone]

#1267200 — Re: [RFC PATCH 1/5] elf: add livepatch-specific elf constants

On Mon 2015-11-09 23:45:51, Jessica Yu wrote:
> Add livepatch elf reloc section flag, livepatch symbol bind
> and section index

Please, add here more detailed description from the initial mail.
It would be helpful to have it in the git history.

> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  include/uapi/linux/elf.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index 71e1d0e..967ce1b 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -118,6 +118,7 @@ typedef __s64	Elf64_Sxword;
>  #define STB_LOCAL  0
>  #define STB_GLOBAL 1
>  #define STB_WEAK   2
> +#define STB_LIVEPATCH_EXT 11
>  
>  #define STT_NOTYPE  0
>  #define STT_OBJECT  1
> @@ -286,6 +287,7 @@ typedef struct elf64_phdr {
>  #define SHF_ALLOC	0x2
>  #define SHF_EXECINSTR	0x4
>  #define SHF_MASKPROC	0xf0000000
> +#define SHF_RELA_LIVEPATCH  0x4000000

I would make the names ordered by the numeric values.

>  /* special section indexes */
>  #define SHN_UNDEF	0
> @@ -295,6 +297,7 @@ typedef struct elf64_phdr {
>  #define SHN_ABS		0xfff1
>  #define SHN_COMMON	0xfff2
>  #define SHN_HIRESERVE	0xffff
> +#define SHN_LIVEPATCH 0xff21

Same here.

Best Regards,
Petr

>  typedef struct elf32_shdr {
>    Elf32_Word	sh_name;
> -- 
> 2.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/

[toc] | [prev] | [next] | [standalone]

#1268031 — Re: [RFC PATCH 1/5] elf: add livepatch-specific elf constants

On Mon, Nov 09, 2015 at 11:45:51PM -0500, Jessica Yu wrote:
> Add livepatch elf reloc section flag, livepatch symbol bind
> and section index
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  include/uapi/linux/elf.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index 71e1d0e..967ce1b 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -118,6 +118,7 @@ typedef __s64	Elf64_Sxword;
>  #define STB_LOCAL  0
>  #define STB_GLOBAL 1
>  #define STB_WEAK   2
> +#define STB_LIVEPATCH_EXT 11
>  
>  #define STT_NOTYPE  0
>  #define STT_OBJECT  1
> @@ -286,6 +287,7 @@ typedef struct elf64_phdr {
>  #define SHF_ALLOC	0x2
>  #define SHF_EXECINSTR	0x4
>  #define SHF_MASKPROC	0xf0000000
> +#define SHF_RELA_LIVEPATCH  0x4000000
>  
>  /* special section indexes */
>  #define SHN_UNDEF	0
> @@ -295,6 +297,7 @@ typedef struct elf64_phdr {
>  #define SHN_ABS		0xfff1
>  #define SHN_COMMON	0xfff2
>  #define SHN_HIRESERVE	0xffff
> +#define SHN_LIVEPATCH 0xff21
>   
>  typedef struct elf32_shdr {
>    Elf32_Word	sh_name;

For all of these it would be good to re-indent the surrounding macros so
their values are all vertically aligned with the new LIVEPATCH ones.

-- 
Josh
--
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/

[toc] | [prev] | [next] | [standalone]

#1268040 — Re: [RFC PATCH 1/5] elf: add livepatch-specific elf constants

On Mon, Nov 09, 2015 at 11:45:51PM -0500, Jessica Yu wrote:
> Add livepatch elf reloc section flag, livepatch symbol bind
> and section index
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  include/uapi/linux/elf.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index 71e1d0e..967ce1b 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -118,6 +118,7 @@ typedef __s64	Elf64_Sxword;
>  #define STB_LOCAL  0
>  #define STB_GLOBAL 1
>  #define STB_WEAK   2
> +#define STB_LIVEPATCH_EXT 11
>  
>  #define STT_NOTYPE  0
>  #define STT_OBJECT  1
> @@ -286,6 +287,7 @@ typedef struct elf64_phdr {
>  #define SHF_ALLOC	0x2
>  #define SHF_EXECINSTR	0x4
>  #define SHF_MASKPROC	0xf0000000
> +#define SHF_RELA_LIVEPATCH  0x4000000

Writing the value with leading zeros (0x04000000) would it more
readable.

Also the OS-specific range mask (SHF_MASKOS) is 0x0ff00000.  Any reason
you went with 0x04000000 as opposed to the first value in the range
(0x00100000)?  I don't see anybody else using that value.

>  /* special section indexes */
>  #define SHN_UNDEF	0
> @@ -295,6 +297,7 @@ typedef struct elf64_phdr {
>  #define SHN_ABS		0xfff1
>  #define SHN_COMMON	0xfff2
>  #define SHN_HIRESERVE	0xffff
> +#define SHN_LIVEPATCH 0xff21

Similar question here, why not use 0xff20 (SHN_LOOS)?

-- 
Josh
--
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/

[toc] | [prev] | [next] | [standalone]

#1268656 — Re: elf: add livepatch-specific elf constants

+++ Josh Poimboeuf [12/11/15 09:45 -0600]:
>On Mon, Nov 09, 2015 at 11:45:51PM -0500, Jessica Yu wrote:
>> Add livepatch elf reloc section flag, livepatch symbol bind
>> and section index
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>>  include/uapi/linux/elf.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
>> index 71e1d0e..967ce1b 100644
>> --- a/include/uapi/linux/elf.h
>> +++ b/include/uapi/linux/elf.h
>> @@ -118,6 +118,7 @@ typedef __s64	Elf64_Sxword;
>>  #define STB_LOCAL  0
>>  #define STB_GLOBAL 1
>>  #define STB_WEAK   2
>> +#define STB_LIVEPATCH_EXT 11
>>
>>  #define STT_NOTYPE  0
>>  #define STT_OBJECT  1
>> @@ -286,6 +287,7 @@ typedef struct elf64_phdr {
>>  #define SHF_ALLOC	0x2
>>  #define SHF_EXECINSTR	0x4
>>  #define SHF_MASKPROC	0xf0000000
>> +#define SHF_RELA_LIVEPATCH  0x4000000
>
>Writing the value with leading zeros (0x04000000) would it more
>readable.
>
>Also the OS-specific range mask (SHF_MASKOS) is 0x0ff00000.  Any reason
>you went with 0x04000000 as opposed to the first value in the range
>(0x00100000)?  I don't see anybody else using that value.

I don't have any particular reason, I think I just picked any value
and ran with it. I'll just change it to the first value in the range
since that makes more sense.

>>  /* special section indexes */
>>  #define SHN_UNDEF	0
>> @@ -295,6 +297,7 @@ typedef struct elf64_phdr {
>>  #define SHN_ABS		0xfff1
>>  #define SHN_COMMON	0xfff2
>>  #define SHN_HIRESERVE	0xffff
>> +#define SHN_LIVEPATCH 0xff21
>
>Similar question here, why not use 0xff20 (SHN_LOOS)?
>
>-- 
>Josh
--
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/

[toc] | [prev] | [next] | [standalone]

#1266228 — [RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module

Intialize the list of relocation sections in the sample
klp_object (even if the list will be empty in this case).
Also mark module as a livepatch module so that the module
loader can appropriately initialize it.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 samples/livepatch/livepatch-sample.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
index fb8c861..2ef9345 100644
--- a/samples/livepatch/livepatch-sample.c
+++ b/samples/livepatch/livepatch-sample.c
@@ -57,6 +57,7 @@ static struct klp_object objs[] = {
 	{
 		/* name being NULL means vmlinux */
 		.funcs = funcs,
+		.reloc_secs = LIST_HEAD_INIT(objs[0].reloc_secs)
 	}, { }
 };
 
@@ -89,3 +90,4 @@ static void livepatch_exit(void)
 module_init(livepatch_init);
 module_exit(livepatch_exit);
 MODULE_LICENSE("GPL");
+MODULE_INFO(livepatch, "Y");
-- 
2.4.3

--
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/

[toc] | [prev] | [next] | [standalone]

#1266311 — Re: [RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module

On 11/10/2015, 05:45 AM, Jessica Yu wrote:
> Intialize the list of relocation sections in the sample
> klp_object (even if the list will be empty in this case).
> Also mark module as a livepatch module so that the module
> loader can appropriately initialize it.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  samples/livepatch/livepatch-sample.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
> index fb8c861..2ef9345 100644
> --- a/samples/livepatch/livepatch-sample.c
> +++ b/samples/livepatch/livepatch-sample.c
> @@ -57,6 +57,7 @@ static struct klp_object objs[] = {
>  	{
>  		/* name being NULL means vmlinux */
>  		.funcs = funcs,
> +		.reloc_secs = LIST_HEAD_INIT(objs[0].reloc_secs)

And I see now. This is not the best place for it though. I would put
INIT_LIST_HEAD in the core code instead.

thanks,
-- 
js
suse labs
--
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/

[toc] | [prev] | [next] | [standalone]

#1266515 — Re: [RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module

On Tue, Nov 10, 2015 at 09:15:54AM +0100, Jiri Slaby wrote:
> On 11/10/2015, 05:45 AM, Jessica Yu wrote:
> > Intialize the list of relocation sections in the sample
> > klp_object (even if the list will be empty in this case).
> > Also mark module as a livepatch module so that the module
> > loader can appropriately initialize it.
> > 
> > Signed-off-by: Jessica Yu <jeyu@redhat.com>
> > ---
> >  samples/livepatch/livepatch-sample.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
> > index fb8c861..2ef9345 100644
> > --- a/samples/livepatch/livepatch-sample.c
> > +++ b/samples/livepatch/livepatch-sample.c
> > @@ -57,6 +57,7 @@ static struct klp_object objs[] = {
> >  	{
> >  		/* name being NULL means vmlinux */
> >  		.funcs = funcs,
> > +		.reloc_secs = LIST_HEAD_INIT(objs[0].reloc_secs)
> 
> And I see now. This is not the best place for it though. I would put
> INIT_LIST_HEAD in the core code instead.

Maybe the reloc_secs list should instead be an array of klp_reloc_sec
structs, with the last entry being 0, similar to how we do other "lists"
in the klp interface (e.g., klp_patch.objs, klp_object.funcs).

-- 
Josh
--
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/

[toc] | [prev] | [next] | [standalone]

#1266707 — Re: samples: livepatch: init reloc list and mark as klp module

+++ Josh Poimboeuf [10/11/15 07:50 -0600]:
>On Tue, Nov 10, 2015 at 09:15:54AM +0100, Jiri Slaby wrote:
>> On 11/10/2015, 05:45 AM, Jessica Yu wrote:
>> > Intialize the list of relocation sections in the sample
>> > klp_object (even if the list will be empty in this case).
>> > Also mark module as a livepatch module so that the module
>> > loader can appropriately initialize it.
>> >
>> > Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> > ---
>> >  samples/livepatch/livepatch-sample.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
>> > index fb8c861..2ef9345 100644
>> > --- a/samples/livepatch/livepatch-sample.c
>> > +++ b/samples/livepatch/livepatch-sample.c
>> > @@ -57,6 +57,7 @@ static struct klp_object objs[] = {
>> >  	{
>> >  		/* name being NULL means vmlinux */
>> >  		.funcs = funcs,
>> > +		.reloc_secs = LIST_HEAD_INIT(objs[0].reloc_secs)
>>
>> And I see now. This is not the best place for it though. I would put
>> INIT_LIST_HEAD in the core code instead.
>
>Maybe the reloc_secs list should instead be an array of klp_reloc_sec
>structs, with the last entry being 0, similar to how we do other "lists"
>in the klp interface (e.g., klp_patch.objs, klp_object.funcs).

Yeah, I admit the asymmetry is a bit awkward. Plus we can add in a
klp_for_each_reloc_sec to keep the api consistent. I'll go ahead and
change this to follow the existing klp "list" format.

Jessica
--
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/

[toc] | [prev] | [next] | [standalone]

#1267262 — Re: [RFC PATCH 4/5] samples: livepatch: init reloc list and mark as klp module

On Mon 2015-11-09 23:45:54, Jessica Yu wrote:
> Intialize the list of relocation sections in the sample
> klp_object (even if the list will be empty in this case).
> Also mark module as a livepatch module so that the module
> loader can appropriately initialize it.
> 
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> ---
>  samples/livepatch/livepatch-sample.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
> index fb8c861..2ef9345 100644
> --- a/samples/livepatch/livepatch-sample.c
> +++ b/samples/livepatch/livepatch-sample.c
> @@ -89,3 +90,4 @@ static void livepatch_exit(void)
>  module_init(livepatch_init);
>  module_exit(livepatch_exit);
>  MODULE_LICENSE("GPL");
> +MODULE_INFO(livepatch, "Y");

This looks a bit error prone. I wonder if we could detect this
information another way. For example, by a check for the
livepatch-related elf sections. If it is missing,
we do not need to preserve struct load_info even
when it is a livepatch.

Best Regards,
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/

[toc] | [prev] | [next] | [standalone]

#1267650 — Re: samples: livepatch: init reloc list and mark as klp module

+++ Petr Mladek [11/11/15 16:42 +0100]:
>On Mon 2015-11-09 23:45:54, Jessica Yu wrote:
>> Intialize the list of relocation sections in the sample
>> klp_object (even if the list will be empty in this case).
>> Also mark module as a livepatch module so that the module
>> loader can appropriately initialize it.
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>> ---
>>  samples/livepatch/livepatch-sample.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
>> index fb8c861..2ef9345 100644
>> --- a/samples/livepatch/livepatch-sample.c
>> +++ b/samples/livepatch/livepatch-sample.c
>> @@ -89,3 +90,4 @@ static void livepatch_exit(void)
>>  module_init(livepatch_init);
>>  module_exit(livepatch_exit);
>>  MODULE_LICENSE("GPL");
>> +MODULE_INFO(livepatch, "Y");
>
>This looks a bit error prone. I wonder if we could detect this
>information another way. For example, by a check for the
>livepatch-related elf sections. If it is missing,
>we do not need to preserve struct load_info even
>when it is a livepatch.

Yeah, I agree that it is unnecessary for a livepatch module without
reloc secs to keep a copy of the load_info struct. My justification
for using MODULE_INFO is that I was trying to be consistent with the
way how other module "characteristics" are checked in the module
loader. For example, if the module came from the staging tree, the
module loader simply checks get_modinfo(info, "staging")). If the
module is a livepatch module, we check get_modinfo(info,
"livepatch")). I also thought that it might be useful additional
information for the user to be able to issue the modinfo command on a
module to see if it's a livepatch module or not (but maybe this
information won't be so useful after all, that's quite subjective).
But if we want to do a more thorough check, we could, like you said,
check for the livepatch-related elf sections before copying load_info.

Thanks,
Jessica
--
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/

[toc] | [prev] | [next] | [standalone]

#1267813 — Re: samples: livepatch: init reloc list and mark as klp module

On Thu, 12 Nov 2015, Jessica Yu wrote:

> +++ Petr Mladek [11/11/15 16:42 +0100]:
> > On Mon 2015-11-09 23:45:54, Jessica Yu wrote:
> > > Intialize the list of relocation sections in the sample
> > > klp_object (even if the list will be empty in this case).
> > > Also mark module as a livepatch module so that the module
> > > loader can appropriately initialize it.
> > > 
> > > Signed-off-by: Jessica Yu <jeyu@redhat.com>
> > > ---
> > >  samples/livepatch/livepatch-sample.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/samples/livepatch/livepatch-sample.c
> > > b/samples/livepatch/livepatch-sample.c
> > > index fb8c861..2ef9345 100644
> > > --- a/samples/livepatch/livepatch-sample.c
> > > +++ b/samples/livepatch/livepatch-sample.c
> > > @@ -89,3 +90,4 @@ static void livepatch_exit(void)
> > >  module_init(livepatch_init);
> > >  module_exit(livepatch_exit);
> > >  MODULE_LICENSE("GPL");
> > > +MODULE_INFO(livepatch, "Y");
> > 
> > This looks a bit error prone. I wonder if we could detect this
> > information another way. For example, by a check for the
> > livepatch-related elf sections. If it is missing,
> > we do not need to preserve struct load_info even
> > when it is a livepatch.
> 
> Yeah, I agree that it is unnecessary for a livepatch module without
> reloc secs to keep a copy of the load_info struct. My justification
> for using MODULE_INFO is that I was trying to be consistent with the
> way how other module "characteristics" are checked in the module
> loader. For example, if the module came from the staging tree, the
> module loader simply checks get_modinfo(info, "staging")). If the
> module is a livepatch module, we check get_modinfo(info,
> "livepatch")). I also thought that it might be useful additional
> information for the user to be able to issue the modinfo command on a
> module to see if it's a livepatch module or not (but maybe this
> information won't be so useful after all, that's quite subjective).

Yup, in my opinion this is a good way to do it. We already impose quite a 
lot on a patch module and this does not make a big difference. Easy 
identification of a patch module is good bonus as well.

> But if we want to do a more thorough check, we could, like you said,
> check for the livepatch-related elf sections before copying load_info.

I wouldn't do that. It could be even more error prone.

I'd like to think that we can live with load_info struct even for patch 
modules which do not use relocations. Don't know.

Miroslav
--
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/

[toc] | [prev] | [next] | [standalone]

#1266229 — [RFC PATCH 5/5] livepatch: x86: remove unused relocation code

Architecture-specific relocation code no longer needed, since symbol
resolution and relocation work will be offloaded to module loader.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 arch/x86/kernel/Makefile    |  1 -
 arch/x86/kernel/livepatch.c | 91 ---------------------------------------------
 2 files changed, 92 deletions(-)
 delete mode 100644 arch/x86/kernel/livepatch.c

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index b1b78ff..c5e9a5c 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -67,7 +67,6 @@ obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
 obj-y				+= apic/
 obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
 obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o
-obj-$(CONFIG_LIVEPATCH)		+= livepatch.o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
 obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.o
 obj-$(CONFIG_X86_TSC)		+= trace_clock.o
diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
deleted file mode 100644
index d1d35cc..0000000
--- a/arch/x86/kernel/livepatch.c
+++ /dev/null
@@ -1,91 +0,0 @@
-/*
- * livepatch.c - x86-specific Kernel Live Patching Core
- *
- * Copyright (C) 2014 Seth Jennings <sjenning@redhat.com>
- * Copyright (C) 2014 SUSE
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version 2
- * of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, see <http://www.gnu.org/licenses/>.
- */
-
-#include <linux/module.h>
-#include <linux/uaccess.h>
-#include <asm/cacheflush.h>
-#include <asm/page_types.h>
-#include <asm/elf.h>
-#include <asm/livepatch.h>
-
-/**
- * klp_write_module_reloc() - write a relocation in a module
- * @mod:	module in which the section to be modified is found
- * @type:	ELF relocation type (see asm/elf.h)
- * @loc:	address that the relocation should be written to
- * @value:	relocation value (sym address + addend)
- *
- * This function writes a relocation to the specified location for
- * a particular module.
- */
-int klp_write_module_reloc(struct module *mod, unsigned long type,
-			   unsigned long loc, unsigned long value)
-{
-	int ret, numpages, size = 4;
-	bool readonly;
-	unsigned long val;
-	unsigned long core = (unsigned long)mod->module_core;
-	unsigned long core_size = mod->core_size;
-
-	switch (type) {
-	case R_X86_64_NONE:
-		return 0;
-	case R_X86_64_64:
-		val = value;
-		size = 8;
-		break;
-	case R_X86_64_32:
-		val = (u32)value;
-		break;
-	case R_X86_64_32S:
-		val = (s32)value;
-		break;
-	case R_X86_64_PC32:
-		val = (u32)(value - loc);
-		break;
-	default:
-		/* unsupported relocation type */
-		return -EINVAL;
-	}
-
-	if (loc < core || loc >= core + core_size)
-		/* loc does not point to any symbol inside the module */
-		return -EINVAL;
-
-	readonly = false;
-
-#ifdef CONFIG_DEBUG_SET_MODULE_RONX
-	if (loc < core + mod->core_ro_size)
-		readonly = true;
-#endif
-
-	/* determine if the relocation spans a page boundary */
-	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
-
-	if (readonly)
-		set_memory_rw(loc & PAGE_MASK, numpages);
-
-	ret = probe_kernel_write((void *)loc, &val, size);
-
-	if (readonly)
-		set_memory_ro(loc & PAGE_MASK, numpages);
-
-	return ret;
-}
-- 
2.4.3

--
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/

[toc] | [prev] | [next] | [standalone]

#1267263 — Re: [RFC PATCH 5/5] livepatch: x86: remove unused relocation code

On Mon 2015-11-09 23:45:55, Jessica Yu wrote:
> Architecture-specific relocation code no longer needed, since symbol
> resolution and relocation work will be offloaded to module loader.
                                 ^^^^
				 has been

I would personally merge this with the 3rd patch. It will be more
clear what code has been replaced there. But it is a matter of taste.

Best Regards,
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/

[toc] | [prev] | [next] | [standalone]

#1268155 — Re: [RFC PATCH 5/5] livepatch: x86: remove unused relocation code

On Wed, Nov 11, 2015 at 04:48:07PM +0100, Petr Mladek wrote:
> On Mon 2015-11-09 23:45:55, Jessica Yu wrote:
> > Architecture-specific relocation code no longer needed, since symbol
> > resolution and relocation work will be offloaded to module loader.
>                                  ^^^^
> 				 has been
> 
> I would personally merge this with the 3rd patch. It will be more
> clear what code has been replaced there. But it is a matter of taste.

Ditto.

-- 
Josh
--
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/

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web