lkml.org 
[lkml]   [2019]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write
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

\
 
 \ /
  Last update: 2019-10-10 14:50    [W:0.071 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site