[PATCH] depmod: Make dependency loops be fatal

[PATCH] depmod: Make dependency loops be fatal

[Lucas De Marchi]

Since the beginning depmod just warned about dependency loops and upon
creation of modules.dep{,.bin} it skipped the modules that were part of
a loop. However just skipping the modules may come as a surprise to
kernel module developers: they will need to try to load the module (or
to pay attention to the log messages) to notice thavt the module has not
been put in the index. Also, differently from module-init-tools we were
not skipping modules that depend on modules with dependency loops,
leading to a segfault in depmod.

So this is a summary of the change in behavior with this patch:

Loop 1)
    A -> B -> C -
    ^           |
    '------------

    Before:
        depmod: WARNING: found 3 modules in dependency cycles!
        depmod: WARNING: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleB.ko in dependency cycle!
        depmod: WARNING: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleC.ko in dependency cycle!
        depmod: WARNING: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleA.ko in dependency cycle!

        return code: 0

    After:
        depmod: ERROR: Found 3 modules in dependency cycles!
        depmod: ERROR: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleB.ko in dependency cycle!
        depmod: ERROR: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleC.ko in dependency cycle!
        depmod: ERROR: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleA.ko in dependency cycle!

        return code: 2

Loop 2)
    A -> B -> C -
         ^      |
         '-------

    Before:
        depmod: WARNING: found 2 modules in dependency cycles!
        depmod: WARNING: /tmp/test-kmod//lib/modules/3.14.2-1-ARCH/kernel/moduleB.ko in dependency cycle!
        depmod: WARNING: /tmp/test-kmod//lib/modules/3.14.2-1-ARCH/kernel/moduleC.ko in dependency cycle!
        Segmentation fault (core dumped)

    After:
        depmod: ERROR: Found 2 modules in dependency cycles!
        depmod: ERROR: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleB.ko in dependency cycle!
        depmod: ERROR: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleC.ko in dependency cycle!

        return code: 2

The segfault above could be fixed, but let's just fail everything
because dependency cycles should be fixed in the modules rather than
just be skipped in the index.
---
 tools/depmod.c | 55 +++++++++----------------------------------------------
 1 file changed, 9 insertions(+), 46 deletions(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index 1aedaaf..7ac1e26 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -927,7 +927,6 @@ struct mod {
 	int dep_sort_idx; /* topological sort index */
 	uint16_t idx; /* index in depmod->modules.array */
 	uint16_t users; /* how many modules depend on this one */
-	uint8_t dep_loop : 1;
 	char modname[];
 };
 
@@ -944,7 +943,6 @@ struct depmod {
 	struct hash *modules_by_uncrelpath;
 	struct hash *modules_by_name;
 	struct hash *symbols;
-	unsigned int dep_loops;
 };
 
 static void mod_free(struct mod *mod)
@@ -1337,12 +1335,6 @@ static int depmod_modules_search(struct depmod *depmod)
 static int mod_cmp(const void *pa, const void *pb) {
 	const struct mod *a = *(const struct mod **)pa;
 	const struct mod *b = *(const struct mod **)pb;
-	if (a->dep_loop == b->dep_loop)
-		return a->sort_idx - b->sort_idx;
-	else if (a->dep_loop)
-		return 1;
-	else if (b->dep_loop)
-		return -1;
 	return a->sort_idx - b->sort_idx;
 }
 
@@ -1566,12 +1558,6 @@ static int dep_cmp(const void *pa, const void *pb)
 {
 	const struct mod *a = *(const struct mod **)pa;
 	const struct mod *b = *(const struct mod **)pb;
-	if (a->dep_loop == b->dep_loop)
-		return a->dep_sort_idx - b->dep_sort_idx;
-	else if (a->dep_loop)
-		return 1;
-	else if (b->dep_loop)
-		return -1;
 	return a->dep_sort_idx - b->dep_sort_idx;
 }
 
@@ -1592,6 +1578,7 @@ static int depmod_calculate_dependencies(struct depmod *depmod)
 	const struct mod **itrm;
 	uint16_t *users, *roots, *sorted;
 	uint16_t i, n_roots = 0, n_sorted = 0, n_mods = depmod->modules.count;
+	int ret = 0;
 
 	users = malloc(sizeof(uint16_t) * n_mods * 3);
 	if (users == NULL)
@@ -1640,27 +1627,26 @@ static int depmod_calculate_dependencies(struct depmod *depmod)
 	}
 
 	if (n_sorted < n_mods) {
-		WRN("found %u modules in dependency cycles!\n",
+		ERR("Found %u modules in dependency cycles!\n",
 		    n_mods - n_sorted);
+		ret = -EINVAL;
 		for (i = 0; i < n_mods; i++) {
 			struct mod *m;
 			if (users[i] == 0)
 				continue;
 			m = depmod->modules.array[i];
-			WRN("%s in dependency cycle!\n", m->path);
-			m->dep_loop = 1;
-			m->dep_sort_idx = INT32_MAX;
-			depmod->dep_loops++;
+			ERR("%s in dependency cycle!\n", m->path);
 		}
+		goto exit;
 	}
 
 	depmod_sort_dependencies(depmod);
 
-	DBG("calculated dependencies and ordering (%u loops, %hu modules)\n",
-	    depmod->dep_loops, n_mods);
+	DBG("calculated dependencies and ordering (%hu modules)\n", n_mods);
 
+exit:
 	free(users);
-	return 0;
+	return ret;
 }
 
 static int depmod_load(struct depmod *depmod)
