module: process aliasing when debugging

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

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.

---
 kernel/module.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

index 3e1c1e217e3f..bb595dc87651 100644
@@ -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

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.

---
 kernel/module.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

index bb595dc87651..6d5f02e86681 100644
@@ -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

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 ...

---
 include/linux/module.h |   4 ++
 init/Kconfig           |   7 ++++
 kernel/module.c        | 107 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 118 insertions(+)

index 548fa09fa806..2efb0e3e39d9 100644
@@ -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;

index 2934249fba46..f216e0da3761 100644
@@ -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

2.15.0

+++ Luis R. Rodriguez [29/11/17 18:36 -0800]:

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

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

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.

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!

tixxdz

+++ Djalal Harouni [04/12/17 10:01 +0100]:

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..

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

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

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

*Poke*

Luis