From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752688AbbKNCJ4 (ORCPT ); Fri, 13 Nov 2015 21:09:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53102 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752064AbbKNCJy (ORCPT ); Fri, 13 Nov 2015 21:09:54 -0500 Date: Fri, 13 Nov 2015 21:09:50 -0500 From: Jessica Yu To: Miroslav Benes Cc: Josh Poimboeuf , Petr Mladek , 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: <20151114020950.GA21414@packer-debian-8-amd64.digitalocean.com> References: <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> <20151112221750.GA13513@packer-debian-8-amd64.digitalocean.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: 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 +++ Miroslav Benes [13/11/15 13:56 +0100]: >On Fri, 13 Nov 2015, Miroslav Benes wrote: > >> 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. > >Which is not true because module_arch_freeing_init is also called from >do_init_module, called from load_module. So we should move it to >module_arch_cleanup. > >That code is like a maze without Ariadne's thread. Heh, I agree with that sentiment. I am slightly confused about the s390 code, and whether the authors originally intended for that double vfree() to happen in both module_finalize() and module_arch_freeing_init() (called from do_init_module). Seems like a mistake. If module load succeeds, do_init_module calls module_arch_freeing_init(). And if load_module fails halfway through, both module_deallocate() and free_module() will also call module_arch_freeing_init(). I feel like that vfree should only happen once in module_arch_freeing_init() and not in module_finalize(). If we can remove the double vfree() code from module_finalize(), we can copy the mod_arch_specific safely before the call to do_init_module(). Jessica From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jessica Yu Subject: Re: module: save load_info for livepatch modules Date: Fri, 13 Nov 2015 21:09:50 -0500 Message-ID: <20151114020950.GA21414@packer-debian-8-amd64.digitalocean.com> References: <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> <20151112221750.GA13513@packer-debian-8-amd64.digitalocean.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Miroslav Benes Cc: Josh Poimboeuf , Petr Mladek , Rusty Russell , Seth Jennings , Jiri Kosina , Vojtech Pavlik , linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, live-patching-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-api@vger.kernel.org +++ Miroslav Benes [13/11/15 13:56 +0100]: >On Fri, 13 Nov 2015, Miroslav Benes wrote: > >> 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. > >Which is not true because module_arch_freeing_init is also called from >do_init_module, called from load_module. So we should move it to >module_arch_cleanup. > >That code is like a maze without Ariadne's thread. Heh, I agree with that sentiment. I am slightly confused about the s390 code, and whether the authors originally intended for that double vfree() to happen in both module_finalize() and module_arch_freeing_init() (called from do_init_module). Seems like a mistake. If module load succeeds, do_init_module calls module_arch_freeing_init(). And if load_module fails halfway through, both module_deallocate() and free_module() will also call module_arch_freeing_init(). I feel like that vfree should only happen once in module_arch_freeing_init() and not in module_finalize(). If we can remove the double vfree() code from module_finalize(), we can copy the mod_arch_specific safely before the call to do_init_module(). Jessica