[PATCH v5 0/6] arch/x86: kprobes: Remove MODULES dependency

[PATCH v5 0/6] arch/x86: kprobes: Remove MODULES dependency

[Jarkko Sakkinen]

Remove MODULES dependency by migrating from module_alloc() to the new
text_alloc() API. Essentially these changes provide preliminaries for
allowing to compile a static kernel with a proper tracing support.

The same API can be used later on in other sites that allocate space for
trampolines, and trivially scaled to other arch's. An arch can inform
with CONFIG_ARCH_HAS_TEXT_ALLOC that it's providing implementation for
text_alloc().

Cc: linux-mm@kvack.org
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>

v4:
* Squash lock_modules() patches into one.
* Remove fallback versions of text_alloc() and text_free(). Instead, use
  ARCH_HAS_TEXT_ALLOC at site when required.
* Use lockdep_assert_irqs_enabled() in text_free() instead of
  WARN_ON(in_interrupt()).

v3:
* Make text_alloc() API disjoint.
* Remove all the possible extra clutter not absolutely required and
  split into more logical pieces.

Jarkko Sakkinen (6):
  kprobes: Remove dependency to the module_mutex
  vmalloc: Add text_alloc() and text_free()
  arch/x86: Implement text_alloc() and text_free()
  arch/x86: kprobes: Use text_alloc() and text_free()
  kprobes: Use text_alloc() and text_free()
  kprobes: Remove CONFIG_MODULES dependency

 arch/Kconfig                   |  2 +-
 arch/x86/Kconfig               |  3 ++
 arch/x86/kernel/Makefile       |  1 +
 arch/x86/kernel/kprobes/core.c |  4 +--
 arch/x86/kernel/text_alloc.c   | 41 +++++++++++++++++++++++
 include/linux/module.h         | 32 ++++++++++++++----
 include/linux/vmalloc.h        | 17 ++++++++++
 kernel/kprobes.c               | 61 +++++++++++++++++++++++-----------
 kernel/trace/trace_kprobe.c    | 20 ++++++++---
 9 files changed, 147 insertions(+), 34 deletions(-)
 create mode 100644 arch/x86/kernel/text_alloc.c

-- 
2.25.1

[PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Jarkko Sakkinen]

Add lock_modules() and unlock_modules() wrappers for acquiring module_mutex
in order to remove the compile time dependency to it.

Cc: linux-mm@kvack.org
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 include/linux/module.h      | 18 ++++++++++++++++++
 kernel/kprobes.c            |  4 ++--
 kernel/trace/trace_kprobe.c |  4 ++--
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 2e6670860d27..8850b9692b8f 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -705,6 +705,16 @@ static inline bool is_livepatch_module(struct module *mod)
 bool is_module_sig_enforced(void);
 void set_module_sig_enforced(void);
 
+static inline void lock_modules(void)
+{
+	mutex_lock(&module_mutex);
+}
+
+static inline void unlock_modules(void)
+{
+	mutex_unlock(&module_mutex);
+}
+
 #else /* !CONFIG_MODULES... */
 
 static inline struct module *__module_address(unsigned long addr)
@@ -852,6 +862,14 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr)
 	return ptr;
 }
 
+static inline void lock_modules(void)
+{
+}
+
+static inline void unlock_modules(void)
+{
+}
+
 #endif /* CONFIG_MODULES */
 
 #ifdef CONFIG_SYSFS
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2e97febeef77..4e46d96d4e16 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -564,7 +564,7 @@ static void kprobe_optimizer(struct work_struct *work)
 	cpus_read_lock();
 	mutex_lock(&text_mutex);
 	/* Lock modules while optimizing kprobes */
-	mutex_lock(&module_mutex);
+	lock_modules();
 
 	/*
 	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
@@ -589,7 +589,7 @@ static void kprobe_optimizer(struct work_struct *work)
 	/* Step 4: Free cleaned kprobes after quiesence period */
 	do_free_cleaned_kprobes();
 
-	mutex_unlock(&module_mutex);
+	unlock_modules();
 	mutex_unlock(&text_mutex);
 	cpus_read_unlock();
 
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index aefb6065b508..710ec6a6aa8f 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -122,9 +122,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
 	if (!p)
 		return true;
 	*p = '\0';
-	mutex_lock(&module_mutex);
+	lock_modules();
 	ret = !!find_module(tk->symbol);
-	mutex_unlock(&module_mutex);
+	unlock_modules();
 	*p = ':';
 
 	return ret;
-- 
2.25.1

[PATCH v5 2/6] vmalloc: Add text_alloc() and text_free()

[Jarkko Sakkinen]

