Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932141AbbKLWR5 (ORCPT ); Thu, 12 Nov 2015 17:17:57 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55469 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753877AbbKLWRz (ORCPT ); Thu, 12 Nov 2015 17:17:55 -0500 Date: Thu, 12 Nov 2015 17:17:51 -0500 From: Jessica Yu To: Josh Poimboeuf Cc: Petr Mladek , Miroslav Benes , Rusty Russell , Seth Jennings , Jiri Kosina , Vojtech Pavlik , linux-api@vger.kernel.org, live-patching@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: module: save load_info for livepatch modules Message-ID: <20151112221750.GA13513@packer-debian-8-amd64.digitalocean.com> References: <1447130755-17383-1-git-send-email-jeyu@redhat.com> <1447130755-17383-3-git-send-email-jeyu@redhat.com> <20151112053228.GD30025@packer-debian-8-amd64.digitalocean.com> <20151112102404.GL4431@pathway.suse.cz> <20151112150345.GT2599@pathway.suse.cz> <20151112170501.GD4038@treble.hsd1.ky.comcast.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20151112170501.GD4038@treble.hsd1.ky.comcast.net> X-OS: Linux eisen.io 3.16.0-4-amd64 x86_64 User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3404 Lines: 68 +++ 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. -- 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/