[PATCH 0/3] module: process aliasing when debugging

[PATCH 0/3] module: process aliasing when debugging

[Luis R. Rodriguez]

Debugging ineractions with userspace can often be a bit of pain, specially
when trying to figure out who is at fault for an issue. Having the kernel
process aliases when debugging can help us much faster find who is the
culprit to an issue.

I've been carrying this around privately in my tree since 2016 but it seems
others can benefit from similar debugging functionality so pushing this out
for integration and wider review.

Further testing, thoughts, reviews, flames are all equally appreciated.

Luis R. Rodriguez (3):
  module: use goto errors on check_modinfo() and layout_and_allocate()
  module: add helper get_modinfo_idx()
  module: add debugging alias parsing support

 include/linux/module.h |   4 ++
 init/Kconfig           |   7 +++
 kernel/module.c        | 129 +++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 135 insertions(+), 5 deletions(-)

-- 
2.15.0

[PATCH 1/3] module: use goto errors on check_modinfo() and layout_and_allocate()

[Luis R. Rodriguez]

Although both routines don't have much complex errors paths we
will expand on these routine in the future, setting up error lables
now for both will make subsequent changes easier to review. This
changes has no functional changes.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 kernel/module.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 3e1c1e217e3f..bb595dc87651 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3037,12 +3037,14 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 
 	err = check_modinfo_livepatch(mod, info);
 	if (err)
-		return err;
+		goto err_out;
 
 	/* Set up license info based on the info section */
 	set_license(mod, get_modinfo(info, "license"));
 
 	return 0;
+err_out:
+	return err;
 }
 
 static int find_module_sections(struct module *mod, struct load_info *info)
@@ -3313,7 +3315,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	err = module_frob_arch_sections(info->hdr, info->sechdrs,
 					info->secstrings, mod);
 	if (err < 0)
-		return ERR_PTR(err);
+		goto err_out;
 
 	/* We will do a special allocation for per-cpu sections later. */
 	info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
@@ -3336,12 +3338,14 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	/* Allocate and move to the final place */
 	err = move_module(mod, info);
 	if (err)
-		return ERR_PTR(err);
+		goto err_out;
 
 	/* Module has been copied to its final place now: return it. */
 	mod = (void *)info->sechdrs[info->index.mod].sh_addr;
 	kmemleak_load_module(mod, info);
 	return mod;
+err_out:
+	return ERR_PTR(err);
 }
 
 /* mod is no longer valid after this! */
-- 
2.15.0

[PATCH 2/3] module: add helper get_modinfo_idx()

[Luis R. Rodriguez]

get_modinfo() lets you look for at the modinfo section for
a value using a specific tag, this however is limited to
only give you the first tag found. In practice you can have
multiple entries using the same tag, one example are aliases

We have not had a need to support hunting for beyond the first
tag entry on the modinfo sectin of a module. For aliases we'll
need infrastructure to pick and choose which tag entry we want.

Add get_modinfo_idx() to enable us to pick and chooes *which*
tag entry we wish to get. get_modinfo() becomes then a wrapper
which uses get_modinfo_idx() to get the first entry.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 kernel/module.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index bb595dc87651..6d5f02e86681 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2485,20 +2485,28 @@ static char *next_string(char *string, unsigned long *secsize)
 	return string;
 }
 
-static char *get_modinfo(struct load_info *info, const char *tag)
+static const char *get_modinfo_idx(struct load_info *info, const char *tag,
+				   unsigned int tag_idx)
 {
 	char *p;
 	unsigned int taglen = strlen(tag);
 	Elf_Shdr *infosec = &info->sechdrs[info->index.info];
 	unsigned long size = infosec->sh_size;
+	unsigned int num_tags = 0;
 
 	for (p = (char *)infosec->sh_addr; p; p = next_string(p, &size)) {
-		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
+		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=' &&
+		    num_tags++ == tag_idx)
 			return p + taglen + 1;
 	}
 	return NULL;
 }
 