Introduce functions for allocating memory for dynamic trampolines, such
as kprobes. An arch can promote the availability of these functions with
CONFIG_ARCH_HAS_TEXT_ALLOC.

Cc: linux-mm@kvack.org
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 include/linux/vmalloc.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 0221f852a7e1..6c151a0ac02a 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -249,4 +249,21 @@ pcpu_free_vm_areas(struct vm_struct **vms, int nr_vms)
 int register_vmap_purge_notifier(struct notifier_block *nb);
 int unregister_vmap_purge_notifier(struct notifier_block *nb);
 
+/*
+ * These functions can be optionally implemented by an arch in order to
+ * provide specialized functions for allocating trampoline code.
+ */
+
+#ifdef CONFIG_ARCH_HAS_TEXT_ALLOC
+/*
+ * Allocate memory to be used for trampoline code.
+ */
+void *text_alloc(unsigned long size);
+
+/*
+ * Free memory returned from text_alloc().
+ */
+void text_free(void *region);
+#endif /* CONFIG_ARCH_HAS_TEXT_ALLOC */
+
 #endif /* _LINUX_VMALLOC_H */
-- 
2.25.1

[PATCH v5 3/6] arch/x86: Implement text_alloc() and text_free()

[Jarkko Sakkinen]

Implement text_alloc() and text_free() with with the vmalloc API. These can
be used to allocate pages for trampoline code without relying on the module
loader code.

Cc: linux-mm@kvack.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Andi Kleen <ak@linux.intel.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/Kconfig             |  3 +++
 arch/x86/kernel/Makefile     |  1 +
 arch/x86/kernel/text_alloc.c | 41 ++++++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+)
 create mode 100644 arch/x86/kernel/text_alloc.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0dea7fdd7a00..a4ee5d1300f6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2035,6 +2035,9 @@ config KEXEC_FILE
 config ARCH_HAS_KEXEC_PURGATORY
 	def_bool KEXEC_FILE
 
+config ARCH_HAS_TEXT_ALLOC
+	def_bool y
+
 config KEXEC_SIG
 	bool "Verify kernel signature during kexec_file_load() syscall"
 	depends on KEXEC_FILE
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index e77261db2391..fa1415424ae3 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -68,6 +68,7 @@ obj-y			+= tsc.o tsc_msr.o io_delay.o rtc.o
 obj-y			+= pci-iommu_table.o
 obj-y			+= resource.o
 obj-y			+= irqflags.o
+obj-y			+= text_alloc.o
 
 obj-y				+= process.o
 obj-y				+= fpu/
diff --git a/arch/x86/kernel/text_alloc.c b/arch/x86/kernel/text_alloc.c
new file mode 100644
index 000000000000..f31c2d82c258
--- /dev/null
+++ b/arch/x86/kernel/text_alloc.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  Kernel module help for x86.
+ *  Copyright (C) 2001 Rusty Russell.
+ */
+
+#include <linux/kasan.h>
+#include <linux/mm.h>
+#include <linux/moduleloader.h>
+#include <linux/vmalloc.h>
+#include <asm/setup.h>
+
+void *text_alloc(unsigned long size)
+{
+	void *p;
+
+	if (PAGE_ALIGN(size) > MODULES_LEN)
+		return NULL;
+
+	p = __vmalloc_node_range(size, MODULE_ALIGN, MODULES_VADDR, MODULES_END,
+				 GFP_KERNEL, PAGE_KERNEL, 0, NUMA_NO_NODE,
+				 __builtin_return_address(0));
+
+	if (p && (kasan_module_alloc(p, size) < 0)) {
+		vfree(p);
+		return NULL;
+	}
+
+	return p;
+}
+
+void text_free(void *region)
+{
+	/*
+	 * This memory may be RO, and freeing RO memory in an interrupt is not
+	 * supported by vmalloc.
+	 */
+	lockdep_assert_irqs_enabled();
+
+	vfree(region);
+}
-- 
2.25.1

[PATCH v5 0/6] arch/x86: kprobes: Remove MODULES dependency

[Jarkko Sakkinen]

Remove MODULES dependency by migrating from module_alloc() to the new
text_alloc() API. Essentially these changes provide preliminaries for
allowing to compile a static kernel with a proper tracing support.

The same API can be used later on in other sites that allocate space for
trampolines, and trivially scaled to other arch's. An arch can inform
with CONFIG_ARCH_HAS_TEXT_ALLOC that it's providing implementation for
text_alloc().

Cc: linux-mm@kvack.org
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>

v4:
* Squash lock_modules() patches into one.
* Remove fallback versions of text_alloc() and text_free(). Instead, use
  ARCH_HAS_TEXT_ALLOC at site when required.
