| Message ID | 20210121074959.313333-3-hch@lst.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [01/13] powerpc/powernv: remove get_cxl_module | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (41d8cb7ece7c81e4eb897ed7ec7d3c3d72fd0af4) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 59 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
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.
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.
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)
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) >
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.
> > 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 --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;
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(-)