[PATCH] x86/livepatch: Fix crash with !CONFIG_DEBUG_SET_MODULE_RONX

[PATCH] x86/livepatch: Fix crash with !CONFIG_DEBUG_SET_MODULE_RONX

[Josh Poimboeuf]

When loading a patch module on a kernel with
!CONFIG_DEBUG_SET_MODULE_RONX, the following crash occurs:

  [  205.988776] livepatch: enabling patch 'kpatch_meminfo_string'
  [  205.989829] BUG: unable to handle kernel paging request at ffffffffa08d2fc0
  [  205.989863] IP: [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
  [  205.989888] PGD 1a10067 PUD 1a11063 PMD 7bcde067 PTE 3740e161
  [  205.989915] Oops: 0003 [#1] SMP
  [  205.990187] CPU: 2 PID: 14570 Comm: insmod Tainted: G           O  K 4.1.12
  [  205.990214] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
  [  205.990249] task: ffff8800374aaa90 ti: ffff8800794b8000 task.ti: ffff8800794b8000
  [  205.990276] RIP: 0010:[<ffffffff8154fecb>]  [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
  [  205.990307] RSP: 0018:ffff8800794bbd58  EFLAGS: 00010246
  [  205.990327] RAX: 0000000000000000 RBX: ffffffffa08d2fc0 RCX: 0000000000000000
  [  205.990356] RDX: 01ffff8000000080 RSI: 0000000000000000 RDI: ffffffff81a54b40
  [  205.990382] RBP: ffff88007b4c4d80 R08: 0000000000000007 R09: 0000000000000000
  [  205.990408] R10: 0000000000000008 R11: ffffea0001f18840 R12: 0000000000000000
  [  205.990433] R13: 0000000000000001 R14: ffffffffa08d2fc0 R15: ffff88007bd0bc40
  [  205.990459] FS:  00007f1128fbc700(0000) GS:ffff88007fc80000(0000) knlGS:0000000000000000
  [  205.990488] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [  205.990509] CR2: ffffffffa08d2fc0 CR3: 000000002606e000 CR4: 00000000001406e0
  [  205.990536] Stack:
  [  205.990545]  ffff8800794bbec8 0000000000000001 ffffffffa08d3010 ffffffff810ecea9
  [  205.990576]  ffffffff810e8e40 000000000005f360 ffff88007bd0bc50 ffffffffa08d3240
  [  205.990608]  ffffffffa08d52c0 ffffffffa08d3210 ffff8800794bbed8 ffff8800794bbf1c
  [  205.990639] Call Trace:
  [  205.990651]  [<ffffffff810ecea9>] ? load_module+0x1e59/0x23a0
  [  205.990672]  [<ffffffff810e8e40>] ? store_uevent+0x40/0x40
  [  205.990693]  [<ffffffff810e99b5>] ? copy_module_from_fd.isra.49+0xb5/0x140
  [  205.990718]  [<ffffffff810ed5bd>] ? SyS_finit_module+0x7d/0xa0
  [  205.990741]  [<ffffffff81556832>] ? system_call_fastpath+0x16/0x75
  [  205.990763] Code: f9 00 00 00 74 23 49 c7 c0 92 e1 60 81 48 8d 53 18 89 c1 4c 89 c6 48 c7 c7 f0 85 7d 81 31 c0 e8 71 fa ff ff e8 58 0e 00 00 31 f6 <c7> 03 00 00 00 00 48 89 da 48 c7 c7 20 c7 a5 81 e8 d0 ec b3 ff
  [  205.990916] RIP  [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
  [  205.990940]  RSP <ffff8800794bbd58>
  [  205.990953] CR2: ffffffffa08d2fc0

With !CONFIG_DEBUG_SET_MODULE_RONX, module text and rodata pages are
writable, and the debug_align() macro allows the module struct to share
a page with executable text.  When klp_write_module_reloc() calls
set_memory_ro() on the page, it effectively turns the module struct into
a read-only structure, resulting in a page fault when load_module() does
"mod->state = MODULE_STATE_LIVE".

Reported-by: Cyril B. <cbay@alwaysdata.com>
Tested-by: Cyril B. <cbay@alwaysdata.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/livepatch.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
index ff3c3101d..d1d35cc 100644
--- a/arch/x86/kernel/livepatch.c
+++ b/arch/x86/kernel/livepatch.c
@@ -42,7 +42,6 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
 	bool readonly;
 	unsigned long val;
 	unsigned long core = (unsigned long)mod->module_core;
-	unsigned long core_ro_size = mod->core_ro_size;
 	unsigned long core_size = mod->core_size;
 
 	switch (type) {
@@ -70,10 +69,12 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
 		/* loc does not point to any symbol inside the module */
 		return -EINVAL;
 
-	if (loc < core + core_ro_size)
+	readonly = false;
+
+#ifdef CONFIG_DEBUG_SET_MODULE_RONX
+	if (loc < core + mod->core_ro_size)
 		readonly = true;
-	else
-		readonly = false;
+#endif
 
 	/* determine if the relocation spans a page boundary */
 	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
-- 
2.4.3

Re: [PATCH] x86/livepatch: Fix crash with !CONFIG_DEBUG_SET_MODULE_RONX

[Miroslav Benes]

On Mon, 2 Nov 2015, Josh Poimboeuf wrote:

> When loading a patch module on a kernel with
> !CONFIG_DEBUG_SET_MODULE_RONX, the following crash occurs:
> 
>   [  205.988776] livepatch: enabling patch 'kpatch_meminfo_string'
>   [  205.989829] BUG: unable to handle kernel paging request at ffffffffa08d2fc0
>   [  205.989863] IP: [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
>   [  205.989888] PGD 1a10067 PUD 1a11063 PMD 7bcde067 PTE 3740e161
>   [  205.989915] Oops: 0003 [#1] SMP
>   [  205.990187] CPU: 2 PID: 14570 Comm: insmod Tainted: G           O  K 4.1.12
>   [  205.990214] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
>   [  205.990249] task: ffff8800374aaa90 ti: ffff8800794b8000 task.ti: ffff8800794b8000
>   [  205.990276] RIP: 0010:[<ffffffff8154fecb>]  [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
>   [  205.990307] RSP: 0018:ffff8800794bbd58  EFLAGS: 00010246
>   [  205.990327] RAX: 0000000000000000 RBX: ffffffffa08d2fc0 RCX: 0000000000000000
>   [  205.990356] RDX: 01ffff8000000080 RSI: 0000000000000000 RDI: ffffffff81a54b40
>   [  205.990382] RBP: ffff88007b4c4d80 R08: 0000000000000007 R09: 0000000000000000
>   [  205.990408] R10: 0000000000000008 R11: ffffea0001f18840 R12: 0000000000000000
>   [  205.990433] R13: 0000000000000001 R14: ffffffffa08d2fc0 R15: ffff88007bd0bc40
>   [  205.990459] FS:  00007f1128fbc700(0000) GS:ffff88007fc80000(0000) knlGS:0000000000000000
>   [  205.990488] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   [  205.990509] CR2: ffffffffa08d2fc0 CR3: 000000002606e000 CR4: 00000000001406e0
>   [  205.990536] Stack:
>   [  205.990545]  ffff8800794bbec8 0000000000000001 ffffffffa08d3010 ffffffff810ecea9
>   [  205.990576]  ffffffff810e8e40 000000000005f360 ffff88007bd0bc50 ffffffffa08d3240
>   [  205.990608]  ffffffffa08d52c0 ffffffffa08d3210 ffff8800794bbed8 ffff8800794bbf1c
>   [  205.990639] Call Trace:
>   [  205.990651]  [<ffffffff810ecea9>] ? load_module+0x1e59/0x23a0
>   [  205.990672]  [<ffffffff810e8e40>] ? store_uevent+0x40/0x40
>   [  205.990693]  [<ffffffff810e99b5>] ? copy_module_from_fd.isra.49+0xb5/0x140
>   [  205.990718]  [<ffffffff810ed5bd>] ? SyS_finit_module+0x7d/0xa0
>   [  205.990741]  [<ffffffff81556832>] ? system_call_fastpath+0x16/0x75
>   [  205.990763] Code: f9 00 00 00 74 23 49 c7 c0 92 e1 60 81 48 8d 53 18 89 c1 4c 89 c6 48 c7 c7 f0 85 7d 81 31 c0 e8 71 fa ff ff e8 58 0e 00 00 31 f6 <c7> 03 00 00 00 00 48 89 da 48 c7 c7 20 c7 a5 81 e8 d0 ec b3 ff
>   [  205.990916] RIP  [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
>   [  205.990940]  RSP <ffff8800794bbd58>
>   [  205.990953] CR2: ffffffffa08d2fc0
> 
> With !CONFIG_DEBUG_SET_MODULE_RONX, module text and rodata pages are
> writable, and the debug_align() macro allows the module struct to share
> a page with executable text.  When klp_write_module_reloc() calls
> set_memory_ro() on the page, it effectively turns the module struct into
> a read-only structure, resulting in a page fault when load_module() does
> "mod->state = MODULE_STATE_LIVE".
> 
> Reported-by: Cyril B. <cbay@alwaysdata.com>
> Tested-by: Cyril B. <cbay@alwaysdata.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---

Oh, this reminds me I had to solve this issue in kgraft while playing with 
relocations. I wanted to fix it in klp as well and forgot about it so it 
is great to see it done.

Anyway...

>  arch/x86/kernel/livepatch.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> index ff3c3101d..d1d35cc 100644
> --- a/arch/x86/kernel/livepatch.c
> +++ b/arch/x86/kernel/livepatch.c
> @@ -42,7 +42,6 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
>  	bool readonly;
>  	unsigned long val;
>  	unsigned long core = (unsigned long)mod->module_core;
> -	unsigned long core_ro_size = mod->core_ro_size;
>  	unsigned long core_size = mod->core_size;
>  
>  	switch (type) {
> @@ -70,10 +69,12 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
>  		/* loc does not point to any symbol inside the module */
>  		return -EINVAL;
>  
> -	if (loc < core + core_ro_size)
> +	readonly = false;
> +
> +#ifdef CONFIG_DEBUG_SET_MODULE_RONX
> +	if (loc < core + mod->core_ro_size)
>  		readonly = true;
> -	else
> -		readonly = false;
> +#endif
>  

Wouldn't it be better to use static inline function defined somewhere in 
our header file which would do the job for CONFIG_DEBUG_SET_MODULE_RONX 
set and nothing for the opposite? I think it is better than to have ifdef 
here. Probably just a matter of taste.

Secondly, why do we call set_memory_rw/ro here for each relocation? 
Wouldn't it be possible to do it once for all the relevant pages of the 
patched module? I mean we could call set_memory_rw in 
klp_write_object_relocations from kernel/livepatch/core.c just before the 
for loop and set_memory_ro on the way out. With the static inline function 
it could look like this

+#ifdef CONFIG_DEBUG_SET_MODULE_RONX
+static inline void set_module_text(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);
+}
+#else
+static inline void set_module_text(void *start, void *end, int (*set)(unsigned long start,
+       int num_pages)) { }
+#endif

somewhere in include/linux/livepatch.h. And in 
klp_write_object_relocations() there could be 

+       set_module_text(mod->module_core, mod->module_core +
+               mod->core_text_size, set_memory_rw);

just before the for loop which goes through all the relocations

and 

+       set_module_text(mod->module_core, mod->module_core +
+               mod->core_text_size, set_memory_ro);

on the way out. 

We would also get rid of readonly variable.

At least this is how I solved it. Take it as an idea. I don't know if it 
is even better and I certainly do not insist on it. Your approach is ok.

Thanks,
Miroslav

Re: [PATCH] x86/livepatch: Fix crash with !CONFIG_DEBUG_SET_MODULE_RONX

[Josh Poimboeuf]

On Tue, Nov 03, 2015 at 11:22:12AM +0100, Miroslav Benes wrote:
> On Mon, 2 Nov 2015, Josh Poimboeuf wrote:
> 
> > When loading a patch module on a kernel with
> > !CONFIG_DEBUG_SET_MODULE_RONX, the following crash occurs:
> > 
> >   [  205.988776] livepatch: enabling patch 'kpatch_meminfo_string'
> >   [  205.989829] BUG: unable to handle kernel paging request at ffffffffa08d2fc0
> >   [  205.989863] IP: [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
> >   [  205.989888] PGD 1a10067 PUD 1a11063 PMD 7bcde067 PTE 3740e161
> >   [  205.989915] Oops: 0003 [#1] SMP
> >   [  205.990187] CPU: 2 PID: 14570 Comm: insmod Tainted: G           O  K 4.1.12
> >   [  205.990214] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
> >   [  205.990249] task: ffff8800374aaa90 ti: ffff8800794b8000 task.ti: ffff8800794b8000
> >   [  205.990276] RIP: 0010:[<ffffffff8154fecb>]  [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
> >   [  205.990307] RSP: 0018:ffff8800794bbd58  EFLAGS: 00010246
> >   [  205.990327] RAX: 0000000000000000 RBX: ffffffffa08d2fc0 RCX: 0000000000000000
> >   [  205.990356] RDX: 01ffff8000000080 RSI: 0000000000000000 RDI: ffffffff81a54b40
> >   [  205.990382] RBP: ffff88007b4c4d80 R08: 0000000000000007 R09: 0000000000000000
> >   [  205.990408] R10: 0000000000000008 R11: ffffea0001f18840 R12: 0000000000000000
> >   [  205.990433] R13: 0000000000000001 R14: ffffffffa08d2fc0 R15: ffff88007bd0bc40
> >   [  205.990459] FS:  00007f1128fbc700(0000) GS:ffff88007fc80000(0000) knlGS:0000000000000000
> >   [  205.990488] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >   [  205.990509] CR2: ffffffffa08d2fc0 CR3: 000000002606e000 CR4: 00000000001406e0
> >   [  205.990536] Stack:
> >   [  205.990545]  ffff8800794bbec8 0000000000000001 ffffffffa08d3010 ffffffff810ecea9
> >   [  205.990576]  ffffffff810e8e40 000000000005f360 ffff88007bd0bc50 ffffffffa08d3240
> >   [  205.990608]  ffffffffa08d52c0 ffffffffa08d3210 ffff8800794bbed8 ffff8800794bbf1c
> >   [  205.990639] Call Trace:
> >   [  205.990651]  [<ffffffff810ecea9>] ? load_module+0x1e59/0x23a0
> >   [  205.990672]  [<ffffffff810e8e40>] ? store_uevent+0x40/0x40
> >   [  205.990693]  [<ffffffff810e99b5>] ? copy_module_from_fd.isra.49+0xb5/0x140
> >   [  205.990718]  [<ffffffff810ed5bd>] ? SyS_finit_module+0x7d/0xa0
> >   [  205.990741]  [<ffffffff81556832>] ? system_call_fastpath+0x16/0x75
> >   [  205.990763] Code: f9 00 00 00 74 23 49 c7 c0 92 e1 60 81 48 8d 53 18 89 c1 4c 89 c6 48 c7 c7 f0 85 7d 81 31 c0 e8 71 fa ff ff e8 58 0e 00 00 31 f6 <c7> 03 00 00 00 00 48 89 da 48 c7 c7 20 c7 a5 81 e8 d0 ec b3 ff
> >   [  205.990916] RIP  [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
> >   [  205.990940]  RSP <ffff8800794bbd58>
> >   [  205.990953] CR2: ffffffffa08d2fc0
> > 
> > With !CONFIG_DEBUG_SET_MODULE_RONX, module text and rodata pages are
> > writable, and the debug_align() macro allows the module struct to share
> > a page with executable text.  When klp_write_module_reloc() calls
> > set_memory_ro() on the page, it effectively turns the module struct into
> > a read-only structure, resulting in a page fault when load_module() does
> > "mod->state = MODULE_STATE_LIVE".
> > 
> > Reported-by: Cyril B. <cbay@alwaysdata.com>
> > Tested-by: Cyril B. <cbay@alwaysdata.com>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> 
> Oh, this reminds me I had to solve this issue in kgraft while playing with 
> relocations. I wanted to fix it in klp as well and forgot about it so it 
> is great to see it done.
> 
> Anyway...
> 
> >  arch/x86/kernel/livepatch.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
> > index ff3c3101d..d1d35cc 100644
> > --- a/arch/x86/kernel/livepatch.c
> > +++ b/arch/x86/kernel/livepatch.c
> > @@ -42,7 +42,6 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
> >  	bool readonly;
> >  	unsigned long val;
> >  	unsigned long core = (unsigned long)mod->module_core;
> > -	unsigned long core_ro_size = mod->core_ro_size;
> >  	unsigned long core_size = mod->core_size;
> >  
> >  	switch (type) {
> > @@ -70,10 +69,12 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
> >  		/* loc does not point to any symbol inside the module */
> >  		return -EINVAL;
> >  
> > -	if (loc < core + core_ro_size)
> > +	readonly = false;
> > +
> > +#ifdef CONFIG_DEBUG_SET_MODULE_RONX
> > +	if (loc < core + mod->core_ro_size)
> >  		readonly = true;
> > -	else
> > -		readonly = false;
> > +#endif
> >  
> 
> Wouldn't it be better to use static inline function defined somewhere in 
> our header file which would do the job for CONFIG_DEBUG_SET_MODULE_RONX 
> set and nothing for the opposite? I think it is better than to have ifdef 
> here. Probably just a matter of taste.

I agree that would be better.  Though I think I'd make a static function
in the .c file so it stays internal.

> Secondly, why do we call set_memory_rw/ro here for each relocation? 
> Wouldn't it be possible to do it once for all the relevant pages of the 
> patched module? I mean we could call set_memory_rw in 
> klp_write_object_relocations from kernel/livepatch/core.c just before the 
> for loop and set_memory_ro on the way out. With the static inline function 
> it could look like this
> 
> +#ifdef CONFIG_DEBUG_SET_MODULE_RONX
> +static inline void set_module_text(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);
> +}
> +#else
> +static inline void set_module_text(void *start, void *end, int (*set)(unsigned long start,
> +       int num_pages)) { }
> +#endif
> 
> somewhere in include/linux/livepatch.h. And in 
> klp_write_object_relocations() there could be 
> 
> +       set_module_text(mod->module_core, mod->module_core +
> +               mod->core_text_size, set_memory_rw);
> 
> just before the for loop which goes through all the relocations
> 
> and 
> 
> +       set_module_text(mod->module_core, mod->module_core +
> +               mod->core_text_size, set_memory_ro);
> 
> on the way out. 
> 
> We would also get rid of readonly variable.
> 
> At least this is how I solved it. Take it as an idea. I don't know if it 
> is even better and I certainly do not insist on it. Your approach is ok.

Yeah, I like this idea.  Though we have to be careful.  Not only can
relocations occur in text, they can also occur in data.  So we'd have to
set read-only data writable as well.  So it should be "mod->module_core
+ mod->core_ro_size".

-- 
Josh

[PATCH] livepatch: Cleanup page permission changes

[Josh Poimboeuf]

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.)

---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 <mbenes@suse.cz>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 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;
 	unsigned long val;
 	unsigned long core = (unsigned long)mod->module_core;
 	unsigned long core_size = mod->core_size;
@@ -69,23 +66,5 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
 		/* loc does not point to any symbol inside the module */
 		return -EINVAL;
 
-	readonly = false;
-
-#ifdef CONFIG_DEBUG_SET_MODULE_RONX
-	if (loc < core + mod->core_ro_size)
-		readonly = true;
-#endif
-
-	/* determine if the relocation spans a page boundary */
-	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
-
-	if (readonly)
-		set_memory_rw(loc & PAGE_MASK, numpages);
-
-	ret = probe_kernel_write((void *)loc, &val, size);
-
-	if (readonly)
-		set_memory_ro(loc & PAGE_MASK, numpages);
-
-	return ret;
+	return probe_kernel_write((void *)loc, &val, size);
 }
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6e53441..328fbd5 100644
--- 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)
+{
+	set_page_attributes(mod->module_core,
+			    mod->module_core + mod->core_ro_size,
+			    set_memory_ro);
+}
+#else
+static void set_module_ro_rw(struct module *mod) {}
+static void set_module_ro_ro(struct module *mod) {}
+#endif
+
 struct klp_find_arg {
 	const char *objname;
 	const char *name;
@@ -283,7 +311,7 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
 static int klp_write_object_relocations(struct module *pmod,
 					struct klp_object *obj)
 {
-	int ret;
+	int ret = 0;
 	struct klp_reloc *reloc;
 
 	if (WARN_ON(!klp_is_object_loaded(obj)))
@@ -292,12 +320,14 @@ static int klp_write_object_relocations(struct module *pmod,
 	if (WARN_ON(!obj->relocs))
 		return -EINVAL;
 
+	set_module_ro_rw(pmod);
+
 	for (reloc = obj->relocs; reloc->name; reloc++) {
 		if (!klp_is_module(obj)) {
 			ret = klp_verify_vmlinux_symbol(reloc->name,
 							reloc->val);
 			if (ret)
-				return ret;
+				goto out;
 		} else {
 			/* module, reloc->val needs to be discovered */
 			if (reloc->external)
@@ -309,18 +339,20 @@ static int klp_write_object_relocations(struct module *pmod,
 							     reloc->name,
 							     &reloc->val);
 			if (ret)
-				return ret;
+				goto out;
 		}
 		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
 					     reloc->val + reloc->addend);
 		if (ret) {
 			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
 			       reloc->name, reloc->val, ret);
-			return ret;
+			goto out;
 		}
 	}
 
-	return 0;
+out:
+	set_module_ro_ro(pmod);
+	return ret;
 }
 
 static void notrace klp_ftrace_handler(unsigned long ip,
-- 
2.4.3

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

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 <mbenes@suse.cz>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

Great, I like the change.

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

Cheers,
Miroslav

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

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

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@suse.cz>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  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

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 <mbenes@suse.cz>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  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.

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

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

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

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

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

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