Re: module: save load_info for livepatch modules

On Thu, Nov 12, 2015 at 04:03:45PM +0100, Petr Mladek wrote:

On Thu 2015-11-12 14:22:28, Miroslav Benes wrote:

quoted

On Thu, 12 Nov 2015, Petr Mladek wrote:

quoted

quoted

quoted

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.

-- 
Josh