From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932935AbbKERdH (ORCPT ); Thu, 5 Nov 2015 12:33:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39853 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756289AbbKERdF (ORCPT ); Thu, 5 Nov 2015 12:33:05 -0500 Date: Thu, 5 Nov 2015 11:33:04 -0600 From: Josh Poimboeuf To: Jiri Kosina Cc: Miroslav Benes , Seth Jennings , Vojtech Pavlik , linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, "Cyril B." Subject: Re: [PATCH] livepatch: Cleanup page permission changes Message-ID: <20151105173304.GE28254@treble.redhat.com> References: <132b018cfe2c6e4cef4d1b62ac3ed70333734111.1446494413.git.jpoimboe@redhat.com> <20151103174228.GN27488@treble.redhat.com> <20151104231252.GA28254@treble.redhat.com> <20151105151759.GC28254@treble.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20151105151759.GC28254@treble.redhat.com> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 05, 2015 at 09:17:59AM -0600, Josh Poimboeuf wrote: > On Thu, Nov 05, 2015 at 10:40:26AM +0100, Jiri Kosina wrote: > > On Thu, 5 Nov 2015, Jiri Kosina wrote: > > > > > > > > +#ifdef CONFIG_DEBUG_SET_MODULE_RONX > > > > > > +static void set_page_attributes(void *start, void *end, > > > > > > + int (*set)(unsigned long start, int num_pages)) > > > > > > +{ > > > > > > + unsigned long begin_pfn = PFN_DOWN((unsigned long)start); > > > > > > + unsigned long end_pfn = PFN_DOWN((unsigned long)end); > > > > > > + > > > > > > + if (end_pfn > begin_pfn) > > > > > > + set(begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn); > > > > > > +} > > > > > > BTW is there any reason not to make use of the function from module.c > > > which does exactly the same, instead of copy pasting it all around? > > > > > > > > > +static void set_module_ro_rw(struct module *mod) > > > > > > +{ > > > > > > + set_page_attributes(mod->module_core, > > > > > > + mod->module_core + mod->core_ro_size, > > > > > > + set_memory_rw); > > > > > > +} > > > > > > +static void set_module_ro_ro(struct module *mod) > > > > > > > > > > Honestly, I find both the function names above horrible and not really > > > > > self-explanatory (especially the _ro_ro variant). At least comment, > > > > > explaining what they are actually doing, or picking up a better name, > > > > > would make the code much more self-explanatory in my eyes. > > > > > > > > Being the patch author, naturally the function names make sense to me. > > > > > > :) > > > > > > > set_module_ro_ro() means "set the module's read-only area to have > > > > read-only permissions." > > > > > > > > Do you have any suggestions for a better name? > > > > > > I'd even say it's superfluous to have the functions at the first place, > > > and just calling set_page_attributes() directly makes the intent clear > > > enough already. > > > > To make my proposal more clear: > > > > - move set_page_attributes() to module.h and provide empty stub for > > !CONFIG_DEBUG_SET_MODULE_RONX case (and probably rename it to something > > like module_set_page_attributes() to avoid namespace conflicts with mm > > code) > > > > - make use of that function both from module.c (where it's already being > > used) and livepatch.c, where it'd be called directly > > Ok, I'll use the module.c version of set_page_attributes() and get rid > of the set_module_ro_(ro|rw) functions. > > I'd rather keep set_page_attributes() named as it already is, because > there's nothing module-specific about it. It just happens to currently > live in module.c. Looking at it more closely, I think there's some additional module.c cleanup that should be done first. So it'll probably end up being at least two patches. Since the cleanup is growing and now affects multiple components, shall we go ahead and merge the original bug fix patch ("Fix crash with !CONFIG_DEBUG_SET_MODULE_RONX") separately for the merge window? -- Josh