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 16:11:07 +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: <20191010141107.GU2311@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>
 <20191010093650.GJ2359@hirez.programming.kicks-ass.net>
 <20191010085011.3734d87b@gandalf.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191010085011.3734d87b@gandalf.local.home>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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