Re: [PATCH 4/4] module: clean up RO/NX handling.

[Rusty Russell <rusty@rustcorp.com.au>]

Josh Poimboeuf <jpoimboe@redhat.com> writes:
> On Tue, Nov 10, 2015 at 12:27:34PM +1030, Rusty Russell wrote:
>> Josh Poimboeuf <jpoimboe@redhat.com> writes:
>> > On Mon, Nov 09, 2015 at 02:53:57PM +1030, Rusty Russell wrote:
>> >
>> >> @@ -1858,74 +1849,75 @@ static void mod_sysfs_teardown(struct module *mod)
>> >>  /*
>> >>   * LKM RO/NX protection: protect module's text/ro-data
>> >>   * from modification and any data from execution.
>> >> + *
>> >> + * General layout of module is:
>> >> + *          [text] [read-only-data] [writable data]
>> >> + * text_size -----^                ^               ^
>> >> + * ro_size ------------------------|               |
>> >> + * size -------------------------------------------|
>> >> + *
>> >> + * These values are always page-aligned (as is base)
>> >>   */
>> >> -void set_page_attributes(void *start, void *end, int (*set)(unsigned long start, int num_pages))
>> >> +static void frob_text(const struct module_layout *layout,
>> >> +		      int (*set_memory)(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);
>> >> +	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
>> >> +	BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1));
>> >> +	set_memory((unsigned long)layout->base,
>> >> +		   layout->text_size >> PAGE_SHIFT);
>> >
>> > Should the set_memory() call be skipped if text_size is 0?
>> 
>> Not AFAICT.  And in practice:
>> 1) Every module on my system has a .text section.
>> 2) Every module has a rodata section (.modinfo)
>> 3) Every module on my system has a .data section.
>> 
>> So I think it would be a premature optimization.
>
> However, the frob functions are also used for init sections.
>
> A search on my Fedora system's modules for .init.* sections shows that
> most modules don't have .init.rodata and .init.data, and some modules
> don't even have .init.text.

Good point!  OK, let's do some trivial benchmarking...

diff --git a/kernel/module.c b/kernel/module.c
index 77212128f34a..94ea51a20958 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1906,10 +1906,19 @@ void module_enable_ro(const struct module *mod)
 
 static void module_enable_nx(const struct module *mod)
 {
+	size_t i;
+	ktime_t start, end;
 	frob_rodata(&mod->core_layout, set_memory_nx);
 	frob_writable_data(&mod->core_layout, set_memory_nx);
-	frob_rodata(&mod->init_layout, set_memory_nx);
-	frob_writable_data(&mod->init_layout, set_memory_nx);
+
+	start = ktime_get_boottime();
+	for (i = 0; i < 1000000; i++) {
+		frob_rodata(&mod->init_layout, set_memory_nx);
+		frob_writable_data(&mod->init_layout, set_memory_nx);
+	}
+	end = ktime_get_boottime();
+	printk("%s init time (ns): %lu\n", module_name(mod),
+	       ktime_to_ns(ktime_sub(end, start)));
 }
 
 static void module_disable_nx(const struct module *mod)

[    2.794462] parport init time (ns): 15277714
[    2.855277] lp init time (ns): 15207768
[    2.909701] mac_hid init time (ns): 15409571
[    2.975350] tpm_tis init time (ns): 15118394
[    3.062865] parport_pc init time (ns): 15646948
[    3.247979] virtio_balloon init time (ns): 15555578
[    3.291373] virtio_net init time (ns): 15236362
[    3.391361] serio_raw init time (ns): 15395063

Range & mean: 15118394-15646948(1.53559e+07+/-1.7e+05)

With a zero-check:

[    2.530933] parport init time (ns): 12133350
[    2.587167] lp init time (ns): 12059255
[    2.642342] mac_hid init time (ns): 12849836
[    2.698726] tpm_tis init time (ns): 12008736
[    2.768969] parport_pc init time (ns): 12057191
[    2.943308] virtio_net init time (ns): 12048224
[    2.989983] virtio_balloon init time (ns): 12077151
[    3.061752] serio_raw init time (ns): 12396804

Range & mean: 12008736-12849836(1.22038e+07+/-2.7e+05)

So, we did save 3ns, but it was 12ns even to call them to do nothing.
We'd save more by removing the BUG_ON checks I suspect...

Cheers,
Rusty.

diff --git a/kernel/module.c b/kernel/module.c
index 77212128f34a..9ac8952a0a72 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1863,8 +1863,9 @@ static void frob_text(const struct module_layout *layout,
 {
 	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
 	BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1));
-	set_memory((unsigned long)layout->base,
-		   layout->text_size >> PAGE_SHIFT);
+	if (layout->text_size)
+		set_memory((unsigned long)layout->base,
+			   layout->text_size >> PAGE_SHIFT);
 }
 
 static void frob_rodata(const struct module_layout *layout,
@@ -1873,8 +1874,9 @@ static void frob_rodata(const struct module_layout *layout,
 	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
 	BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1));
 	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
-	set_memory((unsigned long)layout->base + layout->text_size,
-		   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
+	if (layout->ro_size != layout->text_size)
+		set_memory((unsigned long)layout->base + layout->text_size,
+			   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
 }
 
 static void frob_writable_data(const struct module_layout *layout,
@@ -1883,8 +1885,9 @@ static void frob_writable_data(const struct module_layout *layout,
 	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
 	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
 	BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
-	set_memory((unsigned long)layout->base + layout->ro_size,
-		   (layout->size - layout->ro_size) >> PAGE_SHIFT);
+	if (layout->size != layout->ro_size)
+		set_memory((unsigned long)layout->base + layout->ro_size,
+			   (layout->size - layout->ro_size) >> PAGE_SHIFT);
 }
 
 /* livepatching wants to disable read-only so it can frob module. */