@@ -1761,11 +1747,6 @@ static int output_deps(struct depmod *depmod, FILE *out)
 		const char *p = mod_get_compressed_path(mod);
 		size_t j, n_deps;
 
-		if (mod->dep_loop) {
-			DBG("Ignored %s due dependency loops\n", p);
-			continue;
-		}
-
 		fprintf(out, "%s:", p);
 
 		if (mod->deps.count == 0)
@@ -1779,12 +1760,6 @@ static int output_deps(struct depmod *depmod, FILE *out)
 
 		for (j = 0; j < n_deps; j++) {
 			const struct mod *d = deps[j];
-			if (d->dep_loop) {
-				DBG("Ignored %s (dependency of %s) "
-				    "due dependency loops\n",
-				    mod_get_compressed_path(d), p);
-				continue;
-			}
 			fprintf(out, " %s", mod_get_compressed_path(d));
 		}
 		free(deps);
@@ -1814,11 +1789,6 @@ static int output_deps_bin(struct depmod *depmod, FILE *out)
 		size_t j, n_deps, linepos, linelen, slen;
 		int duplicate;
 
-		if (mod->dep_loop) {
-			DBG("Ignored %s due dependency loops\n", p);
-			continue;
-		}
-
 		deps = mod_get_all_sorted_dependencies(mod, &n_deps);
 		if (deps == NULL && n_deps > 0) {
 			ERR("could not get all sorted dependencies of %s\n", p);
@@ -1828,12 +1798,6 @@ static int output_deps_bin(struct depmod *depmod, FILE *out)
 		linelen = strlen(p) + 1;
 		for (j = 0; j < n_deps; j++) {
 			const struct mod *d = deps[j];
-			if (d->dep_loop) {
-				DBG("Ignored %s (dependency of %s) "
-				    "due dependency loops\n",
-				    mod_get_compressed_path(d), p);
-				continue;
-			}
 			linelen += 1 + strlen(mod_get_compressed_path(d));
 		}
 
@@ -1854,8 +1818,7 @@ static int output_deps_bin(struct depmod *depmod, FILE *out)
 		for (j = 0; j < n_deps; j++) {
 			const struct mod *d = deps[j];
 			const char *dp;
-			if (d->dep_loop)
-				continue;
+
 			line[linepos] = ' ';
 			linepos++;
 
-- 
1.9.2

Re: [PATCH] depmod: Make dependency loops be fatal

[Lucas De Marchi]

On Fri, May 09, 2014 at 10:25:09PM +0400, Matwey V. Kornilov wrote:
> 2014-05-09 22:14 GMT+04:00 Lucas De Marchi <lucas.demarchi@intel.com>:
> 
> > Since the beginning depmod just warned about dependency loops and upon
> > creation of modules.dep{,.bin} it skipped the modules that were part of
> > a loop. However just skipping the modules may come as a surprise to
> > kernel module developers: they will need to try to load the module (or
> > to pay attention to the log messages) to notice thavt the module has not
> > been put in the index. Also, differently from module-init-tools we were
> > not skipping modules that depend on modules with dependency loops,
> > leading to a segfault in depmod.
> >
> 
> Do I understand you correctly that your patch fails to create modules.dep

I'd say it avoids losing the previous, valid modules.dep.

> when there is at least one cycle? If so, I think it is too aggressive
> approach. Everything else besides the cycle remains functional. However,
> this would make developers put more attention to resolve the loops in
> different configurations.

This is what I'm arguing for... dependency loops are bugs in kernel
modules that should rather be fixed instead of papering over.

Btw, I don't think they are that common since we never received a bug
report about this in kmod. I'm open to people chiming in, claiming
otherwise.


-- 
Lucas De Marchi

Re: [PATCH] depmod: Make dependency loops be fatal

[Matwey V. Kornilov]

09.05.2014 22:45, Lucas De Marchi пишет:
>> Do I understand you correctly that your patch fails to create modules.dep
> 
> I'd say it avoids losing the previous, valid modules.dep.

When building an RPM package, we don't have the previous one.

> This is what I'm arguing for... dependency loops are bugs in kernel
> modules that should rather be fixed instead of papering over.
> 
> Btw, I don't think they are that common since we never received a bug
> report about this in kmod. I'm open to people chiming in, claiming
> otherwise.

How about introducing --force switch or something like that to alter the
behavior in runtime?

For instance, the default is to fail, but with --force is to omit looped
modules.

Re: [PATCH] depmod: Make dependency loops be fatal

[Lucas De Marchi]

On Fri, May 9, 2014 at 3:14 PM, Lucas De Marchi
<lucas.demarchi@intel.com> wrote:
> Since the beginning depmod just warned about dependency loops and upon
> creation of modules.dep{,.bin} it skipped the modules that were part of
> a loop. However just skipping the modules may come as a surprise to
> kernel module developers: they will need to try to load the module (or
> to pay attention to the log messages) to notice thavt the module has not
> been put in the index. Also, differently from module-init-tools we were
> not skipping modules that depend on modules with dependency loops,
> leading to a segfault in depmod.
>
> So this is a summary of the change in behavior with this patch:
>
> Loop 1)
>     A -> B -> C -
>     ^           |
>     '------------
>
>     Before:
>         depmod: WARNING: found 3 modules in dependency cycles!
>         depmod: WARNING: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleB.ko in dependency cycle!
>         depmod: WARNING: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleC.ko in dependency cycle!
>         depmod: WARNING: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleA.ko in dependency cycle!
>
>         return code: 0
>
>     After:
>         depmod: ERROR: Found 3 modules in dependency cycles!
>         depmod: ERROR: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleB.ko in dependency cycle!
>         depmod: ERROR: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleC.ko in dependency cycle!
>         depmod: ERROR: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleA.ko in dependency cycle!
>
>         return code: 2
>
> Loop 2)
>     A -> B -> C -
>          ^      |
>          '-------
>
>     Before:
>         depmod: WARNING: found 2 modules in dependency cycles!
>         depmod: WARNING: /tmp/test-kmod//lib/modules/3.14.2-1-ARCH/kernel/moduleB.ko in dependency cycle!
>         depmod: WARNING: /tmp/test-kmod//lib/modules/3.14.2-1-ARCH/kernel/moduleC.ko in dependency cycle!
>         Segmentation fault (core dumped)
>
>     After:
>         depmod: ERROR: Found 2 modules in dependency cycles!
>         depmod: ERROR: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleB.ko in dependency cycle!
>         depmod: ERROR: /tmp/test-kmod/lib/modules/3.14.2-1-ARCH/kernel/moduleC.ko in dependency cycle!
>
>         return code: 2
>
> The segfault above could be fixed, but let's just fail everything
> because dependency cycles should be fixed in the modules rather than
> just be skipped in the index.
> ---
>  tools/depmod.c | 55 +++++++++----------------------------------------------
>  1 file changed, 9 insertions(+), 46 deletions(-)

This is now applied. It's better to fail the upgrade than to provide
the user a broken setup, that maybe can't even boot successfully. The
old behavior can be achieved by just disabling the module in the
kernel config.

--
Lucas De Marchi