Re: module: save load_info for livepatch modules

From	Miroslav Benes <mbenes@suse.cz>
Newsgroups	linux.kernel
Subject	Re: module: save load_info for livepatch modules
Date	2015-11-13 13:30 +0100
Message-ID	<qulEl-6kY-3@gated-at.bofh.it> (permalink)
References	(4 earlier) <qtXiH-7Fc-31@gated-at.bofh.it> <qu06R-ZA-7@gated-at.bofh.it> <qu1FE-240-19@gated-at.bofh.it> <qu3xL-3gW-21@gated-at.bofh.it> <qu8nM-6oo-25@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On Thu, 12 Nov 2015, Jessica Yu wrote:

> +++ Josh Poimboeuf [12/11/15 11:05 -0600]:
> > On Thu, Nov 12, 2015 at 04:03:45PM +0100, Petr Mladek wrote:
> > > On Thu 2015-11-12 14:22:28, Miroslav Benes wrote:
> > > > On Thu, 12 Nov 2015, Petr Mladek wrote:
> > > > > > >Maybe I am missing something but isn't it necessary to call vfree()
> > > on
> > > > > > >info somewhere in the end?
> > > > > >
> > > > > > So free_copy() will call vfree(info->hdr), except in livepatch
> > > modules
> > > > > > we want to keep all the elf section information stored there, so we
> > > > > > avoid calling free_copy(), As for the info struct itself, if you
> > > look
> > > > > > at the init_module and finit_module syscall definitions in
> > > > > > kernel/module.c, you will see that info is actually a local function
> > > > > > variable, simply passed in to the call to load_module(), and will be
> > > > > > automatically deallocated when the syscall returns. :-) No need to
> > > > > > explicitly free info.
> > > > >
> > > > > We still have to free the copied or preserved structures when
> > > > > the module is unloaded.
> > > >
> > > > ...freeing what we allocated. We need to free info->hdr somewhere if not
> > > > here and also mod_arch_specific struct where the patch module is
> > > removed.
> > > > This would unfortunately lead to changes in arch-specific code in
> > > > module.c. For example in arch/s390/kernel/module.c there is vfree call
> > > on
> > > > part of mod_arch_specific in module_finalize. We would call it only if
> > > the
> > > > flag mentioned above is not set and at the same time we would need to
> > > call
> > > > it when the patch module is being removed.
> > > 
> > > Sigh, I am afraid that the flag is not enough. IMHO, we need to split
> > > the load finalizing functions into two pieces. One will be always
> > > called when the module load is finalized. The other part will free
> > > the load_info. It will be called either when the load is finalized or
> > > when the module is unloaded, depending on if we want to preserve
> > > the load_info.
> > > 
> > > Sigh, it is getting complicated. But let's see how it looks in reality.
> > 
> > At the other end of the spectrum, we could do the simplest thing
> > possible: _always_ save the data (even if CONFIG_LIVEPATCH is disabled).
> > 
> > (gdb) print sizeof(*info)
> > $3 = 96
> > (gdb) p sizeof(*info->hdr)
> > $4 = 64
> > s390 mod_arch_syminfo struct: 24 bytes by my reckoning.
> > 
> > So between info, info->hdr, and s390 mod_arch_syminfo, we're talking
> > about 184 bytes on s390 and 160 bytes on x86_64.  That seems like
> > peanuts compared to the size of a typical module.  The benefit is that
> > the code would be simpler because we don't have any special cases and
> > the structs would automatically get freed with the module struct when
> > the module gets unloaded.

Agreed. mod_arch_specific contains more things on certain architectures, 
but compared to the size of a module it is still not much.

> 
> I think I agree with Josh on this one (except, I would always save
> load_info if it is a livepatch module, instead of for every module in the
> !CONFIG_LIVEPATCH case, and we can just check modinfo to see if it is
> a livepatch module).
> 
> If the tradeoff here is between simplicity and readibility of code vs.
> saving some extra space (and by the looks of it, not a lot), I think I
> would choose having clear code over saving some bytes of memory. Hard
> coding checks and edge cases imo might cause confusion and trouble
> down the road.

I agree this seems like the best approach. So if we preserve 
mod_arch_syminfo (in case of s390) we should free it not in 
module_finalize, but somewhere in free_module... where 
module_arch_cleanup() is called... and also module_arch_freeing_init() is 
called there too. And what you find there for s390 is 

	vfree(mod->arch.syminfo);
	mod->arch.syminfo = NULL;

Well, it does nothing here, because mod->arch.syminfo is already NULL. It 
was freed in module_finalize. So we can even remove this code from 
module_finalize and all should be fine. At least for s390.

As for load_info, I don't have a strong opinion whether to keep it for all 
modules or for livepatch modules only.

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/

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