| Message ID | 3c7547d551558c9da02038dda45992f91b1f5141.1651314499.git.mchehab@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Let userspace know when snd-hda-intel needs i915 | expand |
On Sat, Apr 30, 2022 at 11:30:58AM +0100, Mauro Carvalho Chehab wrote: > Sometimes, device drivers are bound into each other via try_module_get(), > making such references invisible when looking at /proc/modules or lsmod. > > Add a function to allow setting up module references for such > cases, and call it when try_module_get() is used. > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org> > --- > > See [PATCH v2 0/2] at: https://lore.kernel.org/all/cover.1651314499.git.mchehab@kernel.org/ > > include/linux/module.h | 4 +++- > kernel/module/main.c | 35 +++++++++++++++++++++++++++++++++-- > 2 files changed, 36 insertions(+), 3 deletions(-) > > diff --git a/include/linux/module.h b/include/linux/module.h > index 46d4d5f2516e..836851baaad4 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -620,7 +620,9 @@ extern void __module_get(struct module *module); > > /* This is the Right Way to get a module: if it fails, it's being removed, > * so pretend it's not there. */ > -extern bool try_module_get(struct module *module); > +extern bool __try_module_get(struct module *module, struct module *this); > + > +#define try_module_get(mod) __try_module_get(mod, THIS_MODULE) > > extern void module_put(struct module *module); > > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 05a42d8fcd7a..9f4416381e65 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -631,6 +631,35 @@ static int ref_module(struct module *a, struct module *b) > return 0; > } > > +static int ref_module_dependency(struct module *mod, > + struct module *this) This can be on one line, right? > +{ > + int ret; > + > + if (!this || !this->name) { > + return -EINVAL; > + } Did you run checkpatch on this? Please do :) > + > + if (mod == this) > + return 0; How can this happen? When people mistakenly call try_module_get(THIS_MODULE)? We should throw up a big warning when that happens anyway as that's always wrong. But that's a different issue from this change, sorry for the noise. > + > + mutex_lock(&module_mutex); > + > + ret = ref_module(this, mod); > + > +#ifdef CONFIG_MODULE_UNLOAD > + if (ret) > + goto ret; > + > + ret = sysfs_create_link(mod->holders_dir, > + &this->mkobj.kobj, this->name); Meta comment, why do we only create links if we can unload things? thanks, greg k-h
Em Sat, 30 Apr 2022 14:04:59 +0200 Greg KH <gregkh@linuxfoundation.org> escreveu: > On Sat, Apr 30, 2022 at 11:30:58AM +0100, Mauro Carvalho Chehab wrote: > Did you run checkpatch on this? Please do :) > > > + > > + if (mod == this) > > + return 0; > > How can this happen? > When people mistakenly call try_module_get(THIS_MODULE)? Yes. There are lots of place where this is happening: $ git grep try_module_get\(THIS_MODULE|wc -l 82 > We should > throw up a big warning when that happens anyway as that's always wrong. > > But that's a different issue from this change, sorry for the noise. It sounds very weird to use try_module_get(THIS_MODULE). We could add a WARN_ON() there - or something similar - but I would do it on a separate patch. > > > + > > + mutex_lock(&module_mutex); > > + > > + ret = ref_module(this, mod); > > + > > +#ifdef CONFIG_MODULE_UNLOAD > > + if (ret) > > + goto ret; > > + > > + ret = sysfs_create_link(mod->holders_dir, > > + &this->mkobj.kobj, this->name); > > Meta comment, why do we only create links if we can unload things? Good question. I don't know for certain. This is the already existing pattern at add_usage_links() - see kernel/module/sysfs.c. Also, lsmod uses sysfs links when showing dependencies. Regards, Mauro
From: Mauro Carvalho Chehab > Sent: 30 April 2022 14:38 > > Em Sat, 30 Apr 2022 14:04:59 +0200 > Greg KH <gregkh@linuxfoundation.org> escreveu: > > > On Sat, Apr 30, 2022 at 11:30:58AM +0100, Mauro Carvalho Chehab wrote: > > > Did you run checkpatch on this? Please do :) > > > > > + > > > + if (mod == this) > > > + return 0; > > > > How can this happen? > > When people mistakenly call try_module_get(THIS_MODULE)? > > Yes. There are lots of place where this is happening: > > $ git grep try_module_get\(THIS_MODULE|wc -l > 82 > > > We should > > throw up a big warning when that happens anyway as that's always wrong. > > > > But that's a different issue from this change, sorry for the noise. > > It sounds very weird to use try_module_get(THIS_MODULE). > > We could add a WARN_ON() there - or something similar - but I would do it > on a separate patch. You could add a compile-time check. But a run-time one seems unnecessary. Clearly try_module_get(THIS_MODULE) usually succeeds. I think I can invent a case where it can fail: The module count must be zero, and a module unload in progress. The thread doing the unload is blocked somewhere. Another thread makes a callback into the module for some request that (for instance) would need to create a kernel thread. It tries to get a reference for the thread. So try_module_get(THIS_MODULE) is the right call - and will fail here. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/include/linux/module.h b/include/linux/module.h index 46d4d5f2516e..836851baaad4 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -620,7 +620,9 @@ extern void __module_get(struct module *module); /* This is the Right Way to get a module: if it fails, it's being removed, * so pretend it's not there. */ -extern bool try_module_get(struct module *module); +extern bool __try_module_get(struct module *module, struct module *this); + +#define try_module_get(mod) __try_module_get(mod, THIS_MODULE) extern void module_put(struct module *module); diff --git a/kernel/module/main.c b/kernel/module/main.c index 05a42d8fcd7a..9f4416381e65 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -631,6 +631,35 @@ static int ref_module(struct module *a, struct module *b) return 0; } +static int ref_module_dependency(struct module *mod, + struct module *this) +{ + int ret; + + if (!this || !this->name) { + return -EINVAL; + } + + if (mod == this) + return 0; + + mutex_lock(&module_mutex); + + ret = ref_module(this, mod); + +#ifdef CONFIG_MODULE_UNLOAD + if (ret) + goto ret; + + ret = sysfs_create_link(mod->holders_dir, + &this->mkobj.kobj, this->name); +#endif + +ret: + mutex_unlock(&module_mutex); + return ret; +} + /* Clear the unload stuff of the module. */ static void module_unload_free(struct module *mod) { @@ -841,7 +870,7 @@ void __module_get(struct module *module) } EXPORT_SYMBOL(__module_get); -bool try_module_get(struct module *module) +bool __try_module_get(struct module *module, struct module *this) { bool ret = true; @@ -856,9 +885,11 @@ bool try_module_get(struct module *module) preempt_enable(); } + if (ret) + ref_module_dependency(module, this); return ret; } -EXPORT_SYMBOL(try_module_get); +EXPORT_SYMBOL(__try_module_get); void module_put(struct module *module) {