[PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Steven Rostedt]

From: Steven Rostedt (VMware) <rostedt@goodmis.org>

In the process of using text_poke_bp() for ftrace on x86, when
performing the following action:

 # rmmod snd_hda_codec_hdmi
 # echo function > /sys/kernel/tracing/current_tracer
 # modprobe snd_hda_codec_hdmi

It triggered this:

 BUG: unable to handle page fault for address: ffffffffa03d0000
 #PF: supervisor write access in kernel mode
 #PF: error_code(0x0003) - permissions violation
 PGD 2a12067 P4D 2a12067 PUD 2a13063 PMD c42bc067 PTE c58a0061
 Oops: 0003 [#1] PREEMPT SMP KASAN PTI
 CPU: 1 PID: 1182 Comm: modprobe Not tainted 5.4.0-rc2-test+ #50
 Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016
 RIP: 0010:memcpy_erms+0x6/0x10
 Code: 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 <f3> a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 fe
 RSP: 0018:ffff8880a10479e0 EFLAGS: 00010246
 RAX: ffffffffa03d0000 RBX: ffffffffa03d0000 RCX: 0000000000000005
 RDX: 0000000000000005 RSI: ffffffff8363e160 RDI: ffffffffa03d0000
 RBP: ffff88807e9ec000 R08: fffffbfff407a001 R09: fffffbfff407a001
 R10: fffffbfff407a000 R11: ffffffffa03d0004 R12: ffffffff8221f160
 R13: ffffffffa03d0000 R14: ffff88807e9ec000 R15: ffffffffa0481640
 FS:  00007eff92e28280(0000) GS:ffff8880d4840000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: ffffffffa03d0000 CR3: 00000000a1048001 CR4: 00000000001606e0
 Call Trace:
  ftrace_make_call+0x76/0x90
  ftrace_module_enable+0x493/0x4f0
  load_module+0x3a31/0x3e10
  ? ring_buffer_read+0x70/0x70
  ? module_frob_arch_sections+0x20/0x20
  ? rb_commit+0xee/0x600
  ? tracing_generic_entry_update+0xe1/0xf0
  ? ring_buffer_unlock_commit+0xfb/0x220
  ? 0xffffffffa0000061
  ? __do_sys_finit_module+0x11a/0x1b0
  __do_sys_finit_module+0x11a/0x1b0
  ? __ia32_sys_init_module+0x40/0x40
  ? ring_buffer_unlock_commit+0xfb/0x220
  ? function_trace_call+0x179/0x260
  ? __do_sys_finit_module+0x1b0/0x1b0
  ? __do_sys_finit_module+0x1b0/0x1b0
  ? do_syscall_64+0x58/0x1a0
  do_syscall_64+0x68/0x1a0
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
 RIP: 0033:0x7eff92f42efd

The reason is that ftrace_module_enable() is called after the module
has set its text to read-only. There's subtle reasons that this needs
to be called afterward, and we need to continue to do so.

Instead of going back to the original way of changing all modules to
read-write to update functions for a module being loaded, add two new
module helpers set_module_text_rw() and set_module_text_ro() that takes
a module as a parameter and only modifies the text for that one module.
Then move the logic for this from the architecture code in ftrace, to
the generic code, and have ftrace on module load do the modification.
It only affects the module being loaded.

Link: http://lkml.kernel.org/r/20191007081716.07616230.8@infradead.org

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
diff --git a/include/linux/module.h b/include/linux/module.h
index 6d20895e7739..143c902bcbcf 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -846,11 +846,15 @@ extern int module_sysfs_initialized;
 #define __MODULE_STRING(x) __stringify(x)
 
 #ifdef CONFIG_STRICT_MODULE_RWX
+extern void set_module_text_rw(const struct module *mod);
+extern void set_module_text_ro(const struct module *mod);
 extern void set_all_modules_text_rw(void);
 extern void set_all_modules_text_ro(void);
 extern void module_enable_ro(const struct module *mod, bool after_init);
 extern void module_disable_ro(const struct module *mod);
 #else
+static inline void set_module_text_rw(const struct module *mod) { }
+static inline void set_module_text_ro(const struct module *mod) { }
 static inline void set_all_modules_text_rw(void) { }
 static inline void set_all_modules_text_ro(void) { }
 static inline void module_enable_ro(const struct module *mod, bool after_init) { }
diff --git a/kernel/module.c b/kernel/module.c
index ff2d7359a418..8f3a18d3ac75 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2029,6 +2029,37 @@ static void module_enable_nx(const struct module *mod)
 	frob_writable_data(&mod->init_layout, set_memory_nx);
 }
 
