Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753059AbbKLDln (ORCPT ); Wed, 11 Nov 2015 22:41:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42612 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752818AbbKLDlm (ORCPT ); Wed, 11 Nov 2015 22:41:42 -0500 Date: Wed, 11 Nov 2015 21:41:40 -0600 From: Josh Poimboeuf To: Rusty Russell Cc: linux-kernel@vger.kernel.org, Peter Zijlstra Subject: Re: [PATCH 4/4] module: clean up RO/NX handling. Message-ID: <20151112034140.GA4038@treble.hsd1.ky.comcast.net> References: <1447043037-10833-1-git-send-email-rusty@rustcorp.com.au> <1447043037-10833-5-git-send-email-rusty@rustcorp.com.au> <20151109195153.GB3914@treble.redhat.com> <87611ak44x.fsf@rustcorp.com.au> <20151110042736.GA18117@treble.hsd1.ky.comcast.net> <87bnb0vwee.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <87bnb0vwee.fsf@rustcorp.com.au> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6897 Lines: 169 On Thu, Nov 12, 2015 at 11:58:25AM +1030, Rusty Russell wrote: > Josh Poimboeuf writes: > > On Tue, Nov 10, 2015 at 12:27:34PM +1030, Rusty Russell wrote: > >> Josh Poimboeuf 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/