Re: [PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf]

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.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Jiri Kosina]

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

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Jiri Kosina]

On Wed, 4 Nov 2015, Josh Poimboeuf wrote:

> > >  int klp_write_module_reloc(struct module *mod, unsigned long type,
> > >  unsigned long loc, unsigned long value)
> > >  {
> > > - int ret, numpages, size = 4;
> > > - bool readonly;
> > > + int size = 4;
> > 
> > BTW I don't see a reason to have 'size' signed here.
> 
> It was already signed to begin with, but I can change it to size_t.

Yes, I know, it's not really related to this patchset, but I stumbled upon 
it during review.

> > > +#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.

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Jiri Kosina]

On Wed, 4 Nov 2015, Josh Poimboeuf wrote:

> > >  int klp_write_module_reloc(struct module *mod, unsigned long type,
> > >  unsigned long loc, unsigned long value)
> > >  {
> > > - int ret, numpages, size = 4;
> > > - bool readonly;
> > > + int size = 4;
> > 
> > BTW I don't see a reason to have 'size' signed here.
> 
> It was already signed to begin with, but I can change it to size_t.

Yes, I know, it's not really related to this patchset, but I stumbled upon 
it during review.

> > > +#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.

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Jiri Kosina]

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

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf]

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.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf]

On Wed, Nov 04, 2015 at 11:56:13PM +0100, Jiri Kosina wrote:
> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> 
> > Subject: [PATCH] livepatch: Cleanup page permission changes
> > 
> > Calling set_memory_rw() and set_memory_ro() for every iteration of the
> > loop in klp_write_object_relocations() is messy and inefficient.  Change
> > all the RO pages to RW before the loop and convert them back to RO after
> > the loop.
> 
> Generally speaking, I like the patch and would like to have this in 4.4 
> still (if worse becomes worst and we don't make it in time for merge 
> window, this still qualifies for -rc bugfix).
> 
> > Suggested-by: Miroslav Benes 
> > Signed-off-by: Josh Poimboeuf 
> > ---
> >  arch/x86/kernel/livepatch.c | 25 ++---
> >  kernel/livepatch/core.c | 42 +-
> >  2 files changed, 39 insertions(+), 28 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> > index d1d35cc..1062eff 100644
> > --- a/arch/x86/kernel/livepatch.c
> > +++ b/arch/x86/kernel/livepatch.c
> > @@ -20,8 +20,6 @@
> >  
> >  #include 
> >  #include 
> > -#include 
> > -#include 
> >  #include 
> >  #include 
> >  
> > @@ -38,8 +36,7 @@
> >  int klp_write_module_reloc(struct module *mod, unsigned long type,
> >unsigned long loc, unsigned long value)
> >  {
> > -   int ret, numpages, size = 4;
> > -   bool readonly;
> > +   int size = 4;
> 
> BTW I don't see a reason to have 'size' signed here.

It was already signed to begin with, but I can change it to size_t.

> [ ... snip ... [
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -28,6 +28,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  /**
> >   * struct klp_ops - structure for tracking registered ftrace ops structs
> > @@ -131,6 +132,33 @@ static bool klp_initialized(void)
> > return !!klp_root_kobj;
> >  }
> >  
> > +#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);
> > +}
> > +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?

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Jiri Kosina]

On Tue, 3 Nov 2015, Josh Poimboeuf wrote:

> Subject: [PATCH] livepatch: Cleanup page permission changes
> 
> Calling set_memory_rw() and set_memory_ro() for every iteration of the
> loop in klp_write_object_relocations() is messy and inefficient.  Change
> all the RO pages to RW before the loop and convert them back to RO after
> the loop.

Generally speaking, I like the patch and would like to have this in 4.4 
still (if worse becomes worst and we don't make it in time for merge 
window, this still qualifies for -rc bugfix).

> Suggested-by: Miroslav Benes 
> Signed-off-by: Josh Poimboeuf 
> ---
>  arch/x86/kernel/livepatch.c | 25 ++---
>  kernel/livepatch/core.c | 42 +-
>  2 files changed, 39 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> index d1d35cc..1062eff 100644
> --- a/arch/x86/kernel/livepatch.c
> +++ b/arch/x86/kernel/livepatch.c
> @@ -20,8 +20,6 @@
>  
>  #include 
>  #include 
> -#include 
> -#include 
>  #include 
>  #include 
>  
> @@ -38,8 +36,7 @@
>  int klp_write_module_reloc(struct module *mod, unsigned long type,
>  unsigned long loc, unsigned long value)
>  {
> - int ret, numpages, size = 4;
> - bool readonly;
> + int size = 4;

BTW I don't see a reason to have 'size' signed here.

[ ... snip ... [
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /**
>   * struct klp_ops - structure for tracking registered ftrace ops structs
> @@ -131,6 +132,33 @@ static bool klp_initialized(void)
>   return !!klp_root_kobj;
>  }
>  
> +#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);
> +}
> +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.

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf]

On Wed, Nov 04, 2015 at 10:10:06AM -0600, Josh Poimboeuf wrote:
> On Wed, Nov 04, 2015 at 10:18:29AM +0100, Jiri Kosina wrote:
> > On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> > 
> > > It's probably a good idea to keep the patches bisectable, so I made this
> > > a separate patch which applies on top of the first one.
> > > 
> > > (Note that it completely removes all the code from the first patch, so
> > > there's no need for a v2 of the first patch which would have had
> > > Miroslav's suggested style changes.)
> > 
> > I like this patch and it's something I'd like to queue for 4.4, but given 
> > the fact that the original "[PATCH] x86/livepatch: Fix crash with 
> > !CONFIG_DEBUG_SET_MODULE_RONX" wasn't queued in any tree yet, I don't 
> > think that sending it as a followup patch is of any use.
> > 
> > So if you agree, I'll just fold the two patches together and use the 
> > changelog below, and queue it for merge with Linus.
> 
> I kept them separate because the first patch is a low risk bug fix, and
> the second is a slightly higher risk cleanup.
> 
> Would it make sense to put the first one into 4.4 and queue the second
> one for 4.5?

BTW, the second one is still pretty low risk, so it's fine with me if
you decide you still want to squash them for 4.4.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf]

On Wed, Nov 04, 2015 at 10:18:29AM +0100, Jiri Kosina wrote:
> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> 
> > It's probably a good idea to keep the patches bisectable, so I made this
> > a separate patch which applies on top of the first one.
> > 
> > (Note that it completely removes all the code from the first patch, so
> > there's no need for a v2 of the first patch which would have had
> > Miroslav's suggested style changes.)
> 
> I like this patch and it's something I'd like to queue for 4.4, but given 
> the fact that the original "[PATCH] x86/livepatch: Fix crash with 
> !CONFIG_DEBUG_SET_MODULE_RONX" wasn't queued in any tree yet, I don't 
> think that sending it as a followup patch is of any use.
> 
> So if you agree, I'll just fold the two patches together and use the 
> changelog below, and queue it for merge with Linus.

I kept them separate because the first patch is a low risk bug fix, and
the second is a slightly higher risk cleanup.

Would it make sense to put the first one into 4.4 and queue the second
one for 4.5?

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Miroslav Benes]

On Tue, 3 Nov 2015, Josh Poimboeuf wrote:

> It's probably a good idea to keep the patches bisectable, so I made this
> a separate patch which applies on top of the first one.
> 
> (Note that it completely removes all the code from the first patch, so
> there's no need for a v2 of the first patch which would have had
> Miroslav's suggested style changes.)

I'd go along with Jiri and fold this patch to the first one.

> ---8<---
> 
> Subject: [PATCH] livepatch: Cleanup page permission changes
> 
> Calling set_memory_rw() and set_memory_ro() for every iteration of the
> loop in klp_write_object_relocations() is messy and inefficient.  Change
> all the RO pages to RW before the loop and convert them back to RO after
> the loop.
> 
> Suggested-by: Miroslav Benes 
> Signed-off-by: Josh Poimboeuf 

Great, I like the change.

Reviewed-by: Miroslav Benes 

Cheers,
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Jiri Kosina]

On Tue, 3 Nov 2015, Josh Poimboeuf wrote:

> It's probably a good idea to keep the patches bisectable, so I made this
> a separate patch which applies on top of the first one.
> 
> (Note that it completely removes all the code from the first patch, so
> there's no need for a v2 of the first patch which would have had
> Miroslav's suggested style changes.)

I like this patch and it's something I'd like to queue for 4.4, but given 
the fact that the original "[PATCH] x86/livepatch: Fix crash with 
!CONFIG_DEBUG_SET_MODULE_RONX" wasn't queued in any tree yet, I don't 
think that sending it as a followup patch is of any use.

So if you agree, I'll just fold the two patches together and use the 
changelog below, and queue it for merge with Linus.

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Jiri Kosina]

On Tue, 3 Nov 2015, Josh Poimboeuf wrote:

> Subject: [PATCH] livepatch: Cleanup page permission changes
> 
> Calling set_memory_rw() and set_memory_ro() for every iteration of the
> loop in klp_write_object_relocations() is messy and inefficient.  Change
> all the RO pages to RW before the loop and convert them back to RO after
> the loop.

Generally speaking, I like the patch and would like to have this in 4.4 
still (if worse becomes worst and we don't make it in time for merge 
window, this still qualifies for -rc bugfix).

> Suggested-by: Miroslav Benes 
> Signed-off-by: Josh Poimboeuf 
> ---
>  arch/x86/kernel/livepatch.c | 25 ++---
>  kernel/livepatch/core.c | 42 +-
>  2 files changed, 39 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> index d1d35cc..1062eff 100644
> --- a/arch/x86/kernel/livepatch.c
> +++ b/arch/x86/kernel/livepatch.c
> @@ -20,8 +20,6 @@
>  
>  #include 
>  #include 
> -#include 
> -#include 
>  #include 
>  #include 
>  
> @@ -38,8 +36,7 @@
>  int klp_write_module_reloc(struct module *mod, unsigned long type,
>  unsigned long loc, unsigned long value)
>  {
> - int ret, numpages, size = 4;
> - bool readonly;
> + int size = 4;

BTW I don't see a reason to have 'size' signed here.

[ ... snip ... [
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /**
>   * struct klp_ops - structure for tracking registered ftrace ops structs
> @@ -131,6 +132,33 @@ static bool klp_initialized(void)
>   return !!klp_root_kobj;
>  }
>  
> +#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);
> +}
> +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.

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf]

On Wed, Nov 04, 2015 at 11:56:13PM +0100, Jiri Kosina wrote:
> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> 
> > Subject: [PATCH] livepatch: Cleanup page permission changes
> > 
> > Calling set_memory_rw() and set_memory_ro() for every iteration of the
> > loop in klp_write_object_relocations() is messy and inefficient.  Change
> > all the RO pages to RW before the loop and convert them back to RO after
> > the loop.
> 
> Generally speaking, I like the patch and would like to have this in 4.4 
> still (if worse becomes worst and we don't make it in time for merge 
> window, this still qualifies for -rc bugfix).
> 
> > Suggested-by: Miroslav Benes 
> > Signed-off-by: Josh Poimboeuf 
> > ---
> >  arch/x86/kernel/livepatch.c | 25 ++---
> >  kernel/livepatch/core.c | 42 +-
> >  2 files changed, 39 insertions(+), 28 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> > index d1d35cc..1062eff 100644
> > --- a/arch/x86/kernel/livepatch.c
> > +++ b/arch/x86/kernel/livepatch.c
> > @@ -20,8 +20,6 @@
> >  
> >  #include 
> >  #include 
> > -#include 
> > -#include 
> >  #include 
> >  #include 
> >  
> > @@ -38,8 +36,7 @@
> >  int klp_write_module_reloc(struct module *mod, unsigned long type,
> >unsigned long loc, unsigned long value)
> >  {
> > -   int ret, numpages, size = 4;
> > -   bool readonly;
> > +   int size = 4;
> 
> BTW I don't see a reason to have 'size' signed here.

It was already signed to begin with, but I can change it to size_t.

> [ ... snip ... [
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -28,6 +28,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  /**
> >   * struct klp_ops - structure for tracking registered ftrace ops structs
> > @@ -131,6 +132,33 @@ static bool klp_initialized(void)
> > return !!klp_root_kobj;
> >  }
> >  
> > +#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);
> > +}
> > +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?

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf]

On Wed, Nov 04, 2015 at 10:10:06AM -0600, Josh Poimboeuf wrote:
> On Wed, Nov 04, 2015 at 10:18:29AM +0100, Jiri Kosina wrote:
> > On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> > 
> > > It's probably a good idea to keep the patches bisectable, so I made this
> > > a separate patch which applies on top of the first one.
> > > 
> > > (Note that it completely removes all the code from the first patch, so
> > > there's no need for a v2 of the first patch which would have had
> > > Miroslav's suggested style changes.)
> > 
> > I like this patch and it's something I'd like to queue for 4.4, but given 
> > the fact that the original "[PATCH] x86/livepatch: Fix crash with 
> > !CONFIG_DEBUG_SET_MODULE_RONX" wasn't queued in any tree yet, I don't 
> > think that sending it as a followup patch is of any use.
> > 
> > So if you agree, I'll just fold the two patches together and use the 
> > changelog below, and queue it for merge with Linus.
> 
> I kept them separate because the first patch is a low risk bug fix, and
> the second is a slightly higher risk cleanup.
> 
> Would it make sense to put the first one into 4.4 and queue the second
> one for 4.5?

BTW, the second one is still pretty low risk, so it's fine with me if
you decide you still want to squash them for 4.4.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf]

On Wed, Nov 04, 2015 at 10:18:29AM +0100, Jiri Kosina wrote:
> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> 
> > It's probably a good idea to keep the patches bisectable, so I made this
> > a separate patch which applies on top of the first one.
> > 
> > (Note that it completely removes all the code from the first patch, so
> > there's no need for a v2 of the first patch which would have had
> > Miroslav's suggested style changes.)
> 
> I like this patch and it's something I'd like to queue for 4.4, but given 
> the fact that the original "[PATCH] x86/livepatch: Fix crash with 
> !CONFIG_DEBUG_SET_MODULE_RONX" wasn't queued in any tree yet, I don't 
> think that sending it as a followup patch is of any use.
> 
> So if you agree, I'll just fold the two patches together and use the 
> changelog below, and queue it for merge with Linus.

I kept them separate because the first patch is a low risk bug fix, and
the second is a slightly higher risk cleanup.

Would it make sense to put the first one into 4.4 and queue the second
one for 4.5?

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Jiri Kosina]

On Tue, 3 Nov 2015, Josh Poimboeuf wrote:

> It's probably a good idea to keep the patches bisectable, so I made this
> a separate patch which applies on top of the first one.
> 
> (Note that it completely removes all the code from the first patch, so
> there's no need for a v2 of the first patch which would have had
> Miroslav's suggested style changes.)

I like this patch and it's something I'd like to queue for 4.4, but given 
the fact that the original "[PATCH] x86/livepatch: Fix crash with 
!CONFIG_DEBUG_SET_MODULE_RONX" wasn't queued in any tree yet, I don't 
think that sending it as a followup patch is of any use.

So if you agree, I'll just fold the two patches together and use the 
changelog below, and queue it for merge with Linus.

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] livepatch: Cleanup page permission changes

[Miroslav Benes]

On Tue, 3 Nov 2015, Josh Poimboeuf wrote:

> It's probably a good idea to keep the patches bisectable, so I made this
> a separate patch which applies on top of the first one.
> 
> (Note that it completely removes all the code from the first patch, so
> there's no need for a v2 of the first patch which would have had
> Miroslav's suggested style changes.)

I'd go along with Jiri and fold this patch to the first one.

> ---8<---
> 
> Subject: [PATCH] livepatch: Cleanup page permission changes
> 
> Calling set_memory_rw() and set_memory_ro() for every iteration of the
> loop in klp_write_object_relocations() is messy and inefficient.  Change
> all the RO pages to RW before the loop and convert them back to RO after
> the loop.
> 
> Suggested-by: Miroslav Benes 
> Signed-off-by: Josh Poimboeuf 

Great, I like the change.

Reviewed-by: Miroslav Benes 

Cheers,
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/