+void set_module_text_rw(const struct module *mod)
+{
+	if (!rodata_enabled)
+		return;
+
+	mutex_lock(&module_mutex);
+	if (mod->state == MODULE_STATE_UNFORMED)
+		goto out;
+
+	frob_text(&mod->core_layout, set_memory_rw);
+	frob_text(&mod->init_layout, set_memory_rw);
+out:
+	mutex_unlock(&module_mutex);
+}
+
+void set_module_text_ro(const struct module *mod)
+{
+	if (!rodata_enabled)
+		return;
+
+	mutex_lock(&module_mutex);
+	if (mod->state == MODULE_STATE_UNFORMED ||
+	    mod->state == MODULE_STATE_GOING)
+		goto out;
+
+	frob_text(&mod->core_layout, set_memory_ro);
+	frob_text(&mod->init_layout, set_memory_ro);
+out:
+	mutex_unlock(&module_mutex);
+}
+
 /* Iterate through all modules and set each module's text as RW */
 void set_all_modules_text_rw(void)
 {
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 62a50bf399d6..93506a51a308 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5815,7 +5815,7 @@ void ftrace_module_enable(struct module *mod)
 	 * so that we can modify the text.
 	 */
 	if (ftrace_start_up)
-		ftrace_arch_code_modify_prepare();
+		set_module_text_rw(mod);
 
 	do_for_each_ftrace_rec(pg, rec) {
 		int cnt;
@@ -5855,7 +5855,7 @@ void ftrace_module_enable(struct module *mod)
 
  out_loop:
 	if (ftrace_start_up)
-		ftrace_arch_code_modify_post_process();
+		set_module_text_ro(mod);
 
  out_unlock:
 	mutex_unlock(&ftrace_lock);

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Peter Zijlstra]

On Wed, Oct 09, 2019 at 10:36:38PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> In the process of using text_poke_bp() for ftrace on x86, when
> performing the following action:
> 
>  # rmmod snd_hda_codec_hdmi
>  # echo function > /sys/kernel/tracing/current_tracer
>  # modprobe snd_hda_codec_hdmi
> 
> It triggered this:
> 
>  BUG: unable to handle page fault for address: ffffffffa03d0000
>  #PF: supervisor write access in kernel mode
>  #PF: error_code(0x0003) - permissions violation
>  PGD 2a12067 P4D 2a12067 PUD 2a13063 PMD c42bc067 PTE c58a0061
>  Oops: 0003 [#1] PREEMPT SMP KASAN PTI
>  CPU: 1 PID: 1182 Comm: modprobe Not tainted 5.4.0-rc2-test+ #50
>  Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016
>  RIP: 0010:memcpy_erms+0x6/0x10
>  Code: 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 <f3> a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 fe
>  RSP: 0018:ffff8880a10479e0 EFLAGS: 00010246
>  RAX: ffffffffa03d0000 RBX: ffffffffa03d0000 RCX: 0000000000000005
>  RDX: 0000000000000005 RSI: ffffffff8363e160 RDI: ffffffffa03d0000
>  RBP: ffff88807e9ec000 R08: fffffbfff407a001 R09: fffffbfff407a001
>  R10: fffffbfff407a000 R11: ffffffffa03d0004 R12: ffffffff8221f160
>  R13: ffffffffa03d0000 R14: ffff88807e9ec000 R15: ffffffffa0481640
>  FS:  00007eff92e28280(0000) GS:ffff8880d4840000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: ffffffffa03d0000 CR3: 00000000a1048001 CR4: 00000000001606e0
>  Call Trace:
>   ftrace_make_call+0x76/0x90
>   ftrace_module_enable+0x493/0x4f0
>   load_module+0x3a31/0x3e10
>   ? ring_buffer_read+0x70/0x70
>   ? module_frob_arch_sections+0x20/0x20
>   ? rb_commit+0xee/0x600
>   ? tracing_generic_entry_update+0xe1/0xf0
>   ? ring_buffer_unlock_commit+0xfb/0x220
>   ? 0xffffffffa0000061
>   ? __do_sys_finit_module+0x11a/0x1b0
>   __do_sys_finit_module+0x11a/0x1b0
>   ? __ia32_sys_init_module+0x40/0x40
>   ? ring_buffer_unlock_commit+0xfb/0x220
>   ? function_trace_call+0x179/0x260
>   ? __do_sys_finit_module+0x1b0/0x1b0
>   ? __do_sys_finit_module+0x1b0/0x1b0
>   ? do_syscall_64+0x58/0x1a0
>   do_syscall_64+0x68/0x1a0
>   entry_SYSCALL_64_after_hwframe+0x49/0xbe
>  RIP: 0033:0x7eff92f42efd
> 
> The reason is that ftrace_module_enable() is called after the module
> has set its text to read-only. There's subtle reasons that this needs
> to be called afterward, and we need to continue to do so.

Please explain.

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Peter Zijlstra]

On Thu, Oct 10, 2019 at 09:31:21AM +0200, Peter Zijlstra wrote:
> On Wed, Oct 09, 2019 at 10:36:38PM -0400, Steven Rostedt wrote:
> > From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > 
> > In the process of using text_poke_bp() for ftrace on x86, when
> > performing the following action:
> > 
> >  # rmmod snd_hda_codec_hdmi
> >  # echo function > /sys/kernel/tracing/current_tracer
> >  # modprobe snd_hda_codec_hdmi
> > 
> > It triggered this:
> > 
> >  BUG: unable to handle page fault for address: ffffffffa03d0000
> >  #PF: supervisor write access in kernel mode
> >  #PF: error_code(0x0003) - permissions violation
> >  PGD 2a12067 P4D 2a12067 PUD 2a13063 PMD c42bc067 PTE c58a0061
> >  Oops: 0003 [#1] PREEMPT SMP KASAN PTI
> >  CPU: 1 PID: 1182 Comm: modprobe Not tainted 5.4.0-rc2-test+ #50
> >  Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016
> >  RIP: 0010:memcpy_erms+0x6/0x10
> >  Code: 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 <f3> a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 fe
> >  RSP: 0018:ffff8880a10479e0 EFLAGS: 00010246
> >  RAX: ffffffffa03d0000 RBX: ffffffffa03d0000 RCX: 0000000000000005
> >  RDX: 0000000000000005 RSI: ffffffff8363e160 RDI: ffffffffa03d0000
> >  RBP: ffff88807e9ec000 R08: fffffbfff407a001 R09: fffffbfff407a001
> >  R10: fffffbfff407a000 R11: ffffffffa03d0004 R12: ffffffff8221f160
> >  R13: ffffffffa03d0000 R14: ffff88807e9ec000 R15: ffffffffa0481640
> >  FS:  00007eff92e28280(0000) GS:ffff8880d4840000(0000) knlGS:0000000000000000
> >  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >  CR2: ffffffffa03d0000 CR3: 00000000a1048001 CR4: 00000000001606e0
> >  Call Trace:
> >   ftrace_make_call+0x76/0x90
> >   ftrace_module_enable+0x493/0x4f0
> >   load_module+0x3a31/0x3e10
> >   ? ring_buffer_read+0x70/0x70
> >   ? module_frob_arch_sections+0x20/0x20
> >   ? rb_commit+0xee/0x600
> >   ? tracing_generic_entry_update+0xe1/0xf0
> >   ? ring_buffer_unlock_commit+0xfb/0x220
> >   ? 0xffffffffa0000061
> >   ? __do_sys_finit_module+0x11a/0x1b0
> >   __do_sys_finit_module+0x11a/0x1b0
> >   ? __ia32_sys_init_module+0x40/0x40
> >   ? ring_buffer_unlock_commit+0xfb/0x220
> >   ? function_trace_call+0x179/0x260
> >   ? __do_sys_finit_module+0x1b0/0x1b0
> >   ? __do_sys_finit_module+0x1b0/0x1b0
> >   ? do_syscall_64+0x58/0x1a0
> >   do_syscall_64+0x68/0x1a0
> >   entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >  RIP: 0033:0x7eff92f42efd
> > 
> > The reason is that ftrace_module_enable() is called after the module
> > has set its text to read-only. There's subtle reasons that this needs
> > to be called afterward, and we need to continue to do so.
> 
> Please explain.

Also, no. If you _have_ to modify code after, then you get to use
text_poke_bp() instead of text_poke_early().

But first we need to document why this needs to be late. That's the
crucial point and you forgot to explain.

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Peter Zijlstra]

On Thu, Oct 10, 2019 at 09:31:21AM +0200, Peter Zijlstra wrote:
> On Wed, Oct 09, 2019 at 10:36:38PM -0400, Steven Rostedt wrote:
> > From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > 
> > In the process of using text_poke_bp() for ftrace on x86, when
> > performing the following action:
> > 
> >  # rmmod snd_hda_codec_hdmi
> >  # echo function > /sys/kernel/tracing/current_tracer
> >  # modprobe snd_hda_codec_hdmi
> > 
> > It triggered this:
> > 
> >  BUG: unable to handle page fault for address: ffffffffa03d0000
> >  #PF: supervisor write access in kernel mode
> >  #PF: error_code(0x0003) - permissions violation
> >  PGD 2a12067 P4D 2a12067 PUD 2a13063 PMD c42bc067 PTE c58a0061
> >  Oops: 0003 [#1] PREEMPT SMP KASAN PTI
> >  CPU: 1 PID: 1182 Comm: modprobe Not tainted 5.4.0-rc2-test+ #50
> >  Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016
> >  RIP: 0010:memcpy_erms+0x6/0x10
> >  Code: 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 <f3> a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 fe
> >  RSP: 0018:ffff8880a10479e0 EFLAGS: 00010246
> >  RAX: ffffffffa03d0000 RBX: ffffffffa03d0000 RCX: 0000000000000005
> >  RDX: 0000000000000005 RSI: ffffffff8363e160 RDI: ffffffffa03d0000
> >  RBP: ffff88807e9ec000 R08: fffffbfff407a001 R09: fffffbfff407a001
> >  R10: fffffbfff407a000 R11: ffffffffa03d0004 R12: ffffffff8221f160
> >  R13: ffffffffa03d0000 R14: ffff88807e9ec000 R15: ffffffffa0481640
> >  FS:  00007eff92e28280(0000) GS:ffff8880d4840000(0000) knlGS:0000000000000000
> >  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >  CR2: ffffffffa03d0000 CR3: 00000000a1048001 CR4: 00000000001606e0
> >  Call Trace:
> >   ftrace_make_call+0x76/0x90
> >   ftrace_module_enable+0x493/0x4f0
> >   load_module+0x3a31/0x3e10
> >   ? ring_buffer_read+0x70/0x70
> >   ? module_frob_arch_sections+0x20/0x20
> >   ? rb_commit+0xee/0x600
> >   ? tracing_generic_entry_update+0xe1/0xf0
> >   ? ring_buffer_unlock_commit+0xfb/0x220
> >   ? 0xffffffffa0000061
> >   ? __do_sys_finit_module+0x11a/0x1b0
> >   __do_sys_finit_module+0x11a/0x1b0
> >   ? __ia32_sys_init_module+0x40/0x40
> >   ? ring_buffer_unlock_commit+0xfb/0x220
> >   ? function_trace_call+0x179/0x260
> >   ? __do_sys_finit_module+0x1b0/0x1b0
> >   ? __do_sys_finit_module+0x1b0/0x1b0
> >   ? do_syscall_64+0x58/0x1a0
> >   do_syscall_64+0x68/0x1a0
> >   entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >  RIP: 0033:0x7eff92f42efd
> > 
> > The reason is that ftrace_module_enable() is called after the module
> > has set its text to read-only. There's subtle reasons that this needs
> > to be called afterward, and we need to continue to do so.
> 
> Please explain.

I don't see any reason what so ever..

load_module()
  ...
  complete_formation()
    mutex_lock(&module_mutex);
    ...
    module_enable_ro();
    module_enable_nx();
    module_enable_x();

    mod->state = MODULE_STATE_COMING;
    mutex_unlock(&module_mutex);

  prepare_coming_module()
    ftrace_module_enable();
    ...

IOW, we're doing ftrace_module_enable() immediately after we flip it
RO+X. There is nothing in between that we can possibly rely on.

I was going to put:

  blocking_notifier_call_chain(&module_notify_list,
			       MODULE_STATE_UNFORMED, mod);

right before module_enable_ro(), in complete_formation(), for jump_label
and static_call. It looks like ftrace (and possibly klp) want that too.

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Peter Zijlstra]

On Thu, Oct 10, 2019 at 11:33:29AM +0200, Peter Zijlstra wrote:
> On Thu, Oct 10, 2019 at 09:31:21AM +0200, Peter Zijlstra wrote:
> > On Wed, Oct 09, 2019 at 10:36:38PM -0400, Steven Rostedt wrote:
> > > From: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > 
> > > In the process of using text_poke_bp() for ftrace on x86, when
> > > performing the following action:
> > > 
> > >  # rmmod snd_hda_codec_hdmi
> > >  # echo function > /sys/kernel/tracing/current_tracer
> > >  # modprobe snd_hda_codec_hdmi
> > > 
> > > It triggered this:
> > > 
> > >  BUG: unable to handle page fault for address: ffffffffa03d0000
> > >  #PF: supervisor write access in kernel mode
> > >  #PF: error_code(0x0003) - permissions violation
> > >  PGD 2a12067 P4D 2a12067 PUD 2a13063 PMD c42bc067 PTE c58a0061
> > >  Oops: 0003 [#1] PREEMPT SMP KASAN PTI
> > >  CPU: 1 PID: 1182 Comm: modprobe Not tainted 5.4.0-rc2-test+ #50
> > >  Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016
> > >  RIP: 0010:memcpy_erms+0x6/0x10
> > >  Code: 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 <f3> a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 fe
> > >  RSP: 0018:ffff8880a10479e0 EFLAGS: 00010246
> > >  RAX: ffffffffa03d0000 RBX: ffffffffa03d0000 RCX: 0000000000000005
> > >  RDX: 0000000000000005 RSI: ffffffff8363e160 RDI: ffffffffa03d0000
> > >  RBP: ffff88807e9ec000 R08: fffffbfff407a001 R09: fffffbfff407a001
> > >  R10: fffffbfff407a000 R11: ffffffffa03d0004 R12: ffffffff8221f160
> > >  R13: ffffffffa03d0000 R14: ffff88807e9ec000 R15: ffffffffa0481640
> > >  FS:  00007eff92e28280(0000) GS:ffff8880d4840000(0000) knlGS:0000000000000000
> > >  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >  CR2: ffffffffa03d0000 CR3: 00000000a1048001 CR4: 00000000001606e0
> > >  Call Trace:
> > >   ftrace_make_call+0x76/0x90
> > >   ftrace_module_enable+0x493/0x4f0
> > >   load_module+0x3a31/0x3e10
> > >   ? ring_buffer_read+0x70/0x70
> > >   ? module_frob_arch_sections+0x20/0x20
> > >   ? rb_commit+0xee/0x600
> > >   ? tracing_generic_entry_update+0xe1/0xf0
> > >   ? ring_buffer_unlock_commit+0xfb/0x220
> > >   ? 0xffffffffa0000061
> > >   ? __do_sys_finit_module+0x11a/0x1b0
> > >   __do_sys_finit_module+0x11a/0x1b0
> > >   ? __ia32_sys_init_module+0x40/0x40
> > >   ? ring_buffer_unlock_commit+0xfb/0x220
> > >   ? function_trace_call+0x179/0x260
> > >   ? __do_sys_finit_module+0x1b0/0x1b0
> > >   ? __do_sys_finit_module+0x1b0/0x1b0
> > >   ? do_syscall_64+0x58/0x1a0
> > >   do_syscall_64+0x68/0x1a0
> > >   entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > >  RIP: 0033:0x7eff92f42efd
> > > 
> > > The reason is that ftrace_module_enable() is called after the module
> > > has set its text to read-only. There's subtle reasons that this needs
> > > to be called afterward, and we need to continue to do so.
> > 
> > Please explain.
> 
> I don't see any reason what so ever..
> 
> load_module()
>   ...
>   complete_formation()
>     mutex_lock(&module_mutex);
>     ...
>     module_enable_ro();
>     module_enable_nx();
>     module_enable_x();
> 
>     mod->state = MODULE_STATE_COMING;
>     mutex_unlock(&module_mutex);
> 
>   prepare_coming_module()
>     ftrace_module_enable();
>     ...
> 
> IOW, we're doing ftrace_module_enable() immediately after we flip it
> RO+X. There is nothing in between that we can possibly rely on.
> 
> I was going to put:
> 
>   blocking_notifier_call_chain(&module_notify_list,
> 			       MODULE_STATE_UNFORMED, mod);
> 
> right before module_enable_ro(), in complete_formation(), for jump_label
> and static_call. It looks like ftrace (and possibly klp) want that too.

Also, you already have ftrace_module_init() right before that. The only
thing inbetween ftrace_module_init() and ftrace_module_enable() is
verify_exported_symbols() and module_bug_finalize().

Do you really need that for patching stuff?

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Peter Zijlstra]

On Thu, Oct 10, 2019 at 11:36:50AM +0200, Peter Zijlstra wrote:
> On Thu, Oct 10, 2019 at 11:33:29AM +0200, Peter Zijlstra wrote:

> > I don't see any reason what so ever..
> > 
> > load_module()
> >   ...
> >   complete_formation()
> >     mutex_lock(&module_mutex);
> >     ...
> >     module_enable_ro();
> >     module_enable_nx();
> >     module_enable_x();
> > 
> >     mod->state = MODULE_STATE_COMING;
> >     mutex_unlock(&module_mutex);
> > 
> >   prepare_coming_module()
> >     ftrace_module_enable();
> >     ...
> > 
> > IOW, we're doing ftrace_module_enable() immediately after we flip it
> > RO+X. There is nothing in between that we can possibly rely on.
> > 
> > I was going to put:
> > 
> >   blocking_notifier_call_chain(&module_notify_list,
> > 			       MODULE_STATE_UNFORMED, mod);
> > 
> > right before module_enable_ro(), in complete_formation(), for jump_label
> > and static_call. It looks like ftrace (and possibly klp) want that too.
> 
> Also, you already have ftrace_module_init() right before that. The only
> thing inbetween ftrace_module_init() and ftrace_module_enable() is
> verify_exported_symbols() and module_bug_finalize().
> 
> Do you really need that for patching stuff?

If you rework the locking slightly, such that you have to call
ftrace_process_locs() with ftrace_lock already held, then it looks like
you can squash ftrace_module_enable() right in there.

There is absolutely no fundamental reason you cannot patch it from
ftrace_module_init().

Yes, your code is anal about checking the NOPs, so you first have to
write NOPs before you can write CALLs, if it is enabled. But afaict you
really can do all that from ftrace_module_init(), as long as you do it
all under the same ftrace_lock section.

If you have two sections, like now, then there is indeed that race that
ftrace can get enabled in between, and all the confusion that that
brings.

That is, what's fundamentally buggered about something like this?

---
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 62a50bf399d6..5f7113f100ce 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5626,6 +5626,48 @@ static int ftrace_process_locs(struct module *mod,
 	ftrace_update_code(mod, start_pg);
 	if (!mod)
 		local_irq_restore(flags);
+
+	if (ftrace_disabled || !mod)
+		goto out_loop;
+
+	do_for_each_ftrace_rec(pg, rec) {
+		int cnt;
+		/*
+		 * do_for_each_ftrace_rec() is a double loop.
+		 * module text shares the pg. If a record is
+		 * not part of this module, then skip this pg,
+		 * which the "break" will do.
+		 */
+		if (!within_module_core(rec->ip, mod) &&
+		    !within_module_init(rec->ip, mod))
+			break;
+
+		cnt = 0;
+
+		/*
+		 * When adding a module, we need to check if tracers are
+		 * currently enabled and if they are, and can trace this record,
+		 * we need to enable the module functions as well as update the
+		 * reference counts for those function records.
+		 */
+		if (ftrace_start_up)
+			cnt += referenced_filters(rec);
+
+		/* This clears FTRACE_FL_DISABLED */
+		rec->flags = cnt;
+
+		if (ftrace_start_up && cnt) {
+			int failed = __ftrace_replace_code(rec, 1);
+			if (failed) {
+				ftrace_bug(failed, rec);
+				goto out_loop;
+			}
+		}
+
+	} while_for_each_ftrace_rec();
+
+ out_loop:
+
 	ret = 0;
  out:
 	mutex_unlock(&ftrace_lock);
@@ -5793,73 +5835,6 @@ void ftrace_release_mod(struct module *mod)
 
 void ftrace_module_enable(struct module *mod)
 {
-	struct dyn_ftrace *rec;
-	struct ftrace_page *pg;
-
-	mutex_lock(&ftrace_lock);
-
-	if (ftrace_disabled)
-		goto out_unlock;
-
-	/*
-	 * If the tracing is enabled, go ahead and enable the record.
-	 *
-	 * The reason not to enable the record immediately is the
-	 * inherent check of ftrace_make_nop/ftrace_make_call for
-	 * correct previous instructions.  Making first the NOP
-	 * conversion puts the module to the correct state, thus
-	 * passing the ftrace_make_call check.
-	 *
-	 * We also delay this to after the module code already set the
-	 * text to read-only, as we now need to set it back to read-write
-	 * so that we can modify the text.
-	 */
-	if (ftrace_start_up)
-		ftrace_arch_code_modify_prepare();
-
-	do_for_each_ftrace_rec(pg, rec) {
-		int cnt;
-		/*
-		 * do_for_each_ftrace_rec() is a double loop.
-		 * module text shares the pg. If a record is
-		 * not part of this module, then skip this pg,
-		 * which the "break" will do.
-		 */
-		if (!within_module_core(rec->ip, mod) &&
-		    !within_module_init(rec->ip, mod))
-			break;
-
-		cnt = 0;
-
-		/*
-		 * When adding a module, we need to check if tracers are
-		 * currently enabled and if they are, and can trace this record,
-		 * we need to enable the module functions as well as update the
-		 * reference counts for those function records.
-		 */
-		if (ftrace_start_up)
-			cnt += referenced_filters(rec);
-
-		/* This clears FTRACE_FL_DISABLED */
-		rec->flags = cnt;
-
-		if (ftrace_start_up && cnt) {
-			int failed = __ftrace_replace_code(rec, 1);
-			if (failed) {
-				ftrace_bug(failed, rec);
-				goto out_loop;
-			}
-		}
-
-	} while_for_each_ftrace_rec();
-
- out_loop:
-	if (ftrace_start_up)
-		ftrace_arch_code_modify_post_process();
-
- out_unlock:
-	mutex_unlock(&ftrace_lock);
-
 	process_cached_mods(mod->name);
 }
 

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Steven Rostedt]

On Thu, 10 Oct 2019 11:36:50 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> > > > The reason is that ftrace_module_enable() is called after the module
> > > > has set its text to read-only. There's subtle reasons that this needs
> > > > to be called afterward, and we need to continue to do so.  
> > > 
> > > Please explain.  

I knew you were going to say that ;-)