* Use lockdep_assert_irqs_enabled() in text_free() instead of
  WARN_ON(in_interrupt()).

v3:
* Make text_alloc() API disjoint.
* Remove all the possible extra clutter not absolutely required and
  split into more logical pieces.

Jarkko Sakkinen (6):
  kprobes: Remove dependency to the module_mutex
  vmalloc: Add text_alloc() and text_free()
  arch/x86: Implement text_alloc() and text_free()
  arch/x86: kprobes: Use text_alloc() and text_free()
  kprobes: Use text_alloc() and text_free()
  kprobes: Remove CONFIG_MODULES dependency

 arch/Kconfig                   |  2 +-
 arch/x86/Kconfig               |  3 ++
 arch/x86/kernel/Makefile       |  1 +
 arch/x86/kernel/kprobes/core.c |  4 +--
 arch/x86/kernel/text_alloc.c   | 41 +++++++++++++++++++++++
 include/linux/module.h         | 32 ++++++++++++++----
 include/linux/vmalloc.h        | 17 ++++++++++
 kernel/kprobes.c               | 61 +++++++++++++++++++++++-----------
 kernel/trace/trace_kprobe.c    | 20 ++++++++---
 9 files changed, 147 insertions(+), 34 deletions(-)
 create mode 100644 arch/x86/kernel/text_alloc.c

-- 
2.25.1

[PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Jarkko Sakkinen]

Add lock_modules() and unlock_modules() wrappers for acquiring module_mutex
in order to remove the compile time dependency to it.

Cc: linux-mm@kvack.org
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 include/linux/module.h      | 18 ++++++++++++++++++
 kernel/kprobes.c            |  4 ++--
 kernel/trace/trace_kprobe.c |  4 ++--
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 2e6670860d27..8850b9692b8f 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -705,6 +705,16 @@ static inline bool is_livepatch_module(struct module *mod)
 bool is_module_sig_enforced(void);
 void set_module_sig_enforced(void);
 
+static inline void lock_modules(void)
+{
+	mutex_lock(&module_mutex);
+}
+
+static inline void unlock_modules(void)
+{
+	mutex_unlock(&module_mutex);
+}
+
 #else /* !CONFIG_MODULES... */
 
 static inline struct module *__module_address(unsigned long addr)
@@ -852,6 +862,14 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr)
 	return ptr;
 }
 
+static inline void lock_modules(void)
+{
+}
+
+static inline void unlock_modules(void)
+{
+}
+
 #endif /* CONFIG_MODULES */
 
 #ifdef CONFIG_SYSFS
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2e97febeef77..4e46d96d4e16 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -564,7 +564,7 @@ static void kprobe_optimizer(struct work_struct *work)
 	cpus_read_lock();
 	mutex_lock(&text_mutex);
 	/* Lock modules while optimizing kprobes */
-	mutex_lock(&module_mutex);
+	lock_modules();
 
 	/*
 	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
@@ -589,7 +589,7 @@ static void kprobe_optimizer(struct work_struct *work)
 	/* Step 4: Free cleaned kprobes after quiesence period */
 	do_free_cleaned_kprobes();
 
-	mutex_unlock(&module_mutex);
+	unlock_modules();
 	mutex_unlock(&text_mutex);
 	cpus_read_unlock();
 
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index aefb6065b508..710ec6a6aa8f 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -122,9 +122,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
 	if (!p)
 		return true;
 	*p = '\0';
-	mutex_lock(&module_mutex);
+	lock_modules();
 	ret = !!find_module(tk->symbol);
-	mutex_unlock(&module_mutex);
+	unlock_modules();
 	*p = ':';
 
 	return ret;
-- 
2.25.1

Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Ingo Molnar]

* Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> Add lock_modules() and unlock_modules() wrappers for acquiring module_mutex
> in order to remove the compile time dependency to it.
> 
> Cc: linux-mm@kvack.org
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  include/linux/module.h      | 18 ++++++++++++++++++
>  kernel/kprobes.c            |  4 ++--
>  kernel/trace/trace_kprobe.c |  4 ++--
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 2e6670860d27..8850b9692b8f 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -705,6 +705,16 @@ static inline bool is_livepatch_module(struct module *mod)
>  bool is_module_sig_enforced(void);
>  void set_module_sig_enforced(void);
>  
> +static inline void lock_modules(void)
> +{
> +	mutex_lock(&module_mutex);
> +}
> +
> +static inline void unlock_modules(void)
> +{
> +	mutex_unlock(&module_mutex);
> +}
> +
>  #else /* !CONFIG_MODULES... */
>  
>  static inline struct module *__module_address(unsigned long addr)
> @@ -852,6 +862,14 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr)
>  	return ptr;
>  }
>  
> +static inline void lock_modules(void)
> +{
> +}
> +
> +static inline void unlock_modules(void)
> +{
> +}

