diff mbox series

[02/13] module: add a module_loaded helper

Message ID 20210121074959.313333-3-hch@lst.de
State Not Applicable
Headers show
Series [01/13] powerpc/powernv: remove get_cxl_module | expand

Checks

Context Check Description
snowpatch_ozlabs/needsstable success Patch has no Fixes tags
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 59 lines checked
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (41d8cb7ece7c81e4eb897ed7ec7d3c3d72fd0af4)

Commit Message

Christoph Hellwig Jan. 21, 2021, 7:49 a.m. UTC
Add a helper that takes modules_mutex and uses find_module to check if a
given module is loaded.  This provides a better abstraction for the two
callers, and allows to unexport modules_mutex and find_module.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/drm_fb_helper.c |  7 +------
 include/linux/module.h          |  3 +++
 kernel/module.c                 | 14 ++++++++++++--
 kernel/trace/trace_kprobe.c     |  4 +---
 4 files changed, 17 insertions(+), 11 deletions(-)

Comments

Christophe Leroy Jan. 21, 2021, 9:59 a.m. UTC | #1
Le 21/01/2021 à 08:49, Christoph Hellwig a écrit :
> Add a helper that takes modules_mutex and uses find_module to check if a
> given module is loaded.  This provides a better abstraction for the two
> callers, and allows to unexport modules_mutex and find_module.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/gpu/drm/drm_fb_helper.c |  7 +------
>   include/linux/module.h          |  3 +++
>   kernel/module.c                 | 14 ++++++++++++--
>   kernel/trace/trace_kprobe.c     |  4 +---
>   4 files changed, 17 insertions(+), 11 deletions(-)
> 

> diff --git a/include/linux/module.h b/include/linux/module.h
> index 7a0bcb5b1ffccd..b4654f8a408134 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -589,6 +589,9 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
>   /* Search for module by name: must hold module_mutex. */
>   struct module *find_module(const char *name);
>   
> +/* Check if a module is loaded. */
> +bool module_loaded(const char *name);

Maybe module_is_loaded() would be a better name.
Christophe Leroy Jan. 21, 2021, 10 a.m. UTC | #2
Le 21/01/2021 à 08:49, Christoph Hellwig a écrit :
 > Add a helper that takes modules_mutex and uses find_module to check if a
 > given module is loaded.  This provides a better abstraction for the two
 > callers, and allows to unexport modules_mutex and find_module.
 >
 > Signed-off-by: Christoph Hellwig <hch@lst.de>
 > ---
 >   drivers/gpu/drm/drm_fb_helper.c |  7 +------
 >   include/linux/module.h          |  3 +++
 >   kernel/module.c                 | 14 ++++++++++++--
 >   kernel/trace/trace_kprobe.c     |  4 +---
 >   4 files changed, 17 insertions(+), 11 deletions(-)
 >

 > diff --git a/include/linux/module.h b/include/linux/module.h
 > index 7a0bcb5b1ffccd..b4654f8a408134 100644
 > --- a/include/linux/module.h
 > +++ b/include/linux/module.h
 > @@ -589,6 +589,9 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
 >   /* Search for module by name: must hold module_mutex. */
 >   struct module *find_module(const char *name);
 >   +/* Check if a module is loaded. */
 > +bool module_loaded(const char *name);

Maybe module_is_loaded() would be a better name.
David Laight Jan. 21, 2021, 10:13 a.m. UTC | #3
From: Christophe Leroy
> Sent: 21 January 2021 10:00
> 
> Le 21/01/2021 à 08:49, Christoph Hellwig a écrit :
> > Add a helper that takes modules_mutex and uses find_module to check if a
> > given module is loaded.  This provides a better abstraction for the two
> > callers, and allows to unexport modules_mutex and find_module.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >   drivers/gpu/drm/drm_fb_helper.c |  7 +------
> >   include/linux/module.h          |  3 +++
> >   kernel/module.c                 | 14 ++++++++++++--
> >   kernel/trace/trace_kprobe.c     |  4 +---
> >   4 files changed, 17 insertions(+), 11 deletions(-)
> >
> 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 7a0bcb5b1ffccd..b4654f8a408134 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -589,6 +589,9 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
> >   /* Search for module by name: must hold module_mutex. */
> >   struct module *find_module(const char *name);
> >
> > +/* Check if a module is loaded. */
> > +bool module_loaded(const char *name);
> 
> Maybe module_is_loaded() would be a better name.

I can't see the original patch.

What is the point of the function.
By the time it returns the information is stale - so mostly useless.

Surely you need to use try_module_get() instead?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Christophe Leroy Jan. 21, 2021, 10:17 a.m. UTC | #4
Le 21/01/2021 à 11:13, David Laight a écrit :
> From: Christophe Leroy

Sorry I hit "Reply to the list" instead of "Reply all"

Cc Christoph H. who is the author.