+static const char *get_modinfo(struct load_info *info, const char *tag)
+{
+	return get_modinfo_idx(info, tag, 0);
+}
+
 static void setup_modinfo(struct module *mod, struct load_info *info)
 {
 	struct module_attribute *attr;
-- 
2.15.0

[PATCH 3/3] module: add debugging alias parsing support

[Luis R. Rodriguez]

Debugging modules can often lead to an alias question. We purposely
don't have alias parsing support upstream as this is all dealt with
in userpace with the assumption that in-kernel we just process aliases
and userspace Does The Right Thing (TM) about aliases.

Obviously userspace can be buggy though, and it can lie to us. We
currently have no easy way to determine this. Parsing aliases is
an example debugging facility we can use to help with these sorts
of problems.

You can debug by adding to your dynamic debug:

GRUB_CMDLINE_LINUX_DEFAULT="dyndbg=\"func module_process_aliases +p;\" "

Upon boot for example here a few entries:

module ext4 num_aliases: 5
alias[0] = fs-ext4
alias[1] = ext3
alias[2] = fs-ext3
alias[3] = ext2
alias[4] = fs-ext2

module xfs num_aliases: 1
alias[0] = fs-xfs

module floppy num_aliases: 3
alias[0] = block-major-2-*
alias[1] = acpi*:PNP0700:*
alias[2] = pnp:dPNP0700*

module ata_piix num_aliases: 89
alias[0] = pci:v00008086d00008C81sv*sd*bc*sc*i*
alias[1] = pci:v00008086d00008C80sv*sd*bc*sc*i*
alias[2] = pci:v00008086d00008C89sv*sd*bc*sc*i*
alias[3] = pci:v00008086d00008C88sv*sd*bc*sc*i*
... etc ...

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 include/linux/module.h |   4 ++
 init/Kconfig           |   7 ++++
 kernel/module.c        | 107 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 118 insertions(+)

diff --git a/include/linux/module.h b/include/linux/module.h
index 548fa09fa806..2efb0e3e39d9 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -341,6 +341,10 @@ struct module {
 	const char *srcversion;
 	struct kobject *holders_dir;
 
+#ifdef CONFIG_MODULE_DEBUG
+	unsigned int num_aliases;
+	const char **aliases;
+#endif
 	/* Exported symbols */
 	const struct kernel_symbol *syms;
 	const s32 *crcs;
diff --git a/init/Kconfig b/init/Kconfig
index 2934249fba46..f216e0da3761 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1688,6 +1688,13 @@ menuconfig MODULES
 
 if MODULES
 
+config MODULE_DEBUG
+	bool "Enable debugging information for modules"
+	default n
+	help
+	  Enables debugging of the module infrastructure. Say no unless you
+	  are debugging the module framework.
+
 config MODULE_FORCE_LOAD
 	bool "Forced module loading"
 	default n
diff --git a/kernel/module.c b/kernel/module.c
index 6d5f02e86681..c1a10ef6ae76 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2129,6 +2129,28 @@ void __weak module_arch_freeing_init(struct module *mod)
 {
 }
 
+#ifdef CONFIG_MODULE_DEBUG
+static void free_mod_aliases(struct module *mod)
+{
+	unsigned int i;
+
+	if (!mod->num_aliases)
+		return;
+
+	for (i=0; i < mod->num_aliases; i++) {
+		kfree(mod->aliases[i]);
+		mod->aliases[i] = NULL;
+	}
+
+	kfree(mod->aliases);
+	mod->aliases = NULL;
+}
+#else
+static void free_mod_aliases(struct module *mod)
+{
+}
+#endif
+
 /* Free a module, remove from lists, etc. */
 static void free_module(struct module *mod)
 {
@@ -2174,6 +2196,7 @@ static void free_module(struct module *mod)
 	module_memfree(mod->init_layout.base);
 	kfree(mod->args);
 	percpu_modfree(mod);
+	free_mod_aliases(mod);
 
 	/* Free lock-classes; relies on the preceding sync_rcu(). */
 	lockdep_free_key_range(mod->core_layout.base, mod->core_layout.size);
@@ -2485,6 +2508,33 @@ static char *next_string(char *string, unsigned long *secsize)
 	return string;
 }
 
+#ifdef CONFIG_MODULE_DEBUG
+static int get_modinfo_tags(struct load_info *info,
+			    const char *tag,
+			    unsigned int *num_entries)
+{
+	char *p;
+	unsigned int taglen = strlen(tag);
+	Elf_Shdr *infosec = &info->sechdrs[info->index.info];
+	unsigned long size = infosec->sh_size;
+	const char *value;
+	unsigned int len, tags_size = 0;
+
+	for (p = (char *)infosec->sh_addr; p; p = next_string(p, &size)) {
+		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=') {
+			value = p + taglen + 1;
+			len = strlen(value);
+			if (len >=0 && len <= PAGE_SIZE) {
+				(*num_entries)++;
+				tags_size+=len;
+			}
+		}
+	}
+
+	return tags_size;
+}
+#endif
+
 static const char *get_modinfo_idx(struct load_info *info, const char *tag,
 				   unsigned int tag_idx)
 {
@@ -3011,6 +3061,53 @@ static struct module *setup_load_info(struct load_info *info, int flags)
 	return mod;
 }
 
+#ifdef CONFIG_MODULE_DEBUG
+static int module_process_aliases(struct module *mod, struct load_info *info)
+{
+	unsigned int size, i, num_entries = 0;
+	const char *alias;
+
+	size = get_modinfo_tags(info, "alias", &num_entries);
+	if (WARN_ON(!size))
+		return 0;
+
+	mod->aliases = kzalloc(num_entries * sizeof(char *), GFP_KERNEL);
+	if (!mod->aliases)
+		return -ENOMEM;
+
+	pr_debug("module %s num_aliases: %u\n", mod->name, num_entries);
+
+	for (i=0; i < num_entries; i++) {
+		alias = get_modinfo_idx(info, "alias", i);
+		pr_debug("alias[%u] = %s\n", i, alias);
+		mod->aliases[i] = kasprintf(GFP_KERNEL, "%s", alias);
+		if (!mod->aliases[i])
+			goto err_free;
+	}
+
+	mod->num_aliases = num_entries;
+
+	return 0;
+
+err_free:
+	while (i!=0) {
+		i--;
+		kfree(mod->aliases[i]);
+		mod->aliases[i] = NULL;
+	}
+
+	kfree(mod->aliases);
+	mod->aliases = NULL;
+
+	return -ENOMEM;
+}
+#else
+static int module_process_aliases(struct module *mod, struct load_info *info)
+{
+	return 0;
+}
+#endif
+
 static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 {
 	const char *modmagic = get_modinfo(info, "vermagic");
@@ -3043,6 +3140,12 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 			"is unknown, you have been warned.\n", mod->name);
 	}
 