Minor namespace nit: when introducing new locking wrappers please 
standardize on the XYZ_lock()/XYZ_unlock() nomenclature, i.e.:

	modules_lock()
	...
	modules_unlock()

Similarly to the mutex_lock/unlock(&module_mutex) API that it is 
wrapping.

Also, JFYI, the overwhelming majority of the modules related APIs use 
module_*(), i.e. singular - not plural, so 
module_lock()/module_unlock() would be the more canonical choice. 
(But both sound fine to me)

Thanks,

	Ingo

Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Jarkko Sakkinen]

On Fri, Jul 24, 2020 at 11:13:19AM +0200, Ingo Molnar wrote:
> 
> * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
> > Add lock_modules() and unlock_modules() wrappers for acquiring module_mutex
> > in order to remove the compile time dependency to it.
> > 
> > Cc: linux-mm@kvack.org
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  include/linux/module.h      | 18 ++++++++++++++++++
> >  kernel/kprobes.c            |  4 ++--
> >  kernel/trace/trace_kprobe.c |  4 ++--
> >  3 files changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 2e6670860d27..8850b9692b8f 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -705,6 +705,16 @@ static inline bool is_livepatch_module(struct module *mod)
> >  bool is_module_sig_enforced(void);
> >  void set_module_sig_enforced(void);
> >  
> > +static inline void lock_modules(void)
> > +{
> > +	mutex_lock(&module_mutex);
> > +}
> > +
> > +static inline void unlock_modules(void)
> > +{
> > +	mutex_unlock(&module_mutex);
> > +}
> > +
> >  #else /* !CONFIG_MODULES... */
> >  
> >  static inline struct module *__module_address(unsigned long addr)
> > @@ -852,6 +862,14 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr)
> >  	return ptr;
> >  }
> >  
> > +static inline void lock_modules(void)
> > +{
> > +}
> > +
> > +static inline void unlock_modules(void)
> > +{
> > +}
> 
> Minor namespace nit: when introducing new locking wrappers please 
> standardize on the XYZ_lock()/XYZ_unlock() nomenclature, i.e.:
> 
> 	modules_lock()
> 	...
> 	modules_unlock()
> 
> Similarly to the mutex_lock/unlock(&module_mutex) API that it is 
> wrapping.
> 
> Also, JFYI, the overwhelming majority of the modules related APIs use 
> module_*(), i.e. singular - not plural, so 
> module_lock()/module_unlock() would be the more canonical choice. 
> (But both sound fine to me)

Thanks, I renamed them as module_lock() and module_unlock().

> Thanks,
> 
> 	Ingo

/Jarkko

Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Ingo Molnar]

* Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -564,7 +564,7 @@ static void kprobe_optimizer(struct work_struct *work)
>  	cpus_read_lock();
>  	mutex_lock(&text_mutex);
>  	/* Lock modules while optimizing kprobes */
> -	mutex_lock(&module_mutex);
> +	lock_modules();
>  
>  	/*
>  	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> @@ -589,7 +589,7 @@ static void kprobe_optimizer(struct work_struct *work)
>  	/* Step 4: Free cleaned kprobes after quiesence period */
>  	do_free_cleaned_kprobes();
>  
> -	mutex_unlock(&module_mutex);
> +	unlock_modules();
>  	mutex_unlock(&text_mutex);
>  	cpus_read_unlock();

BTW., it would be nice to expand on the comments above - exactly which 
parts of the modules code is being serialized against and why?

We already hold the text_mutex here, which should protect against most 
kprobes related activities interfering - and it's unclear (to me) 
which part of the modules code is being serialized with here, and the 
'lock modules while optimizing kprobes' comments is unhelpful. :-)

Thanks,

	Ingo

Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Jarkko Sakkinen]

