RE: [PATCH 02/13] module: add a module_loaded helper
> > 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)
Re: [PATCH 02/13] module: add a module_loaded helper
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.
Re: [PATCH 02/13] module: add a module_loaded helper
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 --- 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.31-3-...@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)
RE: [PATCH 02/13] module: add a module_loaded helper
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 > > --- > > 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)
Re: [PATCH 02/13] module: add a module_loaded helper
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 > --- > 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.
Re: [PATCH 02/13] module: add a module_loaded helper
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 --- 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.