I was too tired last night to go back and see all the issues.

> > 
> > I don't see any reason what so ever..
> > 
> > load_module()
> >   ...
> >   complete_formation()
> >     mutex_lock(&module_mutex);
> >     ...
> >     module_enable_ro();
> >     module_enable_nx();
> >     module_enable_x();
> > 
> >     mod->state = MODULE_STATE_COMING;
> >     mutex_unlock(&module_mutex);
> > 
> >   prepare_coming_module()
> >     ftrace_module_enable();
> >     ...
> > 
> > IOW, we're doing ftrace_module_enable() immediately after we flip it
> > RO+X. There is nothing in between that we can possibly rely on.

One reason for the above is the module_mutex. The lock order is that
module_mutex may be called inside the ftrace_lock, but not vice-versa.

The ftrace_module_init() was called due to the setting of all modules
rw when registering a ftrace function while a module was being loaded.
We may have eliminated this issue on x86 but other archs still call
set_all_modules_text_rw/ro() when enabling function tracing. Thus, the
race will still exist there.

See commit a949ae560a511 for the description of it.

After implementing that commit, I found it a bit cleaner to handle
modules in general by breaking it up into setting nops first, and then
determining if we need to trace that module.


> > 
> > I was going to put:
> > 
> >   blocking_notifier_call_chain(&module_notify_list,
> > 			       MODULE_STATE_UNFORMED, mod);
> > 
> > right before module_enable_ro(), in complete_formation(), for jump_label
> > and static_call. It looks like ftrace (and possibly klp) want that too.  
> 
> Also, you already have ftrace_module_init() right before that. The only
> thing inbetween ftrace_module_init() and ftrace_module_enable() is
> verify_exported_symbols() and module_bug_finalize().