On Fri, Jul 24, 2020 at 11:17:11AM +0200, Ingo Molnar wrote:
> 
> * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -564,7 +564,7 @@ static void kprobe_optimizer(struct work_struct *work)
> >  	cpus_read_lock();
> >  	mutex_lock(&text_mutex);
> >  	/* Lock modules while optimizing kprobes */
> > -	mutex_lock(&module_mutex);
> > +	lock_modules();
> >  
> >  	/*
> >  	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> > @@ -589,7 +589,7 @@ static void kprobe_optimizer(struct work_struct *work)
> >  	/* Step 4: Free cleaned kprobes after quiesence period */
> >  	do_free_cleaned_kprobes();
> >  
> > -	mutex_unlock(&module_mutex);
> > +	unlock_modules();
> >  	mutex_unlock(&text_mutex);
> >  	cpus_read_unlock();
> 
> BTW., it would be nice to expand on the comments above - exactly which 
> parts of the modules code is being serialized against and why?
> 
> We already hold the text_mutex here, which should protect against most 
> kprobes related activities interfering - and it's unclear (to me) 
> which part of the modules code is being serialized with here, and the 
> 'lock modules while optimizing kprobes' comments is unhelpful. :-)
> 
> Thanks,
> 
> 	Ingo

AFAIK, only if you need to call find_module(), you ever need to acquire
this mutex. 99% of time it is internally taken care by kernel/module.c.

I cannot make up any obvious reason to acquire it here.

/Jarkko

Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Ingo Molnar]

* Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> On Fri, Jul 24, 2020 at 11:17:11AM +0200, Ingo Molnar wrote:
> > 
> > * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > 
> > > --- a/kernel/kprobes.c
> > > +++ b/kernel/kprobes.c
> > > @@ -564,7 +564,7 @@ static void kprobe_optimizer(struct work_struct *work)
> > >  	cpus_read_lock();
> > >  	mutex_lock(&text_mutex);
> > >  	/* Lock modules while optimizing kprobes */
> > > -	mutex_lock(&module_mutex);
> > > +	lock_modules();
> > >  
> > >  	/*
> > >  	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> > > @@ -589,7 +589,7 @@ static void kprobe_optimizer(struct work_struct *work)
> > >  	/* Step 4: Free cleaned kprobes after quiesence period */
> > >  	do_free_cleaned_kprobes();
> > >  
> > > -	mutex_unlock(&module_mutex);
> > > +	unlock_modules();
> > >  	mutex_unlock(&text_mutex);
> > >  	cpus_read_unlock();
> > 
> > BTW., it would be nice to expand on the comments above - exactly which 
> > parts of the modules code is being serialized against and why?
> > 
> > We already hold the text_mutex here, which should protect against most 
> > kprobes related activities interfering - and it's unclear (to me) 
> > which part of the modules code is being serialized with here, and the 
> > 'lock modules while optimizing kprobes' comments is unhelpful. :-)
> > 
> > Thanks,
> > 
> > 	Ingo
> 
> AFAIK, only if you need to call find_module(), you ever need to acquire
> this mutex. 99% of time it is internally taken care by kernel/module.c.
> 
> I cannot make up any obvious reason to acquire it here.

If it's unnecessary, then it needs to be removed.

If it's necessary, then it needs to be documented better.

Thanks,

	Ingo

Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Masami Hiramatsu]

On Sat, 25 Jul 2020 12:21:10 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
> > On Fri, Jul 24, 2020 at 11:17:11AM +0200, Ingo Molnar wrote:
> > > 
> > > * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > 
> > > > --- a/kernel/kprobes.c
> > > > +++ b/kernel/kprobes.c
> > > > @@ -564,7 +564,7 @@ static void kprobe_optimizer(struct work_struct *work)
> > > >  	cpus_read_lock();
> > > >  	mutex_lock(&text_mutex);
> > > >  	/* Lock modules while optimizing kprobes */
> > > > -	mutex_lock(&module_mutex);
> > > > +	lock_modules();
> > > >  
> > > >  	/*
> > > >  	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> > > > @@ -589,7 +589,7 @@ static void kprobe_optimizer(struct work_struct *work)
> > > >  	/* Step 4: Free cleaned kprobes after quiesence period */
> > > >  	do_free_cleaned_kprobes();
> > > >  
> > > > -	mutex_unlock(&module_mutex);
> > > > +	unlock_modules();
> > > >  	mutex_unlock(&text_mutex);
> > > >  	cpus_read_unlock();
> > > 
> > > BTW., it would be nice to expand on the comments above - exactly which 
> > > parts of the modules code is being serialized against and why?
> > > 
> > > We already hold the text_mutex here, which should protect against most 
> > > kprobes related activities interfering - and it's unclear (to me) 
> > > which part of the modules code is being serialized with here, and the 
> > > 'lock modules while optimizing kprobes' comments is unhelpful. :-)
> > > 
> > > Thanks,
> > > 
> > > 	Ingo
> > 
> > AFAIK, only if you need to call find_module(), you ever need to acquire
> > this mutex. 99% of time it is internally taken care by kernel/module.c.
> > 
> > I cannot make up any obvious reason to acquire it here.
> 
> If it's unnecessary, then it needs to be removed.
> 
> If it's necessary, then it needs to be documented better.

