Re: [PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf <jpoimboe@redhat.com>]

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