Yep, see commit a949ae560a511 about that too.

> 
> Do you really need that for patching stuff?

Because arm and nds32 still use the set_all_modules_text_rw(), this
patch would at least remove that for all archs, and only modify the
text of a module that isn't running yet. Which I thought was a plus.

Just need to be careful about other archs, or we need to at least make
sure they change too.

-- Steve

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Steven Rostedt]

On Wed, 9 Oct 2019 22:36:38 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2029,6 +2029,37 @@ static void module_enable_nx(const struct module *mod)
>  	frob_writable_data(&mod->init_layout, set_memory_nx);
>  }
>  

Also, if you are worried about these being used anywhere else, we can
add:

> +void set_module_text_rw(const struct module *mod)
> +{
> +	if (!rodata_enabled)
> +		return;
> +
> +	mutex_lock(&module_mutex);
> +	if (mod->state == MODULE_STATE_UNFORMED)

	if (mod->state == MODULE_STATE_UNFORMED ||
	    mod->state == MODULE_STATE_LIVE)

As this will keep it from being used on modules that are executing.

-- Steve


> +		goto out;
> +
> +	frob_text(&mod->core_layout, set_memory_rw);
> +	frob_text(&mod->init_layout, set_memory_rw);
> +out:
> +	mutex_unlock(&module_mutex);
> +}
> +
> +void set_module_text_ro(const struct module *mod)
> +{
> +	if (!rodata_enabled)
> +		return;
> +
> +	mutex_lock(&module_mutex);
> +	if (mod->state == MODULE_STATE_UNFORMED ||
> +	    mod->state == MODULE_STATE_GOING)
> +		goto out;
> +
> +	frob_text(&mod->core_layout, set_memory_ro);
> +	frob_text(&mod->init_layout, set_memory_ro);
> +out:
> +	mutex_unlock(&module_mutex);
> +}
> +

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Peter Zijlstra]

