Re: module: save load_info for livepatch modules
From: Miroslav Benes <hidden>
Date: 2015-11-13 12:24:36
Also in:
lkml
On Thu, 12 Nov 2015, Jessica Yu wrote:
quoted hunk ↗ jump to hunk
+++ Josh Poimboeuf [12/11/15 11:05 -0600]:quoted
On Thu, Nov 12, 2015 at 04:03:45PM +0100, Petr Mladek wrote:quoted
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()onquoted
quoted
quoted
quoted
info somewhere in the end?So free_copy() will call vfree(info->hdr), except in livepatchmodulesquoted
quoted
quoted
we want to keep all the elf section information stored there, so we avoid calling free_copy(), As for the info struct itself, if youlookquoted
quoted
quoted
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 isremoved.quoted
This would unfortunately lead to changes in arch-specific code in module.c. For example in arch/s390/kernel/module.c there is vfree callonquoted
part of mod_arch_specific in module_finalize. We would call it only ifthequoted
flag mentioned above is not set and at the same time we would need tocallquoted
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