Re: module: save load_info for livepatch modules

[Jessica Yu <jeyu@redhat.com>]

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

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.