+	if (get_modinfo(info, "alias")) {
+		err = module_process_aliases(mod, info);
+		if (err)
+			goto err_out_skip_alloc;
+	}
+
 	err = check_modinfo_livepatch(mod, info);
 	if (err)
 		goto err_out;
@@ -3052,6 +3155,8 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 
 	return 0;
 err_out:
+	free_mod_aliases(mod);
+err_out_skip_alloc:
 	return err;
 }
 
@@ -3353,6 +3458,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	kmemleak_load_module(mod, info);
 	return mod;
 err_out:
+	free_mod_aliases(mod);
 	return ERR_PTR(err);
 }
 
@@ -3824,6 +3930,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	synchronize_sched();
 	mutex_unlock(&module_mutex);
  free_module:
+	free_mod_aliases(mod);
 	/*
 	 * Ftrace needs to clean up what it initialized.
 	 * This does nothing if ftrace_module_init() wasn't called,
-- 
2.15.0

Re: module: add debugging alias parsing support

[Jessica Yu]

+++ Luis R. Rodriguez [29/11/17 18:36 -0800]:
>Debugging modules can often lead to an alias question. We purposely
>don't have alias parsing support upstream as this is all dealt with
>in userpace with the assumption that in-kernel we just process aliases
>and userspace Does The Right Thing (TM) about aliases.
>
>Obviously userspace can be buggy though, and it can lie to us. We
>currently have no easy way to determine this. Parsing aliases is
>an example debugging facility we can use to help with these sorts
>of problems.
>
>You can debug by adding to your dynamic debug:
>
>GRUB_CMDLINE_LINUX_DEFAULT="dyndbg=\"func module_process_aliases +p;\" "
>
>Upon boot for example here a few entries:
>
>module ext4 num_aliases: 5
>alias[0] = fs-ext4
>alias[1] = ext3
>alias[2] = fs-ext3
>alias[3] = ext2
>alias[4] = fs-ext2
>
>module xfs num_aliases: 1
>alias[0] = fs-xfs
>
>module floppy num_aliases: 3
>alias[0] = block-major-2-*
>alias[1] = acpi*:PNP0700:*
>alias[2] = pnp:dPNP0700*
>
>module ata_piix num_aliases: 89
>alias[0] = pci:v00008086d00008C81sv*sd*bc*sc*i*
>alias[1] = pci:v00008086d00008C80sv*sd*bc*sc*i*
>alias[2] = pci:v00008086d00008C89sv*sd*bc*sc*i*
>alias[3] = pci:v00008086d00008C88sv*sd*bc*sc*i*
>... etc ...
>
>Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

Hi Luis,

Just some quick questions - are there any plans to use these in-kernel
module aliases anywhere else? Or are you using them just for debugging?

And if it's only for debugging, what's the benefit of printing the
aliases in-kernel when one could already use modinfo to print the
exact same information (namely, in-kernel module name, which was
recently added by Kees, + aliases)? We're essentially parsing the
.modinfo section of the module binary in two different places, just
that this patchset does it in-kernel and modinfo does it from
userspace, so I'm curious why we want to bring this into the kernel as
well without any additional users..

Thanks!

Jessica

Re: module: add debugging alias parsing support

[Luis R. Rodriguez]

On Thu, Nov 30, 2017 at 02:17:11PM +0100, Jessica Yu wrote:
> Just some quick questions - are there any plans to use these in-kernel
> module aliases anywhere else? Or are you using them just for debugging?

As-is for now just debugging, but this could also more easily enable folks to
prototype further evaluation of its uses. IMHO just having this at least posted
online should suffice the later aspect of enabling folks to prototype.

You're right that one can find aliases in userspace. One of the benefits
of having this dump things on the kernel log is just that you can easily
get the aliases printed out for all modules actually loaded for your system
without much effort. I did find this useful when debugging and found it much
more convenient than scraping modules one by one by hand in userspace.

I had this implemented since 2016, and I had some ideas to use them in a
functional way, however I first had to knock out a series of of fixes for
kernel/kmod.c and setting up a baseline test infrastructure for kmod
(tools/testing/selftests/kmod/ and lib/test_kmod.c) as such I hadn't had time
to yet come around and finish benchmarking the alias enhancement ideas I had
started evaluating.

As such having aliases in-kernel currently are only useful for debugging and
prototyping.

  Luis

Re: module: add debugging alias parsing support

[Djalal Harouni]

On Thu, Nov 30, 2017 at 7:39 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Thu, Nov 30, 2017 at 02:17:11PM +0100, Jessica Yu wrote:
>> Just some quick questions - are there any plans to use these in-kernel
>> module aliases anywhere else? Or are you using them just for debugging?
>
> As-is for now just debugging, but this could also more easily enable folks to
> prototype further evaluation of its uses. IMHO just having this at least posted
> online should suffice the later aspect of enabling folks to prototype.

I confirm that, after the module auto-load discussion where it is
clear that we need to improve the infrastructure, this debug
information may save some time, maybe someone can automate a script go
through modules and then on filesystem, however these patches may show
which module lead to load another one, right ? on userspace if there
are multiple dependencies it can be difficult I think.


>
> You're right that one can find aliases in userspace. One of the benefits
> of having this dump things on the kernel log is just that you can easily
> get the aliases printed out for all modules actually loaded for your system
> without much effort. I did find this useful when debugging and found it much
> more convenient than scraping modules one by one by hand in userspace.
>
> I had this implemented since 2016, and I had some ideas to use them in a
> functional way, however I first had to knock out a series of of fixes for
> kernel/kmod.c and setting up a baseline test infrastructure for kmod
> (tools/testing/selftests/kmod/ and lib/test_kmod.c) as such I hadn't had time
> to yet come around and finish benchmarking the alias enhancement ideas I had
> started evaluating.
>
> As such having aliases in-kernel currently are only useful for debugging and
> prototyping.

I would say so, however no strong argument if it should be mainlined.
Luis in your commit log you say:

"Obviously userspace can be buggy though, and it can lie to us. We
currently have no easy way to determine this."

Could you please share some info here ? how userspace can be buggy ?

Thank you!

>   Luis



-- 
tixxdz

Re: module: add debugging alias parsing support

[Jessica Yu]

+++ Djalal Harouni [04/12/17 10:01 +0100]:
>On Thu, Nov 30, 2017 at 7:39 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> On Thu, Nov 30, 2017 at 02:17:11PM +0100, Jessica Yu wrote:
>>> Just some quick questions - are there any plans to use these in-kernel
>>> module aliases anywhere else? Or are you using them just for debugging?
>>
>> As-is for now just debugging, but this could also more easily enable folks to
>> prototype further evaluation of its uses. IMHO just having this at least posted
>> online should suffice the later aspect of enabling folks to prototype.
>
>I confirm that, after the module auto-load discussion where it is
>clear that we need to improve the infrastructure, this debug
>information may save some time, maybe someone can automate a script go
>through modules and then on filesystem,

>however these patches may show
>which module lead to load another one, right ? on userspace if there
>are multiple dependencies it can be difficult I think.

Hm? I'm confused by what you mean here. The patchset just saves and
prints a module's aliases on module load if the debug option is
enabled. There's no dependency tracking here; that's modprobe's job.
And if you need to see which additional modules are being loaded as a
result of a module load there's already modprobe --verbose and
modules.dep..

>> You're right that one can find aliases in userspace. One of the benefits
>> of having this dump things on the kernel log is just that you can easily
>> get the aliases printed out for all modules actually loaded for your system
>> without much effort. I did find this useful when debugging and found it much
>> more convenient than scraping modules one by one by hand in userspace.
>>
>> I had this implemented since 2016, and I had some ideas to use them in a
>> functional way, however I first had to knock out a series of of fixes for
>> kernel/kmod.c and setting up a baseline test infrastructure for kmod
>> (tools/testing/selftests/kmod/ and lib/test_kmod.c) as such I hadn't had time
>> to yet come around and finish benchmarking the alias enhancement ideas I had
>> started evaluating.
>>
>> As such having aliases in-kernel currently are only useful for debugging and
>> prototyping.
>
>I would say so, however no strong argument if it should be mainlined.
>Luis in your commit log you say:
>
>"Obviously userspace can be buggy though, and it can lie to us. We
>currently have no easy way to determine this."
>
>Could you please share some info here ? how userspace can be buggy ?

Ditto, I agree that a concrete debugging example would be helpful in
understanding why having this in-kernel is better than using existing
userspace tools to look up module aliases. AFAIK the debug prints only
on module load, but you could also easily get the aliases for all
modules loaded on your system by doing something like -

for mod in $(awk '{print $1}' /proc/modules) ; do echo $mod; modinfo -F alias $mod ; done

(Assuming they are all modules that modprobe knows about) Is there a
debugging scenario you ran into where the above script wouldn't
suffice?

Jessica

Re: module: add debugging alias parsing support

[Djalal Harouni]

On Mon, Dec 4, 2017 at 2:58 PM, Jessica Yu <jeyu@kernel.org> wrote:
> +++ Djalal Harouni [04/12/17 10:01 +0100]:
>>
>> On Thu, Nov 30, 2017 at 7:39 PM, Luis R. Rodriguez <mcgrof@kernel.org>
>> wrote:
>>>
>>> On Thu, Nov 30, 2017 at 02:17:11PM +0100, Jessica Yu wrote:
>>>>
>>>> Just some quick questions - are there any plans to use these in-kernel
>>>> module aliases anywhere else? Or are you using them just for debugging?
>>>
>>>
>>> As-is for now just debugging, but this could also more easily enable
>>> folks to
>>> prototype further evaluation of its uses. IMHO just having this at least
>>> posted
>>> online should suffice the later aspect of enabling folks to prototype.
>>
>>
>> I confirm that, after the module auto-load discussion where it is
>> clear that we need to improve the infrastructure, this debug
>> information may save some time, maybe someone can automate a script go
>> through modules and then on filesystem,
>
>
>> however these patches may show
>> which module lead to load another one, right ? on userspace if there
>> are multiple dependencies it can be difficult I think.
>
>
> Hm? I'm confused by what you mean here. The patchset just saves and
> prints a module's aliases on module load if the debug option is
> enabled. There's no dependency tracking here; that's modprobe's job.
> And if you need to see which additional modules are being loaded as a
> result of a module load there's already modprobe --verbose and
> modules.dep..

Yes I was referring by the printing or kernel logs order, if two
modules depend on same one, we know which first one triggered it, and
in that context it will be a bit easier in the auto-loading context,
maybe like crypto ones that can be triggered from anywhere.

Thanks!

-- 
tixxdz

Re: module: add debugging alias parsing support

[Luis R. Rodriguez]

On Mon, Dec 04, 2017 at 10:01:02AM +0100, Djalal Harouni wrote:
> Luis in your commit log you say:
> 
> "Obviously userspace can be buggy though, and it can lie to us. We
> currently have no easy way to determine this."
> 
> Could you please share some info here ? how userspace can be buggy ?

Sure, so  kmod <= v19 was broken -- it returns 0 to modprobe calls, assuming
the kernel module is built-in, where really we have a race as the module starts
forming. kmod <= v19 has incorrect userspace heuristics, a userspace kmod fix
is available for it:

http://git.kernel.org/cgit/utils/kernel/kmod/kmod.git/commit/libkmod/libkmod-module.c?id=fd44a98ae2eb5eb32161088954ab21e58e19dfc4
    
Although this is fixed now, in theory userspace can regress again, but debugging
these sorts of issue is not so easy at all.

Technically, with aliases parsed (as this patch series proposed it's just
debugging data), it should be possible for request_module() to also issue a
finished_kmod_load() so that when request_module() completes and returns 0 if
we know for certain the module is loaded. This could be useful either debugging
purposes, for instance to catch when userspace lies once again, or if we do
find value in it, to make a pedantic and patient new request_module_load(),
which for instance would want to just wait for the module to really be loaded.

What I mean is that some request_module() calls uses *assume* that if it
returns 0 that the module is loaded, and this is incorrect. Each
request_module() users today *should* in theory vet and ensure each module is
loaded correctly.  Since there is no generic form to do this today,
try_then_request_module() was added to help with that.
try_then_request_module() solves two issues really, one is it prevents a
request to userspace if the module is already loaded, and two, it double checks
again if the module is loaded at the end using whatever heuristic the caller
wishes.

AFAICT its subjective whether or not each caller should open code its
own try_then_request_module() form, or if we should have a generic validator,
as such even though I have some dev code to use aliases to do something like
finished_kmod_load(), its just debug private code I use right now, and AFAICT
we have no formal justification for a change. Perhaps one could argue that
having a generic validator is much easier than expecting all callers to use
it correctly, but last time I checked I found only one buggy users of
request_module() and I fixed it (see 41124db869b ("fs: warn in case userspace
lied about modprobe return") so the point was moot.

You might think that expanding uses of aliases might benefit from a validator,
but the only thing I can think of right now is that avoiding to call
userspace if the module is already loaded, but this can *already* be
done by using try_then_request_module() or open coding your own checker
as get_fs_type() does, its just none of this is in a very generic form.

If you do come up with a valid argument for a generic form or for a full
generic wait as you expand use of aliases using request_module() do let me
know and we can revisit separately.

In the meantime I just noticed I do have a valid small optimization in my dev
code which does make sense upstream which I'll send in for review.

  Luis

Re: [PATCH 0/3] module: process aliasing when debugging

[Luis R. Rodriguez]

*Poke*

  Luis

On Wed, Nov 29, 2017 at 6:36 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> Debugging ineractions with userspace can often be a bit of pain, specially
> when trying to figure out who is at fault for an issue. Having the kernel
> process aliases when debugging can help us much faster find who is the
> culprit to an issue.
>
> I've been carrying this around privately in my tree since 2016 but it seems
> others can benefit from similar debugging functionality so pushing this out
> for integration and wider review.
>
> Further testing, thoughts, reviews, flames are all equally appreciated.
>
> Luis R. Rodriguez (3):
>   module: use goto errors on check_modinfo() and layout_and_allocate()
>   module: add helper get_modinfo_idx()
>   module: add debugging alias parsing support
>
>  include/linux/module.h |   4 ++
>  init/Kconfig           |   7 +++
>  kernel/module.c        | 129 +++++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 135 insertions(+), 5 deletions(-)
>
> --
> 2.15.0
>