>> Sent: 21 January 2021 10:00
>>
>> Le 21/01/2021 à 08:49, Christoph Hellwig a écrit :
>>> Add a helper that takes modules_mutex and uses find_module to check if a
>>> given module is loaded.  This provides a better abstraction for the two
>>> callers, and allows to unexport modules_mutex and find_module.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>> ---
>>>    drivers/gpu/drm/drm_fb_helper.c |  7 +------
>>>    include/linux/module.h          |  3 +++
>>>    kernel/module.c                 | 14 ++++++++++++--
>>>    kernel/trace/trace_kprobe.c     |  4 +---
>>>    4 files changed, 17 insertions(+), 11 deletions(-)
>>>
>>
>>> diff --git a/include/linux/module.h b/include/linux/module.h
>>> index 7a0bcb5b1ffccd..b4654f8a408134 100644
>>> --- a/include/linux/module.h
>>> +++ b/include/linux/module.h
>>> @@ -589,6 +589,9 @@ static inline bool within_module(unsigned long addr, const struct module *mod)
>>>    /* Search for module by name: must hold module_mutex. */
>>>    struct module *find_module(const char *name);
>>>
>>> +/* Check if a module is loaded. */
>>> +bool module_loaded(const char *name);
>>
>> Maybe module_is_loaded() would be a better name.
> 
> I can't see the original patch.

You have it there 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20210121074959.313333-3-hch@lst.de/

> 
> What is the point of the function.
> By the time it returns the information is stale - so mostly useless.
> 
> Surely you need to use try_module_get() instead?
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
Christoph Hellwig Jan. 21, 2021, 5:11 p.m. UTC | #5
On Thu, Jan 21, 2021 at 11:00:20AM +0100, Christophe Leroy wrote:
> > +bool module_loaded(const char *name);
>
> Maybe module_is_loaded() would be a better name.

Fine with me.
David Laight Jan. 21, 2021, 5:44 p.m. UTC | #6
> 
> On Thu, Jan 21, 2021 at 11:00:20AM +0100, Christophe Leroy wrote:
> > > +bool module_loaded(const char *name);
> >
> > Maybe module_is_loaded() would be a better name.
> 
> Fine with me.

It does look like both callers aren't 'unsafe'.
But it is a bit strange returning a stale value.

It did make be look a bit more deeply at try_module_get().
For THIS_MODULE (eg to get an extra reference for a kthread)
I doubt it can ever fail.

But the other cases require a 'module' structure be passed in.
ISTM that unloading the module could invalidate the pointer
that has just been read from some other structure.

I'm guessing that some other lock must always be held.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 4b81195106875d..ce6d63ca75c32a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2508,13 +2508,8 @@  int __init drm_fb_helper_modinit(void)
 {
 #if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
 	const char name[] = "fbcon";
-	struct module *fbcon;
 
-	mutex_lock(&module_mutex);
-	fbcon = find_module(name);
-	mutex_unlock(&module_mutex);
-
-	if (!fbcon)
+	if (!module_loaded(name))
 		request_module_nowait(name);
 #endif
 	return 0;
diff --git a/include/linux/module.h b/include/linux/module.h
index 7a0bcb5b1ffccd..b4654f8a408134 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -589,6 +589,9 @@  static inline bool within_module(unsigned long addr, const struct module *mod)
 /* Search for module by name: must hold module_mutex. */
 struct module *find_module(const char *name);
 
+/* Check if a module is loaded. */
+bool module_loaded(const char *name);
+
 struct symsearch {
 	const struct kernel_symbol *start, *stop;
 	const s32 *crcs;
diff --git a/kernel/module.c b/kernel/module.c
index 4bf30e4b3eaaa1..619ea682e64cd1 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -88,7 +88,6 @@ 
  * (delete and add uses RCU list operations).
  */
 DEFINE_MUTEX(module_mutex);
-EXPORT_SYMBOL_GPL(module_mutex);
 static LIST_HEAD(modules);
 
 /* Work queue for freeing init sections in success case */
@@ -672,7 +671,18 @@  struct module *find_module(const char *name)
 	module_assert_mutex();
 	return find_module_all(name, strlen(name), false);
 }
-EXPORT_SYMBOL_GPL(find_module);
+
+bool module_loaded(const char *name)
+{
+	bool ret;
+
+	mutex_lock(&module_mutex);
+	ret = !!find_module(name);
+	mutex_unlock(&module_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(module_loaded);
 
 #ifdef CONFIG_SMP
 
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index e6fba1798771b4..c2e453f88bce70 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -124,9 +124,7 @@  static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
 	if (!p)
 		return true;
 	*p = '\0';
-	mutex_lock(&module_mutex);
-	ret = !!find_module(tk->symbol);
-	mutex_unlock(&module_mutex);
+	ret = module_loaded(tk->symbol);
 	*p = ':';
 
 	return ret;