On Thu, Oct 10, 2019 at 08:50:11AM -0400, Steven Rostedt wrote:
> On Thu, 10 Oct 2019 11:36:50 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:

> > > load_module()
> > >   ...
> > >   complete_formation()
> > >     mutex_lock(&module_mutex);
> > >     ...
> > >     module_enable_ro();
> > >     module_enable_nx();
> > >     module_enable_x();
> > > 
> > >     mod->state = MODULE_STATE_COMING;
> > >     mutex_unlock(&module_mutex);
> > > 
> > >   prepare_coming_module()
> > >     ftrace_module_enable();
> > >     ...
> > > 
> > > IOW, we're doing ftrace_module_enable() immediately after we flip it
> > > RO+X. There is nothing in between that we can possibly rely on.
> 
> One reason for the above is the module_mutex. The lock order is that
> module_mutex may be called inside the ftrace_lock, but not vice-versa.
> 
> The ftrace_module_init() was called due to the setting of all modules
> rw when registering a ftrace function while a module was being loaded.
> We may have eliminated this issue on x86 but other archs still call
> set_all_modules_text_rw/ro() when enabling function tracing. Thus, the
> race will still exist there.
> 
> See commit a949ae560a511 for the description of it.
> 
> After implementing that commit, I found it a bit cleaner to handle
> modules in general by breaking it up into setting nops first, and then
> determining if we need to trace that module.

