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 <mbenes@xxxxxxx>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
>  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 <linux/module.h>
>  #include <linux/uaccess.h>
> -#include <asm/cacheflush.h>
> -#include <asm/page_types.h>
>  #include <asm/elf.h>
>  #include <asm/livepatch.h>
>  
> @@ -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 <linux/list.h>
>  #include <linux/kallsyms.h>
>  #include <linux/livepatch.h>
> +#include <asm/cacheflush.h>
>  
>  /**
>   * 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/