Good catch! This is not needed anymore.
It has been introduced to avoid conflict with text modification, at that
point we didn't get text_mutex. But after commit f1c6ece23729 ("kprobes: Fix 
potential deadlock in kprobe_optimizer()") moved the text_mutex in the current
position, we don't need it. (and anyway, keeping kprobe_mutex locked means
any module unloading will be stopped inside kprobes_module_callback())

This may help?

From 2355ecd941c3234b12a6de7443592848ed4e2087 Mon Sep 17 00:00:00 2001
From: Masami Hiramatsu <mhiramat@kernel.org>
Date: Tue, 28 Jul 2020 16:32:34 +0900
Subject: [PATCH] kprobes: Remove unneeded module_mutex lock from the optimizer

Remove unneeded module_mutex locking from the optimizer. Since
we already locks both kprobe_mutex and text_mutex in the optimizer,
text will not be changed and the module unloading will be stopped
inside kprobes_module_callback().

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 4a904cc56d68..d1b02e890793 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -563,8 +563,6 @@ static void kprobe_optimizer(struct work_struct *work)
 	mutex_lock(&kprobe_mutex);
 	cpus_read_lock();
 	mutex_lock(&text_mutex);
-	/* Lock modules while optimizing kprobes */
-	mutex_lock(&module_mutex);
 
 	/*
 	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
@@ -589,7 +587,6 @@ static void kprobe_optimizer(struct work_struct *work)
 	/* Step 4: Free cleaned kprobes after quiesence period */
 	do_free_cleaned_kprobes();
 
-	mutex_unlock(&module_mutex);
 	mutex_unlock(&text_mutex);
 	cpus_read_unlock();
 
-- 
2.25.1
-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Jarkko Sakkinen]

On Tue, Jul 28, 2020 at 04:34:00PM +0900, Masami Hiramatsu wrote:
> On Sat, 25 Jul 2020 12:21:10 +0200
> Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 
> > * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > 
> > > On Fri, Jul 24, 2020 at 11:17:11AM +0200, Ingo Molnar wrote:
> > > > 
> > > > * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > > 
> > > > > --- a/kernel/kprobes.c
> > > > > +++ b/kernel/kprobes.c
> > > > > @@ -564,7 +564,7 @@ static void kprobe_optimizer(struct work_struct *work)
> > > > >  	cpus_read_lock();
> > > > >  	mutex_lock(&text_mutex);
> > > > >  	/* Lock modules while optimizing kprobes */
> > > > > -	mutex_lock(&module_mutex);
> > > > > +	lock_modules();
> > > > >  
> > > > >  	/*
> > > > >  	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> > > > > @@ -589,7 +589,7 @@ static void kprobe_optimizer(struct work_struct *work)
> > > > >  	/* Step 4: Free cleaned kprobes after quiesence period */
> > > > >  	do_free_cleaned_kprobes();
> > > > >  
> > > > > -	mutex_unlock(&module_mutex);
> > > > > +	unlock_modules();
> > > > >  	mutex_unlock(&text_mutex);
> > > > >  	cpus_read_unlock();
> > > > 
> > > > BTW., it would be nice to expand on the comments above - exactly which 
> > > > parts of the modules code is being serialized against and why?
> > > > 
> > > > We already hold the text_mutex here, which should protect against most 
> > > > kprobes related activities interfering - and it's unclear (to me) 
> > > > which part of the modules code is being serialized with here, and the 
> > > > 'lock modules while optimizing kprobes' comments is unhelpful. :-)
> > > > 
> > > > Thanks,
> > > > 
> > > > 	Ingo
> > > 
> > > AFAIK, only if you need to call find_module(), you ever need to acquire
> > > this mutex. 99% of time it is internally taken care by kernel/module.c.
> > > 
> > > I cannot make up any obvious reason to acquire it here.
> > 
> > If it's unnecessary, then it needs to be removed.
> > 
> > If it's necessary, then it needs to be documented better.
> 
> Good catch! This is not needed anymore.
> It has been introduced to avoid conflict with text modification, at that
> point we didn't get text_mutex. But after commit f1c6ece23729 ("kprobes: Fix 
> potential deadlock in kprobe_optimizer()") moved the text_mutex in the current
> position, we don't need it. (and anyway, keeping kprobe_mutex locked means
> any module unloading will be stopped inside kprobes_module_callback())
> 
> This may help?

Hey, thanks a lot. This will help to clean my patch set. I'll send a
follow up version as soon I'm on track with my work.  I have to recall
my set of changes and backtrack some of the discussion.