I still don't get it. So you do both, the initial NOPs and the CALL
patching from ftrace_module_init().

> > > I was going to put:
> > > 
> > >   blocking_notifier_call_chain(&module_notify_list,
> > > 			       MODULE_STATE_UNFORMED, mod);
> > > 
> > > right before module_enable_ro(), in complete_formation(), for jump_label
> > > and static_call. It looks like ftrace (and possibly klp) want that too.  
> > 
> > Also, you already have ftrace_module_init() right before that. The only
> > thing inbetween ftrace_module_init() and ftrace_module_enable() is
> > verify_exported_symbols() and module_bug_finalize().
> 
> Yep, see commit a949ae560a511 about that too.
> 
> > 
> > Do you really need that for patching stuff?
> 
> Because arm and nds32 still use the set_all_modules_text_rw(), this
> patch would at least remove that for all archs, and only modify the
> text of a module that isn't running yet. Which I thought was a plus.
> 
> Just need to be careful about other archs, or we need to at least make
> sure they change too.

They call that from ftrace_arch_code_modofy_prepare(), and the patch I
just send makes that unused. So all should be good ;-)

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Steven Rostedt]

On Thu, 10 Oct 2019 14:29:09 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Yes, your code is anal about checking the NOPs, so you first have to

Yep, because being paranoid about modifying code is always good ;-)

> write NOPs before you can write CALLs, if it is enabled. But afaict you
> really can do all that from ftrace_module_init(), as long as you do it
> all under the same ftrace_lock section.
> 
> If you have two sections, like now, then there is indeed that race that
> ftrace can get enabled in between, and all the confusion that that
> brings.
> 
> That is, what's fundamentally buggered about something like this?
> 
> ---
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 62a50bf399d6..5f7113f100ce 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5626,6 +5626,48 @@ static int ftrace_process_locs(struct module *mod,
>  	ftrace_update_code(mod, start_pg);
>  	if (!mod)
>  		local_irq_restore(flags);
> +
> +	if (ftrace_disabled || !mod)
> +		goto out_loop;
> +
> +	do_for_each_ftrace_rec(pg, rec) {

[snip]

> +
> +		if (ftrace_start_up && cnt) {
> +			int failed = __ftrace_replace_code(rec, 1);

OK, so basically this moves the enabling of function tracing from
within the ftrace_module_enable() code without releasing the
ftrace_lock mutex.

But we have an issue with the state of the module here, as it is still
set as MODULE_STATE_UNFORMED. Let's look at what happens if we have:


	CPU0				CPU1
	----				----
 echo function > current_tracer
				modprobe foo
				  enable foo functions to be traced
				  (foo function records not disabled)
 echo nop > current_tracer

   disable all functions being
   traced including foo functions

   arch calls set_all_modules_text_rw()
    [skips UNFORMED modules, which foo still is ]

				  set foo's text to read-only
				  foo's state to COMING

   tries to disable foo's functions
   foo's text is read-only

   BUG trying to write to ro text!!!


Like I said, this is very subtle. It may no longer be a bug on x86
with your patches, but it will bug on ARM or anything else that still
uses set_all_modules_text_rw() in the ftrace prepare code.

-- Steve




> +			if (failed) {
> +				ftrace_bug(failed, rec);
> +				goto out_loop;
> +			}
> +		}
> +
> +	} while_for_each_ftrace_rec();
> +
> + out_loop:
> +
>  	ret = 0;
>   out:
>  	mutex_unlock(&ftrace_lock);
> @@ -5793,73 +5835,6 @@ void ftrace_release_mod(struct module *mod)
>  
>  void ftrace_module_enable(struct module *mod)
>  {
> -	struct dyn_ftrace *rec;
> -	struct ftrace_page *pg;
> -
> -	mutex_lock(&ftrace_lock);
> -
> -	if (ftrace_disabled)
> -		goto out_unlock;
> -
> -	/*
> -	 * If the tracing is enabled, go ahead and enable the record.
> -	 *
> -	 * The reason not to enable the record immediately is the
> -	 * inherent check of ftrace_make_nop/ftrace_make_call for
> -	 * correct previous instructions.  Making first the NOP
> -	 * conversion puts the module to the correct state, thus
> -	 * passing the ftrace_make_call check.
> -	 *
> -	 * We also delay this to after the module code already set the
> -	 * text to read-only, as we now need to set it back to read-write
> -	 * so that we can modify the text.
> -	 */
> -	if (ftrace_start_up)
> -		ftrace_arch_code_modify_prepare();
> -
> -	do_for_each_ftrace_rec(pg, rec) {
> -		int cnt;
> -		/*
> -		 * do_for_each_ftrace_rec() is a double loop.
> -		 * module text shares the pg. If a record is
> -		 * not part of this module, then skip this pg,
> -		 * which the "break" will do.
> -		 */
> -		if (!within_module_core(rec->ip, mod) &&
> -		    !within_module_init(rec->ip, mod))
> -			break;
> -
> -		cnt = 0;
> -
> -		/*
> -		 * When adding a module, we need to check if tracers are
> -		 * currently enabled and if they are, and can trace this record,
> -		 * we need to enable the module functions as well as update the
> -		 * reference counts for those function records.
> -		 */
> -		if (ftrace_start_up)
> -			cnt += referenced_filters(rec);
> -
> -		/* This clears FTRACE_FL_DISABLED */
> -		rec->flags = cnt;
> -
> -		if (ftrace_start_up && cnt) {
> -			int failed = __ftrace_replace_code(rec, 1);
> -			if (failed) {
> -				ftrace_bug(failed, rec);
> -				goto out_loop;
> -			}
> -		}
> -
> -	} while_for_each_ftrace_rec();
> -
> - out_loop:
> -	if (ftrace_start_up)
> -		ftrace_arch_code_modify_post_process();
> -
> - out_unlock:
> -	mutex_unlock(&ftrace_lock);
> -
>  	process_cached_mods(mod->name);
>  }
>  

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Steven Rostedt]

