mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: "Qiu, PeiyangX" <peiyangx.qiu@intel.com>
To: linux-kernel@vger.kernel.org
Cc: mingo@redhat.com, rostedt@goodmis.org, yanmin_zhang@linux.intel.com
Subject: [PATCH] ftrace: fix race between ftrace and insmod
Date: Mon, 14 Dec 2015 11:16:18 +0800	[thread overview]
Message-ID: <566E3482.1090008@intel.com> (raw)
In-Reply-To: <566E3076.5080708@intel.com>

We hit ftrace_bug report when booting Android on a 64bit ATOM SOC chip.
Basically, there is a race between insmod and ftrace_run_update_code.

After load_module=>ftrace_module_init, another thread jumps in to call
ftrace_run_update_code=>ftrace_arch_code_modify_prepare
                         =>set_all_modules_text_rw, to change all modules
as RW. Since the new module is at MODULE_STATE_UNFORMED, the text attribute
is not changed. Then, the 2nd thread goes ahead to change codes.
However, load_module continues to call 
complete_formation=>set_section_ro_nx,
then 2nd thread would fail when probing the module's TEXT.

Below patch tries to resolve it.

commit a949ae560a511fe4e3adf48fa44fefded93e5c2b
Author: Steven Rostedt (Red Hat) <rostedt@goodmis.org>
Date:   Thu Apr 24 10:40:12 2014 -0400

     ftrace/module: Hardcode ftrace_module_init() call into load_module()

But it can't fully resolve the issue.

THis patch holds ftrace_mutex across ftrace_module_init and
complete_formation.

Signed-off-by: Qiu Peiyang <peiyangx.qiu@intel.com>
Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
---
  include/linux/ftrace.h |  6 ++++--
  kernel/module.c        |  3 ++-
  kernel/trace/ftrace.c  | 33 ++++++++++++++++++++++-----------
  3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index eae6548..4adc279 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -585,7 +585,8 @@ static inline int ftrace_modify_call(struct 
dyn_ftrace *rec, unsigned long old_a
  extern int ftrace_arch_read_dyn_info(char *buf, int size);

  extern int skip_trace(unsigned long ip);
-extern void ftrace_module_init(struct module *mod);
+extern void ftrace_module_init_start(struct module *mod);
+extern void ftrace_module_init_end(struct module *mod);

  extern void ftrace_disable_daemon(void);
  extern void ftrace_enable_daemon(void);
@@ -595,7 +596,8 @@ static inline int ftrace_force_update(void) { return 
0; }
  static inline void ftrace_disable_daemon(void) { }
  static inline void ftrace_enable_daemon(void) { }
  static inline void ftrace_release_mod(struct module *mod) {}
-static inline void ftrace_module_init(struct module *mod) {}
+static inline void ftrace_module_init_start(struct module *mod) {}
+static inline void ftrace_module_init_end(struct module *mod) {}
  static inline __init int register_ftrace_command(struct 
ftrace_func_command *cmd)
  {
      return -EINVAL;
diff --git a/kernel/module.c b/kernel/module.c
index 8f051a1..324c5c6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3506,10 +3506,11 @@ static int load_module(struct load_info *info, 
const char __user *uargs,
      dynamic_debug_setup(info->debug, info->num_debug);

      /* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
-    ftrace_module_init(mod);
+    ftrace_module_init_start(mod);

      /* Finally it's fully formed, ready to start executing. */
      err = complete_formation(mod, info);
+    ftrace_module_init_end(mod);
      if (err)
          goto ddebug_cleanup;

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 3f743b1..436c199 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4795,7 +4795,7 @@ static int ftrace_cmp_ips(const void *a, const 
void *b)
      return 0;
  }

-static int ftrace_process_locs(struct module *mod,
+static int ftrace_process_locs_nolock(struct module *mod,
                     unsigned long *start,
                     unsigned long *end)
  {
@@ -4820,8 +4820,6 @@ static int ftrace_process_locs(struct module *mod,
      if (!start_pg)
          return -ENOMEM;

-    mutex_lock(&ftrace_lock);
-
      /*
       * Core and each module needs their own pages, as
       * modules will free them when they are removed.
@@ -4889,8 +4887,18 @@ static int ftrace_process_locs(struct module *mod,
          local_irq_restore(flags);
      ret = 0;
   out:
-    mutex_unlock(&ftrace_lock);
+    return ret;
+}

+static int ftrace_process_locs(struct module *mod,
+                   unsigned long *start,
+                   unsigned long *end)
+{
+    int ret;
+
+    mutex_lock(&ftrace_lock);
+    ret = ftrace_process_locs_nolock(mod, start, end);
+    mutex_unlock(&ftrace_lock);
      return ret;
  }

@@ -4940,19 +4948,22 @@ void ftrace_release_mod(struct module *mod)
      mutex_unlock(&ftrace_lock);
  }

-static void ftrace_init_module(struct module *mod,
-                   unsigned long *start, unsigned long *end)
+void ftrace_module_init_start(struct module *mod)
  {
+    unsigned long *start = mod->ftrace_callsites;
+    unsigned long *end = mod->ftrace_callsites + mod->num_ftrace_callsites;
+
+    mutex_lock(&ftrace_lock);
+
      if (ftrace_disabled || start == end)
          return;
-    ftrace_process_locs(mod, start, end);
+
+    ftrace_process_locs_nolock(mod, start, end);
  }

-void ftrace_module_init(struct module *mod)
+void ftrace_module_init_end(struct module *mod)
  {
-    ftrace_init_module(mod, mod->ftrace_callsites,
-               mod->ftrace_callsites +
-               mod->num_ftrace_callsites);
+    mutex_unlock(&ftrace_lock);
  }

  static int ftrace_module_notify_exit(struct notifier_block *self,
-- 
1.9.1


  reply	other threads:[~2015-12-14  3:16 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <S1751824AbbLNCkP/20151214024015Z+230@vger.kernel.org>
2015-12-14  2:59 ` Qiu, PeiyangX
2015-12-14  3:16   ` Qiu, PeiyangX [this message]
2015-12-14 15:51     ` Steven Rostedt
2015-12-15  1:05       ` Zhang, Yanmin
2015-12-15  3:26         ` Zhang, Yanmin
2015-12-15 17:37           ` Steven Rostedt
2015-12-16  0:54             ` Zhang, Yanmin
2015-12-16 10:28             ` Zhang, Yanmin
2015-12-16 14:28               ` Steven Rostedt
2015-12-17  6:48                 ` Zhang, Yanmin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=566E3482.1090008@intel.com \
    --to=peiyangx.qiu@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=rostedt@goodmis.org \
    --cc=yanmin_zhang@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome