by Rusty Russell

[permalink] [raw]

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

Josh Poimboeuf <[email protected]> writes:
> On Tue, Nov 10, 2015 at 12:27:34PM +1030, Rusty Russell wrote:
>> Josh Poimboeuf <[email protected]> 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. */

2015-11-12 03:41:43

by Josh Poimboeuf

[permalink] [raw]

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

On Thu, Nov 12, 2015 at 11:58:25AM +1030, Rusty Russell wrote:
> Josh Poimboeuf <[email protected]> writes:
> > On Tue, Nov 10, 2015 at 12:27:34PM +1030, Rusty Russell wrote:
> >> Josh Poimboeuf <[email protected]> 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...

Ok, leaving the checks out seems harmless from a performance standpoint.

I was also curious about whether anything unexpected could potentially
happen in change_page_attr_set_clr() for numpages == 0. But I guess it
seems to be fine (though I'm no mm expert).

I didn't quite get whether you decided to fold in the below patch. But
either way:

Reviewed-by: Josh Poimboeuf <[email protected]>

Thanks!

> 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. */

--
Josh