Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 10 Oct 2019 11:36:50 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     LKML <linux-kernel@vger.kernel.org>, Jessica Yu <jeyu@kernel.org>,
        Ingo Molnar <mingo@kernel.org>
Subject: Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module
 text read-write
Message-ID: <20191010093650.GJ2359@hirez.programming.kicks-ass.net>
References: <20191009223638.60b78727@oasis.local.home>
 <20191010073121.GN2311@hirez.programming.kicks-ass.net>
 <20191010093329.GI2359@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191010093329.GI2359@hirez.programming.kicks-ass.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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?