On Thu, 10 Oct 2019 10:55:15 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> But we have an issue with the state of the module here, as it is still
> set as MODULE_STATE_UNFORMED. Let's look at what happens if we have:
> 
> 
> 	CPU0				CPU1
> 	----				----
>  echo function > current_tracer
> 				modprobe foo
> 				  enable foo functions to be traced
> 				  (foo function records not disabled)
>  echo nop > current_tracer
> 
>    disable all functions being
>    traced including foo functions
> 
>    arch calls set_all_modules_text_rw()
>     [skips UNFORMED modules, which foo still is ]
> 
> 				  set foo's text to read-only
> 				  foo's state to COMING
> 
>    tries to disable foo's functions
>    foo's text is read-only
> 
>    BUG trying to write to ro text!!!
> 
> 
> Like I said, this is very subtle. It may no longer be a bug on x86
> with your patches, but it will bug on ARM or anything else that still
> uses set_all_modules_text_rw() in the ftrace prepare code.

I guess I should have commented this, but the big reason for the split
between ftrace_module_init() and ftrace_module_enable() is that we add
the nops when the module is still UNFORMED, but we only enable it when
the module is COMING or beyond, and not UNFORMED.

-- Steve

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Steven Rostedt]

On Thu, 10 Oct 2019 10:55:15 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

I'll try to add more detail (mapping to functions)

> 
> OK, so basically this moves the enabling of function tracing from
> within the ftrace_module_enable() code without releasing the
> ftrace_lock mutex.
> 
> But we have an issue with the state of the module here, as it is still
> set as MODULE_STATE_UNFORMED. Let's look at what happens if we have:
> 
> 
> 	CPU0				CPU1
> 	----				----
>  echo function > current_tracer

Just need to know that the above will now have all loaded modules have
their functions being traced. That is ftrace_startup > 0.


> 				modprobe foo
> 				  enable foo functions to be traced
> 				  (foo function records not disabled)

In your code, we have here:
				load_module()
				  foo->state == UNFORMED

				  ftrace_module_init()
				    ftrace_process_locs()
				      mutex_lock(ftrace_lock);
				       __ftrace_replace_code(rec, 1)
				      /* Enabling foo's functions */
				      mutex_unlock(ftrace_lock);

				  

>  echo nop > current_tracer
> 
>    disable all functions being
>    traced including foo functions

ftrace_shutdown()
  ftrace_run_update_code()
    ftrace_arch_code_modify_prepare()

    [ on arm and nds32 ]
> 
>    arch calls set_all_modules_text_rw()
>     [skips UNFORMED modules, which foo still is ]
> 
> 				  set foo's text to read-only
> 				  foo's state to COMING

				  complete_formation()
				    module_enable_ro()

> 
>    tries to disable foo's functions
>    foo's text is read-only

    arch_ftrace_update_code()
      ftrace_modify_all_code()

      [ Includes foo's functions ]

> 
>    BUG trying to write to ro text!!!

Hope that makes more sense.

-- Steve

> 
> 
> Like I said, this is very subtle. It may no longer be a bug on x86
> with your patches, but it will bug on ARM or anything else that still
> uses set_all_modules_text_rw() in the ftrace prepare code.

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Peter Zijlstra]

On Thu, Oct 10, 2019 at 10:55:15AM -0400, Steven Rostedt wrote:

> OK, so basically this moves the enabling of function tracing from
> within the ftrace_module_enable() code without releasing the
> ftrace_lock mutex.
> 
> But we have an issue with the state of the module here, as it is still
> set as MODULE_STATE_UNFORMED. Let's look at what happens if we have:
> 
> 
> 	CPU0				CPU1
> 	----				----
>  echo function > current_tracer
> 				modprobe foo
> 				  enable foo functions to be traced
> 				  (foo function records not disabled)
>  echo nop > current_tracer
> 
>    disable all functions being
>    traced including foo functions
> 
>    arch calls set_all_modules_text_rw()
>     [skips UNFORMED modules, which foo still is ]
> 
> 				  set foo's text to read-only
> 				  foo's state to COMING
> 
>    tries to disable foo's functions
>    foo's text is read-only
> 
>    BUG trying to write to ro text!!!
> 
> 
> Like I said, this is very subtle. It may no longer be a bug on x86
> with your patches, but it will bug on ARM or anything else that still
> uses set_all_modules_text_rw() in the ftrace prepare code.

I can't immediately follow, but I think we really should go there.

For now, something like this might work:

--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -34,6 +34,8 @@
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 
+static int ftrace_poke_late = 0;
+
 int ftrace_arch_code_modify_prepare(void)
     __acquires(&text_mutex)
 {
@@ -43,12 +45,15 @@ int ftrace_arch_code_modify_prepare(void
 	 * ftrace has it set to "read/write".
 	 */
 	mutex_lock(&text_mutex);
+	ftrace_poke_late = 1;
 	return 0;
 }
 
 int ftrace_arch_code_modify_post_process(void)
     __releases(&text_mutex)
 {
+	text_poke_finish();
+	ftrace_poke_late = 0;
 	mutex_unlock(&text_mutex);
 	return 0;
 }
@@ -116,7 +121,10 @@ ftrace_modify_code_direct(unsigned long
 		return ret;
 
 	/* replace the text with the new text */
-	text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
+	if (ftrace_poke_late)
+		text_poke_queue((void *)ip, new_code, MCOUNT_INSN_SIZE, NULL);
+	else
+		text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
 	return 0;
 }
 

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Steven Rostedt]