I was two weeks in vacation and last week had bunch of network
connectivity issues last week. Anyway, enough time for details to fade
away :-)

/Jarkko

> 
> From 2355ecd941c3234b12a6de7443592848ed4e2087 Mon Sep 17 00:00:00 2001
> From: Masami Hiramatsu <mhiramat@kernel.org>
> Date: Tue, 28 Jul 2020 16:32:34 +0900
> Subject: [PATCH] kprobes: Remove unneeded module_mutex lock from the optimizer
> 
> Remove unneeded module_mutex locking from the optimizer. Since
> we already locks both kprobe_mutex and text_mutex in the optimizer,
> text will not be changed and the module unloading will be stopped
> inside kprobes_module_callback().
> 
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/kprobes.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 4a904cc56d68..d1b02e890793 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -563,8 +563,6 @@ static void kprobe_optimizer(struct work_struct *work)
>  	mutex_lock(&kprobe_mutex);
>  	cpus_read_lock();
>  	mutex_lock(&text_mutex);
> -	/* Lock modules while optimizing kprobes */
> -	mutex_lock(&module_mutex);
>  
>  	/*
>  	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> @@ -589,7 +587,6 @@ static void kprobe_optimizer(struct work_struct *work)
>  	/* Step 4: Free cleaned kprobes after quiesence period */
>  	do_free_cleaned_kprobes();
>  
> -	mutex_unlock(&module_mutex);
>  	mutex_unlock(&text_mutex);
>  	cpus_read_unlock();
>  
> -- 
> 2.25.1
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Mike Rapoport]

On Fri, Jul 24, 2020 at 08:05:48AM +0300, Jarkko Sakkinen wrote:
> Add lock_modules() and unlock_modules() wrappers for acquiring module_mutex
> in order to remove the compile time dependency to it.
> 
> Cc: linux-mm@kvack.org
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  include/linux/module.h      | 18 ++++++++++++++++++
>  kernel/kprobes.c            |  4 ++--
>  kernel/trace/trace_kprobe.c |  4 ++--
>  3 files changed, 22 insertions(+), 4 deletions(-)

Any reason to convert only kprobes to the new API and leave others with
opencoded implementation?
 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 2e6670860d27..8850b9692b8f 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -705,6 +705,16 @@ static inline bool is_livepatch_module(struct module *mod)
>  bool is_module_sig_enforced(void);
>  void set_module_sig_enforced(void);
>  
> +static inline void lock_modules(void)
> +{
> +	mutex_lock(&module_mutex);
> +}
> +
> +static inline void unlock_modules(void)
> +{
> +	mutex_unlock(&module_mutex);
> +}
> +
>  #else /* !CONFIG_MODULES... */
>  
>  static inline struct module *__module_address(unsigned long addr)
> @@ -852,6 +862,14 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr)
>  	return ptr;
>  }
>  
> +static inline void lock_modules(void)
> +{
> +}
> +
> +static inline void unlock_modules(void)
> +{
> +}
> +
>  #endif /* CONFIG_MODULES */
>  
>  #ifdef CONFIG_SYSFS
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 2e97febeef77..4e46d96d4e16 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -564,7 +564,7 @@ static void kprobe_optimizer(struct work_struct *work)
>  	cpus_read_lock();
>  	mutex_lock(&text_mutex);
>  	/* Lock modules while optimizing kprobes */
> -	mutex_lock(&module_mutex);
> +	lock_modules();
>  
>  	/*
>  	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> @@ -589,7 +589,7 @@ static void kprobe_optimizer(struct work_struct *work)
>  	/* Step 4: Free cleaned kprobes after quiesence period */
>  	do_free_cleaned_kprobes();
>  
> -	mutex_unlock(&module_mutex);
> +	unlock_modules();
>  	mutex_unlock(&text_mutex);
>  	cpus_read_unlock();
>  
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index aefb6065b508..710ec6a6aa8f 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -122,9 +122,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
>  	if (!p)
>  		return true;
>  	*p = '\0';
> -	mutex_lock(&module_mutex);
> +	lock_modules();
>  	ret = !!find_module(tk->symbol);
> -	mutex_unlock(&module_mutex);
> +	unlock_modules();
>  	*p = ':';
>  
>  	return ret;
> -- 
> 2.25.1
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Jarkko Sakkinen]

On Fri, Jul 24, 2020 at 01:22:58PM +0300, Mike Rapoport wrote:
> On Fri, Jul 24, 2020 at 08:05:48AM +0300, Jarkko Sakkinen wrote:
> > Add lock_modules() and unlock_modules() wrappers for acquiring module_mutex
> > in order to remove the compile time dependency to it.
> > 
> > Cc: linux-mm@kvack.org
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  include/linux/module.h      | 18 ++++++++++++++++++
> >  kernel/kprobes.c            |  4 ++--
> >  kernel/trace/trace_kprobe.c |  4 ++--
> >  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> Any reason to convert only kprobes to the new API and leave others with
> opencoded implementation?

