Re: module: save load_info for livepatch modules

[Miroslav Benes <mbenes@suse.cz>]

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