On Thu, 10 Oct 2019 19:01:11 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Oct 10, 2019 at 10:55:15AM -0400, Steven Rostedt wrote:
> 
> > OK, so basically this moves the enabling of function tracing from
> > within the ftrace_module_enable() code without releasing the
> > ftrace_lock mutex.
> > 
> > But we have an issue with the state of the module here, as it is still
> > set as MODULE_STATE_UNFORMED. Let's look at what happens if we have:
> > 
> > 
> > 	CPU0				CPU1
> > 	----				----
> >  echo function > current_tracer
> > 				modprobe foo
> > 				  enable foo functions to be traced
> > 				  (foo function records not disabled)
> >  echo nop > current_tracer
> > 
> >    disable all functions being
> >    traced including foo functions
> > 
> >    arch calls set_all_modules_text_rw()
> >     [skips UNFORMED modules, which foo still is ]
> > 
> > 				  set foo's text to read-only
> > 				  foo's state to COMING
> > 
> >    tries to disable foo's functions
> >    foo's text is read-only
> > 
> >    BUG trying to write to ro text!!!
> > 
> > 
> > Like I said, this is very subtle. It may no longer be a bug on x86
> > with your patches, but it will bug on ARM or anything else that still
> > uses set_all_modules_text_rw() in the ftrace prepare code.  
> 
> I can't immediately follow, but I think we really should go there.
> 
> For now, something like this might work:

Hmm, I'm lost at what the below is trying to do with respect to the
above.

-- Steve

> 
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -34,6 +34,8 @@
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  
> +static int ftrace_poke_late = 0;
> +
>  int ftrace_arch_code_modify_prepare(void)
>      __acquires(&text_mutex)
>  {
> @@ -43,12 +45,15 @@ int ftrace_arch_code_modify_prepare(void
>  	 * ftrace has it set to "read/write".
>  	 */
>  	mutex_lock(&text_mutex);
> +	ftrace_poke_late = 1;
>  	return 0;
>  }
>  
>  int ftrace_arch_code_modify_post_process(void)
>      __releases(&text_mutex)
>  {
> +	text_poke_finish();
> +	ftrace_poke_late = 0;
>  	mutex_unlock(&text_mutex);
>  	return 0;
>  }
> @@ -116,7 +121,10 @@ ftrace_modify_code_direct(unsigned long
>  		return ret;
>  
>  	/* replace the text with the new text */
> -	text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
> +	if (ftrace_poke_late)
> +		text_poke_queue((void *)ip, new_code, MCOUNT_INSN_SIZE, NULL);
> +	else
> +		text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
>  	return 0;
>  }
>  

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Peter Zijlstra]

On Thu, Oct 10, 2019 at 01:20:13PM -0400, Steven Rostedt wrote:
> Hmm, I'm lost at what the below is trying to do with respect to the
> above.

The below is an alternative approach for the module load issue. It
accepts we patch 'late' and then uses text_poke_bp().

It works. We can then look at moving all that patching to
ftrace_module_init() later when we figure out how to do it across
architectures.

> > --- a/arch/x86/kernel/ftrace.c
> > +++ b/arch/x86/kernel/ftrace.c
> > @@ -34,6 +34,8 @@
> >  
> >  #ifdef CONFIG_DYNAMIC_FTRACE
> >  
> > +static int ftrace_poke_late = 0;
> > +
> >  int ftrace_arch_code_modify_prepare(void)
> >      __acquires(&text_mutex)
> >  {
> > @@ -43,12 +45,15 @@ int ftrace_arch_code_modify_prepare(void
> >  	 * ftrace has it set to "read/write".
> >  	 */
> >  	mutex_lock(&text_mutex);
> > +	ftrace_poke_late = 1;
> >  	return 0;
> >  }
> >  
> >  int ftrace_arch_code_modify_post_process(void)
> >      __releases(&text_mutex)
> >  {
> > +	text_poke_finish();
> > +	ftrace_poke_late = 0;
> >  	mutex_unlock(&text_mutex);
> >  	return 0;
> >  }
> > @@ -116,7 +121,10 @@ ftrace_modify_code_direct(unsigned long
> >  		return ret;
> >  
> >  	/* replace the text with the new text */
> > -	text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
> > +	if (ftrace_poke_late)
> > +		text_poke_queue((void *)ip, new_code, MCOUNT_INSN_SIZE, NULL);
> > +	else
> > +		text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
> >  	return 0;
> >  }
> >  
> 

Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Jessica Yu]

+++ Steven Rostedt [10/10/19 08:58 -0400]:
>On Wed, 9 Oct 2019 22:36:38 -0400
>Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -2029,6 +2029,37 @@ static void module_enable_nx(const struct module *mod)
>>  	frob_writable_data(&mod->init_layout, set_memory_nx);
>>  }
>>
>
>Also, if you are worried about these being used anywhere else, we can
>add:
>
>> +void set_module_text_rw(const struct module *mod)
>> +{
>> +	if (!rodata_enabled)
>> +		return;
>> +
>> +	mutex_lock(&module_mutex);
>> +	if (mod->state == MODULE_STATE_UNFORMED)
>
>	if (mod->state == MODULE_STATE_UNFORMED ||
>	    mod->state == MODULE_STATE_LIVE)
>
>As this will keep it from being used on modules that are executing.

Yeah, that'd be good. Aside from the big ftrace_module_init/enable
debate, I'm fine with this patch itself (with the change above), feel
free to include my Ack in case you want to include it with the rest of
the ftrace text_poke stuff.

Acked-by: Jessica Yu <jeyu@kernel.org>

Thanks,

Jessica

>> +		goto out;
>> +
>> +	frob_text(&mod->core_layout, set_memory_rw);
>> +	frob_text(&mod->init_layout, set_memory_rw);
>> +out:
>> +	mutex_unlock(&module_mutex);
>> +}
>> +
>> +void set_module_text_ro(const struct module *mod)
>> +{
>> +	if (!rodata_enabled)
>> +		return;
>> +
>> +	mutex_lock(&module_mutex);
>> +	if (mod->state == MODULE_STATE_UNFORMED ||
>> +	    mod->state == MODULE_STATE_GOING)
>> +		goto out;
>> +
>> +	frob_text(&mod->core_layout, set_memory_ro);
>> +	frob_text(&mod->init_layout, set_memory_ro);
>> +out:
>> +	mutex_unlock(&module_mutex);
>> +}
>> +