Not anything particular.

Lets see:

$ git --no-pager grep "mutex_lock(&module_mutex)"
arch/powerpc/platforms/powernv/pci-cxl.c:       mutex_lock(&module_mutex);
drivers/gpu/drm/drm_fb_helper.c:        mutex_lock(&module_mutex);
include/linux/module.h: mutex_lock(&module_mutex);
kernel/livepatch/core.c:        mutex_lock(&module_mutex);
kernel/livepatch/core.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);
kernel/module.c:        mutex_lock(&module_mutex);

I could refine this commit to patch these sites. Or should I split it
into multiple?

/Jarkko

Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Masami Hiramatsu]

On Fri, 24 Jul 2020 08:05:48 +0300
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> Add lock_modules() and unlock_modules() wrappers for acquiring module_mutex
> in order to remove the compile time dependency to it.

This subject is a bit confusing. This is just wrapping modules_mutex in
kpprobes. We still have compile time dependency, e.g. module_state, right?

Thank you,

> 
> Cc: linux-mm@kvack.org
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  include/linux/module.h      | 18 ++++++++++++++++++
>  kernel/kprobes.c            |  4 ++--
>  kernel/trace/trace_kprobe.c |  4 ++--
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 2e6670860d27..8850b9692b8f 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -705,6 +705,16 @@ static inline bool is_livepatch_module(struct module *mod)
>  bool is_module_sig_enforced(void);
>  void set_module_sig_enforced(void);
>  
> +static inline void lock_modules(void)
> +{
> +	mutex_lock(&module_mutex);
> +}
> +
> +static inline void unlock_modules(void)
> +{
> +	mutex_unlock(&module_mutex);
> +}
> +
>  #else /* !CONFIG_MODULES... */
>  
>  static inline struct module *__module_address(unsigned long addr)
> @@ -852,6 +862,14 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr)
>  	return ptr;
>  }
>  
> +static inline void lock_modules(void)
> +{
> +}
> +
> +static inline void unlock_modules(void)
> +{
> +}
> +
>  #endif /* CONFIG_MODULES */
>  
>  #ifdef CONFIG_SYSFS
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 2e97febeef77..4e46d96d4e16 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -564,7 +564,7 @@ static void kprobe_optimizer(struct work_struct *work)
>  	cpus_read_lock();
>  	mutex_lock(&text_mutex);
>  	/* Lock modules while optimizing kprobes */
> -	mutex_lock(&module_mutex);
> +	lock_modules();
>  
>  	/*
>  	 * Step 1: Unoptimize kprobes and collect cleaned (unused and disarmed)
> @@ -589,7 +589,7 @@ static void kprobe_optimizer(struct work_struct *work)
>  	/* Step 4: Free cleaned kprobes after quiesence period */
>  	do_free_cleaned_kprobes();
>  
> -	mutex_unlock(&module_mutex);
> +	unlock_modules();
>  	mutex_unlock(&text_mutex);
>  	cpus_read_unlock();
>  
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index aefb6065b508..710ec6a6aa8f 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -122,9 +122,9 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
>  	if (!p)
>  		return true;
>  	*p = '\0';
> -	mutex_lock(&module_mutex);
> +	lock_modules();
>  	ret = !!find_module(tk->symbol);
> -	mutex_unlock(&module_mutex);
> +	unlock_modules();
>  	*p = ':';
>  
>  	return ret;
> -- 
> 2.25.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v5 1/6] kprobes: Remove dependency to the module_mutex

[Jarkko Sakkinen]

On Fri, Jul 24, 2020 at 11:46:31PM +0900, Masami Hiramatsu wrote:
> On Fri, 24 Jul 2020 08:05:48 +0300
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
> > Add lock_modules() and unlock_modules() wrappers for acquiring module_mutex
> > in order to remove the compile time dependency to it.
> 
> This subject is a bit confusing. This is just wrapping modules_mutex in
> kpprobes. We still have compile time dependency, e.g. module_state, right?

Yes. This more like a preliminary change to make that happen. The
actual flagging is in 6/6 ("Remove CONFIG_MODULE dependency").

Maybe a better angle would be to make this update all sites that
deal with module_mutex [*] and base the whole rationale on that?

[*] https://lore.kernel.org/lkml/20200725024227.GD17052@linux.intel.com/

/Jarkko