All of lore.kernel.org
 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 ` [PATCH] ftrace: fix race between ftrace and insmod 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.